Add instrumentation for Apache HttpAsyncClient #809
Conversation
tylerbenson
force-pushed
the
tyler/httpasyncclient
branch
2 times, most recently
from
8ee0796
to
10c0656
Compare
Extract http client tests to shared class.
tylerbenson
force-pushed
the
tyler/httpasyncclient
branch
from
10c0656
to
9ad06a6
Compare
...java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientDecorator.java
Outdated
Show resolved
Hide resolved
...java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientDecorator.java
Show resolved
Hide resolved
...java/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientDecorator.java
Outdated
Show resolved
Hide resolved
tylerbenson
force-pushed
the
tyler/httpasyncclient
branch
from
7c5658a
to
0d61488
Compare
tylerbenson
force-pushed
the
tyler/httpasyncclient
branch
3 times, most recently
from
de00a1b
to
cb2eb87
Compare
Make handling of it more consistent in decorator.
tylerbenson
force-pushed
the
tyler/httpasyncclient
branch
from
cb2eb87
to
68a68f1
Compare
|
Making the URI handling consistent took more effort than I expected. Sorry for the big PR. |
| @@ -32,9 +36,39 @@ public Span onRequest(final Span span, final REQUEST request) { | |||
| assert span != null; | |||
| if (request != null) { | |||
| Tags.HTTP_METHOD.set(span, method(request)); | |||
| Tags.HTTP_URL.set(span, url(request)); | |||
|
|
|||
| // Copy of HttpServerDecorator url handling | |||
There was a problem hiding this comment.
[Non-blocking] Just created a task for the future as a reminder to remove this code duplication in both classes. This does not represent a blocker for me at the moment, just wanted to keep track of it.
...ava-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/HttpClientDecorator.java
Show resolved
Hide resolved
| @@ -42,23 +43,31 @@ public Span onRequest(final Span span, final REQUEST request) { | |||
| if (request != null) { | |||
| Tags.HTTP_METHOD.set(span, method(request)); | |||
|
|
|||
| // Copy of HttpClientDecorator url handling | |||
There was a problem hiding this comment.
[Non-blocking] I just noted that we added this here (I thought it already existed). Created a task to remove this code duplication (pairing it with the client class).
Other than dev speed any other reason why we had to duplicate?
There was a problem hiding this comment.
Dev speed and I didn’t see an obvious place to put the shared code. Perhaps in a util class?
There was a problem hiding this comment.
This may be a good place. I think that we may need it over and over in our instrumentations.
There was a problem hiding this comment.
Part of the reason I put it here is so we hopefully don't need it in all the instrumentation. The child decorators and each instrumentation need just return a URI, then this class should handle the special casing.
| @@ -281,7 +275,7 @@ class AwsClientTest extends AgentTestRunner { | |||
| childOf(span(0)) | |||
| tags { | |||
| "$Tags.COMPONENT.key" "apache-httpclient" | |||
| "$Tags.HTTP_URL.key" "http://localhost:$server.address.port/someBucket/someKey" | |||
| "$Tags.HTTP_URL.key" "$server.address/someBucket/someKey" | |||
There was a problem hiding this comment.
This potentially may be a breaking change for our users (say they had set an alert or monitor). What is commonly our approach to this?
There was a problem hiding this comment.
This particular line does not represent an actual change in the value. The main change in this PR for some integrations is forcing a path of / instead of empty for the URL.
Extract http client tests to shared class.