JettyClientMetrics compatibility with Jetty 12 #4756
Conversation
micrometer-jetty12/src/main/java/io/micrometer/jetty12/client/JettyClientTagsProvider.java
Show resolved
Hide resolved
|
The micrometer-samples-jetty12 module can be removed after this and #4753 are merged and the test there is added to the micrometer-jetty12 module. A question that's tangential to this pull request is whether we copy |
| import static java.util.concurrent.TimeUnit.SECONDS; | ||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| @WireMockTest |
There was a problem hiding this comment.
I reworked the tests to use WireMock since that seems easier and more similar to other tests we have.
There was a problem hiding this comment.
Shame it doesn't support HTTP/2 or HTTP/3 (or TLS properly)
shakuzen
force-pushed
the
jetty12-client
branch
3 times, most recently
from
57edf33 to
ff96e60
Compare
|
@joakime if you have some time to take a look at this, it's the client-side instrumentation updated for Jetty 12. It's mostly a copy of the existing Jetty client instrumentation for Jetty 9, but if you spot anything that looks unusual or could be improved, please let us know. |
| public static KeyValue outcome(@Nullable Result result) { | ||
| if (result == null) { | ||
| return OUTCOME_UNKNOWN; | ||
| } | ||
| return Outcome.forStatus(result.getResponse().getStatus()).asKeyValue(); | ||
| } |
There was a problem hiding this comment.
With HTTP/1 the existence of the status code is just what the response commit buffer had, not that the request/response is complete or successful (depending on the mode used in HTTP/1 there could be a socket exception, or even just a normal socket closure with incomplete data see "close-delimited message" on the HTTP/1 specs).
With HTTP/2 and HTTP/3 the status code is just what was seen on the first buffer.
Those responses can also be canceled via the HTTP/2 and HTTP/3 protocols (eg: GOAWAY) and be incomplete, these would be seen as protocol specific exceptions.
There was a problem hiding this comment.
Those are interesting scenarios and I don't think we're properly handling them right now. It sounds like something we could write a test for and then figure out how to (or if we can) improve all the different HTTP instrumentations (/cc @bclozel). For HTTP client instrumentation we have a small suite of tests that we run all of them against; see HttpClientTimingInstrumentationVerificationTests which is extended in this pull request to run the instrumentation against it in Jetty12ClientTimingInstrumentationVerificationTests.
Are there existing tests in Jetty for this scenario that we could get inspiration from to add to our test suite?
We haven't done a good job of testing with HTTP/2 or HTTP/3 so far, so that's another point of improvement.
|
I don't see protocol/version specific details in these metrics. I'm especially concerned about that kind of metric in light of PR jetty/jetty.project#11368 |
Thanks for pointing that out and sharing the Jetty PR. Right now it may be more straightforward to tag the protocol, but it sounds like after that PR, there may be layering of protocols and that makes it complicated to map that to tags. It's certainly something to explore, though. Maybe it'd be best to open an issue for it and track it separately so we don't block merging this on that. What do you think? |
shakuzen
force-pushed
the
jetty12-client
branch
from
ff96e60 to
87c470c
Compare
Add JettyClientMetrics and related classes to a micrometer-jetty12 module and migrate Jetty API accordingly.
shakuzen
force-pushed
the
jetty12-client
branch
from
87c470c to
ce3aa1f
Compare
Copy JettyClientMetrics and related classes to a micrometer-jetty12 module from micrometer-core (based on Jetty 9) and migrate the Jetty API accordingly.
https://eclipse.dev/jetty/documentation/jetty-12/programming-guide/index.html#pg-migration-11-to-12
Resolves #4609