[opamp] Improve error handling#2778
Conversation
…rst when a 200 has a bad body.
There was a problem hiding this comment.
Pull request overview
This PR tightens OpAMP HTTP client “connection success” semantics by only signaling success after a 2xx response is received and the server protobuf response has been successfully decoded, improving behavior in edge cases like HTTP redirects and invalid response bodies.
Changes:
- Move
onConnectionSuccess()to occur only afterisSuccessful(response)and successfulServerToAgentdecoding. - Treat protobuf parsing
IllegalStateExceptionas a request failure (instead of a connection failure). - Add/adjust unit tests to cover non-2xx responses and parsing edge cases (EOF and IllegalStateException).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java |
Refines callback/error-handling flow so “connection success” is only emitted for 2xx + valid protobuf responses; adds special handling for parsing IllegalStateException. |
opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java |
Adds tests for non-2xx and invalid-body scenarios; extends response-mocking helpers. |
| } | ||
| } catch (IllegalStateException e) { | ||
| getCallback().onRequestFailed(e); | ||
| connectionStatus.retryAfter(null); |
There was a problem hiding this comment.
I think there are other parsing errors that could cause other exceptions that are not caught. Instead of the up front IllegalStateException, maybe it's better to have a catch all Exception as the last catch clause which does the onRequestFailed
There was a problem hiding this comment.
Yeah that sounds like a good idea.
The sheer number of permutations of which exception should map into a connectionFailed and/or requestFailed under what circumstances is a bit daunting...so I like that simplifying that.
jackshirazi
left a comment
jackshirazi
left a comment
There was a problem hiding this comment.
Approved as is, there's one feedback suggestion but it's fine to go with this as is, too
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
cb3ab8e
Related to #2777, but probably does not resolve it.
There are two main ideas here, the main one is that the
onConnectionSuccess()is moved into theisSuccessful()check and is also moved under the decoding of the server proto. With this, if the response is non-200 we don't consider this a successful connection, and if the server's response is invalid, we also do not consider this a valid connection.There's another situation where the response body parsing can generate an ISE in the protobuf libs, and we turn that into a request failure.
There should be sufficient test coverage to handle these oddities. The semantics here are admittedly tricky/confusing, so I appreciate another set of eyes.