From e5e3f6b0dcbd8540334a857aebfe0eba52167053 Mon Sep 17 00:00:00 2001 From: Surbhi Agarwal Date: Mon, 29 Jul 2024 13:00:11 -0700 Subject: [PATCH 1/2] Update androidTests to make them compatible for parallel running --- .../internal/HttpUrlConnectionSingletons.java | 48 ++++++++++++++----- .../httpurlconnection/InstrumentationTest.kt | 38 +++++++-------- .../okhttp/v3_0/InstrumentationTest.java | 14 +++--- .../test/common/OpenTelemetryTestUtils.kt | 4 +- 4 files changed, 63 insertions(+), 41 deletions(-) diff --git a/instrumentation/httpurlconnection/library/src/main/java/io/opentelemetry/instrumentation/library/httpurlconnection/internal/HttpUrlConnectionSingletons.java b/instrumentation/httpurlconnection/library/src/main/java/io/opentelemetry/instrumentation/library/httpurlconnection/internal/HttpUrlConnectionSingletons.java index af9054348..10cca95d9 100644 --- a/instrumentation/httpurlconnection/library/src/main/java/io/opentelemetry/instrumentation/library/httpurlconnection/internal/HttpUrlConnectionSingletons.java +++ b/instrumentation/httpurlconnection/library/src/main/java/io/opentelemetry/instrumentation/library/httpurlconnection/internal/HttpUrlConnectionSingletons.java @@ -23,11 +23,17 @@ public final class HttpUrlConnectionSingletons { - private static volatile Instrumenter instrumenter; private static final String INSTRUMENTATION_NAME = "io.opentelemetry.android.http-url-connection"; - private static final Object lock = new Object(); + private static OpenTelemetry openTelemetryInstance; + private static volatile Instrumenter instrumenter; + private static final Object lock = new Object(); + + // ThreadLocal instances for parallel test execution. + private static ThreadLocal testOpentelemetryInstance = new ThreadLocal<>(); + private static ThreadLocal> testInstrumenter = + new ThreadLocal<>(); public static Instrumenter createInstrumenter( OpenTelemetry opentelemetry) { @@ -53,11 +59,9 @@ public static Instrumenter createInstrumenter( httpAttributesGetter, HttpUrlInstrumentationConfig.newPeerServiceResolver()); - openTelemetryInstance = (opentelemetry == null) ? GlobalOpenTelemetry.get() : opentelemetry; - InstrumenterBuilder builder = Instrumenter.builder( - openTelemetryInstance, + opentelemetry, INSTRUMENTATION_NAME, httpSpanNameExtractorBuilder.build()) .setSpanStatusExtractor( @@ -76,23 +80,41 @@ public static Instrumenter createInstrumenter( } public static Instrumenter instrumenter() { - if (instrumenter == null) { - synchronized (lock) { - if (instrumenter == null) { - instrumenter = createInstrumenter(null); + Instrumenter testInstrumenter = + HttpUrlConnectionSingletons.testInstrumenter.get(); + if (testInstrumenter != null) { + return testInstrumenter; + } else { + if (instrumenter == null) { + synchronized (lock) { + if (instrumenter == null) { + openTelemetryInstance = GlobalOpenTelemetry.get(); + instrumenter = createInstrumenter(openTelemetryInstance); + } } } + return instrumenter; } - return instrumenter; } public static OpenTelemetry openTelemetryInstance() { - return openTelemetryInstance; + OpenTelemetry testOpenTelemetry = testOpentelemetryInstance.get(); + if (testOpenTelemetry != null) { + return testOpenTelemetry; + } else { + return openTelemetryInstance; + } } // Used for setting the instrumenter for testing purposes only. - public static void setInstrumenterForTesting(OpenTelemetry opentelemetry) { - instrumenter = createInstrumenter(opentelemetry); + public static void setThreadLocalInstrumenterForTesting(OpenTelemetry opentelemetry) { + testOpentelemetryInstance.set(opentelemetry); + testInstrumenter.set(createInstrumenter(opentelemetry)); + } + + public static void removeThreadLocalInstrumenterForTesting() { + testOpentelemetryInstance.remove(); + testInstrumenter.remove(); } private HttpUrlConnectionSingletons() {} diff --git a/instrumentation/httpurlconnection/testing/src/androidTest/kotlin/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.kt b/instrumentation/httpurlconnection/testing/src/androidTest/kotlin/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.kt index 0cfa4ab09..0203b8328 100644 --- a/instrumentation/httpurlconnection/testing/src/androidTest/kotlin/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.kt +++ b/instrumentation/httpurlconnection/testing/src/androidTest/kotlin/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.kt @@ -10,52 +10,56 @@ import io.opentelemetry.instrumentation.library.httpurlconnection.HttpUrlConnect import io.opentelemetry.instrumentation.library.httpurlconnection.HttpUrlConnectionTestUtil.post import io.opentelemetry.instrumentation.library.httpurlconnection.internal.HttpUrlConnectionSingletons import io.opentelemetry.sdk.testing.exporter.InMemorySpanExporter +import org.junit.After import org.junit.Assert +import org.junit.Before import org.junit.Test import java.util.concurrent.Executors import java.util.concurrent.TimeUnit class InstrumentationTest { + private lateinit var inMemorySpanExporter: InMemorySpanExporter + private lateinit var openTelemetryTestUtils: OpenTelemetryTestUtils + + @Before + fun setUp() { + inMemorySpanExporter = InMemorySpanExporter.create() + openTelemetryTestUtils = OpenTelemetryTestUtils() + HttpUrlConnectionSingletons.setThreadLocalInstrumenterForTesting(openTelemetryTestUtils.setUpSpanExporter(inMemorySpanExporter)) + } + + @After + fun tearDown() { + inMemorySpanExporter.shutdown() + HttpUrlConnectionSingletons.removeThreadLocalInstrumenterForTesting() + } + @Test fun testHttpUrlConnectionGetRequest_ShouldBeTraced() { - val inMemorySpanExporter = InMemorySpanExporter.create() - HttpUrlConnectionSingletons.setInstrumenterForTesting(OpenTelemetryTestUtils.setUpSpanExporter(inMemorySpanExporter)) executeGet("http://httpbin.org/get") Assert.assertEquals(1, inMemorySpanExporter.finishedSpanItems.size) - inMemorySpanExporter.shutdown() } @Test fun testHttpUrlConnectionPostRequest_ShouldBeTraced() { - val inMemorySpanExporter = InMemorySpanExporter.create() - HttpUrlConnectionSingletons.setInstrumenterForTesting(OpenTelemetryTestUtils.setUpSpanExporter(inMemorySpanExporter)) post("http://httpbin.org/post") Assert.assertEquals(1, inMemorySpanExporter.finishedSpanItems.size) - inMemorySpanExporter.shutdown() } @Test fun testHttpUrlConnectionGetRequest_WhenNoStreamFetchedAndNoDisconnectCalled_ShouldNotBeTraced() { - val inMemorySpanExporter = InMemorySpanExporter.create() - HttpUrlConnectionSingletons.setInstrumenterForTesting(OpenTelemetryTestUtils.setUpSpanExporter(inMemorySpanExporter)) executeGet("http://httpbin.org/get", false, false) Assert.assertEquals(0, inMemorySpanExporter.finishedSpanItems.size) - inMemorySpanExporter.shutdown() } @Test fun testHttpUrlConnectionGetRequest_WhenNoStreamFetchedButDisconnectCalled_ShouldBeTraced() { - val inMemorySpanExporter = InMemorySpanExporter.create() - HttpUrlConnectionSingletons.setInstrumenterForTesting(OpenTelemetryTestUtils.setUpSpanExporter(inMemorySpanExporter)) executeGet("http://httpbin.org/get", false) Assert.assertEquals(1, inMemorySpanExporter.finishedSpanItems.size) - inMemorySpanExporter.shutdown() } @Test fun testHttpUrlConnectionGetRequest_WhenFourConcurrentRequestsAreMade_AllShouldBeTraced() { - val inMemorySpanExporter = InMemorySpanExporter.create() - HttpUrlConnectionSingletons.setInstrumenterForTesting(OpenTelemetryTestUtils.setUpSpanExporter(inMemorySpanExporter)) val executor = Executors.newFixedThreadPool(4) try { executor.submit { executeGet("http://httpbin.org/get") } @@ -83,15 +87,12 @@ class InstrumentationTest { if (!executor.isShutdown) { executor.shutdownNow() } - inMemorySpanExporter.shutdown() } } @Test fun testHttpUrlConnectionRequest_ContextPropagationHappensAsExpected() { - val inMemorySpanExporter = InMemorySpanExporter.create() - HttpUrlConnectionSingletons.setInstrumenterForTesting(OpenTelemetryTestUtils.setUpSpanExporter(inMemorySpanExporter)) - val parentSpan = OpenTelemetryTestUtils.getSpan() + val parentSpan = openTelemetryTestUtils.getSpan() parentSpan.makeCurrent().use { executeGet("http://httpbin.org/get") @@ -107,6 +108,5 @@ class InstrumentationTest { parentSpan.end() Assert.assertEquals(2, inMemorySpanExporter.finishedSpanItems.size) - inMemorySpanExporter.shutdown() } } diff --git a/instrumentation/okhttp/okhttp-3.0/testing/src/androidTest/java/io/opentelemetry/instrumentation/library/okhttp/v3_0/InstrumentationTest.java b/instrumentation/okhttp/okhttp-3.0/testing/src/androidTest/java/io/opentelemetry/instrumentation/library/okhttp/v3_0/InstrumentationTest.java index d50d015a4..e8456689a 100644 --- a/instrumentation/okhttp/okhttp-3.0/testing/src/androidTest/java/io/opentelemetry/instrumentation/library/okhttp/v3_0/InstrumentationTest.java +++ b/instrumentation/okhttp/okhttp-3.0/testing/src/androidTest/java/io/opentelemetry/instrumentation/library/okhttp/v3_0/InstrumentationTest.java @@ -30,6 +30,8 @@ public class InstrumentationTest { private MockWebServer server; private static final InMemorySpanExporter inMemorySpanExporter = InMemorySpanExporter.create(); + private static final OpenTelemetryTestUtils openTelemetryTestUtils = + new OpenTelemetryTestUtils(); @Before public void setUp() throws IOException { @@ -45,10 +47,10 @@ public void tearDown() throws IOException { @Test public void okhttpTraces() throws IOException { - OpenTelemetryTestUtils.setUpSpanExporter(inMemorySpanExporter); + openTelemetryTestUtils.setUpSpanExporter(inMemorySpanExporter); server.enqueue(new MockResponse().setResponseCode(200)); - Span span = OpenTelemetryTestUtils.getSpan(); + Span span = openTelemetryTestUtils.getSpan(); try (Scope ignored = span.makeCurrent()) { OkHttpClient client = @@ -72,9 +74,9 @@ public void okhttpTraces() throws IOException { @Test public void okhttpTraces_with_callback() throws InterruptedException { - OpenTelemetryTestUtils.setUpSpanExporter(inMemorySpanExporter); + openTelemetryTestUtils.setUpSpanExporter(inMemorySpanExporter); CountDownLatch lock = new CountDownLatch(1); - Span span = OpenTelemetryTestUtils.getSpan(); + Span span = openTelemetryTestUtils.getSpan(); try (Scope ignored = span.makeCurrent()) { server.enqueue(new MockResponse().setResponseCode(200)); @@ -121,13 +123,13 @@ public void avoidCreatingSpansForInternalOkhttpRequests() throws InterruptedExce // 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(); - OpenTelemetryTestUtils.setUpSpanExporter(exporter); + openTelemetryTestUtils.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. - OpenTelemetryTestUtils.getSpan().end(); + openTelemetryTestUtils.getSpan().end(); // Wait for unwanted extra okhttp requests. int loop = 0; diff --git a/test-common/src/main/kotlin/io/opentelemetry/android/test/common/OpenTelemetryTestUtils.kt b/test-common/src/main/kotlin/io/opentelemetry/android/test/common/OpenTelemetryTestUtils.kt index 6d95cd49b..52ce5d2b3 100644 --- a/test-common/src/main/kotlin/io/opentelemetry/android/test/common/OpenTelemetryTestUtils.kt +++ b/test-common/src/main/kotlin/io/opentelemetry/android/test/common/OpenTelemetryTestUtils.kt @@ -13,15 +13,13 @@ import io.opentelemetry.sdk.trace.SdkTracerProvider import io.opentelemetry.sdk.trace.export.SimpleSpanProcessor import io.opentelemetry.sdk.trace.export.SpanExporter -object OpenTelemetryTestUtils { +class OpenTelemetryTestUtils { lateinit var openTelemetry: OpenTelemetry - @JvmStatic fun getSpan(): Span { return openTelemetry.getTracer("TestTracer").spanBuilder("A Span").startSpan() } - @JvmStatic fun setUpSpanExporter(spanExporter: SpanExporter): OpenTelemetry { openTelemetry = OpenTelemetrySdk.builder() From 11008b4a6a8116fd1832ab9696f9ac2d68c11f11 Mon Sep 17 00:00:00 2001 From: Surbhi Agarwal Date: Mon, 29 Jul 2024 16:21:51 -0700 Subject: [PATCH 2/2] Replace Asserts with fluent assertJ style --- .../testing/build.gradle.kts | 1 + .../httpurlconnection/InstrumentationTest.kt | 24 +++++++++---------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/instrumentation/httpurlconnection/testing/build.gradle.kts b/instrumentation/httpurlconnection/testing/build.gradle.kts index 42c52ee01..e5170f2b8 100644 --- a/instrumentation/httpurlconnection/testing/build.gradle.kts +++ b/instrumentation/httpurlconnection/testing/build.gradle.kts @@ -7,4 +7,5 @@ dependencies { byteBuddy(project(":instrumentation:httpurlconnection:agent")) implementation(project(":instrumentation:httpurlconnection:library")) implementation(project(":test-common")) + androidTestImplementation(libs.assertj.core) } diff --git a/instrumentation/httpurlconnection/testing/src/androidTest/kotlin/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.kt b/instrumentation/httpurlconnection/testing/src/androidTest/kotlin/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.kt index 0203b8328..1fe724f29 100644 --- a/instrumentation/httpurlconnection/testing/src/androidTest/kotlin/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.kt +++ b/instrumentation/httpurlconnection/testing/src/androidTest/kotlin/io/opentelemetry/instrumentation/library/httpurlconnection/InstrumentationTest.kt @@ -10,8 +10,9 @@ import io.opentelemetry.instrumentation.library.httpurlconnection.HttpUrlConnect import io.opentelemetry.instrumentation.library.httpurlconnection.HttpUrlConnectionTestUtil.post import io.opentelemetry.instrumentation.library.httpurlconnection.internal.HttpUrlConnectionSingletons import io.opentelemetry.sdk.testing.exporter.InMemorySpanExporter +import org.assertj.core.api.Assertions.assertThat +import org.assertj.core.api.Assertions.fail import org.junit.After -import org.junit.Assert import org.junit.Before import org.junit.Test import java.util.concurrent.Executors @@ -37,25 +38,25 @@ class InstrumentationTest { @Test fun testHttpUrlConnectionGetRequest_ShouldBeTraced() { executeGet("http://httpbin.org/get") - Assert.assertEquals(1, inMemorySpanExporter.finishedSpanItems.size) + assertThat(inMemorySpanExporter.finishedSpanItems.size).isEqualTo(1) } @Test fun testHttpUrlConnectionPostRequest_ShouldBeTraced() { post("http://httpbin.org/post") - Assert.assertEquals(1, inMemorySpanExporter.finishedSpanItems.size) + assertThat(inMemorySpanExporter.finishedSpanItems.size).isEqualTo(1) } @Test fun testHttpUrlConnectionGetRequest_WhenNoStreamFetchedAndNoDisconnectCalled_ShouldNotBeTraced() { executeGet("http://httpbin.org/get", false, false) - Assert.assertEquals(0, inMemorySpanExporter.finishedSpanItems.size) + assertThat(inMemorySpanExporter.finishedSpanItems.size).isEqualTo(0) } @Test fun testHttpUrlConnectionGetRequest_WhenNoStreamFetchedButDisconnectCalled_ShouldBeTraced() { executeGet("http://httpbin.org/get", false) - Assert.assertEquals(1, inMemorySpanExporter.finishedSpanItems.size) + assertThat(inMemorySpanExporter.finishedSpanItems.size).isEqualTo(1) } @Test @@ -71,10 +72,10 @@ class InstrumentationTest { // Wait for all tasks to finish execution or timeout if (executor.awaitTermination(2, TimeUnit.SECONDS)) { // if all tasks finish before timeout - Assert.assertEquals(4, inMemorySpanExporter.finishedSpanItems.size) + assertThat(inMemorySpanExporter.finishedSpanItems.size).isEqualTo(4) } else { // if all tasks don't finish before timeout - Assert.fail( + fail( "Test could not be completed as tasks did not complete within the 2s timeout period.", ) } @@ -82,7 +83,7 @@ class InstrumentationTest { // print stack trace to decipher lines that threw InterruptedException as it can be // possibly thrown by multiple calls above. e.printStackTrace() - Assert.fail("Test could not be completed due to an interrupted exception.") + fail("Test could not be completed due to an interrupted exception.") } finally { if (!executor.isShutdown) { executor.shutdownNow() @@ -99,14 +100,11 @@ class InstrumentationTest { val spanDataList = inMemorySpanExporter.finishedSpanItems if (spanDataList.isNotEmpty()) { val currentSpanData = spanDataList[0] - Assert.assertEquals( - parentSpan.spanContext.traceId, - currentSpanData.traceId, - ) + assertThat(currentSpanData.traceId).isEqualTo(parentSpan.spanContext.traceId) } } parentSpan.end() - Assert.assertEquals(2, inMemorySpanExporter.finishedSpanItems.size) + assertThat(inMemorySpanExporter.finishedSpanItems.size).isEqualTo(2) } }