[OPIK-853] handle gemini client errors #1138

idoberko2 · 2025-01-26T11:32:33Z

Details

Gemini client errors were not handled correctly. The purpose of this PR is fix the Gemini error handling method to respond with a human readable error.
Gemini API throws RuntimeExceptions with the error details as a JSON inside the message. This PR adds parsing to the error message in order to extract the meaningful part out of it.

Issues

OPIK-853

Testing

Tested locally:

❯ curl -v -X "POST" -H "Content-Type: application/json" --data '{"model":"gemini-1.5-flash","messages":[{"role":"user","content":"why are the sky blue?"}],"stream":true,"stream_options":{"include_usage":true},"temperature":0,"max_completion_tokens":1024,"top_p":1}' http://localhost:8080/v1/private/chat/completions

* Host localhost:8080 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:8080...
* Connected to localhost (::1) port 8080
> POST /v1/private/chat/completions HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/8.7.1
> Accept: */*
> Content-Type: application/json
> Content-Length: 200
>
* upload completely sent off: 200 bytes
< HTTP/1.1 200 OK
< Date: Sun, 26 Jan 2025 11:23:34 GMT
< Content-Type: text/event-stream
< Transfer-Encoding: chunked
<
{"code":429,"message":"Resource has been exhausted (e.g. check quota).","details":"RESOURCE_EXHAUSTED"}
* Connection #0 to host localhost left intact

thiagohora · 2025-01-26T13:14:48Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java

+                JsonNode errorNode = JsonUtils.MAPPER.readTree(jsonPart).get("error");
+                if (errorNode != null) {
+                    var code = errorNode.get("code").asInt();
+                    if (familyOf(code) == Response.Status.Family.CLIENT_ERROR) {


If we are familiar with the structure they send use, they not creating a DTO and trying to parse the DTO?

Agree here, let's just deserialise to an object for the known format.

thiagohora

Minor suggestion: It looks good!

BorisTkachenko · 2025-01-27T10:15:05Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java

+                    }
+                }
+            } catch (Exception e) {
+                return Optional.empty();


I would add some log here for visibility. If something goes wrong now it just swallows it.

Agree, probably not at error level as this can be triggered by errors in the parsing of the LLM provider response which we don't control, but maybe at warning level.

Additionally, in order to be sure that this is a parsing error and not a programmatic error from our side, you should probably narrow down this catch to the smaller exception type for that. Probably UncheckedIOException which is what comes from our JsonUtils calls.

BorisTkachenko · 2025-01-27T10:17:32Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java

+                JsonNode errorNode = JsonUtils.MAPPER.readTree(jsonPart).get("error");
+                if (errorNode != null) {
+                    var code = errorNode.get("code").asInt();
+                    if (familyOf(code) == Response.Status.Family.CLIENT_ERROR) {


Why do we return only errors related to CLIENT_ERROR? Potential SERVER_ERRORs are ignored.

Agree here, we should forward any known error received from the LLM provider, in this case Gemini. What we shouldn't leak externally is any programmatic or internal error from our side.

BorisTkachenko

Left a few comments

andrescrz

Please address the important open comments.

andrescrz · 2025-01-29T09:15:40Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java

+        /// gemini throws RuntimeExceptions with message structure as follows:
+        /// ```
+        /// java.lang.RuntimeException: HTTP error (429): {
+        ///   "error": {
+        ///     "code": 429,
+        ///     "message": "Resource has been exhausted (e.g. check quota).",
+        ///     "status": "RESOURCE_EXHAUSTED"
+        ///   }
+        /// }
+        ///  ```


Minor: please move it as Javadoc comment for this method.

andrescrz · 2025-01-29T09:16:44Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java

+        if (message.contains("{")) {
+            String jsonPart = message.substring(message.indexOf("{")); // Extract JSON part


Minor: you can simplify the logic here by just calling indexOf first and checking that not equal to -1.

andrescrz · 2025-01-29T09:17:03Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java

+                JsonNode errorNode = JsonUtils.MAPPER.readTree(jsonPart).get("error");
+                if (errorNode != null) {
+                    var code = errorNode.get("code").asInt();
+                    if (familyOf(code) == Response.Status.Family.CLIENT_ERROR) {


Agree here, let's just deserialise to an object for the known format.

andrescrz · 2025-01-29T09:18:11Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java

+                JsonNode errorNode = JsonUtils.MAPPER.readTree(jsonPart).get("error");
+                if (errorNode != null) {
+                    var code = errorNode.get("code").asInt();
+                    if (familyOf(code) == Response.Status.Family.CLIENT_ERROR) {


Agree here, we should forward any known error received from the LLM provider, in this case Gemini. What we shouldn't leak externally is any programmatic or internal error from our side.

andrescrz · 2025-01-29T09:20:15Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java

+                    }
+                }
+            } catch (Exception e) {
+                return Optional.empty();


Agree, probably not at error level as this can be triggered by errors in the parsing of the LLM provider response which we don't control, but maybe at warning level.

Additionally, in order to be sure that this is a parsing error and not a programmatic error from our side, you should probably narrow down this catch to the smaller exception type for that. Probably UncheckedIOException which is what comes from our JsonUtils calls.

idoberko2 added 2 commits January 26, 2025 13:27

OPIK-853 handle gemini errors correctly

ff71048

OPIK-853 handle gemini client errors correctly

99f55e0

idoberko2 self-assigned this Jan 26, 2025

idoberko2 requested a review from a team as a code owner January 26, 2025 11:32

thiagohora reviewed Jan 26, 2025

View reviewed changes

thiagohora approved these changes Jan 26, 2025

View reviewed changes

BorisTkachenko reviewed Jan 27, 2025

View reviewed changes

andrescrz requested changes Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OPIK-853] handle gemini client errors #1138

[OPIK-853] handle gemini client errors #1138

idoberko2 commented Jan 26, 2025 •

edited

Loading

thiagohora Jan 26, 2025

andrescrz Jan 29, 2025 •

edited

Loading

thiagohora left a comment

BorisTkachenko Jan 27, 2025

andrescrz Jan 29, 2025 •

edited

Loading

BorisTkachenko Jan 27, 2025

andrescrz Jan 29, 2025 •

edited

Loading

BorisTkachenko left a comment

andrescrz left a comment

andrescrz Jan 29, 2025

andrescrz Jan 29, 2025

andrescrz Jan 29, 2025 •

edited

Loading

andrescrz Jan 29, 2025 •

edited

Loading

andrescrz Jan 29, 2025 •

edited

Loading

		if (message.contains("{")) {
		String jsonPart = message.substring(message.indexOf("{")); // Extract JSON part

[OPIK-853] handle gemini client errors #1138

Are you sure you want to change the base?

[OPIK-853] handle gemini client errors #1138

Conversation

idoberko2 commented Jan 26, 2025 • edited Loading

Details

Issues

Testing

thiagohora Jan 26, 2025

Choose a reason for hiding this comment

andrescrz Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

thiagohora left a comment

Choose a reason for hiding this comment

BorisTkachenko Jan 27, 2025

Choose a reason for hiding this comment

andrescrz Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

BorisTkachenko Jan 27, 2025

Choose a reason for hiding this comment

andrescrz Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

BorisTkachenko left a comment

Choose a reason for hiding this comment

andrescrz left a comment

Choose a reason for hiding this comment

andrescrz Jan 29, 2025

Choose a reason for hiding this comment

andrescrz Jan 29, 2025

Choose a reason for hiding this comment

andrescrz Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

andrescrz Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

andrescrz Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

idoberko2 commented Jan 26, 2025 •

edited

Loading

andrescrz Jan 29, 2025 •

edited

Loading

andrescrz Jan 29, 2025 •

edited

Loading

andrescrz Jan 29, 2025 •

edited

Loading

andrescrz Jan 29, 2025 •

edited

Loading

andrescrz Jan 29, 2025 •

edited

Loading

andrescrz Jan 29, 2025 •

edited

Loading