Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve end strategy after WithSpan method instead of before. #5756

Merged
merged 2 commits into from
Apr 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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<Method, Object> operationEndSupport,
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope) {

Expand All @@ -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<Method, Object> operationEndSupport,
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope,
@Advice.Return(typing = Assigner.Typing.DYNAMIC, readOnly = false) Object returnValue,
Expand All @@ -150,6 +144,9 @@ public static void stopSpan(
return;
}
scope.close();

AsyncOperationEndSupport<Method, Object> operationEndSupport =
AsyncOperationEndSupport.create(instrumenter(), Object.class, method.getReturnType());
returnValue = operationEndSupport.asyncEnd(context, method, returnValue, throwable);
}
}
Expand Down
18 changes: 18 additions & 0 deletions instrumentation/reactor/reactor-3.1/javaagent/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,21 @@ 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.opentelemetry:opentelemetry-extension-annotations")
implementation("io.projectreactor:reactor-test:3.1.0.RELEASE")
}
}
}
}

tasks {
check {
dependsOn(testing.suites)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +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.extension.annotations.WithSpan;
import java.util.stream.Collectors;
import org.junit.jupiter.api.Test;
import reactor.core.Scannable;
import reactor.core.publisher.Mono;

// Isolated test to use clean classloader because reactor instrumentation is applied on static
// initialization.
Comment on lines +16 to +17
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

class InitializationTest {

@Test
void contextPropagated() {
Mono<String> 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");
}

static class Traced {
@WithSpan
Mono<String> traceMe() {
return Mono.just("foo");
}
}
}