Remove finish span on close by carlosalberto · Pull Request #301 · opentracing/opentracing-java

carlosalberto · 2018-08-26T22:26:33Z

Addresses #291

SpanBuilder.startActive(Span, boolean) and ScopeManager.activate(Span, bool) have been kept, but as deprecated methods.

Updated the opentracing-testbed code to use this change, and added a new, small, error-reporting case.

cc @felixbarny @adriancole @yurishkuro @tedsuo @sjoerdtalsma

coveralls · 2018-08-26T22:30:03Z

Coverage decreased (-0.6%) to 73.464% when pulling fdb6699 on carlosalberto:remove_finish_span_on_close into d6c6509 on opentracing:v0.32.0.

yurishkuro · 2018-08-27T16:18:46Z

opentracing-api/src/main/java/io/opentracing/ScopeManager.java

+     * @return a {@link Scope} instance to control the end of the active period for the {@link Span}. It is a
+     * programming error to neglect to call {@link Scope#close()} on the returned instance.
+     */
+    Scope activate(Span span);


I would add sample code like

Span span = tracer.startSpan... try (Scope scope = tracer.activate(span)) { } catch (Exception e) { span.log(..) } finally { span.finish() }

yurishkuro · 2018-08-27T16:20:05Z

opentracing-api/src/main/java/io/opentracing/Tracer.java

         * @see Scope
         */
-        Scope startActive(boolean finishSpanOnClose);
+        Scope startActive();


Do we actually need this method? It seems to promote the exact same bad practice.

Could you elaborate?

The Scope and auto-finish were originally designed for use with try-with-resource, but that pattern does not allow for logging exceptions to the span since the scope is not visible in the catch clause. Thus the recommended pattern is what I showed in https://github.com/opentracing/opentracing-java/pull/301/files#r213032222, which requires access to the span reference and only using Scope as a way to move the spans on and off the thread-local stack.

Using startActive() essentially suffers from the same problem - you can use it with try-catch, but not in the catch clause.

So it's back to: what is ultimate goal of this PR? The existing API works, but has gotchas for people unfamiliar with it.

Oh, that crossed my mind too. So I think that, if it stays, we would need to be clear about the implications of its usage - that is needs to be used with a standard try block, i.e.

Scope scope = tracer.buildSpan("foo").startActive(); try { } catch (Exception e) { Tags.ERROR.set(scope.span(), true); } finally { scope.span().finish(); scope.close(); }

(That's why I used such usage in its updated inline documentation ;) )

I'd be up for keeping it (with the proper documentation), but definitely understand it's not a perfect thing, and I'm open to have it removed if deemed so (in such case I will update the PR).

Good point Yuri. I think the fact that I fell for it again, suggesting to use try-with-resources + catch demonstrates that the API can easily misused. I also suggest removing it. Isn’t the intention of this PR to avoid potential misuse of the API? Also,startActive is just syntactic sugar (which turned out to be bittersweet ;).

This is a strange method, please remove it.

Seems we have found agreement on this point, so I will update this PR in the next days to have it removed ;)

felixbarny · 2018-08-27T17:47:25Z

opentracing-api/src/main/java/io/opentracing/Tracer.java

+         * The span will not be automatically finished when {@link Scope#close()} is called.
+         *
+         * <p>
+         * The returned {@link Scope} supports try-with-resources, but is usually


Why is it not usually used with try-with-resources?

That's related to Yuri's comment about removing it - essentially, that, as opposed to using ScopeManager.activate(), here we need an usual try block to do the cleanup (that is, having a reference to the Span to finish it after any error has occurred).

try (Scope scope = tracer.buildSpan("foo").startActive()) { } catch (Exception e) { // scope and scope.span() are not reachable here nor in the finally section :/ }

(I wish we could simply have something like try (scope) { ... but Java doesn't have support for that)

If this method isn't deleted, I think it would be cleaner if done similar to census and brave's "scoped span" Instead of messing with scope, mess with span.finish(). In other words, return a Span, and do mention it is a programming error to not call span.finish on the same thread. Another way is to leave it out..

Here's the javadoc from brave's ScopedSpan:

/** * Closes the {@link CurrentTraceContext#newScope(TraceContext) scope} associated with this span, * then reports the span complete, assigning the most precise duration possible. */ public abstract void finish();

felixbarny · 2018-08-27T17:47:48Z

opentracing-api/src/main/java/io/opentracing/Tracer.java

+         *         scope = tracer.buildSpan("...").startActive();
         *         // (Do work)
         *         scope.span().setTag( ... );  // etc, etc
+         *     } finally {


add catch clause

Will add it 👍

codefromthecrypt

Thanks for this.. I'm not feeling great about startActive as it is defined right now. It can either be left out or refactored.

codefromthecrypt · 2018-08-29T04:06:24Z

opentracing-api/src/main/java/io/opentracing/ScopeManager.java

 public interface ScopeManager {

    /**
+     * @deprecated use {@link #activate} instead.


nit move to the bottom and say when it will be removed (ideally next release as would the other deprecated methods added prior.. deprecation pre 1.0 is a strange thing but anyway)

I don't think there's a date to remove this one (we should discuss this topic, but another time ;) ), but we could say "in the next releases" (and definitely before 1.0 ;) )

codefromthecrypt · 2018-08-29T04:08:13Z

opentracing-api/src/main/java/io/opentracing/ScopeManager.java

    Scope activate(Span span, boolean finishSpanOnClose);

+    /**
+     * Make a {@link Span} instance active.


add some description of what active means as the word isn't intuitive.

For example "Makes this span visible via commands like (XX) until the corresponding scope is close"

Also, note that it is a programming error to leave the scope unclosed or to attempt to close it on a different thread.

Thanks, will add such explanation.

codefromthecrypt · 2018-08-29T04:08:24Z

opentracing-api/src/main/java/io/opentracing/Tracer.java

        SpanBuilder withStartTimestamp(long microseconds);

        /**
+         * @deprecated use {@link #startActive} instead.


same.. move down

codefromthecrypt · 2018-08-29T04:10:45Z

opentracing-api/src/main/java/io/opentracing/Tracer.java

         * @see Scope
         */
-        Scope startActive(boolean finishSpanOnClose);
+        Scope startActive();


This is a strange method, please remove it.

codefromthecrypt · 2018-08-29T04:15:15Z

opentracing-api/src/main/java/io/opentracing/Tracer.java

+         * The span will not be automatically finished when {@link Scope#close()} is called.
+         *
+         * <p>
+         * The returned {@link Scope} supports try-with-resources, but is usually


If this method isn't deleted, I think it would be cleaner if done similar to census and brave's "scoped span" Instead of messing with scope, mess with span.finish(). In other words, return a Span, and do mention it is a programming error to not call span.finish on the same thread. Another way is to leave it out..

Here's the javadoc from brave's ScopedSpan:

/** * Closes the {@link CurrentTraceContext#newScope(TraceContext) scope} associated with this span, * then reports the span complete, assigning the most precise duration possible. */ public abstract void finish();

codefromthecrypt · 2018-08-29T04:23:23Z

on the note about scope.. it is odd because scope in opentracing still has an accessor to the span. it makes it awkward and still encourages passing the scope around. I think this should be removed along with this change.

When scope.span() is removed, it leaves you with at least two paths for doing "startActive" if chosen as a method to retain.

encourage use of "current span" logic like census
https://github.com/census-instrumentation/opencensus-java/blob/c943c6c63637cc866e92b90dae0e48dccd488130/api/src/main/java/io/opencensus/trace/SpanBuilder.java#L186
use a special span that closes an implicit scope on finish like brave
https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/ScopedSpan.java#L7

Either way, please don't forget to remove your scope.span as it encourages people to leak scopes.

carlosalberto · 2018-09-04T17:19:11Z

Hey @adriancole

Thanks for the feedback. So my personal take for your points is:

That's not a bad thing, but then people would need to remember to do something like:

Span span = tracer.buildSpan("one").startActive();
try {
} catch (Exception e) {
  ...
  span.finish();
} finally {
   tracer.scopeManager.active().close();
}

This being implicit kinda makes me wonder (but definitely thing to consider).

A special Span is something that would add complexity to the current (relatively simple) API. For example: what happens if this very special Span is passed to another thread? I'd leave it as a potential addition in the future.

So the thing about removing Scope.span() is probably fine, as long as we have ScopeManager.activeSpan() (so we always expose the current active Span and the active Scope without explicitly linking them together). However, I'd like to hear the opinion of other people here.

Opinions? @felixbarny @yurishkuro @tedsuo

yurishkuro · 2018-09-05T00:07:40Z

are we shooting for this?

Span span = tracer.buildSpan("nasty").start();
try (Scope scope = tracer.activate(span)) {
  // do nasty stuff
} catch (Exception e) {
  span.SetTag(...);
  ...
} finally {
  span.finish();
}

It makes the usage slightly more verbose than the current api, especially without try-with-resource (only if <=1.6, who cares), but avoids the misuse. scope.span() is removed.

felixbarny · 2018-09-05T06:31:59Z

I agree with @yurishkuro

Remove* SpanBuilder#startActive
Remove* Scope.span() (this is only useful in combination with SpanBuilder#startActive)
Remove* ScopeManager.activate(Span, boolean)

*deprecate and eventually remove

carlosalberto · 2018-09-06T14:39:20Z

Hey,

Was wondering, after the feedback, about this (small variation):

SpanBuilder.startActive() and Scope.span() get removed
ScopeManager.activeSpan() and ScopeManager.active() (so we have the two of them available, but not linked to each other, thus less chances to have users pass Scope objects arounds).
Trace.activeSpan() and Tracer.activate() as aliases to ScopeManager's respective methods.

carlosalberto · 2018-09-06T18:01:33Z

For additional information: some frameworks do not let us 'wrap' their entire operation, but provide hooks for start and end events (guaranteed to run in the same thread), so we either need a) to let Scope be passed around (even if there is a risk that some users may pass it to another thread), or b) expose it in ScopeManager.active() (which is also risky).

In some cases, as mentioned in the main discussion, the user does not even have a place to store context information as part of the aforementioned start/end hooks, so the latter is definitely helpful.

Either way, we decide to go a) or b) (or even both), and improving greatly the documentation (mentioning what should never be done with Scopes and related) should be the way to go.

felixbarny · 2018-09-07T06:39:12Z

Sounds fine to me

codefromthecrypt · 2018-09-13T06:18:16Z

opentracing-api/src/main/java/io/opentracing/Tracer.java

-         *     try (Scope scope = tracer.buildSpan("...").startActive(true)) {
+         *     Scope scope = null;
+         *     try {
+         *         scope = tracer.buildSpan("...").startActive();


please inline this before the try. That removes the null checking later.

Also, as mentioned I think the scope having an accessor to the span is problematic.

carlosalberto · 2018-09-24T16:28:57Z

Hey all,

This PR is updated to reflect the latest requested changes, namely:

Deprecate SpanBuilder.startActive(boolean), ScopeManager.activate(Span, boolean) and Scope.span().
Removed SpanBuilder.startActive() (parameterless overload, which had been added early in this PR).
Add ScopeManager.activeSpan() and leave ScopeManager.active() in place (So ScopeManager.active() is still reachable, but designed to stay for more advanced use, as well as still exposing ScopeManager, in case the user wants to unwrap it himself).
Add Tracer.activateSpan(), as a shorthand for tracer.scopeManager().activate(span).
Updated the testbed module and inline docs overall.

@yurishkuro @adriancole @felixbarny

tylerbenson

Update the examples too.

opentracing-testbed/src/test/java/io/opentracing/testbed/activate_deactivate/README.md

yurishkuro · 2018-09-28T17:02:44Z

opentracing-api/src/main/java/io/opentracing/ScopeManager.java

+     *     } catch (Exception e) {
+     *         span.log(...);
+     *     } finally {
+     *         span.finish(); // Optionally finish the Span.


// Optionally finish the Span if the operation it represents is logically completed at this point.

yurishkuro · 2018-09-28T17:05:09Z

opentracing-api/src/main/java/io/opentracing/ScopeManager.java

+     * Return the currently active {@link Span}.
+     *
+     * <p>
+     * If there is an {@link Span non-null active span}, it becomes an implicit parent


Is it necessary to have this mentioned here? It think this comment only belongs to the span builder methods. Scope Manager as an abstraction is not tied to the builder's behavior.

yurishkuro · 2018-09-28T17:05:29Z

opentracing-api/src/main/java/io/opentracing/ScopeManager.java

+     *
+     * <p>
+     * Because both {@link #active()} and {@link #activeSpan()} reference the current
+     * the active state, they both will be either null or non-null.


s/the active/active/

yurishkuro · 2018-09-28T17:06:48Z

opentracing-api/src/main/java/io/opentracing/Tracer.java

+     * This is a shorthand for {@code Tracer.scopeManager().activate(span)}.
+     *
+     * @return a {@link Scope} instance to control the end of the active period for the {@link Span}. It is a
+     * programming error to neglect to call {@link Scope#close()} on the returned instance.


and may lead to memory leaks and the scope may remain in the thread-local stack.

yurishkuro · 2018-09-28T17:07:23Z

opentracing-api/src/main/java/io/opentracing/Tracer.java

-     *   // It's also possible to create Spans manually, bypassing the ScopeManager activation.
-     *   Span http = tracer.buildSpan("HandleHTTPRequest")
+     *   // Note: if there is a `tracer.activeSpan()` instance, it will be used as the target
+     *   // of an implicit CHILD_OF Reference when `start()` is invoked.


unless another span reference is explicitly provided to the builder

carlosalberto · 2018-10-04T00:38:23Z

@yurishkuro Thanks for the edit notes, appreciated. Please take a look whenever you can ;)

concern resolved.

carlosalberto · 2018-10-10T10:42:54Z

Hey @felixbarny @adriancole Wondering if you are fine with this PR? If yes, I'd like to have it merged, and probably have a Release Candidate to test things out soon after.

@yurishkuro Docs edits look good to you? ;)

felixbarny · 2018-10-11T08:45:38Z

LGTM

carlosalberto · 2018-10-16T11:41:41Z

Thanks @felixbarny !

I will proceed to merge this PR tomorrow as if seems to have found a proper trade-off of features, unless there's any complain from anybody ;)

carlosalberto · 2018-10-17T22:20:13Z

Merged, thanks everybody for their feedback! Remember that this is not a final change, and it could be changed if/as needed (we will likely be doing a Release Candidate to test this and other changes out) ;)

sjoerdtalsma · 2018-10-18T05:53:16Z

@carlosalberto I didn't participate in this PR as my opinion is already voiced by others, but I forgot to thank you for creating this!
So thanks 😄

* Deprecate the StringTag.set() overload taking a StringTag. (#262) * Implement Trace Identifiers. (#280) * Bump JaCoCo to use a Java 10 friendly version (#306) * Remove finish span on close (#301) * Deprecate finishSpanOnClose on activation. * Add ScopeManager.activeSpan() and Tracer.activateSpan(). * Clarify the API changes and deprecations. * Add an error reporting sample to opentracing-testbed. * Simple layer on top of ByteBuffer for BINARY format. (#276) * Add generic typed setTag/withTag (#311) * Allow injecting into maps of type Map<String,Object> (#310) * Add simple registerIfAbsent to global tracer (#289) * Split Inject and Extract Builtin interfaces (#316) * Deprecate ScopeManager.active() (#326) * Make Tracer extends Closable. (#329) * Do not make isRegistered() synchronized. (#333) * Deprecate AutoFinishScopeManager (#335)

carlosalberto added 2 commits August 24, 2018 23:27

Deprecate finishSpanOnClose on activation.

04875a7

Add a error reporting sample to opentracing-testbed.

81fe31a

yurishkuro reviewed Aug 27, 2018

View reviewed changes

felixbarny reviewed Aug 27, 2018

View reviewed changes

codefromthecrypt reviewed Aug 29, 2018

View reviewed changes

codefromthecrypt reviewed Sep 13, 2018

View reviewed changes

Add ScopeManager.activeSpan() and Tracer.activateSpan().

9607986

carlosalberto force-pushed the remove_finish_span_on_close branch from 2402d20 to 9607986 Compare September 21, 2018 16:27

carlosalberto added 2 commits September 24, 2018 13:41

Edits to clafiry the API changes and deprecations.

959d566

Remove the last traces of the parameterless startActive().

1f583f2

tylerbenson previously requested changes Sep 28, 2018

View reviewed changes

opentracing-testbed/src/test/java/io/opentracing/testbed/activate_deactivate/README.md Show resolved Hide resolved

yurishkuro reviewed Sep 28, 2018

View reviewed changes

Inline docs edits.

fdb6699

carlosalberto merged commit ac3c666 into opentracing:v0.32.0 Oct 17, 2018

carlosalberto mentioned this pull request Oct 18, 2018

Remove finishSpanOnClose flag #291

Closed

carlosalberto mentioned this pull request Oct 23, 2018

0.32.0 Release #314

Open

carlosalberto deleted the remove_finish_span_on_close branch March 7, 2019 01:32

skabashnyuk mentioned this pull request Oct 18, 2019

Upgrade metrics and tracing dependencies eclipse-che/che#14923

Merged

dhalperi mentioned this pull request May 1, 2020

try-with-resources (which doesn't work) is still the main documented way to use OpenTracing in java #376

Open

Conversation

carlosalberto commented Aug 26, 2018

Uh oh!

coveralls commented Aug 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosalberto Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt commented Aug 29, 2018

Uh oh!

carlosalberto commented Sep 4, 2018

Uh oh!

yurishkuro commented Sep 5, 2018

Uh oh!

felixbarny commented Sep 5, 2018

Uh oh!

carlosalberto commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlosalberto commented Sep 6, 2018

Uh oh!

felixbarny commented Sep 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosalberto commented Sep 24, 2018

Uh oh!

tylerbenson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 26, 2018 •

edited

Loading

carlosalberto Aug 28, 2018 •

edited

Loading

carlosalberto commented Sep 6, 2018 •

edited

Loading