Fix azure-core nested HTTP suppression, update libs (#12489)

Co-authored-by: Trask Stalnaker <[email protected]>
open-telemetry · Oct 26, 2024 · d3f808e · d3f808e
1 parent f1c75fa
commit d3f808e
Show file tree

Hide file tree

Showing 6 changed files with 140 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+- Update azure-core-tracing-opentelemetry version and improve HTTP suppression to back off
+  when Azure SDK tracing was disabled.
+  ([#12489](https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/12489))
+
 ## Version 2.9.0 (2024-10-17)
 
 ### 📈 Enhancements

diff --git a/instrumentation/azure-core/azure-core-1.36/javaagent/build.gradle.kts b/instrumentation/azure-core/azure-core-1.36/javaagent/build.gradle.kts
@@ -33,6 +33,12 @@ dependencies {
 
 val latestDepTest = findProperty("testLatestDeps") as Boolean
 
+tasks {
+  withType<Test>().configureEach {
+    systemProperty("testLatestDeps", findProperty("testLatestDeps") as Boolean)
+  }
+}
+
 testing {
   suites {
     // using a test suite to ensure that classes from library-instrumentation-shaded that were
@@ -41,8 +47,10 @@ testing {
       dependencies {
         if (latestDepTest) {
           implementation("com.azure:azure-core:+")
+          implementation("com.azure:azure-core-test:+")
         } else {
           implementation("com.azure:azure-core:1.36.0")
+          implementation("com.azure:azure-core-test:1.16.2")
         }
       }
     }

diff --git a/...entelemetry/javaagent/instrumentation/azurecore/v1_36/AzureHttpClientInstrumentation.java b/...entelemetry/javaagent/instrumentation/azurecore/v1_36/AzureHttpClientInstrumentation.java
@@ -12,6 +12,7 @@
 import static net.bytebuddy.matcher.ElementMatchers.isPublic;
 import static net.bytebuddy.matcher.ElementMatchers.named;
 import static net.bytebuddy.matcher.ElementMatchers.returns;
+import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 
 import com.azure.core.http.HttpResponse;
 import io.opentelemetry.context.Scope;
@@ -35,30 +36,36 @@ public void transform(TypeTransformer transformer) {
         isMethod()
             .and(isPublic())
             .and(named("send"))
+            .and(takesArgument(1, named("com.azure.core.util.Context")))
             .and(returns(named("reactor.core.publisher.Mono"))),
         this.getClass().getName() + "$SuppressNestedClientMonoAdvice");
     transformer.applyAdviceToMethod(
         isMethod()
             .and(isPublic())
             .and(named("sendSync"))
+            .and(takesArgument(1, named("com.azure.core.util.Context")))
             .and(returns(named("com.azure.core.http.HttpResponse"))),
         this.getClass().getName() + "$SuppressNestedClientSyncAdvice");
   }
 
   @SuppressWarnings("unused")
   public static class SuppressNestedClientMonoAdvice {
     @Advice.OnMethodExit(suppress = Throwable.class)
-    public static void asyncSendExit(@Advice.Return(readOnly = false) Mono<HttpResponse> mono) {
-      mono = disallowNestedClientSpanMono(mono);
+    public static void asyncSendExit(
+        @Advice.Argument(1) com.azure.core.util.Context azContext,
+        @Advice.Return(readOnly = false) Mono<HttpResponse> mono) {
+      mono = disallowNestedClientSpanMono(mono, azContext);
     }
   }
 
   @SuppressWarnings("unused")
   public static class SuppressNestedClientSyncAdvice {
 
     @Advice.OnMethodEnter(suppress = Throwable.class)
-    public static void syncSendEnter(@Advice.Local("otelScope") Scope scope) {
-      scope = disallowNestedClientSpanSync();
+    public static void syncSendEnter(
+        @Advice.Argument(1) com.azure.core.util.Context azContext,
+        @Advice.Local("otelScope") Scope scope) {
+      scope = disallowNestedClientSpanSync(azContext);
     }
 
     @Advice.OnMethodExit(suppress = Throwable.class)

diff --git a/...o/opentelemetry/javaagent/instrumentation/azurecore/v1_36/SuppressNestedClientHelper.java b/...o/opentelemetry/javaagent/instrumentation/azurecore/v1_36/SuppressNestedClientHelper.java
@@ -16,20 +16,24 @@
 
 public class SuppressNestedClientHelper {
 
-  public static Scope disallowNestedClientSpanSync() {
+  public static Scope disallowNestedClientSpanSync(com.azure.core.util.Context azContext) {
     Context parentContext = currentContext();
-    if (doesNotHaveClientSpan(parentContext)) {
+    boolean hasAzureClientSpan = azContext.getData("client-method-call-flag").isPresent();
+    if (doesNotHaveClientSpan(parentContext) && hasAzureClientSpan) {
       return disallowNestedClientSpan(parentContext).makeCurrent();
     }
     return null;
   }
 
-  public static <T> Mono<T> disallowNestedClientSpanMono(Mono<T> delegate) {
+  public static <T> Mono<T> disallowNestedClientSpanMono(
+      Mono<T> delegate, com.azure.core.util.Context azContext) {
     return new Mono<T>() {
       @Override
       public void subscribe(CoreSubscriber<? super T> coreSubscriber) {
         Context parentContext = currentContext();
-        if (doesNotHaveClientSpan(parentContext)) {
+
+        boolean hasAzureClientSpan = azContext.getData("client-method-call-flag").isPresent();
+        if (doesNotHaveClientSpan(parentContext) && hasAzureClientSpan) {
           try (Scope ignored = disallowNestedClientSpan(parentContext).makeCurrent()) {
             delegate.subscribe(coreSubscriber);
           }

diff --git a/...stAzure/java/io/opentelemetry/javaagent/instrumentation/azurecore/v1_36/AzureSdkTest.java b/...stAzure/java/io/opentelemetry/javaagent/instrumentation/azurecore/v1_36/AzureSdkTest.java
@@ -7,14 +7,35 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+import com.azure.core.annotation.ExpectedResponses;
+import com.azure.core.annotation.Get;
+import com.azure.core.annotation.Host;
+import com.azure.core.annotation.ServiceInterface;
+import com.azure.core.http.HttpClient;
+import com.azure.core.http.HttpPipeline;
+import com.azure.core.http.HttpPipelineBuilder;
+import com.azure.core.http.policy.HttpPipelinePolicy;
+import com.azure.core.http.policy.HttpPolicyProviders;
+import com.azure.core.http.rest.Response;
+import com.azure.core.http.rest.RestProxy;
+import com.azure.core.test.http.MockHttpResponse;
+import com.azure.core.util.ClientOptions;
 import com.azure.core.util.Context;
+import com.azure.core.util.TracingOptions;
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.instrumentation.api.internal.SpanKey;
 import io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension;
 import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
 import io.opentelemetry.sdk.trace.data.StatusData;
+import io.opentelemetry.semconv.HttpAttributes;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import reactor.core.publisher.Mono;
+import reactor.test.StepVerifier;
 
 class AzureSdkTest {
 
@@ -48,9 +69,94 @@ void testSpan() {
                         .hasAttributesSatisfying(Attributes::isEmpty)));
   }
 
+  @Test
+  void testPipelineAndSuppression() {
+    AtomicBoolean hasClientAndHttpKeys = new AtomicBoolean(false);
+
+    HttpClient mockClient =
+        request ->
+            Mono.defer(
+                () -> {
+                  // check if suppression is working
+                  hasClientAndHttpKeys.set(hasClientAndHttpSpans());
+                  return Mono.just(new MockHttpResponse(request, 200));
+                });
+
+    StepVerifier.create(createService(mockClient, true).testMethod())
+        .expectNextCount(1)
+        .expectComplete()
+        .verify();
+
+    assertThat(hasClientAndHttpKeys.get()).isTrue();
+    testing.waitAndAssertTracesWithoutScopeVersionVerification(
+        trace ->
+            trace.hasSpansSatisfyingExactly(
+                span ->
+                    span.hasName("myService.testMethod")
+                        .hasKind(SpanKind.INTERNAL)
+                        .hasStatus(StatusData.unset())
+                        .hasAttributes(Attributes.empty()),
+                span ->
+                    span.hasKind(SpanKind.CLIENT)
+                        .hasName(Boolean.getBoolean("testLatestDeps") ? "GET" : "HTTP GET")
+                        .hasStatus(StatusData.unset())
+                        .hasAttribute(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200L)));
+  }
+
+  @Test
+  void testDisabledTracingNoSuppression() {
+    AtomicBoolean hasClientAndHttpKeys = new AtomicBoolean(false);
+
+    HttpClient mockClient =
+        request ->
+            Mono.defer(
+                () -> {
+                  // check no suppression
+                  hasClientAndHttpKeys.set(hasClientAndHttpSpans());
+                  return Mono.just(new MockHttpResponse(request, 200));
+                });
+
+    StepVerifier.create(createService(mockClient, false).testMethod())
+        .expectNextCount(1)
+        .expectComplete()
+        .verify();
+
+    assertThat(hasClientAndHttpKeys.get()).isFalse();
+  }
+
   private static com.azure.core.util.tracing.Tracer createAzTracer() {
     com.azure.core.util.tracing.TracerProvider azProvider =
         com.azure.core.util.tracing.TracerProvider.getDefaultProvider();
     return azProvider.createTracer("test-lib", "test-version", "otel.tests", null);
   }
+
+  private static TestInterface createService(HttpClient httpClient, boolean tracingEnabled) {
+    List<HttpPipelinePolicy> policies = new ArrayList<>();
+    HttpPolicyProviders.addAfterRetryPolicies(policies);
+
+    ClientOptions clientOptions =
+        new ClientOptions().setTracingOptions(new TracingOptions().setEnabled(tracingEnabled));
+    HttpPipeline pipeline =
+        new HttpPipelineBuilder()
+            .policies(policies.toArray(new HttpPipelinePolicy[0]))
+            .httpClient(httpClient)
+            .clientOptions(clientOptions)
+            .build();
+
+    return RestProxy.create(TestInterface.class, pipeline);
+  }
+
+  private static boolean hasClientAndHttpSpans() {
+    io.opentelemetry.context.Context ctx = io.opentelemetry.context.Context.current();
+    return SpanKey.KIND_CLIENT.fromContextOrNull(ctx) != null
+        && SpanKey.HTTP_CLIENT.fromContextOrNull(ctx) != null;
+  }
+
+  @Host("https://azure.com")
+  @ServiceInterface(name = "myService")
+  interface TestInterface {
+    @Get("path")
+    @ExpectedResponses({200})
+    Mono<Response<Void>> testMethod();
+  }
 }
diff --git a/instrumentation/azure-core/azure-core-1.36/library-instrumentation-shaded/build.gradle.kts b/instrumentation/azure-core/azure-core-1.36/library-instrumentation-shaded/build.gradle.kts
@@ -6,7 +6,9 @@ plugins {
 group = "io.opentelemetry.javaagent.instrumentation"
 
 dependencies {
-  implementation("com.azure:azure-core-tracing-opentelemetry:1.0.0-beta.42")
+  // this is the last good version that works with indy build
+  // update to 1.49 or latest once https://github.com/Azure/azure-sdk-for-java/pull/42586 is released.
+  implementation("com.azure:azure-core-tracing-opentelemetry:1.0.0-beta.45")
 }
 
 tasks {