diff --git a/agent-bootstrap/agent-bootstrap.gradle b/agent-bootstrap/agent-bootstrap.gradle index 4efa2ba41b..e37ad7ed50 100644 --- a/agent-bootstrap/agent-bootstrap.gradle +++ b/agent-bootstrap/agent-bootstrap.gradle @@ -11,6 +11,7 @@ minimumInstructionCoverage = 0.0 dependencies { compile deps.opentelemetryApi + compile project(':utils:thread-utils') compile deps.slf4j compile group: 'org.slf4j', name: 'slf4j-simple', version: versions.slf4j // ^ Generally a bad idea for libraries, but we're shadowing. diff --git a/agent-bootstrap/src/main/java/io/opentelemetry/auto/config/Config.java b/agent-bootstrap/src/main/java/io/opentelemetry/auto/config/Config.java index cac33c5357..d49d256fc2 100644 --- a/agent-bootstrap/src/main/java/io/opentelemetry/auto/config/Config.java +++ b/agent-bootstrap/src/main/java/io/opentelemetry/auto/config/Config.java @@ -380,7 +380,6 @@ public class Config { private static T valueOf( final String value, @NonNull final Class tClass, final T defaultValue) { if (value == null || value.trim().isEmpty()) { - log.debug("valueOf: using defaultValue '{}' for '{}' of '{}' ", defaultValue, value, tClass); return defaultValue; } try { diff --git a/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/AgentTooling.java b/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/AgentTooling.java index c0116cde10..a67d4214cb 100644 --- a/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/AgentTooling.java +++ b/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/AgentTooling.java @@ -37,7 +37,7 @@ public class AgentTooling { private static void registerWeakMapProvider() { if (!WeakMap.Provider.isProviderRegistered()) { - WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent(new Cleaner())); + WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent()); // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent.Inline()); // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.Guava()); } diff --git a/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/Cleaner.java b/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/Cleaner.java deleted file mode 100644 index 539e372c3d..0000000000 --- a/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/Cleaner.java +++ /dev/null @@ -1,73 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.opentelemetry.auto.tooling; - -import io.opentelemetry.auto.common.exec.CommonTaskExecutor; -import java.lang.ref.WeakReference; -import java.util.concurrent.RejectedExecutionException; -import java.util.concurrent.ScheduledFuture; -import java.util.concurrent.TimeUnit; -import lombok.extern.slf4j.Slf4j; - -@Slf4j -class Cleaner { - - void scheduleCleaning( - final T target, final Adapter adapter, final long frequency, final TimeUnit unit) { - final CleanupRunnable command = new CleanupRunnable<>(target, adapter); - if (CommonTaskExecutor.INSTANCE.isShutdown()) { - log.warn( - "Cleaning scheduled but task scheduler is shutdown. Target won't be cleaned {}", target); - } else { - try { - // Schedule job and save future to allow job to be canceled if target is GC'd. - command.setFuture( - CommonTaskExecutor.INSTANCE.scheduleAtFixedRate(command, frequency, frequency, unit)); - } catch (final RejectedExecutionException e) { - log.warn("Cleaning task rejected. Target won't be cleaned {}", target); - } - } - } - - public interface Adapter { - void clean(T target); - } - - private static class CleanupRunnable implements Runnable { - private final WeakReference target; - private final Adapter adapter; - private volatile ScheduledFuture future = null; - - private CleanupRunnable(final T target, final Adapter adapter) { - this.target = new WeakReference<>(target); - this.adapter = adapter; - } - - @Override - public void run() { - final T t = target.get(); - if (t != null) { - adapter.clean(t); - } else if (future != null) { - future.cancel(false); - } - } - - public void setFuture(final ScheduledFuture future) { - this.future = future; - } - } -} diff --git a/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/Constants.java b/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/Constants.java index e05b34186d..e88b06b7c9 100644 --- a/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/Constants.java +++ b/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/Constants.java @@ -29,6 +29,7 @@ public final class Constants { * io.opentelemetry.auto.test.SpockRunner#BOOTSTRAP_PACKAGE_PREFIXES_COPY */ public static final String[] BOOTSTRAP_PACKAGE_PREFIXES = { + "io.opentelemetry.auto.common.exec", "io.opentelemetry.auto.slf4j", "io.opentelemetry.auto.config", "io.opentelemetry.auto.bootstrap", @@ -39,6 +40,7 @@ public final class Constants { // This is used in IntegrationTestUtils.java public static final String[] AGENT_PACKAGE_PREFIXES = { "io.opentelemetry.auto", + "io.opentelemetry.auto.common.exec", "io.opentelemetry.auto.instrumentation", // guava "com.google.auto", diff --git a/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/WeakMapSuppliers.java b/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/WeakMapSuppliers.java index 4573adb894..9b368f27b8 100644 --- a/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/WeakMapSuppliers.java +++ b/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/WeakMapSuppliers.java @@ -19,6 +19,7 @@ import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.MapMaker; import io.opentelemetry.auto.bootstrap.WeakMap; +import io.opentelemetry.auto.common.exec.CommonTaskExecutor; import java.util.concurrent.TimeUnit; class WeakMapSuppliers { @@ -46,24 +47,27 @@ class WeakMapSuppliers { static class WeakConcurrent implements WeakMap.Implementation { @VisibleForTesting static final long CLEAN_FREQUENCY_SECONDS = 1; - private final Cleaner cleaner; - - WeakConcurrent(final Cleaner cleaner) { - this.cleaner = cleaner; - } @Override public WeakMap get() { final WeakConcurrentMap map = new WeakConcurrentMap<>(false, true); - cleaner.scheduleCleaning(map, MapCleaner.CLEANER, CLEAN_FREQUENCY_SECONDS, TimeUnit.SECONDS); + CommonTaskExecutor.INSTANCE.scheduleAtFixedRate( + MapCleaningTask.INSTANCE, + map, + CLEAN_FREQUENCY_SECONDS, + CLEAN_FREQUENCY_SECONDS, + TimeUnit.SECONDS, + "cleaner for " + map); return new Adapter<>(map); } - private static class MapCleaner implements Cleaner.Adapter { - private static final MapCleaner CLEANER = new MapCleaner(); + // Important to use explicit class to avoid implicit hard references to target + private static class MapCleaningTask implements CommonTaskExecutor.Task { + + static final MapCleaningTask INSTANCE = new MapCleaningTask(); @Override - public void clean(final WeakConcurrentMap target) { + public void run(final WeakConcurrentMap target) { target.expungeStaleEntries(); } } diff --git a/agent-tooling/src/test/groovy/io/opentelemetry/auto/tooling/WeakConcurrentSupplierTest.groovy b/agent-tooling/src/test/groovy/io/opentelemetry/auto/tooling/WeakConcurrentSupplierTest.groovy index 1edc79cd3b..377de9a798 100644 --- a/agent-tooling/src/test/groovy/io/opentelemetry/auto/tooling/WeakConcurrentSupplierTest.groovy +++ b/agent-tooling/src/test/groovy/io/opentelemetry/auto/tooling/WeakConcurrentSupplierTest.groovy @@ -28,9 +28,7 @@ import java.util.concurrent.TimeUnit // These tests fail sometimes in CI. class WeakConcurrentSupplierTest extends AgentSpecification { @Shared - def cleaner = new Cleaner() - @Shared - def weakConcurrentSupplier = new WeakMapSuppliers.WeakConcurrent(cleaner) + def weakConcurrentSupplier = new WeakMapSuppliers.WeakConcurrent() @Shared def weakInlineSupplier = new WeakMapSuppliers.WeakConcurrent.Inline() @Shared @@ -74,7 +72,7 @@ class WeakConcurrentSupplierTest extends AgentSpecification { where: name | supplierSupplier - "WeakConcurrent" | { -> new WeakMapSuppliers.WeakConcurrent(cleaner) } + "WeakConcurrent" | { -> new WeakMapSuppliers.WeakConcurrent() } "WeakInline" | { -> new WeakMapSuppliers.WeakConcurrent.Inline() } "Guava" | { -> new WeakMapSuppliers.Guava() } } diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 84cac83dd8..3a1ab9eda5 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -1,13 +1,13 @@ -def groovyVer = "2.5.10" +def groovyVer = "2.5.11" def spockGroovyVer = groovyVer.replaceAll(/\.\d+$/, '') ext { versions = [ opentelemetry: '0.3.0', - slf4j : "1.7.29", + slf4j : "1.7.30", guava : "20.0", // Last version to support Java 7 - okhttp : "3.12.8", // 3.12.x is last version to support Java7 + okhttp : "3.12.10", // 3.12.x is last version to support Java7 spock : "1.3-groovy-$spockGroovyVer", groovy : groovyVer, @@ -15,7 +15,7 @@ ext { lombok : "1.18.10", bytebuddy : "1.10.6", scala : "2.11.12", // Last version to support Java 7 (2.12+ require Java 8+) - kotlin : "1.3.61", + kotlin : "1.3.72", coroutines : "1.3.0" ] diff --git a/instrumentation/aws-sdk/aws-sdk-1.11/src/test/groovy/AWSClientTest.groovy b/instrumentation/aws-sdk/aws-sdk-1.11/src/test/groovy/AWS1ClientTest.groovy similarity index 99% rename from instrumentation/aws-sdk/aws-sdk-1.11/src/test/groovy/AWSClientTest.groovy rename to instrumentation/aws-sdk/aws-sdk-1.11/src/test/groovy/AWS1ClientTest.groovy index 1938c4fc4a..ce2cb60824 100644 --- a/instrumentation/aws-sdk/aws-sdk-1.11/src/test/groovy/AWSClientTest.groovy +++ b/instrumentation/aws-sdk/aws-sdk-1.11/src/test/groovy/AWS1ClientTest.groovy @@ -59,7 +59,7 @@ import static io.opentelemetry.auto.test.utils.PortUtils.UNUSABLE_PORT import static io.opentelemetry.trace.Span.Kind.CLIENT import static io.opentelemetry.trace.Span.Kind.INTERNAL -class AWSClientTest extends AgentTestRunner { +class AWS1ClientTest extends AgentTestRunner { private static final CREDENTIALS_PROVIDER_CHAIN = new AWSCredentialsProviderChain( new EnvironmentVariableCredentialsProvider(), diff --git a/instrumentation/aws-sdk/aws-sdk-1.11/src/test_before_1_11_106/groovy/AWSClientTest.groovy b/instrumentation/aws-sdk/aws-sdk-1.11/src/test_before_1_11_106/groovy/AWS0ClientTest.groovy similarity index 99% rename from instrumentation/aws-sdk/aws-sdk-1.11/src/test_before_1_11_106/groovy/AWSClientTest.groovy rename to instrumentation/aws-sdk/aws-sdk-1.11/src/test_before_1_11_106/groovy/AWS0ClientTest.groovy index 270b7b3220..248155bdbc 100644 --- a/instrumentation/aws-sdk/aws-sdk-1.11/src/test_before_1_11_106/groovy/AWSClientTest.groovy +++ b/instrumentation/aws-sdk/aws-sdk-1.11/src/test_before_1_11_106/groovy/AWS0ClientTest.groovy @@ -46,7 +46,7 @@ import static io.opentelemetry.auto.test.utils.PortUtils.UNUSABLE_PORT import static io.opentelemetry.trace.Span.Kind.CLIENT import static io.opentelemetry.trace.Span.Kind.INTERNAL -class AWSClientTest extends AgentTestRunner { +class AWS0ClientTest extends AgentTestRunner { private static final CREDENTIALS_PROVIDER_CHAIN = new AWSCredentialsProviderChain( new EnvironmentVariableCredentialsProvider(), diff --git a/instrumentation/aws-sdk/aws-sdk-2.2/src/test/groovy/AwsClientTest.groovy b/instrumentation/aws-sdk/aws-sdk-2.2/src/test/groovy/Aws2ClientTest.groovy similarity index 99% rename from instrumentation/aws-sdk/aws-sdk-2.2/src/test/groovy/AwsClientTest.groovy rename to instrumentation/aws-sdk/aws-sdk-2.2/src/test/groovy/Aws2ClientTest.groovy index 01d54c2540..b02b21348a 100644 --- a/instrumentation/aws-sdk/aws-sdk-2.2/src/test/groovy/AwsClientTest.groovy +++ b/instrumentation/aws-sdk/aws-sdk-2.2/src/test/groovy/Aws2ClientTest.groovy @@ -53,7 +53,7 @@ import static io.opentelemetry.auto.test.server.http.TestHttpServer.httpServer import static io.opentelemetry.trace.Span.Kind.CLIENT import static io.opentelemetry.trace.Span.Kind.INTERNAL -class AwsClientTest extends AgentTestRunner { +class Aws2ClientTest extends AgentTestRunner { private static final StaticCredentialsProvider CREDENTIALS_PROVIDER = StaticCredentialsProvider .create(AwsBasicCredentials.create("my-access-key", "my-secret-key")) diff --git a/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy b/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy index 31f8846053..f06816f0c7 100644 --- a/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy +++ b/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy @@ -68,6 +68,12 @@ class FinatraServerTest extends HttpServerTest { return true } + @Override + boolean testNotFound() { + // Resource name is set to "GET /notFound" + false + } + @Override void stopServer(HttpServer httpServer) { Await.ready(httpServer.close(), TIMEOUT) diff --git a/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala b/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala index a0d8166e3d..eca7d065a3 100644 --- a/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala +++ b/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala @@ -37,14 +37,6 @@ class FinatraController extends Controller { }) } - any(NOT_FOUND.getPath) { request: Request => - controller(NOT_FOUND, new Closure[Response](null) { - override def call(): Response = { - response.notFound(NOT_FOUND.getBody) - } - }) - } - any(QUERY_PARAM.getPath) { request: Request => controller(QUERY_PARAM, new Closure[Response](null) { override def call(): Response = { diff --git a/instrumentation/jaxrs/jaxrs-1.0/src/main/java/io/opentelemetry/auto/instrumentation/jaxrs/v1_0/JaxRsAnnotationsInstrumentation.java b/instrumentation/jaxrs/jaxrs-1.0/src/main/java/io/opentelemetry/auto/instrumentation/jaxrs/v1_0/JaxRsAnnotationsInstrumentation.java index b24bd6b3fa..f588e1d6cc 100644 --- a/instrumentation/jaxrs/jaxrs-1.0/src/main/java/io/opentelemetry/auto/instrumentation/jaxrs/v1_0/JaxRsAnnotationsInstrumentation.java +++ b/instrumentation/jaxrs/jaxrs-1.0/src/main/java/io/opentelemetry/auto/instrumentation/jaxrs/v1_0/JaxRsAnnotationsInstrumentation.java @@ -29,11 +29,13 @@ import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.not; import com.google.auto.service.AutoService; +import io.opentelemetry.auto.bootstrap.CallDepthThreadLocalMap; import io.opentelemetry.auto.instrumentation.api.SpanWithScope; import io.opentelemetry.auto.tooling.Instrumenter; import io.opentelemetry.trace.Span; import java.lang.reflect.Method; import java.util.Map; +import javax.ws.rs.Path; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; @@ -94,6 +96,10 @@ public final class JaxRsAnnotationsInstrumentation extends Instrumenter.Default @Advice.OnMethodEnter(suppress = Throwable.class) public static SpanWithScope nameSpan( @Advice.This final Object target, @Advice.Origin final Method method) { + if (CallDepthThreadLocalMap.incrementCallDepth(Path.class) > 0) { + return null; + } + // Rename the parent span according to the path represented by these annotations. final Span parent = TRACER.getCurrentSpan(); @@ -107,6 +113,11 @@ public final class JaxRsAnnotationsInstrumentation extends Instrumenter.Default @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( @Advice.Enter final SpanWithScope spanWithScope, @Advice.Thrown final Throwable throwable) { + if (spanWithScope == null) { + return; + } + CallDepthThreadLocalMap.reset(Path.class); + final Span span = spanWithScope.getSpan(); DECORATE.onError(span, throwable); DECORATE.beforeFinish(span); diff --git a/instrumentation/jaxrs/jaxrs-1.0/src/test/groovy/JerseyTest.groovy b/instrumentation/jaxrs/jaxrs-1.0/src/test/groovy/JerseyTest.groovy index 336f8858c9..334a413c88 100644 --- a/instrumentation/jaxrs/jaxrs-1.0/src/test/groovy/JerseyTest.groovy +++ b/instrumentation/jaxrs/jaxrs-1.0/src/test/groovy/JerseyTest.groovy @@ -19,6 +19,7 @@ import org.junit.ClassRule import spock.lang.Shared import static io.opentelemetry.auto.test.utils.TraceUtils.runUnderTrace +import static io.opentelemetry.trace.Span.Kind.INTERNAL class JerseyTest extends AgentTestRunner { @@ -63,4 +64,37 @@ class JerseyTest extends AgentTestRunner { "/test2/hello/bob" | "POST /test2/hello/{name}" | "Test2.hello" | "Test2 bob!" "/test3/hi/bob" | "POST /test3/hi/{name}" | "Test3.hello" | "Test3 bob!" } + + def "test nested call"() { + + when: + // start a trace because the test doesn't go through any servlet or other instrumentation. + def response = runUnderTrace("test.span") { + resources.client().resource(resource).post(String) + } + + then: + response == expectedResponse + + assertTraces(1) { + trace(0, 2) { + span(0) { + operationName expectedSpanName + tags { + } + } + span(1) { + childOf span(0) + operationName controller1Name + spanKind INTERNAL + tags { + } + } + } + } + + where: + resource | expectedSpanName | controller1Name | expectedResponse + "/test3/nested" | "POST /test3/nested" | "Test3.nested" | "Test3 nested!" + } } diff --git a/instrumentation/jaxrs/jaxrs-1.0/src/test/java/Resource.java b/instrumentation/jaxrs/jaxrs-1.0/src/test/java/Resource.java index 399b8440f4..b04af49d48 100644 --- a/instrumentation/jaxrs/jaxrs-1.0/src/test/java/Resource.java +++ b/instrumentation/jaxrs/jaxrs-1.0/src/test/java/Resource.java @@ -54,5 +54,11 @@ public interface Resource { public String hello(@PathParam("name") final String name) { return "Test3 " + name + "!"; } + + @POST + @Path("/nested") + public String nested() { + return hello("nested"); + } } } diff --git a/instrumentation/jaxrs/jaxrs-2.0/src/main/java/io/opentelemetry/auto/instrumentation/jaxrs/v2_0/JaxRsAnnotationsDecorator.java b/instrumentation/jaxrs/jaxrs-2.0/src/main/java/io/opentelemetry/auto/instrumentation/jaxrs/v2_0/JaxRsAnnotationsDecorator.java index aa8ad13a3e..ca86fae446 100644 --- a/instrumentation/jaxrs/jaxrs-2.0/src/main/java/io/opentelemetry/auto/instrumentation/jaxrs/v2_0/JaxRsAnnotationsDecorator.java +++ b/instrumentation/jaxrs/jaxrs-2.0/src/main/java/io/opentelemetry/auto/instrumentation/jaxrs/v2_0/JaxRsAnnotationsDecorator.java @@ -15,6 +15,8 @@ */ package io.opentelemetry.auto.instrumentation.jaxrs.v2_0; +import static io.opentelemetry.auto.bootstrap.WeakMap.Provider.newWeakMap; + import io.opentelemetry.OpenTelemetry; import io.opentelemetry.auto.bootstrap.WeakMap; import io.opentelemetry.auto.bootstrap.instrumentation.decorator.BaseDecorator; @@ -43,7 +45,7 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator { public static final Tracer TRACER = OpenTelemetry.getTracerProvider().get("io.opentelemetry.auto.jaxrs-2.0"); - private final WeakMap, Map> spanNames = WeakMap.Provider.newWeakMap(); + private final WeakMap, Map> spanNames = newWeakMap(); public void onJaxRsSpan( final Span span, final Span parent, final Class target, final Method method) { diff --git a/instrumentation/jaxrs/jaxrs-2.0/src/main/java/io/opentelemetry/auto/instrumentation/jaxrs/v2_0/JaxRsAnnotationsInstrumentation.java b/instrumentation/jaxrs/jaxrs-2.0/src/main/java/io/opentelemetry/auto/instrumentation/jaxrs/v2_0/JaxRsAnnotationsInstrumentation.java index ff6d918359..f93bd9c1cd 100644 --- a/instrumentation/jaxrs/jaxrs-2.0/src/main/java/io/opentelemetry/auto/instrumentation/jaxrs/v2_0/JaxRsAnnotationsInstrumentation.java +++ b/instrumentation/jaxrs/jaxrs-2.0/src/main/java/io/opentelemetry/auto/instrumentation/jaxrs/v2_0/JaxRsAnnotationsInstrumentation.java @@ -28,12 +28,14 @@ import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.named; import com.google.auto.service.AutoService; +import io.opentelemetry.auto.bootstrap.CallDepthThreadLocalMap; import io.opentelemetry.auto.bootstrap.InstrumentationContext; import io.opentelemetry.auto.instrumentation.api.SpanWithScope; import io.opentelemetry.auto.tooling.Instrumenter; import io.opentelemetry.trace.Span; import java.lang.reflect.Method; import java.util.Map; +import javax.ws.rs.Path; import javax.ws.rs.container.AsyncResponse; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; @@ -98,6 +100,10 @@ public final class JaxRsAnnotationsInstrumentation extends Instrumenter.Default @Advice.OnMethodEnter(suppress = Throwable.class) public static SpanWithScope nameSpan( @Advice.This final Object target, @Advice.Origin final Method method) { + if (CallDepthThreadLocalMap.incrementCallDepth(Path.class) > 0) { + return null; + } + // Rename the parent span according to the path represented by these annotations. final Span parent = TRACER.getCurrentSpan(); @@ -113,6 +119,11 @@ public final class JaxRsAnnotationsInstrumentation extends Instrumenter.Default @Advice.Enter final SpanWithScope spanWithScope, @Advice.Thrown final Throwable throwable, @Advice.AllArguments final Object[] args) { + if (spanWithScope == null) { + return; + } + CallDepthThreadLocalMap.reset(Path.class); + final Span span = spanWithScope.getSpan(); if (throwable != null) { DECORATE.onError(span, throwable); diff --git a/instrumentation/jaxrs/jaxrs-2.0/src/test/groovy/JaxRsFilterTest.groovy b/instrumentation/jaxrs/jaxrs-2.0/src/test/groovy/JaxRsFilterTest.groovy index a4858b6cbf..850903f35b 100644 --- a/instrumentation/jaxrs/jaxrs-2.0/src/test/groovy/JaxRsFilterTest.groovy +++ b/instrumentation/jaxrs/jaxrs-2.0/src/test/groovy/JaxRsFilterTest.groovy @@ -32,6 +32,7 @@ import javax.ws.rs.core.Response import javax.ws.rs.ext.Provider import static io.opentelemetry.auto.test.utils.TraceUtils.runUnderTrace +import static io.opentelemetry.trace.Span.Kind.INTERNAL @Unroll abstract class JaxRsFilterTest extends AgentTestRunner { @@ -102,6 +103,46 @@ abstract class JaxRsFilterTest extends AgentTestRunner { "/test3/hi/bob" | false | true | null | "PrematchRequestFilter.filter" | "Aborted Prematch" } + def "test nested call"() { + given: + simpleRequestFilter.abort = false + prematchRequestFilter.abort = false + + when: + def responseText + def responseStatus + + // start a trace because the test doesn't go through any servlet or other instrumentation. + runUnderTrace("test.span") { + (responseText, responseStatus) = makeRequest(resource) + } + + then: + responseStatus == Response.Status.OK.statusCode + responseText == expectedResponse + + assertTraces(1) { + trace(0, 2) { + span(0) { + operationName parentResourceName + tags { + } + } + span(1) { + childOf span(0) + operationName controller1Name + spanKind INTERNAL + tags { + } + } + } + } + + where: + resource | parentResourceName | controller1Name | expectedResponse + "/test3/nested" | "POST /test3/nested" | "Test3.nested" | "Test3 nested!" + } + @Provider class SimpleRequestFilter implements ContainerRequestFilter { boolean abort = false diff --git a/instrumentation/jaxrs/jaxrs-2.0/src/test/java/Resource.java b/instrumentation/jaxrs/jaxrs-2.0/src/test/java/Resource.java index 399b8440f4..b04af49d48 100644 --- a/instrumentation/jaxrs/jaxrs-2.0/src/test/java/Resource.java +++ b/instrumentation/jaxrs/jaxrs-2.0/src/test/java/Resource.java @@ -54,5 +54,11 @@ public interface Resource { public String hello(@PathParam("name") final String name) { return "Test3 " + name + "!"; } + + @POST + @Path("/nested") + public String nested() { + return hello("nested"); + } } } diff --git a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Decorator.java b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Decorator.java index e522e62d55..1cc4458521 100644 --- a/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Decorator.java +++ b/instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Decorator.java @@ -19,12 +19,17 @@ import io.opentelemetry.OpenTelemetry; import io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerDecorator; import io.opentelemetry.trace.Span; import io.opentelemetry.trace.Tracer; +import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; +import javax.servlet.Filter; +import javax.servlet.ServletContext; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import lombok.extern.slf4j.Slf4j; +@Slf4j public class Servlet3Decorator extends HttpServerDecorator { public static final Tracer TRACER = @@ -68,18 +73,35 @@ public class Servlet3Decorator public Span onRequest(final Span span, final HttpServletRequest request) { assert span != null; if (request != null) { - final String sc = request.getContextPath(); - if (sc != null && !sc.isEmpty()) { - span.setAttribute("servlet.context", sc); - } - final String sp = request.getServletPath(); - if (sp != null && !sp.isEmpty()) { - span.setAttribute("servlet.path", sp); - } + span.setAttribute("servlet.path", request.getServletPath()); + span.setAttribute("servlet.context", request.getContextPath()); + onContext(span, request, request.getServletContext()); } return super.onRequest(span, request); } + /** + * This method executes the filter created by + * io.opentelemetry.auto.instrumentation.springwebmvc.DispatcherServletInstrumentation$HandlerMappingAdvice. + * This was easier and less "hacky" than other ways to add the filter to the front of the filter + * chain. + */ + private void onContext( + final Span span, final HttpServletRequest request, final ServletContext context) { + final Object attribute = context.getAttribute("ota.dispatcher-filter"); + if (attribute instanceof Filter) { + final Object priorAttr = request.getAttribute(SPAN_ATTRIBUTE); + request.setAttribute(SPAN_ATTRIBUTE, span); + try { + ((Filter) attribute).doFilter(request, null, null); + } catch (final IOException | ServletException e) { + log.debug("Exception unexpectedly thrown by filter", e); + } finally { + request.setAttribute(SPAN_ATTRIBUTE, priorAttr); + } + } + } + @Override public Span onError(final Span span, final Throwable throwable) { if (throwable instanceof ServletException && throwable.getCause() != null) { diff --git a/instrumentation/servlet/src/main/java/io/opentelemetry/auto/instrumentation/servlet/dispatcher/RequestDispatcherInstrumentation.java b/instrumentation/servlet/src/main/java/io/opentelemetry/auto/instrumentation/servlet/dispatcher/RequestDispatcherInstrumentation.java index a2bd4370ac..761fe24454 100644 --- a/instrumentation/servlet/src/main/java/io/opentelemetry/auto/instrumentation/servlet/dispatcher/RequestDispatcherInstrumentation.java +++ b/instrumentation/servlet/src/main/java/io/opentelemetry/auto/instrumentation/servlet/dispatcher/RequestDispatcherInstrumentation.java @@ -89,12 +89,31 @@ public final class RequestDispatcherInstrumentation extends Instrumenter.Default @Advice.This final RequestDispatcher dispatcher, @Advice.Local("_originalServletSpan") Object originalServletSpan, @Advice.Argument(0) final ServletRequest request) { - if (!TRACER.getCurrentSpan().getContext().isValid()) { + final Span parentSpan = TRACER.getCurrentSpan(); + + final Object servletSpanObject = request.getAttribute(SPAN_ATTRIBUTE); + final Span servletSpan = servletSpanObject instanceof Span ? (Span) servletSpanObject : null; + + if (!parentSpan.getContext().isValid() && servletSpan == null) { // Don't want to generate a new top-level span return null; } + final Span parent; + if (servletSpan == null + || (parentSpan.getContext().isValid() + && servletSpan + .getContext() + .getTraceId() + .equals(parentSpan.getContext().getTraceId()))) { + // Use the parentSpan if the servletSpan is null or part of the same trace. + parent = parentSpan; + } else { + // parentSpan is part of a different trace, so lets ignore it. + // This can happen with the way Tomcat does error handling. + parent = servletSpan; + } - final Span span = TRACER.spanBuilder("servlet." + method).startSpan(); + final Span span = TRACER.spanBuilder("servlet." + method).setParent(parent).startSpan(); DECORATE.afterStart(span); final String target = diff --git a/instrumentation/servlet/src/test/groovy/RequestDispatcherTest.groovy b/instrumentation/servlet/src/test/groovy/RequestDispatcherTest.groovy index 92ed4a6fab..1e301cf2ef 100644 --- a/instrumentation/servlet/src/test/groovy/RequestDispatcherTest.groovy +++ b/instrumentation/servlet/src/test/groovy/RequestDispatcherTest.groovy @@ -37,6 +37,7 @@ class RequestDispatcherTest extends AgentTestRunner { dispatcher.include("") then: + 2 * request.getAttribute(SPAN_ATTRIBUTE) assertTraces(2) { trace(0, 1) { basicSpan(it, 0, "forward-child") @@ -57,6 +58,7 @@ class RequestDispatcherTest extends AgentTestRunner { } then: + 1 * request.getAttribute(SPAN_ATTRIBUTE) assertTraces(1) { trace(0, 3) { basicSpan(it, 0, "parent") @@ -103,6 +105,7 @@ class RequestDispatcherTest extends AgentTestRunner { def th = thrown(ServletException) th == ex + 1 * request.getAttribute(SPAN_ATTRIBUTE) assertTraces(1) { trace(0, 3) { basicSpan(it, 0, "parent", null, ex) diff --git a/instrumentation/spring-webmvc-3.1/spring-webmvc-3.1.gradle b/instrumentation/spring-webmvc-3.1/spring-webmvc-3.1.gradle index 1c9e862857..dcf8a39beb 100644 --- a/instrumentation/spring-webmvc-3.1/spring-webmvc-3.1.gradle +++ b/instrumentation/spring-webmvc-3.1/spring-webmvc-3.1.gradle @@ -48,6 +48,7 @@ dependencies { } // Include servlet instrumentation for verifying the tomcat requests + testCompile project(':instrumentation:servlet') testCompile project(':instrumentation:servlet:request-3.0') testCompile group: 'javax.validation', name: 'validation-api', version: '1.1.0.Final' diff --git a/instrumentation/spring-webmvc-3.1/src/main/java/io/opentelemetry/auto/instrumentation/springwebmvc/DispatcherServletInstrumentation.java b/instrumentation/spring-webmvc-3.1/src/main/java/io/opentelemetry/auto/instrumentation/springwebmvc/DispatcherServletInstrumentation.java index ad02318d2b..880239a1ec 100644 --- a/instrumentation/spring-webmvc-3.1/src/main/java/io/opentelemetry/auto/instrumentation/springwebmvc/DispatcherServletInstrumentation.java +++ b/instrumentation/spring-webmvc-3.1/src/main/java/io/opentelemetry/auto/instrumentation/springwebmvc/DispatcherServletInstrumentation.java @@ -18,23 +18,31 @@ package io.opentelemetry.auto.instrumentation.springwebmvc; import static io.opentelemetry.auto.instrumentation.springwebmvc.SpringWebMvcDecorator.DECORATE; import static io.opentelemetry.auto.instrumentation.springwebmvc.SpringWebMvcDecorator.TRACER; import static io.opentelemetry.trace.TracingContextUtils.currentContextWith; +import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isProtected; import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith; import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; +import static net.bytebuddy.matcher.ElementMatchers.takesArguments; import com.google.auto.service.AutoService; +import io.opentelemetry.auto.bootstrap.ContextStore; +import io.opentelemetry.auto.bootstrap.InstrumentationContext; import io.opentelemetry.auto.instrumentation.api.SpanWithScope; import io.opentelemetry.auto.tooling.Instrumenter; import io.opentelemetry.trace.Span; import java.util.HashMap; +import java.util.List; import java.util.Map; +import javax.servlet.ServletContext; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; -import org.springframework.web.method.HandlerMethod; +import org.springframework.context.ApplicationContext; +import org.springframework.web.servlet.DispatcherServlet; +import org.springframework.web.servlet.HandlerMapping; import org.springframework.web.servlet.ModelAndView; @AutoService(Instrumenter.class) @@ -49,20 +57,36 @@ public final class DispatcherServletInstrumentation extends Instrumenter.Default return named("org.springframework.web.servlet.DispatcherServlet"); } + @Override + public Map contextStore() { + return singletonMap( + "org.springframework.web.servlet.DispatcherServlet", + packageName + ".HandlerMappingResourceNameFilter"); + } + @Override public String[] helperClassNames() { - return new String[] {packageName + ".SpringWebMvcDecorator"}; + return new String[] { + packageName + ".SpringWebMvcDecorator", packageName + ".HandlerMappingResourceNameFilter" + }; } @Override public Map, String> transformers() { final Map, String> transformers = new HashMap<>(); + transformers.put( + isMethod() + .and(isProtected()) + .and(named("onRefresh")) + .and(takesArgument(0, named("org.springframework.context.ApplicationContext"))) + .and(takesArguments(1)), + DispatcherServletInstrumentation.class.getName() + "$HandlerMappingAdvice"); transformers.put( isMethod() .and(isProtected()) .and(named("render")) .and(takesArgument(0, named("org.springframework.web.servlet.ModelAndView"))), - DispatcherServletInstrumentation.class.getName() + "$DispatcherAdvice"); + DispatcherServletInstrumentation.class.getName() + "$RenderAdvice"); transformers.put( isMethod() .and(isProtected()) @@ -72,7 +96,37 @@ public final class DispatcherServletInstrumentation extends Instrumenter.Default return transformers; } - public static class DispatcherAdvice { + /** + * This advice creates a filter that has reference to the handlerMappings from DispatcherServlet + * which allows the mappings to be evaluated at the beginning of the filter chain. This evaluation + * is done inside the Servlet3Decorator.onContext method. + */ + public static class HandlerMappingAdvice { + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void afterRefresh( + @Advice.This final DispatcherServlet dispatcher, + @Advice.Argument(0) final ApplicationContext springCtx, + @Advice.FieldValue("handlerMappings") final List handlerMappings, + @Advice.Thrown final Throwable throwable) { + final ServletContext servletContext = springCtx.getBean(ServletContext.class); + if (handlerMappings != null && servletContext != null) { + final ContextStore contextStore = + InstrumentationContext.get( + DispatcherServlet.class, HandlerMappingResourceNameFilter.class); + HandlerMappingResourceNameFilter filter = contextStore.get(dispatcher); + if (filter == null) { + filter = new HandlerMappingResourceNameFilter(); + contextStore.put(dispatcher, filter); + } + filter.setHandlerMappings(handlerMappings); + servletContext.setAttribute( + "ota.dispatcher-filter", filter); // used by Servlet3Decorator.onContext + } + } + } + + public static class RenderAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static SpanWithScope onEnter(@Advice.Argument(0) final ModelAndView mv) { @@ -91,11 +145,6 @@ public final class DispatcherServletInstrumentation extends Instrumenter.Default span.end(); spanWithScope.closeScope(); } - - // Make this advice match consistently with HandlerAdapterInstrumentation - private void muzzleCheck(final HandlerMethod method) { - method.getMethod(); - } } public static class ErrorHandlerAdvice { diff --git a/instrumentation/spring-webmvc-3.1/src/main/java/io/opentelemetry/auto/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java b/instrumentation/spring-webmvc-3.1/src/main/java/io/opentelemetry/auto/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java new file mode 100644 index 0000000000..0334d9fd0a --- /dev/null +++ b/instrumentation/spring-webmvc-3.1/src/main/java/io/opentelemetry/auto/instrumentation/springwebmvc/HandlerMappingResourceNameFilter.java @@ -0,0 +1,80 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.opentelemetry.auto.instrumentation.springwebmvc; + +import static io.opentelemetry.auto.bootstrap.instrumentation.decorator.HttpServerDecorator.SPAN_ATTRIBUTE; +import static io.opentelemetry.auto.instrumentation.springwebmvc.SpringWebMvcDecorator.DECORATE; + +import io.opentelemetry.trace.Span; +import java.util.List; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; +import org.springframework.web.servlet.HandlerExecutionChain; +import org.springframework.web.servlet.HandlerMapping; + +public class HandlerMappingResourceNameFilter implements Filter { + private volatile List handlerMappings; + + @Override + public void init(final FilterConfig filterConfig) {} + + @Override + public void doFilter( + final ServletRequest servletRequest, + final ServletResponse servletResponse, + final FilterChain filterChain) { + if (servletRequest instanceof HttpServletRequest && handlerMappings != null) { + final HttpServletRequest request = (HttpServletRequest) servletRequest; + try { + if (findMapping(request)) { + // Name the parent span based on the matching pattern + final Object parentSpan = request.getAttribute(SPAN_ATTRIBUTE); + if (parentSpan instanceof Span) { + // Let the parent span resource name be set with the attribute set in findMapping. + DECORATE.onRequest((Span) parentSpan, request); + } + } + } catch (final Exception e) { + } + } + } + + @Override + public void destroy() {} + + /** + * When a HandlerMapping matches a request, it sets HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE + * as an attribute on the request. This attribute is read by + * SpringWebHttpServerDecorator.onRequest and set as the resource name. + */ + private boolean findMapping(final HttpServletRequest request) throws Exception { + for (final HandlerMapping mapping : handlerMappings) { + final HandlerExecutionChain handler = mapping.getHandler(request); + if (handler != null) { + return true; + } + } + return false; + } + + public void setHandlerMappings(final List handlerMappings) { + this.handlerMappings = handlerMappings; + } +} diff --git a/instrumentation/spring-webmvc-3.1/src/test/groovy/test/AppConfig.groovy b/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/AppConfig.groovy similarity index 96% rename from instrumentation/spring-webmvc-3.1/src/test/groovy/test/AppConfig.groovy rename to instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/AppConfig.groovy index 140ff6c98c..afc99b6bc0 100644 --- a/instrumentation/spring-webmvc-3.1/src/test/groovy/test/AppConfig.groovy +++ b/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/AppConfig.groovy @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package test +package test.boot import org.apache.catalina.connector.Connector import org.springframework.boot.autoconfigure.SpringBootApplication @@ -40,7 +40,7 @@ class AppConfig extends WebMvcConfigurerAdapter { .defaultContentTypeStrategy(new ContentNegotiationStrategy() { @Override List resolveMediaTypes(NativeWebRequest webRequest) throws HttpMediaTypeNotAcceptableException { - return [MediaType.TEXT_PLAIN, MediaType.APPLICATION_JSON] + return [MediaType.TEXT_PLAIN] } }) } diff --git a/instrumentation/spring-webmvc-3.1/src/test/groovy/test/SpringBootBasedTest.groovy b/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy similarity index 91% rename from instrumentation/spring-webmvc-3.1/src/test/groovy/test/SpringBootBasedTest.groovy rename to instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy index b27982bd85..6774ad12e2 100644 --- a/instrumentation/spring-webmvc-3.1/src/test/groovy/test/SpringBootBasedTest.groovy +++ b/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package test +package test.boot import io.opentelemetry.auto.instrumentation.api.MoreTags import io.opentelemetry.auto.instrumentation.api.Tags @@ -53,6 +53,11 @@ class SpringBootBasedTest extends HttpServerTest true } + @Override + boolean hasResponseSpan(ServerEndpoint endpoint) { + endpoint == REDIRECT + } + @Override boolean hasRenderSpan(ServerEndpoint endpoint) { endpoint == REDIRECT @@ -70,6 +75,17 @@ class SpringBootBasedTest extends HttpServerTest true } + @Override + void responseSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + trace.span(index) { + operationName "HttpServletResponse.sendRedirect" + spanKind INTERNAL + errored false + tags { + } + } + } + @Override void renderSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { trace.span(index) { diff --git a/instrumentation/spring-webmvc-3.1/src/test/groovy/test/TestController.groovy b/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/TestController.groovy similarity index 99% rename from instrumentation/spring-webmvc-3.1/src/test/groovy/test/TestController.groovy rename to instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/TestController.groovy index 7ee55ba5ed..ddfa31c111 100644 --- a/instrumentation/spring-webmvc-3.1/src/test/groovy/test/TestController.groovy +++ b/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/TestController.groovy @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package test +package test.boot import io.opentelemetry.auto.test.base.HttpServerTest import org.springframework.http.HttpStatus diff --git a/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/FilteredAppConfig.groovy b/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/FilteredAppConfig.groovy new file mode 100644 index 0000000000..d04cb85e22 --- /dev/null +++ b/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/FilteredAppConfig.groovy @@ -0,0 +1,156 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package test.filter + +import com.google.common.base.Charsets +import io.opentelemetry.auto.test.base.HttpServerTest +import org.apache.catalina.connector.Connector +import org.springframework.boot.autoconfigure.SpringBootApplication +import org.springframework.boot.context.embedded.EmbeddedServletContainerFactory +import org.springframework.boot.context.embedded.tomcat.TomcatConnectorCustomizer +import org.springframework.boot.context.embedded.tomcat.TomcatEmbeddedServletContainerFactory +import org.springframework.context.annotation.Bean +import org.springframework.http.HttpInputMessage +import org.springframework.http.HttpOutputMessage +import org.springframework.http.MediaType +import org.springframework.http.converter.AbstractHttpMessageConverter +import org.springframework.http.converter.HttpMessageConverter +import org.springframework.http.converter.HttpMessageNotReadableException +import org.springframework.http.converter.HttpMessageNotWritableException +import org.springframework.util.StreamUtils +import org.springframework.web.HttpMediaTypeNotAcceptableException +import org.springframework.web.accept.ContentNegotiationStrategy +import org.springframework.web.context.request.NativeWebRequest +import org.springframework.web.servlet.config.annotation.ContentNegotiationConfigurer +import org.springframework.web.servlet.config.annotation.WebMvcConfigurerAdapter + +import javax.servlet.Filter +import javax.servlet.FilterChain +import javax.servlet.FilterConfig +import javax.servlet.ServletException +import javax.servlet.ServletRequest +import javax.servlet.ServletResponse +import javax.servlet.http.HttpServletRequest +import javax.servlet.http.HttpServletResponse + +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.ERROR +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.REDIRECT +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +@SpringBootApplication +class FilteredAppConfig extends WebMvcConfigurerAdapter { + + @Override + void configureContentNegotiation(ContentNegotiationConfigurer configurer) { + configurer.favorPathExtension(false) + .favorParameter(true) + .ignoreAcceptHeader(true) + .useJaf(false) + .defaultContentTypeStrategy(new ContentNegotiationStrategy() { + @Override + List resolveMediaTypes(NativeWebRequest webRequest) throws HttpMediaTypeNotAcceptableException { + return [MediaType.TEXT_PLAIN] + } + }) + } + + @Bean + EmbeddedServletContainerFactory servletContainerFactory() { + def factory = new TomcatEmbeddedServletContainerFactory() + + factory.addConnectorCustomizers( + new TomcatConnectorCustomizer() { + @Override + void customize(final Connector connector) { + connector.setEnableLookups(true) + } + }) + + return factory + } + + @Bean + HttpMessageConverter> createPlainMapMessageConverter() { + return new AbstractHttpMessageConverter>(MediaType.TEXT_PLAIN) { + + @Override + protected boolean supports(Class clazz) { + return Map.isAssignableFrom(clazz) + } + + @Override + protected Map readInternal(Class> clazz, HttpInputMessage inputMessage) throws IOException, HttpMessageNotReadableException { + return null + } + + @Override + protected void writeInternal(Map stringObjectMap, HttpOutputMessage outputMessage) throws IOException, HttpMessageNotWritableException { + StreamUtils.copy(stringObjectMap.get("message"), Charsets.UTF_8, outputMessage.getBody()) + } + } + } + + @Bean + Filter servletFilter() { + return new Filter() { + + @Override + void init(FilterConfig filterConfig) throws ServletException { + } + + @Override + void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { + HttpServletRequest req = (HttpServletRequest) request + HttpServletResponse resp = (HttpServletResponse) response + HttpServerTest.ServerEndpoint endpoint = HttpServerTest.ServerEndpoint.forPath(req.servletPath) + HttpServerTest.controller(endpoint) { + resp.contentType = "text/plain" + switch (endpoint) { + case SUCCESS: + resp.status = endpoint.status + resp.writer.print(endpoint.body) + break + case QUERY_PARAM: + resp.status = endpoint.status + resp.writer.print(req.queryString) + break + case PATH_PARAM: + resp.status = endpoint.status + resp.writer.print(endpoint.body) + break + case REDIRECT: + resp.sendRedirect(endpoint.body) + break + case ERROR: + resp.sendError(endpoint.status, endpoint.body) + break + case EXCEPTION: + throw new Exception(endpoint.body) + default: + chain.doFilter(request, response) + } + } + } + + @Override + void destroy() { + } + } + } +} diff --git a/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/ServletFilterTest.groovy b/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/ServletFilterTest.groovy new file mode 100644 index 0000000000..4bb2daad61 --- /dev/null +++ b/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/ServletFilterTest.groovy @@ -0,0 +1,162 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package test.filter + +import io.opentelemetry.auto.instrumentation.api.MoreTags +import io.opentelemetry.auto.instrumentation.api.Tags +import io.opentelemetry.auto.test.asserts.TraceAssert +import io.opentelemetry.auto.test.base.HttpServerTest +import io.opentelemetry.sdk.trace.data.SpanData +import org.apache.catalina.core.ApplicationFilterChain +import org.springframework.boot.SpringApplication +import org.springframework.context.ConfigurableApplicationContext + +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.ERROR +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.REDIRECT +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.SUCCESS +import static io.opentelemetry.trace.Span.Kind.INTERNAL +import static io.opentelemetry.trace.Span.Kind.SERVER +import static java.util.Collections.singletonMap + +class ServletFilterTest extends HttpServerTest { + + @Override + ConfigurableApplicationContext startServer(int port) { + def app = new SpringApplication(FilteredAppConfig) + app.setDefaultProperties(singletonMap("server.port", port)) + def context = app.run() + return context + } + + @Override + void stopServer(ConfigurableApplicationContext ctx) { + ctx.close() + } + + @Override + boolean hasHandlerSpan() { + false + } + + @Override + boolean hasResponseSpan(ServerEndpoint endpoint) { + endpoint == REDIRECT || endpoint == ERROR + } + + @Override + boolean hasErrorPageSpans(ServerEndpoint endpoint) { + endpoint == ERROR || endpoint == EXCEPTION + } + + @Override + boolean testPathParam() { + true + } + + @Override + boolean testExceptionBody() { + false + } + + @Override + boolean testNotFound() { + // FIXME: the instrumentation adds an extra controller span which is not consistent. + // Fix tests or remove extra span. + false + } + + @Override + void responseSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + trace.span(index) { + operationName endpoint == REDIRECT ? "HttpServletResponse.sendRedirect" : "HttpServletResponse.sendError" + spanKind INTERNAL + errored false + childOf((SpanData) parent) + tags { + } + } + } + + @Override + void handlerSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + trace.span(index) { + operationName "TestController.${endpoint.name().toLowerCase()}" + spanKind INTERNAL + errored endpoint == EXCEPTION + childOf((SpanData) parent) + tags { + if (endpoint == EXCEPTION) { + errorTags(Exception, EXCEPTION.body) + } + } + } + } + + @Override + void serverSpan(TraceAssert trace, int index, String traceID = null, String parentID = null, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + trace.span(index) { + operationName endpoint == PATH_PARAM ? "/path/{id}/param" : endpoint.resolvePath(address).path + spanKind SERVER + errored endpoint.errored + if (parentID != null) { + traceId traceID + parentId parentID + } else { + parent() + } + tags { + "$MoreTags.NET_PEER_IP" { it == null || it == "127.0.0.1" } // Optional + "$MoreTags.NET_PEER_PORT" Long + "$Tags.HTTP_URL" { it == "${endpoint.resolve(address)}" || it == "${endpoint.resolveWithoutFragment(address)}" } + "$Tags.HTTP_METHOD" method + "$Tags.HTTP_STATUS" endpoint.status + "span.origin.type" ApplicationFilterChain.name + "servlet.path" endpoint.path + if (endpoint.errored) { + "error.msg" { it == null || it == EXCEPTION.body } + "error.type" { it == null || it == Exception.name } + "error.stack" { it == null || it instanceof String } + } + if (endpoint.query) { + "$MoreTags.HTTP_QUERY" endpoint.query + } + } + } + } + + @Override + void errorPageSpans(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + trace.span(index) { + operationName "/error" + spanKind INTERNAL + errored false + childOf((SpanData) parent) + tags { + "dispatcher.target" "/error" + } + } + trace.span(index + 1) { + operationName "BasicErrorController.error" + spanKind INTERNAL + errored false + childOf trace.spans[index] + tags { + } + } + } +} diff --git a/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/TestController.groovy b/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/TestController.groovy new file mode 100644 index 0000000000..7c75263550 --- /dev/null +++ b/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/TestController.groovy @@ -0,0 +1,89 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package test.filter + +import io.opentelemetry.auto.test.base.HttpServerTest +import org.springframework.http.HttpStatus +import org.springframework.http.ResponseEntity +import org.springframework.stereotype.Controller +import org.springframework.web.bind.annotation.ExceptionHandler +import org.springframework.web.bind.annotation.PathVariable +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.RequestParam +import org.springframework.web.bind.annotation.ResponseBody +import org.springframework.web.servlet.view.RedirectView + +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.ERROR +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.REDIRECT +import static io.opentelemetry.auto.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +@Controller +class TestController { + + @RequestMapping("/success") + @ResponseBody + String success() { + HttpServerTest.controller(SUCCESS) { + SUCCESS.body + } + } + + @RequestMapping("/query") + @ResponseBody + String query_param(@RequestParam("some") String param) { + HttpServerTest.controller(QUERY_PARAM) { + "some=$param" + } + } + + @RequestMapping("/path/{id}/param") + @ResponseBody + String path_param(@PathVariable Integer id) { + HttpServerTest.controller(PATH_PARAM) { + "$id" + } + } + + @RequestMapping("/redirect") + @ResponseBody + RedirectView redirect() { + HttpServerTest.controller(REDIRECT) { + new RedirectView(REDIRECT.body) + } + } + + @RequestMapping("/error-status") + ResponseEntity error() { + HttpServerTest.controller(ERROR) { + new ResponseEntity(ERROR.body, HttpStatus.valueOf(ERROR.status)) + } + } + + @RequestMapping("/exception") + ResponseEntity exception() { + HttpServerTest.controller(EXCEPTION) { + throw new Exception(EXCEPTION.body) + } + } + + @ExceptionHandler + ResponseEntity handleException(Throwable throwable) { + new ResponseEntity(throwable.message, HttpStatus.INTERNAL_SERVER_ERROR) + } +} diff --git a/instrumentation/trace-annotation/src/test/groovy/TraceAnnotationsTest.groovy b/instrumentation/trace-annotation/src/test/groovy/TraceAnnotationsTest.groovy index f3adcd968d..7336f03575 100644 --- a/instrumentation/trace-annotation/src/test/groovy/TraceAnnotationsTest.groovy +++ b/instrumentation/trace-annotation/src/test/groovy/TraceAnnotationsTest.groovy @@ -86,7 +86,6 @@ class TraceAnnotationsTest extends AgentTestRunner { def "test exception exit"() { setup: - Throwable error = null try { SayTracedHello.sayERROR() diff --git a/instrumentation/twilio-6.6/src/test/groovy/test/TwilioClientTest.groovy b/instrumentation/twilio-6.6/src/test/groovy/test/TwilioClientTest.groovy index 2c3f8596a6..40766a02f2 100644 --- a/instrumentation/twilio-6.6/src/test/groovy/test/TwilioClientTest.groovy +++ b/instrumentation/twilio-6.6/src/test/groovy/test/TwilioClientTest.groovy @@ -41,7 +41,6 @@ import java.util.concurrent.TimeUnit import static io.opentelemetry.auto.test.utils.TraceUtils.runUnderTrace import static io.opentelemetry.trace.Span.Kind.CLIENT -import static io.opentelemetry.trace.TracingContextUtils.currentContextWith class TwilioClientTest extends AgentTestRunner { final static String ACCOUNT_SID = "abc" @@ -122,6 +121,12 @@ class TwilioClientTest extends AgentTestRunner { Twilio.init(ACCOUNT_SID, AUTH_TOKEN) } + def cleanup() { + Twilio.getExecutorService().shutdown() + Twilio.setExecutorService(null) + Twilio.setRestClient(null) + } + def "synchronous message"() { setup: twilioRestClient.getObjectMapper() >> new ObjectMapper() @@ -340,7 +345,6 @@ class TwilioClientTest extends AgentTestRunner { } expect: - message.body == "Hello, World!" assertTraces(1) { @@ -436,7 +440,6 @@ class TwilioClientTest extends AgentTestRunner { .build() Message message = runUnderTrace("test") { - ListenableFuture future = Message.creator( new PhoneNumber("+1 555 720 5913"), // To number new PhoneNumber("+1 555 555 5215"), // From number @@ -452,7 +455,6 @@ class TwilioClientTest extends AgentTestRunner { } expect: - message.body == "Hello, World!" assertTraces(1) { @@ -524,31 +526,27 @@ class TwilioClientTest extends AgentTestRunner { 1 * twilioRestClient.request(_) >> new Response(new ByteArrayInputStream(ERROR_RESPONSE_BODY.getBytes()), 500) - def testSpan = TEST_TRACER.spanBuilder("test").startSpan() - def testScope = currentContextWith(testSpan) - when: - Message.creator( - new PhoneNumber("+1 555 720 5913"), // To number - new PhoneNumber("+1 555 555 5215"), // From number - "Hello world!" // SMS body - ).create(twilioRestClient) + runUnderTrace("test") { + Message.creator( + new PhoneNumber("+1 555 720 5913"), // To number + new PhoneNumber("+1 555 555 5215"), // From number + "Hello world!" // SMS body + ).create(twilioRestClient) + } then: thrown(ApiException) - testSpan.end() - testScope.close() - expect: - assertTraces(1) { trace(0, 2) { span(0) { operationName "test" - errored false + errored true parent() tags { + errorTags(ApiException, "Testing Failure") } } span(1) { @@ -677,25 +675,19 @@ class TwilioClientTest extends AgentTestRunner { 1 * twilioRestClient.request(_) >> new Response(new ByteArrayInputStream(ERROR_RESPONSE_BODY.getBytes()), 500) - def testSpan = TEST_TRACER.spanBuilder("test").startSpan() - def testScope = currentContextWith(testSpan) - - ListenableFuture future = Message.creator( - new PhoneNumber("+1 555 720 5913"), // To number - new PhoneNumber("+1 555 555 5215"), // From number - "Hello world!" // SMS body - ).createAsync(twilioRestClient) - - when: - Message message - try { - message = future.get(10, TimeUnit.SECONDS) + runUnderTrace("test") { + ListenableFuture future = Message.creator( + new PhoneNumber("+1 555 720 5913"), // To number + new PhoneNumber("+1 555 555 5215"), // From number + "Hello world!" // SMS body + ).createAsync(twilioRestClient) - } finally { - Thread.sleep(1000) - testSpan.end() - testScope.close() + try { + return future.get(10, TimeUnit.SECONDS) + } finally { + Thread.sleep(1000) + } } then: @@ -707,9 +699,10 @@ class TwilioClientTest extends AgentTestRunner { trace(0, 3) { span(0) { operationName "test" - errored false + errored true parent() tags { + errorTags(ApiException, "Testing Failure") } } span(1) { @@ -730,11 +723,6 @@ class TwilioClientTest extends AgentTestRunner { } } } - - cleanup: - Twilio.getExecutorService().shutdown() - Twilio.setExecutorService(null) - Twilio.setRestClient(null) } String expectedOperationName(String method) { diff --git a/testing/src/main/groovy/io/opentelemetry/auto/test/SpockRunner.java b/testing/src/main/groovy/io/opentelemetry/auto/test/SpockRunner.java index 465d82834d..b1ce47293d 100644 --- a/testing/src/main/groovy/io/opentelemetry/auto/test/SpockRunner.java +++ b/testing/src/main/groovy/io/opentelemetry/auto/test/SpockRunner.java @@ -46,6 +46,7 @@ public class SpockRunner extends Sputnik { * references bootstrap classes (e.g. AgentClassLoader). */ public static final String[] BOOTSTRAP_PACKAGE_PREFIXES_COPY = { + "io.opentelemetry.auto.common.exec", "io.opentelemetry.auto.slf4j", "io.opentelemetry.auto.config", "io.opentelemetry.auto.bootstrap", diff --git a/testing/src/main/groovy/io/opentelemetry/auto/test/base/HttpServerTest.groovy b/testing/src/main/groovy/io/opentelemetry/auto/test/base/HttpServerTest.groovy index ba3c429c9d..1826c21047 100644 --- a/testing/src/main/groovy/io/opentelemetry/auto/test/base/HttpServerTest.groovy +++ b/testing/src/main/groovy/io/opentelemetry/auto/test/base/HttpServerTest.groovy @@ -100,6 +100,14 @@ abstract class HttpServerTest extends AgentTestRunner { false } + boolean hasResponseSpan(ServerEndpoint endpoint) { + false + } + + boolean hasErrorPageSpans(ServerEndpoint endpoint) { + false + } + boolean redirectHasBody() { false } @@ -360,6 +368,12 @@ abstract class HttpServerTest extends AgentTestRunner { if (hasRenderSpan(endpoint)) { spanCount++ } + if (hasResponseSpan(endpoint)) { + spanCount++ + } + if (hasErrorPageSpans(endpoint)) { + spanCount += 2 + } } assertTraces(size * 2) { (0..size - 1).each { @@ -381,6 +395,13 @@ abstract class HttpServerTest extends AgentTestRunner { if (hasRenderSpan(endpoint)) { renderSpan(it, spanIndex++, span(0), method, endpoint) } + if (hasResponseSpan(endpoint)) { + responseSpan(it, spanIndex, span(spanIndex - 1), method, endpoint) + spanIndex++ + } + if (hasErrorPageSpans(endpoint)) { + errorPageSpans(it, spanIndex, span(0), method, endpoint) + } } } } @@ -408,6 +429,14 @@ abstract class HttpServerTest extends AgentTestRunner { throw new UnsupportedOperationException("renderSpan not implemented in " + getClass().name) } + void responseSpan(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + throw new UnsupportedOperationException("responseSpan not implemented in " + getClass().name) + } + + void errorPageSpans(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + throw new UnsupportedOperationException("errorPageSpans not implemented in " + getClass().name) + } + // parent span must be cast otherwise it breaks debugging classloading (junit loads it early) void serverSpan(TraceAssert trace, int index, String traceID = null, String parentID = null, String method = "GET", ServerEndpoint endpoint = SUCCESS) { trace.span(index) { diff --git a/utils/thread-utils/src/main/java/io/opentelemetry/auto/common/exec/CommonTaskExecutor.java b/utils/thread-utils/src/main/java/io/opentelemetry/auto/common/exec/CommonTaskExecutor.java index 159202f9f6..18757ebded 100644 --- a/utils/thread-utils/src/main/java/io/opentelemetry/auto/common/exec/CommonTaskExecutor.java +++ b/utils/thread-utils/src/main/java/io/opentelemetry/auto/common/exec/CommonTaskExecutor.java @@ -15,9 +15,12 @@ */ package io.opentelemetry.auto.common.exec; +import java.lang.ref.WeakReference; import java.util.List; import java.util.concurrent.AbstractExecutorService; +import java.util.concurrent.Delayed; import java.util.concurrent.Executors; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -40,9 +43,56 @@ public final class CommonTaskExecutor extends AbstractExecutorService { } } - public ScheduledFuture scheduleAtFixedRate( - final Runnable command, final long initialDelay, final long period, final TimeUnit unit) { - return executorService.scheduleAtFixedRate(command, initialDelay, period, unit); + /** + * Run {@code task} periodically providing it with {@code target} + * + *

Important implementation detail here is that internally we do not hold any strong references + * to {@code target} which means it can be GCed even while periodic task is still scheduled. + * + *

If {@code target} is GCed periodic task is canceled. + * + *

This method should be able to schedule task in majority of cases. The only reasonable case + * when this would fail is when task is being scheduled during JVM shutdown. In this case this + * method will return 'fake' future that can still be canceled to avoid confusing callers. + * + * @param task task to run. Important: must not hold any strong references to target (or anything + * else non static) + * @param target target object to pass to task + * @param initialDelay initialDelay, see {@link + * ScheduledExecutorService#scheduleAtFixedRate(Runnable, long, long, TimeUnit)} + * @param period period, see {@link ScheduledExecutorService#scheduleAtFixedRate(Runnable, long, + * long, TimeUnit)} + * @param unit unit, see {@link ScheduledExecutorService#scheduleAtFixedRate(Runnable, long, long, + * TimeUnit)} + * @param name name to use in logs when task cannot be scheduled + * @return future that can be canceled + */ + public ScheduledFuture scheduleAtFixedRate( + final Task task, + final T target, + final long initialDelay, + final long period, + final TimeUnit unit, + final String name) { + if (CommonTaskExecutor.INSTANCE.isShutdown()) { + log.warn("Periodic task scheduler is shutdown. Will not run: {}", name); + } else { + try { + final PeriodicTask periodicTask = new PeriodicTask<>(task, target); + final ScheduledFuture future = + executorService.scheduleAtFixedRate( + new PeriodicTask<>(task, target), initialDelay, period, unit); + periodicTask.setFuture(future); + return future; + } catch (final RejectedExecutionException e) { + log.warn("Periodic task rejected. Will not run: {}", name); + } + } + /* + * Return a 'fake' unscheduled future to allow caller call 'cancel' on it if needed. + * We are using 'fake' object instead of null to avoid callers needing to deal with nulls. + */ + return new UnscheduledFuture(name); } @Override @@ -97,4 +147,79 @@ public final class CommonTaskExecutor extends AbstractExecutorService { } } } + + public interface Task { + void run(T target); + } + + private static class PeriodicTask implements Runnable { + private final WeakReference target; + private final Task task; + private volatile ScheduledFuture future = null; + + public PeriodicTask(final Task task, final T target) { + this.target = new WeakReference<>(target); + this.task = task; + } + + @Override + public void run() { + final T t = target.get(); + if (t != null) { + task.run(t); + } else if (future != null) { + future.cancel(false); + } + } + + public void setFuture(final ScheduledFuture future) { + this.future = future; + } + } + + // Unscheduled future + @Slf4j + private static class UnscheduledFuture implements ScheduledFuture { + private final String name; + + public UnscheduledFuture(final String name) { + this.name = name; + } + + @Override + public long getDelay(final TimeUnit unit) { + return 0; + } + + @Override + public int compareTo(final Delayed o) { + return 0; + } + + @Override + public boolean cancel(final boolean mayInterruptIfRunning) { + log.debug("Cancelling unscheduled future for: {}", name); + return false; + } + + @Override + public boolean isCancelled() { + return false; + } + + @Override + public boolean isDone() { + return false; + } + + @Override + public Object get() { + return null; + } + + @Override + public Object get(final long timeout, final TimeUnit unit) { + return null; + } + } } diff --git a/agent-tooling/src/test/groovy/io/opentelemetry/auto/tooling/CleanerTest.groovy b/utils/thread-utils/src/test/groovy/io/opentelemetry/auto/common/exec/PeriodicSchedulingTest.groovy similarity index 69% rename from agent-tooling/src/test/groovy/io/opentelemetry/auto/tooling/CleanerTest.groovy rename to utils/thread-utils/src/test/groovy/io/opentelemetry/auto/common/exec/PeriodicSchedulingTest.groovy index 5f0b69f279..e809171a32 100644 --- a/agent-tooling/src/test/groovy/io/opentelemetry/auto/tooling/CleanerTest.groovy +++ b/utils/thread-utils/src/test/groovy/io/opentelemetry/auto/common/exec/PeriodicSchedulingTest.groovy @@ -13,13 +13,11 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package io.opentelemetry.auto.tooling +package io.opentelemetry.auto.common.exec -import io.opentelemetry.auto.common.exec.CommonTaskExecutor import io.opentelemetry.auto.util.gc.GCUtils import io.opentelemetry.auto.util.test.AgentSpecification import spock.lang.Retry -import spock.lang.Subject import java.lang.ref.WeakReference import java.util.concurrent.CountDownLatch @@ -28,19 +26,15 @@ import java.util.concurrent.atomic.AtomicInteger import static java.util.concurrent.TimeUnit.MILLISECONDS @Retry -class CleanerTest extends AgentSpecification { - - @Subject - def cleaner = new Cleaner() +class PeriodicSchedulingTest extends AgentSpecification { def "test scheduling"() { setup: def latch = new CountDownLatch(2) - def target = new Object() - def action = new Cleaner.Adapter() { + def task = new CommonTaskExecutor.Task() { @Override - void clean(Object t) { - latch.countDown() + void run(CountDownLatch target) { + target.countDown() } } @@ -48,7 +42,7 @@ class CleanerTest extends AgentSpecification { !CommonTaskExecutor.INSTANCE.isShutdown() when: - cleaner.scheduleCleaning(target, action, 10, MILLISECONDS) + CommonTaskExecutor.INSTANCE.scheduleAtFixedRate(task, latch, 10, 10, MILLISECONDS, "test") then: latch.await(500, MILLISECONDS) @@ -58,10 +52,10 @@ class CleanerTest extends AgentSpecification { setup: def callCount = new AtomicInteger() def target = new WeakReference(new Object()) - def action = new Cleaner.Adapter() { + def task = new CommonTaskExecutor.Task() { @Override - void clean(Object t) { - callCount.incrementAndGet() + void run(Object t) { + callCount.countDown() } } @@ -69,7 +63,7 @@ class CleanerTest extends AgentSpecification { !CommonTaskExecutor.INSTANCE.isShutdown() when: - cleaner.scheduleCleaning(target.get(), action, 10, MILLISECONDS) + CommonTaskExecutor.INSTANCE.scheduleAtFixedRate(task, target.get(), 10, 10, MILLISECONDS, "test") GCUtils.awaitGC(target) Thread.sleep(1) def snapshot = callCount.get() @@ -82,10 +76,10 @@ class CleanerTest extends AgentSpecification { def "test null target"() { setup: def callCount = new AtomicInteger() - def action = new Cleaner.Adapter() { + def task = new CommonTaskExecutor.Task() { @Override - void clean(Object t) { - callCount.incrementAndGet() + void run(Object t) { + callCount.countDown() } } @@ -93,7 +87,7 @@ class CleanerTest extends AgentSpecification { !CommonTaskExecutor.INSTANCE.isShutdown() when: - cleaner.scheduleCleaning(null, action, 10, MILLISECONDS) + CommonTaskExecutor.INSTANCE.scheduleAtFixedRate(task, null, 10, 10, MILLISECONDS, "test") Thread.sleep(11) then: diff --git a/utils/thread-utils/thread-utils.gradle b/utils/thread-utils/thread-utils.gradle index 29bb468d6e..fd5a0b31c8 100644 --- a/utils/thread-utils/thread-utils.gradle +++ b/utils/thread-utils/thread-utils.gradle @@ -1,5 +1,12 @@ apply from: "${rootDir}/gradle/java.gradle" +// TODO: add more tests +excludedClassesCoverage += [ + 'datadog.common.exec*' +] + dependencies { compile deps.slf4j + + testCompile project(':utils:test-utils') }