From b2258992b368ab4575a26d774f02acc67672d886 Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Wed, 6 Apr 2022 06:10:19 +0000 Subject: [PATCH 1/2] WIP --- .../reactor-3.1/javaagent/build.gradle.kts | 17 +++++++++ .../reactor/InitializationTest.java | 37 +++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 instrumentation/reactor/reactor-3.1/javaagent/src/testInitialization/java/io/opentelemetry/javaagent/instrumentation/reactor/InitializationTest.java diff --git a/instrumentation/reactor/reactor-3.1/javaagent/build.gradle.kts b/instrumentation/reactor/reactor-3.1/javaagent/build.gradle.kts index 7fb51121f9dd..2e28f8245ecf 100644 --- a/instrumentation/reactor/reactor-3.1/javaagent/build.gradle.kts +++ b/instrumentation/reactor/reactor-3.1/javaagent/build.gradle.kts @@ -34,3 +34,20 @@ dependencies { // Looks like later versions on reactor need this dependency for some reason even though it is marked as optional. latestDepTestLibrary("io.micrometer:micrometer-core:1.+") } + +testing { + suites { + val testInitialization by registering(JvmTestSuite::class) { + dependencies { + implementation(project(":instrumentation:reactor:reactor-3.1:library")) + implementation("io.projectreactor:reactor-test:3.1.0.RELEASE") + } + } + } +} + +tasks { + check { + dependsOn(testing.suites) + } +} \ No newline at end of file diff --git a/instrumentation/reactor/reactor-3.1/javaagent/src/testInitialization/java/io/opentelemetry/javaagent/instrumentation/reactor/InitializationTest.java b/instrumentation/reactor/reactor-3.1/javaagent/src/testInitialization/java/io/opentelemetry/javaagent/instrumentation/reactor/InitializationTest.java new file mode 100644 index 000000000000..7cd76a932d5f --- /dev/null +++ b/instrumentation/reactor/reactor-3.1/javaagent/src/testInitialization/java/io/opentelemetry/javaagent/instrumentation/reactor/InitializationTest.java @@ -0,0 +1,37 @@ +package io.opentelemetry.javaagent.instrumentation.reactor; + +import static org.assertj.core.api.Assertions.assertThat; + +import io.opentelemetry.context.ContextKey; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Collectors; +import org.junit.jupiter.api.Test; +import reactor.core.Scannable; +import reactor.core.publisher.Mono; +import reactor.core.publisher.UnicastProcessor; + +// +class InitializationTest { + + private static final ContextKey ANIMAL = ContextKey.named("animal"); + + @Test + void contextPropagated() { + AtomicReference capturedAnimal = new AtomicReference<>(); + + UnicastProcessor source1 = UnicastProcessor.create(); + Mono mono1 = source1.singleOrEmpty(); + source1.onNext("foo"); + source1.onComplete(); + mono1.block(); + + List parents1 = ((Scannable) mono1).parents().collect(Collectors.toList()); + + UnicastProcessor source2 = UnicastProcessor.create(); + Mono mono2 = source2.singleOrEmpty(); + + List parents2 = ((Scannable) mono1).parents().collect(Collectors.toList()); + assertThat(parents1).isNotEmpty(); + } +} From 52783c40227017564828873053be811cdddc450c Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Wed, 6 Apr 2022 07:30:53 +0000 Subject: [PATCH 2/2] Resolve end strategy after WithSpan method instead of before. --- .../WithSpanInstrumentation.java | 9 ++-- .../reactor-3.1/javaagent/build.gradle.kts | 3 +- .../reactor/InitializationTest.java | 54 +++++++++++-------- 3 files changed, 38 insertions(+), 28 deletions(-) diff --git a/instrumentation/opentelemetry-annotations-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/otelannotations/WithSpanInstrumentation.java b/instrumentation/opentelemetry-annotations-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/otelannotations/WithSpanInstrumentation.java index 3f63a52ae713..7334865711ea 100644 --- a/instrumentation/opentelemetry-annotations-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/otelannotations/WithSpanInstrumentation.java +++ b/instrumentation/opentelemetry-annotations-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/otelannotations/WithSpanInstrumentation.java @@ -117,8 +117,6 @@ public static class WithSpanAdvice { public static void onEnter( @Advice.Origin Method originMethod, @Advice.Local("otelMethod") Method method, - @Advice.Local("otelOperationEndSupport") - AsyncOperationEndSupport operationEndSupport, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { @@ -132,16 +130,12 @@ public static void onEnter( if (instrumenter.shouldStart(current, method)) { context = instrumenter.start(current, method); scope = context.makeCurrent(); - operationEndSupport = - AsyncOperationEndSupport.create(instrumenter, Object.class, method.getReturnType()); } } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( @Advice.Local("otelMethod") Method method, - @Advice.Local("otelOperationEndSupport") - AsyncOperationEndSupport operationEndSupport, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope, @Advice.Return(typing = Assigner.Typing.DYNAMIC, readOnly = false) Object returnValue, @@ -150,6 +144,9 @@ public static void stopSpan( return; } scope.close(); + + AsyncOperationEndSupport operationEndSupport = + AsyncOperationEndSupport.create(instrumenter(), Object.class, method.getReturnType()); returnValue = operationEndSupport.asyncEnd(context, method, returnValue, throwable); } } diff --git a/instrumentation/reactor/reactor-3.1/javaagent/build.gradle.kts b/instrumentation/reactor/reactor-3.1/javaagent/build.gradle.kts index 2e28f8245ecf..5525a074390f 100644 --- a/instrumentation/reactor/reactor-3.1/javaagent/build.gradle.kts +++ b/instrumentation/reactor/reactor-3.1/javaagent/build.gradle.kts @@ -40,6 +40,7 @@ testing { val testInitialization by registering(JvmTestSuite::class) { dependencies { implementation(project(":instrumentation:reactor:reactor-3.1:library")) + implementation("io.opentelemetry:opentelemetry-extension-annotations") implementation("io.projectreactor:reactor-test:3.1.0.RELEASE") } } @@ -50,4 +51,4 @@ tasks { check { dependsOn(testing.suites) } -} \ No newline at end of file +} diff --git a/instrumentation/reactor/reactor-3.1/javaagent/src/testInitialization/java/io/opentelemetry/javaagent/instrumentation/reactor/InitializationTest.java b/instrumentation/reactor/reactor-3.1/javaagent/src/testInitialization/java/io/opentelemetry/javaagent/instrumentation/reactor/InitializationTest.java index 7cd76a932d5f..199b7bdb1b51 100644 --- a/instrumentation/reactor/reactor-3.1/javaagent/src/testInitialization/java/io/opentelemetry/javaagent/instrumentation/reactor/InitializationTest.java +++ b/instrumentation/reactor/reactor-3.1/javaagent/src/testInitialization/java/io/opentelemetry/javaagent/instrumentation/reactor/InitializationTest.java @@ -1,37 +1,49 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.javaagent.instrumentation.reactor; import static org.assertj.core.api.Assertions.assertThat; -import io.opentelemetry.context.ContextKey; -import java.util.List; -import java.util.concurrent.atomic.AtomicReference; +import io.opentelemetry.extension.annotations.WithSpan; import java.util.stream.Collectors; import org.junit.jupiter.api.Test; import reactor.core.Scannable; import reactor.core.publisher.Mono; -import reactor.core.publisher.UnicastProcessor; -// +// Isolated test to use clean classloader because reactor instrumentation is applied on static +// initialization. class InitializationTest { - private static final ContextKey ANIMAL = ContextKey.named("animal"); - @Test void contextPropagated() { - AtomicReference capturedAnimal = new AtomicReference<>(); - - UnicastProcessor source1 = UnicastProcessor.create(); - Mono mono1 = source1.singleOrEmpty(); - source1.onNext("foo"); - source1.onComplete(); - mono1.block(); - - List parents1 = ((Scannable) mono1).parents().collect(Collectors.toList()); - - UnicastProcessor source2 = UnicastProcessor.create(); - Mono mono2 = source2.singleOrEmpty(); + Mono mono = new Traced().traceMe(); + + // If reactor augmentation of WithSpan is working correctly, we will end up with these + // implementation details. + // TODO(anuraaga): This should just check actual context propagation instead of implementation + // but couldn't figure out how. + assertThat(((Scannable) mono).parents().collect(Collectors.toList())) + .anySatisfy( + op -> { + assertThat(op.getClass().getSimpleName()).isEqualTo("MonoFlatMap"); + assertThat(op) + .extracting("source") + .satisfies( + source -> + assertThat(source.getClass().getSimpleName()) + .isEqualTo("ScalarPropagatingMono")); + }); + + assertThat(mono.block()).isEqualTo("foo"); + } - List parents2 = ((Scannable) mono1).parents().collect(Collectors.toList()); - assertThat(parents1).isNotEmpty(); + static class Traced { + @WithSpan + Mono traceMe() { + return Mono.just("foo"); + } } }