Implement a Call.Factory for okhttp 3.x+ library instrumentation #3812
Conversation
jkwatson
requested review from
anuraaga,
iNikem,
laurit,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
...0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/TracingCallFactory.java
Outdated
Show resolved
Hide resolved
...0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/TracingCallFactory.java
Outdated
Show resolved
Hide resolved
| Request request = chain.request(); | ||
| Context parentContext = TracingCallFactory.getCallingContextForRequest(request); | ||
| if (parentContext == null) { | ||
| parentContext = Context.current(); |
There was a problem hiding this comment.
does this seem like the right default, or should we not try to propagate at all if we don't have one in the cache?
There was a problem hiding this comment.
Let's not propagate at all
There was a problem hiding this comment.
If we don't propagate, then interceptor-only users who have wired up a Dispatcher and only use synchronous calls will lose the context propagation, correct?
| response.body().withCloseable { | ||
| return response.code() | ||
| } |
There was a problem hiding this comment.
it's really important to close the response body always when using okhttp, otherwise we're leaking responses in our test code.
...ry/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpAttributesExtractor.java
Show resolved
Hide resolved
...tp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTracing.java
Outdated
Show resolved
Hide resolved
...tp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTracing.java
Outdated
Show resolved
Hide resolved
...0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/TracingCallFactory.java
Outdated
Show resolved
Hide resolved
...0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/TracingCallFactory.java
Outdated
Show resolved
Hide resolved
| Request request = chain.request(); | ||
| Context parentContext = TracingCallFactory.getCallingContextForRequest(request); | ||
| if (parentContext == null) { | ||
| parentContext = Context.current(); |
There was a problem hiding this comment.
Let's not propagate at all
| @Override | ||
| OkHttpClient.Builder configureClient(OkHttpClient.Builder clientBuilder) { | ||
| return clientBuilder | ||
| // The double "new Dispatcher" style is the simplest way to decorate the default executor. |
There was a problem hiding this comment.
Well it's also nice for this to be gone :)
| response.body().withCloseable { | ||
| return response.code() | ||
| } |
| return clientBuilder.build() | ||
| } | ||
|
|
||
| def "reused builder has one interceptor"() { |
|
For the record, we see a similar issue as aws sdk instrumentation, where muzzle is forcing reflection where we otherwise wouldn't need it. An annotation approach to solving this seems like a nice Splunk intern project :-) (guess it's too late this year...) |
This exposes a `Call.Factory` which will properly handle internal context propagation when used with async callbacks.
jkwatson
force-pushed
the
okhttp_3_library_context
branch
from
2675cdb
to
c1d65a2
Compare
|
One downside to this approach (not that there's a better one) is that libraries that only accept an actual OkHttpClient as a dependency won't be able to consume the Call.Factory and use it. I guess if users run into that, we can create issues in the relevant library and submit fixes. :) |
| @Nullable private static Method timeoutMethod; | ||
| @Nullable private static Method cloneMethod; |
There was a problem hiding this comment.
Can we use MethodHandles instead of reflection?
There was a problem hiding this comment.
I'll take a look. I've been meaning to learn how to use them, so this is a good chance!
There was a problem hiding this comment.
done. please take a look because I'm not sure how to test that it works without using reflection.
| @Override | ||
| boolean testCausalityWithCallback() { | ||
| false | ||
| } |
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
|
any idea what we need to do to get this thing to finish the checks? |
|
Closed and reopened -- hope they'll get triggered now. |
This now actually will handle async context connections correctly!