Implement a Call.Factory for okhttp 3.x+ library instrumentation #3812

jkwatson · 2021-08-10T18:42:57Z

This now actually will handle async context connections correctly!

...0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/TracingCallFactory.java

jkwatson · 2021-08-10T18:46:47Z

...0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/TracingInterceptor.java

    Request request = chain.request();
+    Context parentContext = TracingCallFactory.getCallingContextForRequest(request);
+    if (parentContext == null) {
+      parentContext = Context.current();


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?

Let's not propagate at all

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?

jkwatson · 2021-08-10T18:47:39Z

...ting/src/main/groovy/io/opentelemetry/instrumentation/okhttp/v3_0/AbstractOkHttp3Test.groovy

+    response.body().withCloseable {
+      return response.code()
+    }


it's really important to close the response body always when using okhttp, otherwise we're leaking responses in our test code.

instrumentation/okhttp/okhttp-3.0/library/README.md

...ry/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpAttributesExtractor.java

...tp-3.0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttpTracing.java

...0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/TracingCallFactory.java

anuraaga · 2021-08-11T07:02:57Z

...0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/TracingInterceptor.java

    Request request = chain.request();
+    Context parentContext = TracingCallFactory.getCallingContextForRequest(request);
+    if (parentContext == null) {
+      parentContext = Context.current();


Let's not propagate at all

anuraaga · 2021-08-11T07:03:14Z

...-3.0/library/src/test/groovy/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttp3Test.groovy

  @Override
-  OkHttpClient.Builder configureClient(OkHttpClient.Builder clientBuilder) {
-    return clientBuilder
-      // The double "new Dispatcher" style is the simplest way to decorate the default executor.


Well it's also nice for this to be gone :)

anuraaga · 2021-08-11T07:03:34Z

...ting/src/main/groovy/io/opentelemetry/instrumentation/okhttp/v3_0/AbstractOkHttp3Test.groovy

+    response.body().withCloseable {
+      return response.code()
+    }


anuraaga · 2021-08-11T07:04:21Z

...nt/src/test/groovy/io/opentelemetry/javaagent/instrumentation/okhttp/v3_0/OkHttp3Test.groovy

+    return clientBuilder.build()
+  }
+
+  def "reused builder has one interceptor"() {


anuraaga · 2021-08-11T07:05:59Z

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.

…tation to work

jkwatson · 2021-08-11T18:11:11Z

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. :)

mateuszrzeszutek · 2021-08-12T11:00:00Z

...0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/TracingCallFactory.java

+  @Nullable private static Method timeoutMethod;
+  @Nullable private static Method cloneMethod;


Can we use MethodHandles instead of reflection?

I'll take a look. I've been meaning to learn how to use them, so this is a good chance!

done. please take a look because I'm not sure how to test that it works without using reflection.

instrumentation/okhttp/okhttp-3.0/library/README.md

trask · 2021-08-14T03:42:22Z

...-3.0/library/src/test/groovy/io/opentelemetry/instrumentation/okhttp/v3_0/OkHttp3Test.groovy

-  @Override
-  boolean testCausalityWithCallback() {
-    false
-  }


Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

jkwatson · 2021-08-17T15:31:50Z

any idea what we need to do to get this thing to finish the checks?

mateuszrzeszutek · 2021-08-17T15:34:37Z

Closed and reopened -- hope they'll get triggered now.

jkwatson requested review from anuraaga, iNikem, laurit, mateuszrzeszutek, pavolloffay, trask and tylerbenson as code owners August 10, 2021 18:42

jkwatson mentioned this pull request Aug 10, 2021

Add new library instrumentation for okhttp3 4.x+ #3780

Closed

jkwatson commented Aug 10, 2021

View reviewed changes

...0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/TracingCallFactory.java Outdated Show resolved Hide resolved

jkwatson commented Aug 10, 2021

View reviewed changes

...0/library/src/main/java/io/opentelemetry/instrumentation/okhttp/v3_0/TracingCallFactory.java Outdated Show resolved Hide resolved

jkwatson commented Aug 10, 2021

View reviewed changes

anuraaga reviewed Aug 11, 2021

View reviewed changes

jkwatson added 5 commits August 11, 2021 09:38

Add a README for the okhttp library instrumentation.

3991690

Create new instrumentation for okhttp3 4.x+

126bc4a

This exposes a `Call.Factory` which will properly handle internal context propagation when used with async callbacks.

update the "4.x" instrumentation to support 3.x

8497876

Get rid of the 4.x instrumentation, and just update the 3.x instrumen…

1d194df

…tation to work

updates from PR review

c1d65a2

jkwatson force-pushed the okhttp_3_library_context branch from 2675cdb to c1d65a2 Compare August 11, 2021 17:13

anuraaga approved these changes Aug 12, 2021

View reviewed changes

mateuszrzeszutek approved these changes Aug 12, 2021

View reviewed changes

replace old reflection with method handle usage

b2a7208

trask approved these changes Aug 14, 2021

View reviewed changes

Apply suggestions from code review

6a07679

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

mateuszrzeszutek closed this Aug 17, 2021

mateuszrzeszutek reopened this Aug 17, 2021

anuraaga merged commit 6dbb64e into open-telemetry:main Aug 18, 2021

jkwatson deleted the okhttp_3_library_context branch August 18, 2021 03:58

laurit mentioned this pull request Aug 20, 2021

OkHttp library instrumentation does not pass concurrency test #3671

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a Call.Factory for okhttp 3.x+ library instrumentation #3812

Implement a Call.Factory for okhttp 3.x+ library instrumentation #3812

jkwatson commented Aug 10, 2021

jkwatson Aug 10, 2021

anuraaga Aug 11, 2021

jkwatson Aug 11, 2021

anuraaga Aug 12, 2021

jkwatson Aug 10, 2021

anuraaga Aug 11, 2021

anuraaga Aug 11, 2021

anuraaga Aug 11, 2021

anuraaga Aug 11, 2021

anuraaga Aug 11, 2021

anuraaga commented Aug 11, 2021

jkwatson commented Aug 11, 2021

mateuszrzeszutek Aug 12, 2021

jkwatson Aug 12, 2021

jkwatson Aug 12, 2021

trask Aug 14, 2021

jkwatson commented Aug 17, 2021

mateuszrzeszutek commented Aug 17, 2021

		@Nullable private static Method timeoutMethod;
		@Nullable private static Method cloneMethod;

Implement a Call.Factory for okhttp 3.x+ library instrumentation #3812

Implement a Call.Factory for okhttp 3.x+ library instrumentation #3812

Conversation

jkwatson commented Aug 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga commented Aug 11, 2021

jkwatson commented Aug 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson commented Aug 17, 2021

mateuszrzeszutek commented Aug 17, 2021