-
Notifications
You must be signed in to change notification settings - Fork 979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Apache HTTP client instrumentation with Observation #3312
Apache HTTP client instrumentation with Observation #3312
Conversation
.../java/io/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpRequestExecutor.java
Outdated
Show resolved
Hide resolved
...eter/core/instrument/binder/httpcomponents/DefaultApacheHttpClientObservationConvention.java
Outdated
Show resolved
Hide resolved
.../java/io/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpRequestExecutor.java
Outdated
Show resolved
Hide resolved
.../java/io/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpRequestExecutor.java
Outdated
Show resolved
Hide resolved
...eter/core/instrument/binder/httpcomponents/DefaultApacheHttpClientObservationConvention.java
Show resolved
Hide resolved
.../java/io/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpRequestExecutor.java
Outdated
Show resolved
Hide resolved
.../java/io/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpRequestExecutor.java
Outdated
Show resolved
Hide resolved
b8fa4ca
to
de6229f
Compare
6b068fd
to
c6011b5
Compare
micrometer-core/src/main/java/io/micrometer/core/instrument/observation/ObservationOrTimer.java
Outdated
Show resolved
Hide resolved
Work-in-progress on adding Observation-based instrumentation to Apache HTTP client while being backwards compatible.
Adapts to breaking changes made in main, and updates the code to work with global conventions and allows custom conventions to be configured locally. Previously the default convention relied on accepting config that would not be available to third-part implementations. This has been moved to the context instead.
c6011b5
to
93166ee
Compare
4fcade7
to
3bf0bff
Compare
...c/main/java/io/micrometer/core/instrument/binder/httpcomponents/ApacheHttpClientContext.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public String getContextualName(ApacheHttpClientContext context) { | ||
return "HTTP " + getMethodString(context.getCarrier()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen this hard coded quite a few times - I wonder if we can extract this somehow, somewhere? 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since how to get the method will be different for each implementation I'm not sure we really can. This is exactly what we were trying to do with the HTTP abstractions for request/response, but we ended up going a different direction.
|
||
} | ||
|
||
// TODO add test for status = IO_ERROR case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to add a test for this, but I wasn't sure how. If someone knows how to test this scenario with the Apache HTTP client, let me know. The scenarios are when the response returned from execute
is null
and when calling execute
results in an IOException
Adds Observation-based instrumentation for the Apache HTTP client while being backwards compatible. Configuring an ObservationRegistry (that is not a no-op) causes the
Observation
instrumentation to be used instead of instrumentation directly withTimer
.