From 7a3f345c18b141c10108220f949b986e3ffdc895 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Tue, 2 Feb 2021 09:12:46 +0100 Subject: [PATCH] Clean up BaseTracer, part 1 (#2159) * Move context leak debugging to ContextPropagationDebug * Remove getCurrentSpan() * Add end(Context, ...) & endExceptionally(Context, ...) methods - they're supposed to replace the ones that accept Spans in the future --- .../api/context/ContextPropagationDebug.java | 42 +++++++++++++ .../api/tracer/BaseTracer.java | 60 ++++++------------- .../api/tracer/DatabaseClientTracer.java | 9 --- .../api/tracer/HttpClientTracer.java | 10 ---- .../apachecamel/CamelRoutePolicy.java | 4 +- .../batch/chunk/ChunkExecutionTracer.java | 8 --- .../spring/batch/item/ItemTracer.java | 8 --- .../spring/batch/job/JobExecutionTracer.java | 4 -- .../batch/step/StepExecutionTracer.java | 4 -- .../instrumentation/twilio/TwilioTracer.java | 4 -- 10 files changed, 62 insertions(+), 91 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/context/ContextPropagationDebug.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/context/ContextPropagationDebug.java index acce17558c..8b7be1e930 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/context/ContextPropagationDebug.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/context/ContextPropagationDebug.java @@ -5,18 +5,26 @@ package io.opentelemetry.instrumentation.api.context; +import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; import io.opentelemetry.context.ContextKey; +import java.util.Iterator; import java.util.List; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public final class ContextPropagationDebug { + private static final Logger log = LoggerFactory.getLogger(ContextPropagationDebug.class); // locations where the context was propagated to another thread (tracking multiple steps is // helpful in akka where there is so much recursive async spawning of new work) private static final ContextKey> THREAD_PROPAGATION_LOCATIONS = ContextKey.named("thread-propagation-locations"); + private static final boolean THREAD_PROPAGATION_DEBUGGER = Boolean.getBoolean("otel.threadPropagationDebugger"); + private static final boolean FAIL_ON_CONTEXT_LEAK = + Boolean.getBoolean("otel.internal.failOnContextLeak"); public static boolean isThreadPropagationDebuggerEnabled() { return THREAD_PROPAGATION_DEBUGGER; @@ -30,5 +38,39 @@ public final class ContextPropagationDebug { return context.with(THREAD_PROPAGATION_LOCATIONS, locations); } + public static void debugContextLeakIfEnabled() { + if (!isThreadPropagationDebuggerEnabled()) { + return; + } + + Context current = Context.current(); + if (current != Context.root()) { + log.error("Unexpected non-root current context found when extracting remote context!"); + Span currentSpan = Span.fromContextOrNull(current); + if (currentSpan != null) { + log.error("It contains this span: {}", currentSpan); + } + List locations = ContextPropagationDebug.getLocations(current); + if (locations != null) { + StringBuilder sb = new StringBuilder(); + Iterator i = locations.iterator(); + while (i.hasNext()) { + for (StackTraceElement ste : i.next()) { + sb.append("\n"); + sb.append(ste); + } + if (i.hasNext()) { + sb.append("\nwhich was propagated from:"); + } + } + log.error("a context leak was detected. it was propagated from:{}", sb); + } + + if (FAIL_ON_CONTEXT_LEAK) { + throw new IllegalStateException("Context leak detected"); + } + } + } + private ContextPropagationDebug() {} } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java index fd3dcc201f..a25f884922 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java @@ -17,8 +17,6 @@ import io.opentelemetry.context.propagation.TextMapPropagator; import io.opentelemetry.instrumentation.api.InstrumentationVersion; import io.opentelemetry.instrumentation.api.context.ContextPropagationDebug; import java.lang.reflect.Method; -import java.util.Iterator; -import java.util.List; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import org.slf4j.Logger; @@ -27,9 +25,6 @@ import org.slf4j.LoggerFactory; public abstract class BaseTracer { private static final Logger log = LoggerFactory.getLogger(HttpServerTracer.class); - private static final boolean FAIL_ON_CONTEXT_LEAK = - Boolean.getBoolean("otel.internal.failOnContextLeak"); - // Keeps track of the server span for the current trace. // TODO(anuraaga): Should probably be renamed to local root key since it could be a consumer span // or other non-server root. @@ -76,10 +71,6 @@ public abstract class BaseTracer { return Context.current().with(span).makeCurrent(); } - public Span getCurrentSpan() { - return Span.current(); - } - protected final boolean shouldStartSpan(Kind proposedKind, Context context) { switch (proposedKind) { case CLIENT: @@ -146,6 +137,14 @@ public abstract class BaseTracer { return className; } + public void end(Context context) { + end(context, -1); + } + + public void end(Context context, long endTimeNanos) { + end(Span.fromContext(context), endTimeNanos); + } + public void end(Span span) { end(span, -1); } @@ -158,6 +157,14 @@ public abstract class BaseTracer { } } + public void endExceptionally(Context context, Throwable throwable) { + endExceptionally(context, throwable, -1); + } + + public void endExceptionally(Context context, Throwable throwable, long endTimeNanos) { + endExceptionally(Span.fromContext(context), throwable, endTimeNanos); + } + public void endExceptionally(Span span, Throwable throwable) { endExceptionally(span, throwable, -1); } @@ -181,9 +188,8 @@ public abstract class BaseTracer { } public static Context extract(C carrier, TextMapPropagator.Getter getter) { - if (ContextPropagationDebug.isThreadPropagationDebuggerEnabled()) { - debugContextLeak(); - } + ContextPropagationDebug.debugContextLeakIfEnabled(); + // Using Context.ROOT here may be quite unexpected, but the reason is simple. // We want either span context extracted from the carrier or invalid one. // We DO NOT want any span context potentially lingering in the current context. @@ -192,36 +198,6 @@ public abstract class BaseTracer { .extract(Context.root(), carrier, getter); } - private static void debugContextLeak() { - Context current = Context.current(); - if (current != Context.root()) { - log.error("Unexpected non-root current context found when extracting remote context!"); - Span currentSpan = Span.fromContextOrNull(current); - if (currentSpan != null) { - log.error("It contains this span: {}", currentSpan); - } - List locations = ContextPropagationDebug.getLocations(current); - if (locations != null) { - StringBuilder sb = new StringBuilder(); - Iterator i = locations.iterator(); - while (i.hasNext()) { - for (StackTraceElement ste : i.next()) { - sb.append("\n"); - sb.append(ste); - } - if (i.hasNext()) { - sb.append("\nwhich was propagated from:"); - } - } - log.error("a context leak was detected. it was propagated from:{}", sb); - } - - if (FAIL_ON_CONTEXT_LEAK) { - throw new IllegalStateException("Context leak detected"); - } - } - } - /** Returns span of type SERVER from the current context or null if not found. */ public static Span getCurrentServerSpan() { return getCurrentServerSpan(Context.current()); diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/DatabaseClientTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/DatabaseClientTracer.java index 421750e541..6a1229ede1 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/DatabaseClientTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/DatabaseClientTracer.java @@ -51,15 +51,6 @@ public abstract class DatabaseClientTracer extends BaseTracer return withClientSpan(parentContext, span); } - @Override - public Span getCurrentSpan() { - return Span.current(); - } - - public void end(Context context) { - Span.fromContext(context).end(); - } - public void endExceptionally(Context context, Throwable throwable) { Span span = Span.fromContext(context); onError(span, throwable); diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java index 5d34b6514d..bce23e0dc5 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java @@ -103,11 +103,6 @@ public abstract class HttpClientTracer extends BaseT super.end(span, endTimeNanos); } - public void end(Context context) { - Span span = Span.fromContext(context); - super.end(span); - } - public void endExceptionally(Context context, RESPONSE response, Throwable throwable) { endExceptionally(context, response, throwable, -1); } @@ -119,11 +114,6 @@ public abstract class HttpClientTracer extends BaseT super.endExceptionally(span, throwable, endTimeNanos); } - public void endExceptionally(Context context, Throwable throwable) { - Span span = Span.fromContext(context); - super.endExceptionally(span, throwable, -1); - } - // TODO (trask) see if we can reduce the number of end..() variants // see https://github.com/open-telemetry/opentelemetry-java-instrumentation // /pull/1893#discussion_r542111699 diff --git a/instrumentation/apache-camel-2.20/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachecamel/CamelRoutePolicy.java b/instrumentation/apache-camel-2.20/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachecamel/CamelRoutePolicy.java index ffe50cb905..b66ccaea0e 100644 --- a/instrumentation/apache-camel-2.20/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachecamel/CamelRoutePolicy.java +++ b/instrumentation/apache-camel-2.20/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachecamel/CamelRoutePolicy.java @@ -38,7 +38,7 @@ final class CamelRoutePolicy extends RoutePolicySupport { private Span spanOnExchangeBegin( Route route, Exchange exchange, SpanDecorator sd, Span.Kind spanKind) { - Span activeSpan = CamelTracer.TRACER.getCurrentSpan(); + Span activeSpan = Span.current(); String name = sd.getOperationName(exchange, route.getEndpoint(), CamelDirection.INBOUND); SpanBuilder builder = CamelTracer.TRACER.spanBuilder(name); builder.setSpanKind(spanKind); @@ -52,7 +52,7 @@ final class CamelRoutePolicy extends RoutePolicySupport { } private Span.Kind spanKind(SpanDecorator sd) { - Span activeSpan = CamelTracer.TRACER.getCurrentSpan(); + Span activeSpan = Span.current(); // if there's an active span, this is not a root span which we always mark as INTERNAL return (activeSpan.getSpanContext().isValid() ? Span.Kind.INTERNAL : sd.getReceiverSpanKind()); } diff --git a/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/chunk/ChunkExecutionTracer.java b/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/chunk/ChunkExecutionTracer.java index 867b9c73f8..a478ccbd01 100644 --- a/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/chunk/ChunkExecutionTracer.java +++ b/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/chunk/ChunkExecutionTracer.java @@ -43,14 +43,6 @@ public class ChunkExecutionTracer extends BaseTracer { } } - public void end(Context context) { - end(Span.fromContext(context)); - } - - public void endExceptionally(Context context, Throwable throwable) { - endExceptionally(Span.fromContext(context), throwable); - } - @Override protected String getInstrumentationName() { return "io.opentelemetry.javaagent.spring-batch"; diff --git a/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/item/ItemTracer.java b/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/item/ItemTracer.java index 14aaca49fe..5c2563dbc1 100644 --- a/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/item/ItemTracer.java +++ b/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/item/ItemTracer.java @@ -68,14 +68,6 @@ public class ItemTracer extends BaseTracer { return currentContext.with(span); } - public void end(Context context) { - end(Span.fromContext(context)); - } - - public void endExceptionally(Context context, Throwable throwable) { - endExceptionally(Span.fromContext(context), throwable); - } - @Override protected String getInstrumentationName() { return "io.opentelemetry.javaagent.spring-batch"; diff --git a/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/job/JobExecutionTracer.java b/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/job/JobExecutionTracer.java index 5e76b61ae2..422c65f15b 100644 --- a/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/job/JobExecutionTracer.java +++ b/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/job/JobExecutionTracer.java @@ -25,10 +25,6 @@ public class JobExecutionTracer extends BaseTracer { return Context.current().with(span); } - public void end(Context context) { - end(Span.fromContext(context)); - } - @Override protected String getInstrumentationName() { return "io.opentelemetry.javaagent.spring-batch"; diff --git a/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/step/StepExecutionTracer.java b/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/step/StepExecutionTracer.java index 3fb666702f..ec109f12b5 100644 --- a/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/step/StepExecutionTracer.java +++ b/instrumentation/spring/spring-batch-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/batch/step/StepExecutionTracer.java @@ -26,10 +26,6 @@ public class StepExecutionTracer extends BaseTracer { return Context.current().with(span); } - public void end(Context context) { - end(Span.fromContext(context)); - } - @Override protected String getInstrumentationName() { return "io.opentelemetry.javaagent.spring-batch"; diff --git a/instrumentation/twilio-6.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/twilio/TwilioTracer.java b/instrumentation/twilio-6.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/twilio/TwilioTracer.java index 89e2a2954d..2624a2ccd5 100644 --- a/instrumentation/twilio-6.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/twilio/TwilioTracer.java +++ b/instrumentation/twilio-6.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/twilio/TwilioTracer.java @@ -100,10 +100,6 @@ public class TwilioTracer extends BaseTracer { super.end(span); } - public void endExceptionally(Context context, Throwable throwable) { - super.endExceptionally(Span.fromContext(context), throwable); - } - /** * Helper method for calling a getter using reflection. This will be slow, so only use when * required.