Skip to content

Remove activeScope() use in OpenTracing shim #8478

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public String[] helperClassNames() {
packageName + ".OTTextMapSetter",
packageName + ".OTScopeManager",
packageName + ".OTScopeManager$OTScope",
packageName + ".OTScopeManager$FakeScope",
packageName + ".TypeConverter",
packageName + ".OTSpan",
packageName + ".OTSpanContext",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package datadog.trace.instrumentation.opentracing31;

import datadog.trace.api.Config;
import datadog.trace.bootstrap.instrumentation.api.AgentScope;
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
Expand All @@ -8,8 +9,12 @@
import io.opentracing.Scope;
import io.opentracing.ScopeManager;
import io.opentracing.Span;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class OTScopeManager implements ScopeManager {
static final Logger log = LoggerFactory.getLogger(OTScopeManager.class);

private final TypeConverter converter;
private final AgentTracer.TracerAPI tracer;

Expand All @@ -33,8 +38,12 @@ public Scope activate(final Span span, final boolean finishSpanOnClose) {
@Deprecated
@Override
public Scope active() {
AgentSpan agentSpan = tracer.activeSpan();
if (null == agentSpan) {
return null;
}
// WARNING... Making an assumption about finishSpanOnClose
return converter.toScope(tracer.activeScope(), false);
return new OTScope(new FakeScope(agentSpan), false, converter);
}

static class OTScope implements Scope, TraceScope {
Expand Down Expand Up @@ -67,4 +76,33 @@ public boolean isFinishSpanOnClose() {
return finishSpanOnClose;
}
}

private final class FakeScope implements AgentScope {
private final AgentSpan agentSpan;

FakeScope(AgentSpan agentSpan) {
this.agentSpan = agentSpan;
}

@Override
public AgentSpan span() {
return agentSpan;
}

@Override
public byte source() {
return ScopeSource.MANUAL.id();
}

@Override
public void close() {
if (agentSpan == tracer.activeSpan()) {
tracer.closeActive();
} else if (Config.get().isScopeStrictMode()) {
throw new RuntimeException("Tried to close " + agentSpan + " scope when not on top");
} else {
log.warn("Tried to close {} scope when not on top", agentSpan);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ class OpenTracing31Test extends AgentTestRunner {
firstScope.close()

then:
tracer.scopeManager().active().delegate == secondScope.delegate
tracer.scopeManager().active().span() == secondScope.span()
_ * TEST_PROFILING_CONTEXT_INTEGRATION._
0 * _

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public String[] helperClassNames() {
packageName + ".OTTextMapInjectSetter",
packageName + ".OTScopeManager",
packageName + ".OTScopeManager$OTScope",
packageName + ".OTScopeManager$FakeScope",
packageName + ".TypeConverter",
packageName + ".OTSpan",
packageName + ".OTSpanContext",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package datadog.trace.instrumentation.opentracing32;

import datadog.trace.api.Config;
import datadog.trace.bootstrap.instrumentation.api.AgentScope;
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
Expand All @@ -8,8 +9,12 @@
import io.opentracing.Scope;
import io.opentracing.ScopeManager;
import io.opentracing.Span;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class OTScopeManager implements ScopeManager {
static final Logger log = LoggerFactory.getLogger(OTScopeManager.class);

private final TypeConverter converter;
private final AgentTracer.TracerAPI tracer;

Expand Down Expand Up @@ -38,8 +43,12 @@ public Scope activate(final Span span, final boolean finishSpanOnClose) {
@Deprecated
@Override
public Scope active() {
AgentSpan agentSpan = tracer.activeSpan();
if (null == agentSpan) {
return null;
}
// WARNING... Making an assumption about finishSpanOnClose
return converter.toScope(tracer.activeScope(), false);
return new OTScope(new FakeScope(agentSpan), false, converter);
}

@Override
Expand Down Expand Up @@ -77,4 +86,33 @@ public boolean isFinishSpanOnClose() {
return finishSpanOnClose;
}
}

private final class FakeScope implements AgentScope {
private final AgentSpan agentSpan;

FakeScope(AgentSpan agentSpan) {
this.agentSpan = agentSpan;
}

@Override
public AgentSpan span() {
return agentSpan;
}

@Override
public byte source() {
return ScopeSource.MANUAL.id();
}

@Override
public void close() {
if (agentSpan == tracer.activeSpan()) {
tracer.closeActive();
} else if (Config.get().isScopeStrictMode()) {
throw new RuntimeException("Tried to close " + agentSpan + " scope when not on top");
} else {
log.warn("Tried to close {} scope when not on top", agentSpan);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ class OpenTracing32Test extends AgentTestRunner {
firstScope.close()

then:
tracer.scopeManager().active().delegate == secondScope.delegate
tracer.scopeManager().active().span() == secondScope.span()
_ * TEST_PROFILING_CONTEXT_INTEGRATION._
0 * _

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -977,6 +977,14 @@ public AgentScope activeScope() {
return scopeManager.active();
}

@Override
public void closeActive() {
AgentScope activeScope = this.scopeManager.active();
if (activeScope != null) {
activeScope.close();
}
}

@Override
public AgentSpanContext notifyExtensionStart(Object event) {
return LambdaHandler.notifyStartInvocation(this, event);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public final void close() {
byte source = source();
scopeManager.healthMetrics.onScopeCloseError(source);
if (source == ScopeSource.MANUAL.id() && scopeManager.strictMode) {
throw new RuntimeException("Tried to close scope when not on top");
throw new RuntimeException("Tried to close " + span + " scope when not on top");
}
}

Expand Down
1 change: 1 addition & 0 deletions dd-trace-ot/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ minimumInstructionCoverage = 0.5
excludedClassesCoverage += [
// This is mainly equals() and hashCode()
"datadog.opentracing.OTScopeManager.OTScope",
"datadog.opentracing.OTScopeManager.FakeScope",
"datadog.opentracing.OTSpan",
"datadog.opentracing.OTSpanContext",
"datadog.opentracing.CustomScopeManagerWrapper.CustomScopeState",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package datadog.opentracing;

import datadog.trace.api.Config;
import datadog.trace.bootstrap.instrumentation.api.AgentScope;
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
Expand All @@ -8,9 +9,13 @@
import io.opentracing.Scope;
import io.opentracing.ScopeManager;
import io.opentracing.Span;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/** One of the two possible scope managers. See CustomScopeManagerWrapper */
class OTScopeManager implements ScopeManager {
static final Logger log = LoggerFactory.getLogger(OTScopeManager.class);

private final TypeConverter converter;
private final AgentTracer.TracerAPI tracer;

Expand Down Expand Up @@ -39,8 +44,12 @@ public Scope activate(final Span span, final boolean finishSpanOnClose) {
@Deprecated
@Override
public Scope active() {
AgentSpan agentSpan = tracer.activeSpan();
if (null == agentSpan) {
return null;
}
// WARNING... Making an assumption about finishSpanOnClose
return converter.toScope(tracer.activeScope(), false);
return new OTScope(new FakeScope(agentSpan), false, converter);
}

@Override
Expand Down Expand Up @@ -83,16 +92,45 @@ public boolean equals(final Object o) {
return false;
}
final OTScope otScope = (OTScope) o;
return delegate.equals(otScope.delegate);
return delegate.span().equals(otScope.delegate.span());
}

@Override
public int hashCode() {
return delegate.hashCode();
return delegate.span().hashCode();
}

boolean isFinishSpanOnClose() {
return finishSpanOnClose;
}
}

private final class FakeScope implements AgentScope {
private final AgentSpan agentSpan;

FakeScope(AgentSpan agentSpan) {
this.agentSpan = agentSpan;
}

@Override
public AgentSpan span() {
return agentSpan;
}

@Override
public byte source() {
return ScopeSource.MANUAL.id();
}

@Override
public void close() {
if (agentSpan == tracer.activeSpan()) {
tracer.closeActive();
} else if (Config.get().isScopeStrictMode()) {
throw new RuntimeException("Tried to close " + agentSpan + " scope when not on top");
} else {
log.warn("Tried to close {} scope when not on top", agentSpan);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ class OT33ApiTest extends DDSpecification {

then:
tracer.activeSpan().delegate == span.delegate
coreTracer.activeScope().span() == span.delegate
coreTracer.activeSpan() == span.delegate

cleanup:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class IterationSpansForkedTest extends DDSpecification {

and:
scope1.span() == span1
scopeManager.active().delegate == scope1
scopeManager.active().span().delegate == span1
!spanFinished(span1)

when:
Expand All @@ -54,7 +54,7 @@ class IterationSpansForkedTest extends DDSpecification {

and:
scope2.span() == span2
scopeManager.active().delegate == scope2
scopeManager.active().span().delegate == span2
!spanFinished(span2)

when:
Expand All @@ -69,7 +69,7 @@ class IterationSpansForkedTest extends DDSpecification {

and:
scope3.span() == span3
scopeManager.active().delegate == scope3
scopeManager.active().span().delegate == span3
!spanFinished(span3)

when:
Expand All @@ -96,7 +96,7 @@ class IterationSpansForkedTest extends DDSpecification {

and:
scope1.span() == span1
scopeManager.active().delegate == scope1
scopeManager.active().span().delegate == span1
!spanFinished(span1)

when:
Expand All @@ -110,7 +110,7 @@ class IterationSpansForkedTest extends DDSpecification {

and:
scope2.span() == span2
scopeManager.active().delegate == scope2
scopeManager.active().span().delegate == span2
!spanFinished(span2)

when:
Expand All @@ -124,7 +124,7 @@ class IterationSpansForkedTest extends DDSpecification {

and:
scope3.span() == span3
scopeManager.active().delegate == scope3
scopeManager.active().span().delegate == span3
!spanFinished(span3)

// close and finish the surrounding (non-iteration) span to complete the trace
Expand All @@ -150,7 +150,7 @@ class IterationSpansForkedTest extends DDSpecification {

and:
scope1.span() == span1
scopeManager.active().delegate == scope1
scopeManager.active().span().delegate == span1
!spanFinished(span1)

when:
Expand All @@ -168,7 +168,7 @@ class IterationSpansForkedTest extends DDSpecification {

and:
scope1A1.span() == span1A1
scopeManager.active().delegate == scope1A1
scopeManager.active().span().delegate == span1A1
!spanFinished(span1A1)

when:
Expand All @@ -182,7 +182,7 @@ class IterationSpansForkedTest extends DDSpecification {

and:
scope1A2.span() == span1A2
scopeManager.active().delegate == scope1A2
scopeManager.active().span().delegate == span1A2
!spanFinished(span1A2)

when:
Expand Down
Loading