[Breaking change] Deprecate scope finishOnClose and continuation close

These deprecated methods will be removed in the following release.
DataDog · May 4, 2020 · c4958fb · c4958fb
1 parent 9891b02
commit c4958fb
Show file tree

Hide file tree

Showing 10 changed files with 102 additions and 10 deletions.
diff --git a/dd-trace-api/src/main/java/datadog/trace/context/TraceScope.java b/dd-trace-api/src/main/java/datadog/trace/context/TraceScope.java
@@ -5,8 +5,8 @@
 /** An object which can propagate a datadog trace across multiple threads. */
 public interface TraceScope extends Closeable {
   /**
-   * Prevent the trace attached to this TraceScope from reporting until the returned Continuation
-   * finishes.
+   * Prevent the trace attached to this TraceScope from reporting until the returned Continuation is
+   * either activated (and the returned scope is closed), or canceled.
    *
    * <p>Should be called on the parent thread.
    */
@@ -26,28 +26,40 @@ public interface TraceScope extends Closeable {
    */
   void setAsyncPropagation(boolean value);
 
-  /** Used to pass async context between workers. */
+  /**
+   * Used to pass async context between workers. Either activate or cancel must be called on each
+   * continuation instance to allow the trace to be reported.
+   */
   interface Continuation {
+
     /**
      * Activate the continuation.
      *
      * <p>Should be called on the child thread.
+     *
+     * <p>Consider calling this in a try-with-resources initialization block to ensure the returned
+     * scope is closed properly.
      */
     TraceScope activate();
 
+    /** Allow trace to stop waiting on this continuation for reporting. */
+    void cancel();
+
     /**
      * Cancel the continuation. This also closes parent scope.
      *
-     * <p>FIXME: the fact that this is closing parent scope is confusing, we should review this in
-     * new API.
+     * @deprecated use {@link #cancel()} instead. Will be removed in the next release.
      */
+    @Deprecated
     void close();
 
     /**
      * Close the continuation.
      *
+     * @deprecated use {@link #cancel()} instead. Will be removed in the next release.
      * @param closeContinuationScope true iff parent scope should also be closed
      */
+    @Deprecated
     void close(boolean closeContinuationScope);
   }
 }
diff --git a/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java b/dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java
@@ -281,6 +281,13 @@ public AgentSpan startSpan(
         .start();
   }
 
+  @Override
+  public AgentScope activateSpan(final AgentSpan span) {
+    return activateSpan(span, false);
+  }
+
+  /** @deprecated use {@link #activateSpan(AgentSpan)} instead */
+  @Deprecated
   @Override
   public AgentScope activateSpan(final AgentSpan span, final boolean finishSpanOnClose) {
     return scopeManager.activate(span, finishSpanOnClose);
@@ -475,6 +482,8 @@ private DDSpan buildSpan() {
       return new DDSpan(timestampMicro, buildSpanContext());
     }
 
+    /** @deprecated use {@link #start()} instead. */
+    @Deprecated
     public AgentScope startActive(final boolean finishSpanOnClose) {
       final AgentSpan span = buildSpan();
       final AgentScope scope = scopeManager.activate(span, finishSpanOnClose);

diff --git a/dd-trace-core/src/main/java/datadog/trace/core/scopemanager/ContextualScopeManager.java b/dd-trace-core/src/main/java/datadog/trace/core/scopemanager/ContextualScopeManager.java
@@ -24,6 +24,13 @@ public ContextualScopeManager(final int depthLimit, final DDScopeEventFactory sc
     this.scopeEventFactory = scopeEventFactory;
   }
 
+  @Override
+  public AgentScope activate(final AgentSpan span) {
+    return activate(span, false);
+  }
+
+  /** @deprecated use {@link #activate(AgentSpan)} instead. */
+  @Deprecated
   @Override
   public AgentScope activate(final AgentSpan span, final boolean finishOnClose) {
     final DDScope active = tlsScope.get();

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
@@ -183,11 +183,24 @@ public ContinuableScope activate() {
       }
     }
 
+    public void cancel() {
+      assert !finishOnClose : "cancel() should not be used with finishOnClose=true";
+      if (used.compareAndSet(false, true)) {
+        trace.cancelContinuation(this);
+      } else {
+        log.debug("Failed to close continuation {}. Already used.", this);
+      }
+    }
+
+    /** @deprecated use {@link #cancel()} instead. */
+    @Deprecated
     @Override
     public void close() {
       close(true);
     }
 
+    /** @deprecated use {@link #cancel()} instead. */
+    @Deprecated
     @Override
     public void close(final boolean closeContinuationScope) {
       if (used.compareAndSet(false, true)) {

diff --git a/dd-trace-core/src/main/java/datadog/trace/core/scopemanager/DDScopeManager.java b/dd-trace-core/src/main/java/datadog/trace/core/scopemanager/DDScopeManager.java
@@ -8,6 +8,10 @@
  * Allows custom scope managers. See OTScopeManager, CustomScopeManager, and ContextualScopeManager
  */
 public interface DDScopeManager {
+  AgentScope activate(AgentSpan span);
+
+  /** @deprecated use {@link #activate(AgentSpan)} instead. */
+  @Deprecated
   AgentScope activate(AgentSpan span, boolean finishOnClose);
 
   TraceScope active();

diff --git a/dd-trace-core/src/test/groovy/datadog/trace/core/scopemanager/ScopeManagerTest.groovy b/dd-trace-core/src/test/groovy/datadog/trace/core/scopemanager/ScopeManagerTest.groovy
@@ -185,6 +185,20 @@ class ScopeManagerTest extends DDSpecification {
     continuation.close()
   }
 
+  def "Continuation.cancel doesn't close parent scope"() {
+    setup:
+    def span = tracer.buildSpan("test").start()
+    def scope = (ContinuableScope) tracer.activateSpan(span)
+    scope.setAsyncPropagation(true)
+    def continuation = scope.capture()
+
+    when:
+    continuation.cancel()
+
+    then:
+    scopeManager.active() == scope
+  }
+
   def "ContinuableScope doesn't close if non-zero"() {
     setup:
     def builder = tracer.buildSpan("test")

diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/CustomScopeManagerWrapper.java b/dd-trace-ot/src/main/java/datadog/opentracing/CustomScopeManagerWrapper.java
@@ -29,10 +29,17 @@ class CustomScopeManagerWrapper implements DDScopeManager {
   private final TypeConverter converter;
 
   CustomScopeManagerWrapper(final ScopeManager scopeManager, final TypeConverter converter) {
-    this.delegate = scopeManager;
+    delegate = scopeManager;
     this.converter = converter;
   }
 
+  @Override
+  public AgentScope activate(final AgentSpan span) {
+    return activate(span, false);
+  }
+
+  /** @deprecated use {@link #activate(AgentSpan)} instead. */
+  @Deprecated
   @Override
   public AgentScope activate(final AgentSpan agentSpan, final boolean finishOnClose) {
     final Span span = converter.toSpan(agentSpan);

diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java
@@ -180,8 +180,8 @@ public DDTracer(
   @Deprecated
   public DDTracer(final CoreTracer coreTracer) {
     this.coreTracer = coreTracer;
-    this.converter = new TypeConverter(new DefaultLogHandler());
-    this.scopeManager = new OTScopeManager(coreTracer, converter);
+    converter = new TypeConverter(new DefaultLogHandler());
+    scopeManager = new OTScopeManager(coreTracer, converter);
   }
 
   @Builder
@@ -202,9 +202,9 @@ private DDTracer(
       final LogHandler logHandler) {
 
     if (logHandler != null) {
-      this.converter = new TypeConverter(logHandler);
+      converter = new TypeConverter(logHandler);
     } else {
-      this.converter = new TypeConverter(new DefaultLogHandler());
+      converter = new TypeConverter(new DefaultLogHandler());
     }
 
     // Each of these are only overriden if set
@@ -471,6 +471,8 @@ public Span start() {
       return converter.toSpan(agentSpan);
     }
 
+    /** @deprecated use {@link #start()} instead. */
+    @Deprecated
     @Override
     public Scope startActive(final boolean finishSpanOnClose) {
       final AgentScope agentScope = delegate.startActive(finishSpanOnClose);

diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/CustomScopeManagerTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/CustomScopeManagerTest.groovy
@@ -269,6 +269,10 @@ class TestScopeManager implements ScopeManager {
           return TestTraceScope.this
         }
 
+        @Override
+        void cancel() {
+        }
+
         @Override
         void close() {
         }

diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentTracer.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentTracer.java
@@ -28,6 +28,12 @@ public static AgentSpan startSpan(
     return get().startSpan(spanName, parent, startTimeMicros);
   }
 
+  public static AgentScope activateSpan(final AgentSpan span) {
+    return get().activateSpan(span);
+  }
+
+  /** @deprecated use {@link #activateSpan(AgentSpan)} instead */
+  @Deprecated
   public static AgentScope activateSpan(final AgentSpan span, final boolean finishSpanOnClose) {
     return get().activateSpan(span, finishSpanOnClose);
   }
@@ -72,6 +78,10 @@ public interface TracerAPI {
 
     AgentSpan startSpan(String spanName, AgentSpan.Context parent, long startTimeMicros);
 
+    AgentScope activateSpan(AgentSpan span);
+
+    /** @deprecated use {@link #activateSpan(AgentSpan)} instead */
+    @Deprecated
     AgentScope activateSpan(AgentSpan span, boolean finishSpanOnClose);
 
     AgentSpan activeSpan();
@@ -108,6 +118,13 @@ public AgentSpan startSpan(
       return NoopAgentSpan.INSTANCE;
     }
 
+    @Override
+    public AgentScope activateSpan(final AgentSpan span) {
+      return activateSpan(span);
+    }
+
+    /** @deprecated use {@link #activateSpan(AgentSpan)} instead */
+    @Deprecated
     @Override
     public AgentScope activateSpan(final AgentSpan span, final boolean finishSpanOnClose) {
       return NoopAgentScope.INSTANCE;
@@ -261,6 +278,9 @@ public TraceScope activate() {
       return NoopAgentScope.INSTANCE;
     }
 
+    @Override
+    public void cancel() {}
+
     @Override
     public void close() {}