Skip to content
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

Add support for apache httpclient5 #2254

Merged
merged 4 commits into from
Feb 17, 2021

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Feb 11, 2021

Adds support for apache httpclient5 classic. Related to #2157 though the issue does not specify whether they are interested in class or async httpclient.

@anuraaga
Copy link
Contributor

Hi @laurit - can we start with library instrumentation for this? We should generally use the library's native support for instrumentation instead of intercepting execute where it's possible - so if we start with library instrumentation registering interceptors, I think we'll know how to then wire that into the javaagent code.

https://hc.apache.org/httpcomponents-client-5.0.x/httpclient5/xref-test/org/apache/hc/client5/http/examples/ClientInterceptors.html

@laurit
Copy link
Contributor Author

laurit commented Feb 12, 2021

@anuraaga This is an adaption of httpclient-4.0 instrumentation and works exactly the same way. The approach you suggested would work when http client is built by HttpClientBuilder. HttpClients.createMinimal https://github.com/apache/httpcomponents-client/blob/master/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpClients.java does not use HttpClientBuilder and might need a different approach. I won't have time to work on this in the near future. If you believe that this approach is unacceptable then let me know and I'll close this pr.

Copy link
Contributor

@iNikem iNikem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is exact adaption of the instrumentation for 4.0, then I am Ok with this. But I somewhat agree with @anuraaga sentiment, so will let him decide if we accept this without library instrumentation.

@anuraaga
Copy link
Contributor

I do want to discourage agent-only PRs in general to not box out non-agent users, but at the same time I feel very bad to delete working code. If @laurit doesn't have time for it though that's totally fine and I would recommend merging if this is just copy paste from our other instrumentation with package rename, let's not waste it.

FYI the biggest thing on my mind is it's 2021 - if a new HTTP library is released without a proper instrumentation hook (which may be the case given the comment about createMinimal) we probably need to get in touch with the authors of the code 🤷‍♂️

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a skim and I'm ok with this. @laurit just to be clear thanks for doing this, regardless of where this ends up long term it's for our users. I really appreciate it!

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also port #2276 to this new instrumentation?

@laurit laurit force-pushed the apache-httpclient-5 branch from b04c320 to a97b850 Compare February 15, 2021 11:03
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Override
public T handleResponse(ClassicHttpResponse response) throws IOException, HttpException {
tracer().end(context, response);
try (Scope ignored = parentContext.makeCurrent()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment added to #2276 just before merging (if this is still the same in httpclient5?):

Suggested change
try (Scope ignored = parentContext.makeCurrent()) {
// ending the span before executing the callback handler (and scoping the callback handler to
// the parent context), even though we are inside of a synchronous http client callback
// underneath HttpClient.execute(..), in order to not attribute other CLIENT span timings that
// may be performed in the callback handler to the http client span (and so we don't end up with
// nested CLIENT spans, which we currently suppress)
try (Scope ignored = parentContext.makeCurrent()) {

@laurit laurit force-pushed the apache-httpclient-5 branch from bb53fb3 to 8f96bbc Compare February 17, 2021 09:54
@iNikem iNikem merged commit 50cc8dd into open-telemetry:main Feb 17, 2021
@laurit laurit deleted the apache-httpclient-5 branch August 24, 2021 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants