From 017f6c5c043eea0a0f1a5a0a7e3af2bbf5911f7c Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Tue, 11 Mar 2025 10:06:55 +0000 Subject: [PATCH] Remove AttachableWrapper aspect from scopes (#8534) In the majority of cases the scope wrapper will only be accessed once when activating a span or context. The other cases are deprecated calls to check the active scope, where the returned scope wrapper is short-lived and cheap to recreate. --- .../opentelemetry/TypeConverter.java | 10 --------- .../src/test/groovy/TypeConverterTest.groovy | 19 ++-------------- .../opentracing31/TypeConverter.java | 13 ----------- .../src/test/groovy/TypeConverterTest.groovy | 22 ++----------------- .../opentracing32/TypeConverter.java | 13 ----------- .../src/test/groovy/TypeConverterTest.groovy | 22 ++----------------- .../core/scopemanager/ContinuableScope.java | 19 +--------------- .../datadog/opentracing/TypeConverter.java | 13 ----------- .../opentracing/TypeConverterTest.groovy | 22 ++----------------- 9 files changed, 9 insertions(+), 144 deletions(-) diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-0.3/src/main/java/datadog/trace/instrumentation/opentelemetry/TypeConverter.java b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-0.3/src/main/java/datadog/trace/instrumentation/opentelemetry/TypeConverter.java index 1e4ff5dbdbe..0258076a08e 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-0.3/src/main/java/datadog/trace/instrumentation/opentelemetry/TypeConverter.java +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-0.3/src/main/java/datadog/trace/instrumentation/opentelemetry/TypeConverter.java @@ -55,16 +55,6 @@ public Scope toScope(final AgentScope scope) { if (scope == null) { return null; } - if (scope instanceof AttachableWrapper) { - AttachableWrapper attachableScopeWrapper = (AttachableWrapper) scope; - Object wrapper = attachableScopeWrapper.getWrapper(); - if (wrapper instanceof Scope) { - return (Scope) wrapper; - } - Scope otScope = new OtelScope(scope); - attachableScopeWrapper.attachWrapper(otScope); - return otScope; - } if (scope == noopScope()) { return noopScopeWrapper; } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-0.3/src/test/groovy/TypeConverterTest.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-0.3/src/test/groovy/TypeConverterTest.groovy index 7bc4b64953b..3c1a0fe2dc6 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-0.3/src/test/groovy/TypeConverterTest.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-0.3/src/test/groovy/TypeConverterTest.groovy @@ -1,18 +1,17 @@ import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.DDSpanId import datadog.trace.api.DDTraceId -import datadog.trace.api.sampling.PrioritySampling import datadog.trace.api.datastreams.NoopPathwayContext +import datadog.trace.api.sampling.PrioritySampling import datadog.trace.core.DDSpan import datadog.trace.core.DDSpanContext import datadog.trace.core.PendingTrace import datadog.trace.core.propagation.PropagationTags -import datadog.trace.core.scopemanager.ContinuableScopeManager import datadog.trace.instrumentation.opentelemetry.TypeConverter import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopScope -import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpanContext import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpan +import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpanContext class TypeConverterTest extends AgentTestRunner { TypeConverter typeConverter = new TypeConverter() @@ -46,20 +45,6 @@ class TypeConverterTest extends AgentTestRunner { typeConverter.toScope(noopScope) is typeConverter.toScope(noopScope) } - def "should avoid extra allocation for a scope wrapper"() { - def scopeManager = new ContinuableScopeManager(0, false) - def context = createTestSpanContext() - def span1 = new DDSpan("test", 0, context, null) - def span2 = new DDSpan("test", 0, context, null) - def scope1 = scopeManager.activateManualSpan(span1) - def scope2 = scopeManager.activateManualSpan(span2) - expect: - // return the same wrapper for the same scope - typeConverter.toScope(scope1) is typeConverter.toScope(scope1) - // return distinct wrapper for another context - !typeConverter.toScope(scope1).is(typeConverter.toScope(scope2)) - } - def createTestSpanContext() { def trace = Stub(PendingTrace) return new DDSpanContext( diff --git a/dd-java-agent/instrumentation/opentracing/api-0.31/src/main/java/datadog/trace/instrumentation/opentracing31/TypeConverter.java b/dd-java-agent/instrumentation/opentracing/api-0.31/src/main/java/datadog/trace/instrumentation/opentracing31/TypeConverter.java index bd85386c0c8..09693fda11b 100644 --- a/dd-java-agent/instrumentation/opentracing/api-0.31/src/main/java/datadog/trace/instrumentation/opentracing31/TypeConverter.java +++ b/dd-java-agent/instrumentation/opentracing/api-0.31/src/main/java/datadog/trace/instrumentation/opentracing31/TypeConverter.java @@ -58,19 +58,6 @@ public Scope toScope(final AgentScope scope, final boolean finishSpanOnClose) { if (scope == null) { return null; } - if (scope instanceof AttachableWrapper) { - AttachableWrapper attachableScopeWrapper = (AttachableWrapper) scope; - Object wrapper = attachableScopeWrapper.getWrapper(); - if (wrapper instanceof OTScopeManager.OTScope) { - OTScopeManager.OTScope attachedScopeWrapper = (OTScopeManager.OTScope) wrapper; - if (attachedScopeWrapper.isFinishSpanOnClose() == finishSpanOnClose) { - return (Scope) wrapper; - } - } - Scope otScope = new OTScopeManager.OTScope(scope, finishSpanOnClose, this); - attachableScopeWrapper.attachWrapper(otScope); - return otScope; - } if (scope == noopScope()) { return noopScopeWrapper; } diff --git a/dd-java-agent/instrumentation/opentracing/api-0.31/src/test/groovy/TypeConverterTest.groovy b/dd-java-agent/instrumentation/opentracing/api-0.31/src/test/groovy/TypeConverterTest.groovy index be34e94442a..aaab5ab89d2 100644 --- a/dd-java-agent/instrumentation/opentracing/api-0.31/src/test/groovy/TypeConverterTest.groovy +++ b/dd-java-agent/instrumentation/opentracing/api-0.31/src/test/groovy/TypeConverterTest.groovy @@ -1,19 +1,18 @@ import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.DDSpanId import datadog.trace.api.DDTraceId -import datadog.trace.api.sampling.PrioritySampling import datadog.trace.api.datastreams.NoopPathwayContext +import datadog.trace.api.sampling.PrioritySampling import datadog.trace.core.DDSpan import datadog.trace.core.DDSpanContext import datadog.trace.core.PendingTrace import datadog.trace.core.propagation.PropagationTags -import datadog.trace.core.scopemanager.ContinuableScopeManager import datadog.trace.instrumentation.opentracing.DefaultLogHandler import datadog.trace.instrumentation.opentracing31.TypeConverter import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopScope -import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpanContext import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpan +import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpanContext class TypeConverterTest extends AgentTestRunner { TypeConverter typeConverter = new TypeConverter(new DefaultLogHandler()) @@ -51,23 +50,6 @@ class TypeConverterTest extends AgentTestRunner { typeConverter.toScope(noopScope, false) is typeConverter.toScope(noopScope, true) } - def "should avoid extra allocation for a scope wrapper"() { - def scopeManager = new ContinuableScopeManager(0, false) - def context = createTestSpanContext() - def span1 = new DDSpan("test", 0, context, null) - def span2 = new DDSpan("test", 0, context, null) - def scope1 = scopeManager.activateManualSpan(span1) - def scope2 = scopeManager.activateManualSpan(span2) - expect: - // return the same wrapper for the same scope - typeConverter.toScope(scope1, true) is typeConverter.toScope(scope1, true) - typeConverter.toScope(scope1, false) is typeConverter.toScope(scope1, false) - !typeConverter.toScope(scope1, true).is(typeConverter.toScope(scope1, false)) - !typeConverter.toScope(scope1, false).is(typeConverter.toScope(scope1, true)) - // return distinct wrapper for another context - !typeConverter.toScope(scope1, true).is(typeConverter.toScope(scope2, true)) - } - def createTestSpanContext() { def trace = Stub(PendingTrace) return new DDSpanContext( diff --git a/dd-java-agent/instrumentation/opentracing/api-0.32/src/main/java/datadog/trace/instrumentation/opentracing32/TypeConverter.java b/dd-java-agent/instrumentation/opentracing/api-0.32/src/main/java/datadog/trace/instrumentation/opentracing32/TypeConverter.java index c0fd113cdb5..6067d9c2be3 100644 --- a/dd-java-agent/instrumentation/opentracing/api-0.32/src/main/java/datadog/trace/instrumentation/opentracing32/TypeConverter.java +++ b/dd-java-agent/instrumentation/opentracing/api-0.32/src/main/java/datadog/trace/instrumentation/opentracing32/TypeConverter.java @@ -58,19 +58,6 @@ public Scope toScope(final AgentScope scope, final boolean finishSpanOnClose) { if (scope == null) { return null; } - if (scope instanceof AttachableWrapper) { - AttachableWrapper attachableScopeWrapper = (AttachableWrapper) scope; - Object wrapper = attachableScopeWrapper.getWrapper(); - if (wrapper instanceof OTScopeManager.OTScope) { - OTScopeManager.OTScope attachedScopeWrapper = (OTScopeManager.OTScope) wrapper; - if (attachedScopeWrapper.isFinishSpanOnClose() == finishSpanOnClose) { - return (Scope) wrapper; - } - } - Scope otScope = new OTScopeManager.OTScope(scope, finishSpanOnClose, this); - attachableScopeWrapper.attachWrapper(otScope); - return otScope; - } if (scope == noopScope()) { return noopScopeWrapper; } diff --git a/dd-java-agent/instrumentation/opentracing/api-0.32/src/test/groovy/TypeConverterTest.groovy b/dd-java-agent/instrumentation/opentracing/api-0.32/src/test/groovy/TypeConverterTest.groovy index c1855eb4657..f141b5b1b09 100644 --- a/dd-java-agent/instrumentation/opentracing/api-0.32/src/test/groovy/TypeConverterTest.groovy +++ b/dd-java-agent/instrumentation/opentracing/api-0.32/src/test/groovy/TypeConverterTest.groovy @@ -1,19 +1,18 @@ import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.DDSpanId import datadog.trace.api.DDTraceId -import datadog.trace.api.sampling.PrioritySampling import datadog.trace.api.datastreams.NoopPathwayContext +import datadog.trace.api.sampling.PrioritySampling import datadog.trace.core.DDSpan import datadog.trace.core.DDSpanContext import datadog.trace.core.PendingTrace import datadog.trace.core.propagation.PropagationTags -import datadog.trace.core.scopemanager.ContinuableScopeManager import datadog.trace.instrumentation.opentracing.DefaultLogHandler import datadog.trace.instrumentation.opentracing32.TypeConverter import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopScope -import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpanContext import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpan +import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpanContext class TypeConverterTest extends AgentTestRunner { TypeConverter typeConverter = new TypeConverter(new DefaultLogHandler()) @@ -51,23 +50,6 @@ class TypeConverterTest extends AgentTestRunner { typeConverter.toScope(noopScope, false) is typeConverter.toScope(noopScope, true) } - def "should avoid extra allocation for a scope wrapper"() { - def scopeManager = new ContinuableScopeManager(0, false) - def context = createTestSpanContext() - def span1 = new DDSpan("test", 0, context, null) - def span2 = new DDSpan("test", 0, context, null) - def scope1 = scopeManager.activateManualSpan(span1) - def scope2 = scopeManager.activateManualSpan(span2) - expect: - // return the same wrapper for the same scope - typeConverter.toScope(scope1, true) is typeConverter.toScope(scope1, true) - typeConverter.toScope(scope1, false) is typeConverter.toScope(scope1, false) - !typeConverter.toScope(scope1, true).is(typeConverter.toScope(scope1, false)) - !typeConverter.toScope(scope1, false).is(typeConverter.toScope(scope1, true)) - // return distinct wrapper for another context - !typeConverter.toScope(scope1, true).is(typeConverter.toScope(scope2, true)) - } - def createTestSpanContext() { def trace = Stub(PendingTrace) return new DDSpanContext( diff --git a/dd-trace-core/src/main/java/datadog/trace/core/scopemanager/ContinuableScope.java b/dd-trace-core/src/main/java/datadog/trace/core/scopemanager/ContinuableScope.java index 23441c9fa3b..eb38e2289ff 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/scopemanager/ContinuableScope.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/scopemanager/ContinuableScope.java @@ -6,11 +6,8 @@ import datadog.trace.api.scopemanager.ScopeListener; import datadog.trace.bootstrap.instrumentation.api.AgentScope; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; -import datadog.trace.bootstrap.instrumentation.api.AttachableWrapper; -import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; -import javax.annotation.Nonnull; -class ContinuableScope implements AgentScope, AttachableWrapper { +class ContinuableScope implements AgentScope { // different sources of scopes static final byte INSTRUMENTATION = 0; @@ -34,10 +31,6 @@ class ContinuableScope implements AgentScope, AttachableWrapper { private short referenceCount = 1; - private volatile Object wrapper; - private static final AtomicReferenceFieldUpdater WRAPPER_FIELD_UPDATER = - AtomicReferenceFieldUpdater.newUpdater(ContinuableScope.class, Object.class, "wrapper"); - private final Stateful scopeState; ContinuableScope( @@ -207,14 +200,4 @@ public final void afterActivated() { public byte source() { return (byte) (source & 0x7F); } - - @Override - public void attachWrapper(@Nonnull Object wrapper) { - WRAPPER_FIELD_UPDATER.set(this, wrapper); - } - - @Override - public Object getWrapper() { - return WRAPPER_FIELD_UPDATER.get(this); - } } diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/TypeConverter.java b/dd-trace-ot/src/main/java/datadog/opentracing/TypeConverter.java index 0a43a508482..b02f96a3272 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/TypeConverter.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/TypeConverter.java @@ -61,19 +61,6 @@ public Scope toScope(final AgentScope scope, final boolean finishSpanOnClose) { if (scope == null) { return null; } - if (scope instanceof AttachableWrapper) { - AttachableWrapper attachableScopeWrapper = (AttachableWrapper) scope; - Object wrapper = attachableScopeWrapper.getWrapper(); - if (wrapper instanceof OTScopeManager.OTScope) { - OTScopeManager.OTScope attachedScopeWrapper = (OTScopeManager.OTScope) wrapper; - if (attachedScopeWrapper.isFinishSpanOnClose() == finishSpanOnClose) { - return (Scope) wrapper; - } - } - Scope otScope = new OTScopeManager.OTScope(scope, finishSpanOnClose, this); - attachableScopeWrapper.attachWrapper(otScope); - return otScope; - } if (scope == noopScope()) { return noopScopeWrapper; } diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/TypeConverterTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/TypeConverterTest.groovy index a9e5f7a8494..6646832e1bd 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/TypeConverterTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/TypeConverterTest.groovy @@ -2,19 +2,18 @@ package datadog.opentracing import datadog.trace.api.DDSpanId import datadog.trace.api.DDTraceId -import datadog.trace.api.sampling.PrioritySampling import datadog.trace.api.datastreams.NoopPathwayContext +import datadog.trace.api.sampling.PrioritySampling import datadog.trace.core.CoreTracer import datadog.trace.core.DDSpan import datadog.trace.core.DDSpanContext import datadog.trace.core.PendingTrace import datadog.trace.core.propagation.PropagationTags -import datadog.trace.core.scopemanager.ContinuableScopeManager import datadog.trace.test.util.DDSpecification import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopScope -import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpanContext import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpan +import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpanContext class TypeConverterTest extends DDSpecification { TypeConverter typeConverter = new TypeConverter(new DefaultLogHandler()) @@ -52,23 +51,6 @@ class TypeConverterTest extends DDSpecification { typeConverter.toScope(noopScope, false) is typeConverter.toScope(noopScope, true) } - def "should avoid extra allocation for a scope wrapper"() { - def scopeManager = new ContinuableScopeManager(0, false) - def context = createTestSpanContext() - def span1 = new DDSpan("test", 0, context, null) - def span2 = new DDSpan("test", 0, context, null) - def scope1 = scopeManager.activateManualSpan(span1) - def scope2 = scopeManager.activateManualSpan(span2) - expect: - // return the same wrapper for the same scope - typeConverter.toScope(scope1, true) is typeConverter.toScope(scope1, true) - typeConverter.toScope(scope1, false) is typeConverter.toScope(scope1, false) - !typeConverter.toScope(scope1, true).is(typeConverter.toScope(scope1, false)) - !typeConverter.toScope(scope1, false).is(typeConverter.toScope(scope1, true)) - // return distinct wrapper for another context - !typeConverter.toScope(scope1, true).is(typeConverter.toScope(scope2, true)) - } - def createTestSpanContext() { def tracer = Stub(CoreTracer) def trace = Stub(PendingTrace)