-
Notifications
You must be signed in to change notification settings - Fork 791
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
Refactor HttpClient typed Decorators to Tracers #893
Conversation
Need help with this CircleCI Muzzle validation failure: org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':instrumentation:grizzly-client-1.9:muzzle-AssertPass-com.ning-async-http-client-1.9.29'. I renamed all the classes under grizzly-client-1.9. |
.../opentelemetry/auto/instrumentation/grizzly/client/GrizzlyClientResponseInstrumentation.java
Outdated
Show resolved
Hide resolved
This test failure is not reproducible locally. Any ideas? |
this one fails a ton, see #430, we even had a meeting yesterday to discuss it 😄, working towards resolving... |
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.
thx @heyams!
...ain/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/DatabaseClientTracer.java
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/auto/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java
Outdated
Show resolved
Hide resolved
...a/io/opentelemetry/auto/instrumentation/netty/v3_8/ChannelFutureListenerInstrumentation.java
Outdated
Show resolved
Hide resolved
...o/opentelemetry/auto/instrumentation/netty/v3_8/client/HttpClientResponseTracingHandler.java
Outdated
Show resolved
Hide resolved
...a/io/opentelemetry/auto/instrumentation/netty/v4_1/ChannelFutureListenerInstrumentation.java
Outdated
Show resolved
Hide resolved
...-2.2/src/main/java/io/opentelemetry/auto/instrumentation/okhttp/v2_2/TracingInterceptor.java
Show resolved
Hide resolved
...-3.0/src/main/java/io/opentelemetry/auto/instrumentation/okhttp/v3_0/OkHttpClientTracer.java
Show resolved
Hide resolved
...-3.0/src/main/java/io/opentelemetry/auto/instrumentation/okhttp/v3_0/TracingInterceptor.java
Outdated
Show resolved
Hide resolved
...io/opentelemetry/auto/instrumentation/netty/v4_1/client/HttpClientRequestTracingHandler.java
Outdated
Show resolved
Hide resolved
...io/opentelemetry/auto/instrumentation/netty/v4_0/client/HttpClientRequestTracingHandler.java
Outdated
Show resolved
Hide resolved
...io/opentelemetry/auto/instrumentation/netty/v3_8/client/HttpClientRequestTracingHandler.java
Outdated
Show resolved
Hide resolved
...trap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/BaseTracer.java
Outdated
Show resolved
Hide resolved
...n/java/io/opentelemetry/auto/instrumentation/rabbitmq/amqp/RabbitChannelInstrumentation.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/auto/instrumentation/httpurlconnection/UrlInstrumentation.java
Outdated
Show resolved
Hide resolved
...ap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/ClientTracer.java
Outdated
Show resolved
Hide resolved
d24c82b
to
8366c3b
Compare
...a/io/opentelemetry/auto/instrumentation/netty/v3_8/ChannelFutureListenerInstrumentation.java
Show resolved
Hide resolved
…/auto/instrumentation/netty/v3_8/ChannelFutureListenerInstrumentation.java
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.
Looks good!
We aren't currently checking everywhere if the span returned from TRACER.startSpan()
is invalid before starting a scope with it. I'll open a tracking issue for that, I think it's better in a follow-up anyways.
...rc/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpClientTracer.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpClientTracer.java
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpClientTracer.java
Show resolved
Hide resolved
.../main/java/io/opentelemetry/auto/instrumentation/akkahttp/AkkaHttpClientInstrumentation.java
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpClientTracer.java
Show resolved
Hide resolved
...trap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/BaseTracer.java
Outdated
Show resolved
Hide resolved
...trap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/BaseTracer.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpClientTracer.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpClientTracer.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpClientTracer.java
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpClientTracer.java
Show resolved
Hide resolved
...rc/main/java/io/opentelemetry/auto/instrumentation/httpurlconnection/UrlInstrumentation.java
Show resolved
Hide resolved
I have review only part of all changed files. This PR is a great step forward in the right direction, thank you @heyams for doing it. But there is quite a lot of common code between existing Tracers from the "server branch" and ones introduced here. We have to consolidate that into proper hierarchy. Now, the question is should we do that consolidation in this PR or in a follow up. I have a strong opinion about the end result should look like and I will enjoy cleaning up the hierarchy. So I will be happy to do that follow up job. But if you, @heyams , want to play with it, be my guest and do it right here :) |
...rc/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpClientTracer.java
Show resolved
Hide resolved
...-10.0/src/main/java/io/opentelemetry/auto/instrumentation/akkahttp/AkkaHttpClientTracer.java
Show resolved
Hide resolved
...in/java/io/opentelemetry/auto/instrumentation/httpurlconnection/HttpUrlConnectionTracer.java
Show resolved
Hide resolved
I can refactor it further in another follow up PR. My goal is to finish all the decorators in a 2nd PR, then refactor the common code between server and client. I can continue with this path since I am familiar with the code now. Good point. Created #912 to track this. |
…opentelemetry-java-instrumentation into heya/http-client-tracers
...rc/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpClientTracer.java
Outdated
Show resolved
Hide resolved
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.
Thanks!
🎉 |
* initial work * Change OkHttp decorators to tracers * Change AkkaHttpClient decorator to tracer * Change ClientDecorator to GrizzlyClientTracer * Change NettyClientDecorator to Tracer * Change netty3.8 client decorator to tracer * Update netty4.1 client decorator to tracer * Override startScope in child tracers when applicable * Change KHttpDecorator to tracer * Change HttpUrlConnectionDecorator to tracer * Fix muzzle validation failure for grizzly client * Fix a ratpack client test failure * Address feedback * Update delegate for setPeer * Remove nested try * Remove unnecessary null check * Remove asserts * Add a comment for overriding startSpan(String spanName) * Remove an irrelevant comment * Throw an exception when getSetter is null * Change onPeerConnection to static * Change getSetter() to be abstract
Fix #828
This is the 1st PR and the rest will be in the follow-up PR. Try to divide changes into smaller PRs.