Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support Apache HttpAsyncClient "client received" #308

Merged

Conversation

robinst
Copy link
Contributor

@robinst robinst commented Dec 22, 2016

Before, BraveHttpResponseInterceptor would call
ClientResponseInterceptor and it would call setClientReceived. But it
would not submit the end annotation because the current client span was
null. This is because the response interceptor is not called on the same
thread as the request interceptor.

This can be solved by storing the client span as an attribute on the
HttpContext instead.

Before, BraveHttpResponseInterceptor would call
ClientResponseInterceptor and it would call setClientReceived. But it
would not submit the end annotation because the current client span was
null. This is because the response interceptor is not called on the same
thread as the request interceptor.

This can be solved by storing the client span as an attribute on the
HttpContext instead.
@codefromthecrypt
Copy link
Member

yep this will do it. Thanks for the help!

<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpasyncclient</artifactId>
<scope>test</scope>
Copy link
Member

Choose a reason for hiding this comment

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

build seems to break on a snapshot repo #309

wonder if it is actually only this PR. can you try adding the version either here as <version>${httpcomponents.version}</version> or doing the same in the root pom under dependency management?

@@ -188,6 +188,11 @@
<version>${httpcomponents.version}</version>
</dependency>
<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpasyncclient</artifactId>
<version>4.1.2</version>
Copy link
Member

Choose a reason for hiding this comment

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

actually can you switch to the same version pattern <version>${httpcomponents.version}</version> or is this unrelated to the other versions?

Copy link
Member

Choose a reason for hiding this comment

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

ahh I see it is unrelated. this might be obvious to those who know this library, but not intuitive. mind adding an XML comment that this has a different versioning scheme vs the other files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was also confused because I tried to use ${httpcomponents.version} first. I'll raise a PR tomorrow :).

@codefromthecrypt
Copy link
Member

nuked the build cache which might let #309 pass

@codefromthecrypt codefromthecrypt merged commit a4309cf into openzipkin:master Dec 22, 2016
@codefromthecrypt
Copy link
Member

thanks. point I mentioned wasn't important :P will be released in the next day or two, once #304 is done

@robinst robinst deleted the support-apache-async-client branch December 22, 2016 07:49
@robinst
Copy link
Contributor Author

robinst commented Dec 22, 2016

Awesome, thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants