From 5c15f5e29fd71d9afd4e10dcad13fb123ceb24ff Mon Sep 17 00:00:00 2001 From: HaloFour Date: Wed, 28 Apr 2021 08:38:19 -0400 Subject: [PATCH] Async @WithSpan Instrumentation for Guava ListenableFuture (#2811) * Add Guava instrumentation library with AsyncSpanEndStrategy * Enable span strategy in advice * Spotless * Nix attempt at typeInitializer advice, leave TODO comment to revisit * Move async span strategy registration to helper class * Remove use of sameThreadExecutor * Make helper class final and add comment about relying on static initializer --- .../javaagent/guava-10.0-javaagent.gradle | 4 + .../guava/GuavaInstrumentationModule.java | 16 ++- .../guava/InstrumentationHelper.java | 23 ++++ .../GuavaWithSpanInstrumentationTest.groovy | 105 ++++++++++++++++++ .../instrumentation/guava/TracedWithSpan.java | 16 +++ .../library/guava-10.0-library.gradle | 5 + .../guava/GuavaAsyncSpanEndStrategy.java | 40 +++++++ .../GuavaAsyncSpanEndStrategyTest.groovy | 89 +++++++++++++++ settings.gradle | 1 + 9 files changed, 297 insertions(+), 2 deletions(-) create mode 100644 instrumentation/guava-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/guava/InstrumentationHelper.java create mode 100644 instrumentation/guava-10.0/javaagent/src/test/groovy/GuavaWithSpanInstrumentationTest.groovy create mode 100644 instrumentation/guava-10.0/javaagent/src/test/java/io/opentelemetry/instrumentation/guava/TracedWithSpan.java create mode 100644 instrumentation/guava-10.0/library/guava-10.0-library.gradle create mode 100644 instrumentation/guava-10.0/library/src/main/java/io/opentelemetry/instrumentation/guava/GuavaAsyncSpanEndStrategy.java create mode 100644 instrumentation/guava-10.0/library/src/test/groovy/GuavaAsyncSpanEndStrategyTest.groovy diff --git a/instrumentation/guava-10.0/javaagent/guava-10.0-javaagent.gradle b/instrumentation/guava-10.0/javaagent/guava-10.0-javaagent.gradle index 7c9ae527f0da..29620bec7d09 100644 --- a/instrumentation/guava-10.0/javaagent/guava-10.0-javaagent.gradle +++ b/instrumentation/guava-10.0/javaagent/guava-10.0-javaagent.gradle @@ -11,4 +11,8 @@ muzzle { dependencies { library group: 'com.google.guava', name: 'guava', version: '10.0' + + implementation project(':instrumentation:guava-10.0:library') + + testImplementation deps.opentelemetryExtAnnotations } diff --git a/instrumentation/guava-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/guava/GuavaInstrumentationModule.java b/instrumentation/guava-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/guava/GuavaInstrumentationModule.java index 58486286f395..e9f973f162f0 100644 --- a/instrumentation/guava-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/guava/GuavaInstrumentationModule.java +++ b/instrumentation/guava-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/guava/GuavaInstrumentationModule.java @@ -6,7 +6,7 @@ package io.opentelemetry.javaagent.instrumentation.guava; import static java.util.Collections.singletonList; -import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.isConstructor; import static net.bytebuddy.matcher.ElementMatchers.named; import com.google.auto.service.AutoService; @@ -20,6 +20,7 @@ import io.opentelemetry.javaagent.instrumentation.api.concurrent.State; import io.opentelemetry.javaagent.tooling.InstrumentationModule; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; @@ -49,9 +50,20 @@ public ElementMatcher typeMatcher() { @Override public Map, String> transformers() { - return singletonMap( + Map, String> map = new HashMap<>(); + map.put( + isConstructor(), GuavaInstrumentationModule.class.getName() + "$AbstractFutureAdvice"); + map.put( named("addListener").and(ElementMatchers.takesArguments(Runnable.class, Executor.class)), GuavaInstrumentationModule.class.getName() + "$AddListenerAdvice"); + return map; + } + } + + public static class AbstractFutureAdvice { + @Advice.OnMethodExit(suppress = Throwable.class) + public static void onConstruction() { + InstrumentationHelper.initialize(); } } diff --git a/instrumentation/guava-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/guava/InstrumentationHelper.java b/instrumentation/guava-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/guava/InstrumentationHelper.java new file mode 100644 index 000000000000..aca219d1d04f --- /dev/null +++ b/instrumentation/guava-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/guava/InstrumentationHelper.java @@ -0,0 +1,23 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.guava; + +import io.opentelemetry.instrumentation.api.tracer.async.AsyncSpanEndStrategies; +import io.opentelemetry.instrumentation.guava.GuavaAsyncSpanEndStrategy; + +public final class InstrumentationHelper { + static { + AsyncSpanEndStrategies.getInstance().registerStrategy(GuavaAsyncSpanEndStrategy.INSTANCE); + } + + /** + * This method is invoked to trigger the runtime system to execute the static initializer block + * ensuring that the {@link GuavaAsyncSpanEndStrategy} is registered exactly once. + */ + public static void initialize() {} + + private InstrumentationHelper() {} +} diff --git a/instrumentation/guava-10.0/javaagent/src/test/groovy/GuavaWithSpanInstrumentationTest.groovy b/instrumentation/guava-10.0/javaagent/src/test/groovy/GuavaWithSpanInstrumentationTest.groovy new file mode 100644 index 000000000000..b331ac5c3196 --- /dev/null +++ b/instrumentation/guava-10.0/javaagent/src/test/groovy/GuavaWithSpanInstrumentationTest.groovy @@ -0,0 +1,105 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +import com.google.common.util.concurrent.Futures +import com.google.common.util.concurrent.SettableFuture +import io.opentelemetry.api.trace.SpanKind +import io.opentelemetry.instrumentation.guava.TracedWithSpan +import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification + +class GuavaWithSpanInstrumentationTest extends AgentInstrumentationSpecification { + + def "should capture span for already done ListenableFuture"() { + setup: + new TracedWithSpan().listenableFuture(Futures.immediateFuture("Value")) + + expect: + assertTraces(1) { + trace(0, 1) { + span(0) { + name "TracedWithSpan.listenableFuture" + kind SpanKind.INTERNAL + hasNoParent() + errored false + attributes { + } + } + } + } + } + + def "should capture span for already failed ListenableFuture"() { + setup: + def error = new IllegalArgumentException("Boom") + new TracedWithSpan().listenableFuture(Futures.immediateFailedFuture(error)) + + expect: + assertTraces(1) { + trace(0, 1) { + span(0) { + name "TracedWithSpan.listenableFuture" + kind SpanKind.INTERNAL + hasNoParent() + errored true + errorEvent(IllegalArgumentException, "Boom") + attributes { + } + } + } + } + } + + def "should capture span for eventually done ListenableFuture"() { + setup: + def future = SettableFuture.create() + new TracedWithSpan().listenableFuture(future) + + expect: + Thread.sleep(500) // sleep a bit just to make sure no span is captured + assertTraces(0) {} + + future.set("Value") + + assertTraces(1) { + trace(0, 1) { + span(0) { + name "TracedWithSpan.listenableFuture" + kind SpanKind.INTERNAL + hasNoParent() + errored false + attributes { + } + } + } + } + } + + def "should capture span for eventually failed ListenableFuture"() { + setup: + def error = new IllegalArgumentException("Boom") + def future = SettableFuture.create() + new TracedWithSpan().listenableFuture(future) + + expect: + Thread.sleep(500) // sleep a bit just to make sure no span is captured + assertTraces(0) {} + + future.setException(error) + + assertTraces(1) { + trace(0, 1) { + span(0) { + name "TracedWithSpan.listenableFuture" + kind SpanKind.INTERNAL + hasNoParent() + errored true + errorEvent(IllegalArgumentException, "Boom") + attributes { + } + } + } + } + } +} diff --git a/instrumentation/guava-10.0/javaagent/src/test/java/io/opentelemetry/instrumentation/guava/TracedWithSpan.java b/instrumentation/guava-10.0/javaagent/src/test/java/io/opentelemetry/instrumentation/guava/TracedWithSpan.java new file mode 100644 index 000000000000..89f52ed13cd8 --- /dev/null +++ b/instrumentation/guava-10.0/javaagent/src/test/java/io/opentelemetry/instrumentation/guava/TracedWithSpan.java @@ -0,0 +1,16 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.guava; + +import com.google.common.util.concurrent.ListenableFuture; +import io.opentelemetry.extension.annotations.WithSpan; + +public class TracedWithSpan { + @WithSpan + public ListenableFuture listenableFuture(ListenableFuture future) { + return future; + } +} diff --git a/instrumentation/guava-10.0/library/guava-10.0-library.gradle b/instrumentation/guava-10.0/library/guava-10.0-library.gradle new file mode 100644 index 000000000000..360e3f8b70ef --- /dev/null +++ b/instrumentation/guava-10.0/library/guava-10.0-library.gradle @@ -0,0 +1,5 @@ +apply from: "$rootDir/gradle/instrumentation-library.gradle" + +dependencies { + library group: 'com.google.guava', name: 'guava', version: '10.0' +} \ No newline at end of file diff --git a/instrumentation/guava-10.0/library/src/main/java/io/opentelemetry/instrumentation/guava/GuavaAsyncSpanEndStrategy.java b/instrumentation/guava-10.0/library/src/main/java/io/opentelemetry/instrumentation/guava/GuavaAsyncSpanEndStrategy.java new file mode 100644 index 000000000000..6c6b6d71d4d5 --- /dev/null +++ b/instrumentation/guava-10.0/library/src/main/java/io/opentelemetry/instrumentation/guava/GuavaAsyncSpanEndStrategy.java @@ -0,0 +1,40 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.guava; + +import com.google.common.util.concurrent.ListenableFuture; +import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.tracer.BaseTracer; +import io.opentelemetry.instrumentation.api.tracer.async.AsyncSpanEndStrategy; + +public enum GuavaAsyncSpanEndStrategy implements AsyncSpanEndStrategy { + INSTANCE; + + @Override + public boolean supports(Class returnType) { + return ListenableFuture.class.isAssignableFrom(returnType); + } + + @Override + public Object end(BaseTracer tracer, Context context, Object returnValue) { + ListenableFuture future = (ListenableFuture) returnValue; + if (future.isDone()) { + endSpan(tracer, context, future); + } else { + future.addListener(() -> endSpan(tracer, context, future), Runnable::run); + } + return future; + } + + private void endSpan(BaseTracer tracer, Context context, ListenableFuture future) { + try { + future.get(); + tracer.end(context); + } catch (Throwable exception) { + tracer.endExceptionally(context, exception); + } + } +} diff --git a/instrumentation/guava-10.0/library/src/test/groovy/GuavaAsyncSpanEndStrategyTest.groovy b/instrumentation/guava-10.0/library/src/test/groovy/GuavaAsyncSpanEndStrategyTest.groovy new file mode 100644 index 000000000000..54be4cb44a49 --- /dev/null +++ b/instrumentation/guava-10.0/library/src/test/groovy/GuavaAsyncSpanEndStrategyTest.groovy @@ -0,0 +1,89 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +import com.google.common.util.concurrent.Futures +import com.google.common.util.concurrent.ListenableFuture +import com.google.common.util.concurrent.SettableFuture +import io.opentelemetry.context.Context +import io.opentelemetry.instrumentation.api.tracer.BaseTracer +import io.opentelemetry.instrumentation.guava.GuavaAsyncSpanEndStrategy +import spock.lang.Specification + +class GuavaAsyncSpanEndStrategyTest extends Specification { + BaseTracer tracer + + Context context + + def underTest = GuavaAsyncSpanEndStrategy.INSTANCE + + void setup() { + tracer = Mock() + context = Mock() + } + + def "ListenableFuture is supported"() { + expect: + underTest.supports(ListenableFuture) + } + + def "SettableFuture is also supported"() { + expect: + underTest.supports(SettableFuture) + } + + def "ends span on already done future"() { + when: + underTest.end(tracer, context, Futures.immediateFuture("Value")) + + then: + 1 * tracer.end(context) + } + + def "ends span on already failed future"() { + given: + def exception = new IllegalStateException() + + when: + underTest.end(tracer, context, Futures.immediateFailedFuture(exception)) + + then: + 1 * tracer.endExceptionally(context, { it.getCause() == exception }) + } + + def "ends span on eventually done future"() { + given: + def future = SettableFuture.create() + + when: + underTest.end(tracer, context, future) + + then: + 0 * tracer._ + + when: + future.set("Value") + + then: + 1 * tracer.end(context) + } + + def "ends span on eventually failed future"() { + given: + def future = SettableFuture.create() + def exception = new IllegalStateException() + + when: + underTest.end(tracer, context, future) + + then: + 0 * tracer._ + + when: + future.setException(exception) + + then: + 1 * tracer.endExceptionally(context, { it.getCause() == exception }) + } +} diff --git a/settings.gradle b/settings.gradle index 1809cd3948e6..8e83dd7c2ecd 100644 --- a/settings.gradle +++ b/settings.gradle @@ -118,6 +118,7 @@ include ':instrumentation:grpc-1.5:javaagent' include ':instrumentation:grpc-1.5:library' include ':instrumentation:grpc-1.5:testing' include ':instrumentation:guava-10.0:javaagent' +include ':instrumentation:guava-10.0:library' include ':instrumentation:gwt-2.0:javaagent' include ':instrumentation:hibernate:hibernate-3.3:javaagent' include ':instrumentation:hibernate:hibernate-4.0:javaagent'