diff --git a/instrumentation/executors/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/javaconcurrent/JavaExecutorInstrumentation.java b/instrumentation/executors/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/javaconcurrent/JavaExecutorInstrumentation.java index 66b60cd6f3..809a847f01 100644 --- a/instrumentation/executors/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/javaconcurrent/JavaExecutorInstrumentation.java +++ b/instrumentation/executors/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/javaconcurrent/JavaExecutorInstrumentation.java @@ -184,17 +184,20 @@ public class JavaExecutorInstrumentation extends AbstractExecutorInstrumentation Collection> wrappedTasks = new ArrayList<>(tasks.size()); Context context = Java8BytecodeBridge.currentContext(); for (Callable task : tasks) { - // TODO: missing shouldPropagateContext() check - if (task != null) { + if (ExecutorAdviceHelper.shouldPropagateContext(context, task)) { Callable newTask = CallableWrapper.wrapIfNeeded(task); wrappedTasks.add(newTask); ContextStore, PropagatedContext> contextStore = InstrumentationContext.get(Callable.class, PropagatedContext.class); ExecutorAdviceHelper.attachContextToTask(context, contextStore, newTask); + } else { + // note that task may be null here + wrappedTasks.add(task); } } tasks = wrappedTasks; - // TODO: why are we returning tasks and not just states? + // returning tasks and not propagatedContexts to avoid allocating another list just for an + // edge case (exception) return tasks; } @@ -211,23 +214,13 @@ public class JavaExecutorInstrumentation extends AbstractExecutorInstrumentation any parent spans in case of an error. (according to ExecutorService docs and AbstractExecutorService code) */ - if (null != throwable) { + if (throwable != null) { for (Callable task : wrappedTasks) { if (task != null) { ContextStore, PropagatedContext> contextStore = InstrumentationContext.get(Callable.class, PropagatedContext.class); PropagatedContext propagatedContext = contextStore.get(task); - if (propagatedContext != null) { - /* - Note: this may potentially clear somebody else's parent span if we didn't set it - up in setupState because it was already present before us. This should be safe but - may lead to non-attributed async work in some very rare cases. - Alternative is to not clear parent span here if we did not set it up in setupState - but this may potentially lead to memory leaks if callers do not properly handle - exceptions. - */ - propagatedContext.clear(); - } + ExecutorAdviceHelper.cleanUpAfterSubmit(propagatedContext, throwable); } } } diff --git a/instrumentation/guava-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/guava/GuavaListenableFutureInstrumentation.java b/instrumentation/guava-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/guava/GuavaListenableFutureInstrumentation.java index bf294ee85a..231c30fe5b 100644 --- a/instrumentation/guava-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/guava/GuavaListenableFutureInstrumentation.java +++ b/instrumentation/guava-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/guava/GuavaListenableFutureInstrumentation.java @@ -53,14 +53,12 @@ public class GuavaListenableFutureInstrumentation implements TypeInstrumentation @Advice.OnMethodEnter(suppress = Throwable.class) public static PropagatedContext addListenerEnter( @Advice.Argument(value = 0, readOnly = false) Runnable task) { - Runnable newTask = RunnableWrapper.wrapIfNeeded(task); Context context = Java8BytecodeBridge.currentContext(); - // TODO: shouldn't we check task? not newTask? - if (ExecutorAdviceHelper.shouldPropagateContext(context, newTask)) { - task = newTask; + if (ExecutorAdviceHelper.shouldPropagateContext(context, task)) { + task = RunnableWrapper.wrapIfNeeded(task); ContextStore contextStore = InstrumentationContext.get(Runnable.class, PropagatedContext.class); - return ExecutorAdviceHelper.attachContextToTask(context, contextStore, newTask); + return ExecutorAdviceHelper.attachContextToTask(context, contextStore, task); } return null; } diff --git a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/JettyQueuedThreadPoolInstrumentation.java b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/JettyQueuedThreadPoolInstrumentation.java index 04b8b26c54..4e25771556 100644 --- a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/JettyQueuedThreadPoolInstrumentation.java +++ b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/JettyQueuedThreadPoolInstrumentation.java @@ -42,14 +42,12 @@ public class JettyQueuedThreadPoolInstrumentation implements TypeInstrumentation @Advice.OnMethodEnter(suppress = Throwable.class) public static PropagatedContext enterJobSubmit( @Advice.Argument(value = 0, readOnly = false) Runnable task) { - Runnable newTask = RunnableWrapper.wrapIfNeeded(task); Context context = Java8BytecodeBridge.currentContext(); - // TODO: shouldn't we check task? not newTask? - if (ExecutorAdviceHelper.shouldPropagateContext(context, newTask)) { - task = newTask; + if (ExecutorAdviceHelper.shouldPropagateContext(context, task)) { + task = RunnableWrapper.wrapIfNeeded(task); ContextStore contextStore = InstrumentationContext.get(Runnable.class, PropagatedContext.class); - return ExecutorAdviceHelper.attachContextToTask(context, contextStore, newTask); + return ExecutorAdviceHelper.attachContextToTask(context, contextStore, task); } return null; }