Merge pull request #159 from LikeTheSalad/okhttp-autoinstrumentation

Okhttp autoinstrumentation stabilization

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

@@ -1,6 +1,6 @@
 # Android Instrumentation for OkHttp version 3.0 and higher
 
-# 🚧Work in progress 🚧
+## Status: Experimental
 
 Provides OpenTelemetry instrumentation for [okhttp3](https://square.github.io/okhttp/).
 

auto-instrumentation/okhttp/okhttp-3.0/agent/build.gradle.kts

@@ -1,10 +1,14 @@
 plugins {
-    id("otel.java-library-conventions")
+    id("otel.android-library-conventions")
     id("otel.publish-conventions")
 }
 
 description = "OpenTelemetry build-time auto-instrumentation for OkHttp on Android"
 
+android {
+    namespace = "io.opentelemetry.android.okhttp.agent"
+}
+
 dependencies {
     implementation(project(":auto-instrumentation:okhttp:okhttp-3.0:library"))
     implementation(libs.okhttp)

auto-instrumentation/okhttp/okhttp-3.0/library/build.gradle.kts

@@ -1,10 +1,18 @@
 plugins {
-    id("otel.java-library-conventions")
+    id("otel.android-library-conventions")
     id("otel.publish-conventions")
 }
 
 description = "OpenTelemetry OkHttp library instrumentation for Android"
 
+android {
+    namespace = "io.opentelemetry.android.okhttp.library"
+
+    defaultConfig {
+        consumerProguardFiles("consumer-rules.pro")
+    }
+}
+
 dependencies {
     compileOnly(libs.okhttp)
     api(libs.opentelemetry.instrumentation.okhttp)

auto-instrumentation/okhttp/okhttp-3.0/library/consumer-rules.pro

@@ -0,0 +1 @@
+-keep class io.opentelemetry.exporter.internal.InstrumentationUtil { *; }

auto-instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/library/okhttp/v3_0/internal/CachedSupplier.java

@@ -0,0 +1,40 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.library.okhttp.v3_0.internal;
+
+import java.util.function.Supplier;
+
+/**
+ * This class is internal and is hence not for public use. Its APIs are unstable and can change at
+ * any time.
+ */
+public class CachedSupplier<T> implements Supplier<T> {
+    private Supplier<T> supplier;
+    private T instance;
+    private final Object lock = new Object();
+
+    public static <T> CachedSupplier<T> create(Supplier<T> instance) {
+        return new CachedSupplier<>(instance);
+    }
+
+    private CachedSupplier(Supplier<T> supplier) {
+        this.supplier = supplier;
+    }
+
+    @Override
+    public T get() {
+        synchronized (lock) {
+            if (instance == null) {
+                instance = supplier.get();
+                if (instance == null) {
+                    throw new NullPointerException("Supplier provided null.");
+                }
+                supplier = null;
+            }
+            return instance;
+        }
+    }
+}

auto-instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/library/okhttp/v3_0/internal/LazyInterceptor.java

@@ -0,0 +1,29 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.library.okhttp.v3_0.internal;
+
+import java.io.IOException;
+import okhttp3.Interceptor;
+import okhttp3.Response;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * This class is internal and is hence not for public use. Its APIs are unstable and can change at
+ * any time.
+ */
+public class LazyInterceptor<T extends Interceptor> implements Interceptor {
+    private final CachedSupplier<T> interceptorSupplier;
+
+    public LazyInterceptor(CachedSupplier<T> interceptorSupplier) {
+        this.interceptorSupplier = interceptorSupplier;
+    }
+
+    @NotNull
+    @Override
+    public Response intercept(@NotNull Chain chain) throws IOException {
+        return interceptorSupplier.get().intercept(chain);
+    }
+}

auto-instrumentation/okhttp/okhttp-3.0/library/src/main/java/io/opentelemetry/instrumentation/library/okhttp/v3_0/internal/OkHttp3Singletons.java

@@ -18,6 +18,7 @@
 import io.opentelemetry.instrumentation.okhttp.v3_0.internal.OkHttpAttributesGetter;
 import io.opentelemetry.instrumentation.okhttp.v3_0.internal.OkHttpInstrumenterFactory;
 import io.opentelemetry.instrumentation.okhttp.v3_0.internal.TracingInterceptor;
+import java.util.function.Supplier;
 import okhttp3.Interceptor;
 import okhttp3.Request;
 import okhttp3.Response;
@@ -28,24 +29,31 @@
  */
 public final class OkHttp3Singletons {
 
-    private static final Instrumenter<Request, Response> INSTRUMENTER =
-            OkHttpInstrumenterFactory.create(
-                    GlobalOpenTelemetry.get(),
-                    builder ->
-                            builder.setCapturedRequestHeaders(
-                                            OkHttpInstrumentationConfig.getCapturedRequestHeaders())
-                                    .setCapturedResponseHeaders(
-                                            OkHttpInstrumentationConfig
-                                                    .getCapturedResponseHeaders())
-                                    .setKnownMethods(OkHttpInstrumentationConfig.getKnownMethods()),
-                    spanNameExtractorConfigurer ->
-                            spanNameExtractorConfigurer.setKnownMethods(
-                                    OkHttpInstrumentationConfig.getKnownMethods()),
-                    singletonList(
-                            PeerServiceAttributesExtractor.create(
-                                    OkHttpAttributesGetter.INSTANCE,
-                                    OkHttpInstrumentationConfig.newPeerServiceResolver())),
-                    OkHttpInstrumentationConfig.emitExperimentalHttpClientMetrics());
+    private static final Supplier<Instrumenter<Request, Response>> INSTRUMENTER =
+            CachedSupplier.create(
+                    () ->
+                            OkHttpInstrumenterFactory.create(
+                                    GlobalOpenTelemetry.get(),
+                                    builder ->
+                                            builder.setCapturedRequestHeaders(
+                                                            OkHttpInstrumentationConfig
+                                                                    .getCapturedRequestHeaders())
+                                                    .setCapturedResponseHeaders(
+                                                            OkHttpInstrumentationConfig
+                                                                    .getCapturedResponseHeaders())
+                                                    .setKnownMethods(
+                                                            OkHttpInstrumentationConfig
+                                                                    .getKnownMethods()),
+                                    spanNameExtractorConfigurer ->
+                                            spanNameExtractorConfigurer.setKnownMethods(
+                                                    OkHttpInstrumentationConfig.getKnownMethods()),
+                                    singletonList(
+                                            PeerServiceAttributesExtractor.create(
+                                                    OkHttpAttributesGetter.INSTANCE,
+                                                    OkHttpInstrumentationConfig
+                                                            .newPeerServiceResolver())),
+                                    OkHttpInstrumentationConfig
+                                            .emitExperimentalHttpClientMetrics()));
 
     public static final Interceptor CALLBACK_CONTEXT_INTERCEPTOR =
             chain -> {
@@ -70,10 +78,17 @@ public final class OkHttp3Singletons {
             };
 
     public static final Interceptor CONNECTION_ERROR_INTERCEPTOR =
-            new ConnectionErrorSpanInterceptor(INSTRUMENTER);
+            new LazyInterceptor<>(
+                    CachedSupplier.create(
+                            () -> new ConnectionErrorSpanInterceptor(INSTRUMENTER.get())));
 
     public static final Interceptor TRACING_INTERCEPTOR =
-            new TracingInterceptor(INSTRUMENTER, GlobalOpenTelemetry.getPropagators());
+            new LazyInterceptor<>(
+                    CachedSupplier.create(
+                            () ->
+                                    new TracingInterceptor(
+                                            INSTRUMENTER.get(),
+                                            GlobalOpenTelemetry.getPropagators())));
 
     private OkHttp3Singletons() {}
 }

auto-instrumentation/okhttp/okhttp-3.0/library/src/test/java/io/opentelemetry/instrumentation/library/okhttp/v3_0/internal/CachedSupplierTest.java

@@ -0,0 +1,47 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.library.okhttp.v3_0.internal;
+
+import static org.junit.jupiter.api.Assertions.fail;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+
+import java.util.function.Supplier;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+class CachedSupplierTest {
+
+    @Test
+    void provideAndCacheSuppliersResult() {
+        Supplier<String> original =
+                spy(
+                        new Supplier<String>() {
+                            @Override
+                            public String get() {
+                                return "Hello World";
+                            }
+                        });
+        CachedSupplier<String> cached = CachedSupplier.create(original);
+
+        for (int i = 0; i < 3; i++) {
+            Assertions.assertThat(cached.get()).isEqualTo("Hello World");
+        }
+        verify(original).get();
+    }
+
+    @Test
+    void validateSupplierDoesNotReturnNull() {
+        Supplier<String> original = () -> null;
+        CachedSupplier<String> cached = CachedSupplier.create(original);
+
+        try {
+            cached.get();
+            fail();
+        } catch (NullPointerException ignored) {
+        }
+    }
+}

auto-instrumentation/okhttp/okhttp-3.0/testing/build.gradle.kts

@@ -3,9 +3,20 @@ plugins {
     id("net.bytebuddy.byte-buddy-gradle-plugin")
 }
 
+android {
+    buildTypes {
+        debug {
+            isMinifyEnabled = true
+            proguardFile("proguard-rules.pro")
+            testProguardFile("proguard-test-rules.pro")
+        }
+    }
+}
+
 dependencies {
     byteBuddy(project(":auto-instrumentation:okhttp:okhttp-3.0:agent"))
     implementation(project(":auto-instrumentation:okhttp:okhttp-3.0:library"))
     implementation(libs.okhttp)
+    implementation(libs.opentelemetry.exporter.otlp)
     androidTestImplementation(libs.okhttp.mockwebserver)
 }

auto-instrumentation/okhttp/okhttp-3.0/testing/proguard-rules.pro

@@ -0,0 +1,7 @@
+-keep class kotlin.** { *; }
+-keep class okhttp3.** { *; }
+-keep class io.opentelemetry.api.** { *; }
+-keep class io.opentelemetry.sdk.** { *; }
+-keep class io.opentelemetry.exporter.otlp.http.trace.OtlpHttpSpanExporter { *; }
+-keep class io.opentelemetry.exporter.otlp.http.trace.OtlpHttpSpanExporterBuilder { *; }
+-dontwarn *

auto-instrumentation/okhttp/okhttp-3.0/testing/proguard-test-rules.pro

@@ -0,0 +1 @@
+-include proguard-rules.pro

auto-instrumentation/okhttp/okhttp-3.0/testing/src/androidTest/java/io/opentelemetry/instrumentation/library/okhttp/v3_0/InstrumentationTest.java

@@ -12,10 +12,12 @@
 import io.opentelemetry.api.trace.Span;
 import io.opentelemetry.api.trace.SpanContext;
 import io.opentelemetry.context.Scope;
+import io.opentelemetry.exporter.otlp.http.trace.OtlpHttpSpanExporter;
 import io.opentelemetry.sdk.OpenTelemetrySdk;
 import io.opentelemetry.sdk.testing.exporter.InMemorySpanExporter;
 import io.opentelemetry.sdk.trace.SdkTracerProvider;
 import io.opentelemetry.sdk.trace.export.SimpleSpanProcessor;
+import io.opentelemetry.sdk.trace.export.SpanExporter;
 import java.io.IOException;
 import java.util.concurrent.CountDownLatch;
 import okhttp3.Call;
@@ -27,17 +29,11 @@
 import okhttp3.mockwebserver.MockWebServer;
 import org.junit.After;
 import org.junit.Before;
-import org.junit.BeforeClass;
 import org.junit.Test;
 
 public class InstrumentationTest {
     private MockWebServer server;
-    private static InMemorySpanExporter spanExporter;
-
-    @BeforeClass
-    public static void setUpAll() {
-        setUpInMemorySpanExporter();
-    }
+    private static final InMemorySpanExporter inMemorySpanExporter = InMemorySpanExporter.create();
 
     @Before
     public void setUp() throws IOException {
@@ -48,11 +44,12 @@ public void setUp() throws IOException {
     @After
     public void tearDown() throws IOException {
         server.shutdown();
-        spanExporter.reset();
+        inMemorySpanExporter.reset();
     }
 
     @Test
     public void okhttpTraces() throws IOException {
+        setUpSpanExporter(inMemorySpanExporter);
         server.enqueue(new MockResponse().setResponseCode(200));
 
         Span span = getSpan();
@@ -74,11 +71,12 @@ public void okhttpTraces() throws IOException {
 
         span.end();
 
-        assertEquals(2, spanExporter.getFinishedSpanItems().size());
+        assertEquals(2, inMemorySpanExporter.getFinishedSpanItems().size());
     }
 
     @Test
     public void okhttpTraces_with_callback() throws InterruptedException {
+        setUpSpanExporter(inMemorySpanExporter);
         CountDownLatch lock = new CountDownLatch(1);
         Span span = getSpan();
 
@@ -117,18 +115,49 @@ public void onResponse(
         lock.await();
         span.end();
 
-        assertEquals(2, spanExporter.getFinishedSpanItems().size());
+        assertEquals(2, inMemorySpanExporter.getFinishedSpanItems().size());
+    }
+
+    @Test
+    public void avoidCreatingSpansForInternalOkhttpRequests() throws InterruptedException {
+        // NOTE: For some reason this test always passes when running all the tests in this file at
+        // once,
+        // so it should be run isolated to actually get it to fail when it's expected to fail.
+        OtlpHttpSpanExporter exporter =
+                OtlpHttpSpanExporter.builder().setEndpoint(server.url("").toString()).build();
+        setUpSpanExporter(exporter);
+
+        server.enqueue(new MockResponse().setResponseCode(200));
+
+        // This span should trigger 1 export okhttp call, which is the only okhttp call expected
+        // for this test case.
+        getSpan().end();
+
+        // Wait for unwanted extra okhttp requests.
+        int loop = 0;
+        while (loop < 10) {
+            Thread.sleep(100);
+            // Stop waiting if we get at least one unwanted request.
+            if (server.getRequestCount() > 1) {
+                break;
+            }
+            loop++;
+        }
+
+        assertEquals(1, server.getRequestCount());
     }
 
     private static Span getSpan() {
         return GlobalOpenTelemetry.getTracer("TestTracer").spanBuilder("A Span").startSpan();
     }
 
-    private static void setUpInMemorySpanExporter() {
-        spanExporter = InMemorySpanExporter.create();
-        OpenTelemetrySdk.builder()
-                .setTracerProvider(getSimpleTracerProvider(spanExporter))
-                .buildAndRegisterGlobal();
+    private void setUpSpanExporter(SpanExporter spanExporter) {
+        OpenTelemetrySdk openTelemetry =
+                OpenTelemetrySdk.builder()
+                        .setTracerProvider(getSimpleTracerProvider(spanExporter))
+                        .build();
+        GlobalOpenTelemetry.resetForTest();
+        GlobalOpenTelemetry.set(openTelemetry);
     }
 
     private Call createCall(OkHttpClient client, String urlPath) {
@@ -137,7 +166,7 @@ private Call createCall(OkHttpClient client, String urlPath) {
     }
 
     @NonNull
-    private static SdkTracerProvider getSimpleTracerProvider(InMemorySpanExporter spanExporter) {
+    private SdkTracerProvider getSimpleTracerProvider(SpanExporter spanExporter) {
         return SdkTracerProvider.builder()
                 .addSpanProcessor(SimpleSpanProcessor.create(spanExporter))
                 .build();