Remove default finish behaviour for `activate()` #219

pavolloffay · 2017-11-15T14:58:24Z

coveralls · 2017-11-15T15:02:44Z

Coverage remained the same at 78.598% when pulling cd26a39 on pavolloffay:scope-do-not-close-by-default into b9feb48 on opentracing:v0.31.0.

carlosalberto · 2017-11-20T16:16:52Z

I personally think either way is fine - hopefully users won't have to manually create their Span instances, but rely on existing instrumentation primarily ;)

carlosalberto · 2017-11-28T23:16:19Z

I think we could merge this prior to a final release of the 0.31.0 version. Anybody against it? Maybe @yurishkuro?

yurishkuro

Left some comments, mostly about documentation. The change itself makes sense, esp. in light of try-with behavior.

yurishkuro · 2017-11-29T01:40:22Z

README.md

-    //
-    // If we create async work, `scope.span()` allows us to pass the `Span` along as well.
+} catch {
+    Tags.ERROR.set(span(), true);


Was this meant to call scope.span() ?

yurishkuro · 2017-11-29T01:41:51Z

README.md

@@ -51,28 +43,34 @@ if (scope != null) {

 ### Starting a new Span

-The common case starts an `Scope` that's automatically registered for intra-process propagation via `ScopeManager`. The best practice is to use a try-with-resources pattern which handles Exceptions and early returns:
+The common case starts an `Scope` that's automatically registered for intra-process propagation via `ScopeManager`.
+Note that the default behaviour of `startActive()` is not to finish the span on `Scope.close()`.


, only to restore previous scope

?

yurishkuro · 2017-11-29T01:46:27Z

README.md

-} finally {
-    scope.deactivate();
+    //
+    // If we create async work, `scope.span()` allows us to pass the `Span` along as well.


This seems misleading. If async work is spawned here the span will be finished by the time async thread has a chance to start.

It was in the original text. I will rephrase it.

yurishkuro · 2017-11-29T01:48:34Z

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

-         *     // Span does not finish automatically when the Scope is closed as
-         *     // 'finishOnClose' is false
+         *     // Span does finish automatically when the Scope is closed as
+         *     // 'finishOnClose' is true


Don't follow - even with true it doesn't finish?

coveralls · 2017-11-29T10:39:34Z

Coverage remained the same at 78.598% when pulling 1bb2e14 on pavolloffay:scope-do-not-close-by-default into b9feb48 on opentracing:v0.31.0.

objectiser

Looking good - just some minor rewording.

objectiser · 2017-11-29T10:43:41Z

README.md

@@ -51,28 +43,34 @@ if (scope != null) {

 ### Starting a new Span

-The common case starts an `Scope` that's automatically registered for intra-process propagation via `ScopeManager`. The best practice is to use a try-with-resources pattern which handles Exceptions and early returns:
+The common case starts an `Scope` that's automatically registered for intra-process propagation via `ScopeManager`.


"starts a Scope..."

objectiser · 2017-11-29T10:46:24Z

README.md

-The common case starts an `Scope` that's automatically registered for intra-process propagation via `ScopeManager`. The best practice is to use a try-with-resources pattern which handles Exceptions and early returns:
+The common case starts an `Scope` that's automatically registered for intra-process propagation via `ScopeManager`.
+Note that the default behaviour of `startActive()` does not finish the span on `Scope.close()`.
+This was made on purpose because `try-with-resources` construct would finished the span before


"This decision was made on purpose because the try-with-resources construct would finish the span ..."

objectiser · 2017-11-29T10:47:34Z

README.md

+The common case starts an `Scope` that's automatically registered for intra-process propagation via `ScopeManager`.
+Note that the default behaviour of `startActive()` does not finish the span on `Scope.close()`.
+This was made on purpose because `try-with-resources` construct would finished the span before
+`catch` or `finally` blocks are executed, which makes logging exceptions impossible.


"the catch ...."
"makes logging exceptions and setting tags impossible"?

objectiser · 2017-11-29T10:49:42Z

README.md

@@ -89,29 +87,15 @@ try {
 }
 ```

-**If there is an `Scope`, it will act as the parent to any newly started `Span`** unless the programmer invokes `ignoreActiveSpan()` at `buildSpan()` time, like so:
+**If there is an `Scope`, it will act as the parent to any newly started `Span`** unless 


"is a Scope..."

objectiser · 2017-11-29T10:50:46Z

README.md

@@ -89,29 +87,15 @@ try {
 }
 ```

-**If there is an `Scope`, it will act as the parent to any newly started `Span`** unless the programmer invokes `ignoreActiveSpan()` at `buildSpan()` time, like so:
+**If there is an `Scope`, it will act as the parent to any newly started `Span`** unless 
+the programmer invokes `ignoreActiveSpan()` at `buildSpan()` time or specified parent context explicitly:


Not directly related to this PR, but wondering whether ignoreActiveSpan should be changed to ignoreScope - to avoid confusion later?

could be :)

objectiser · 2017-11-29T10:54:35Z

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

         *         // (Do work)
         *         scope.span().setTag( ... );  // etc, etc
         *     }
-         *     // Span does not finish automatically when the Scope is closed as
-         *     // 'finishOnClose' is false
+         *     // Span does finish automatically only when 'finishSpanOnClose' is true


"Span finishes automatically..."

objectiser · 2017-11-29T10:55:33Z

opentracing-examples/src/test/java/io/opentracing/examples/client_server/Server.java

@@ -37,7 +37,8 @@ private void process(Message message) {
        try (Scope scope = tracer.buildSpan("receive")
                .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_SERVER)
                .withTag(Tags.COMPONENT.getKey(), "example-server")
-                .asChildOf(context).startActive()) {
+                .asChildOf(context)
+            .startActive(true)) {


nit: indentation

objectiser · 2017-11-29T10:58:42Z

opentracing-util/src/test/java/io/opentracing/util/ThreadLocalScopeManagerTest.java

@@ -53,15 +53,15 @@ public void activateSpan() throws Exception {
        }

        // Make sure the Span got finish()ed.


Need to change comment to say "did not get finish()ed"

pavolloffay · 2017-11-29T14:18:04Z

@yurishkuro @objectiser thanks, changes applied.

coveralls · 2017-11-29T14:19:54Z

Coverage remained the same at 78.598% when pulling be7ac02 on pavolloffay:scope-do-not-close-by-default into b9feb48 on opentracing:v0.31.0.

tedsuo · 2017-11-29T23:29:01Z

Any objections to merging this at this point?

tylerbenson · 2017-11-30T03:44:05Z

I think my main request before merging would be to update the description with better context, rather than just linking off to a lengthy discussion. In particular, I'd like to see better rationale for why this is desirable (easier for framework instrumenters) vs the existing implementation (easier for end api users) and why we chose the tradeoff.

change my mind

yurishkuro · 2017-11-30T05:40:13Z

So it seems like there are reasons to have default = true but also reasons to have default = false. Which brings me to my original suggestion - eliminate default altogether and let the user decide. Passing true/false argument is not a huge burden on usability, but not understanding the default and getting burned by it is what wastes people's time. And one can get burned by either default value.

PS - agree about s/startManual/start/

coveralls · 2017-11-30T10:13:56Z

Coverage remained the same at 78.598% when pulling 8cb95fb on pavolloffay:scope-do-not-close-by-default into b9feb48 on opentracing:v0.31.0.

pavolloffay · 2017-11-30T10:29:39Z

@tylerbenson I have added more comments in 8cb95fb

tedsuo · 2017-11-30T20:45:42Z

I agree with @yurishkuro at this point, the utility of an extra method just to avoid a bool parameter is not a great cost/value, and there really isn't a correct "default" in the sense of wanting it to be false or true 90% of the time - it's pretty 50/50. Let's just drop it and simplify the API.

pavolloffay · 2017-12-01T10:25:06Z

To be honest I still think that default not-finishing the span is a better option for a couple of reasons:

it forces users to properly log/tag exceptional states
OT is mostly used in instrumentations and this is the use case most used there
ScopeManager.activate(span) - almost never finishes the span. (Look at the examples what is the usual use case)

I have a removed activate(span) in 6423db2. To not block this, If others think that this is the way to go.

coveralls · 2017-12-01T10:27:06Z

Coverage increased (+0.2%) to 78.81% when pulling 6423db2 on pavolloffay:scope-do-not-close-by-default into b9feb48 on opentracing:v0.31.0.

tylerbenson · 2017-12-01T16:56:14Z

@pavolloffay can you update the PR description too please?

carlosalberto · 2017-12-07T06:59:25Z

Is there any chance to merge this PR as well? I know Pavol is still in favor of keeping a default, but we can always add it later (when we have more feedback overall). It would be great to have this patch in the RC 2.

yurishkuro · 2017-12-07T14:27:48Z

README.md

    // Do things.
+} catch {
+    Tags.ERROR.set(scope.span(), true);


scope.span() is inaccessible here according to the comment

yurishkuro · 2017-12-07T14:29:04Z

README.md

+try (Scope scope = tracer.buildSpan("someWork").startActive(true)) {
+    // Do things.
+    //
+    // `scope.span()` allows us to pass the `Span` to newly created threads.
 }


might be good to add a catch clause with a comment

// cannot record the error in the span since scope is not accessible

yurishkuro · 2017-12-07T14:31:30Z

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

@@ -15,22 +15,13 @@

 /**
 * The {@link ScopeManager} interface abstracts both the activation of {@link Span} instances (via
- * {@link ScopeManager#activate(Span)}) and access to an active {@link Span}/{@link Scope}
+ * {@link ScopeManager#activate(Span, boolean)}) and access to an active {@link Span}/{@link Scope}


nit: parentheses around (via ...) in both places are not necessary.

yurishkuro · 2017-12-07T14:33:37Z

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

@@ -48,7 +39,7 @@
     *
     * <p>
     * If there is an {@link Scope non-null scope}, its wrapped {@link Span} becomes an implicit parent of any
-     * newly-created {@link Span} at {@link Tracer.SpanBuilder#startActive()} time (rather than at


live above: s/becomes an implicit parent/becomes an implicit CHILD_OF parent (or something like that).

yurishkuro · 2017-12-07T14:35:36Z

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

@@ -48,7 +39,7 @@
     *
     * <p>
     * If there is an {@link Scope non-null scope}, its wrapped {@link Span} becomes an implicit parent of any
-     * newly-created {@link Span} at {@link Tracer.SpanBuilder#startActive()} time (rather than at
+     * newly-created {@link Span} at {@link Tracer.SpanBuilder#startActive(boolean)} time (rather than at
     * {@link Tracer#buildSpan(String)} time).


not just startActive but start as well.

no need for parentheses around (rather than at buildSpan)

carlosalberto · 2017-12-11T20:53:31Z

Any update on this @pavolloffay ? It would be sweet to have this change in RC 2 ;) (else I can cook a patch on top of your PR to apply Yuri's feedback ;) )

coveralls · 2017-12-15T00:51:39Z

Coverage increased (+0.2%) to 80.87% when pulling 1a1a0ec on pavolloffay:scope-do-not-close-by-default into 004ad1f on opentracing:v0.31.0.

* Do not auto finish on scope.close * Fix review comments * Fix review comments * Add explanatory statement about not auto-finishing * Define only activate(s, bool)

@deprecated

* Code cleanup (opentracing#199) - Propagate @deprecated annotation to implementations - Remove redundant "static final" definition from interface - Use Collections.emptyMap instead of Collections.EMPTY_MAP to preserve type * Add support for multi reference and expose references context and type from MockSpan. (opentracing#213) * Publish test artifact for Globaltracer testutil (opentracing#209) * Fix MockTracer.ChildOf not fail if there is a null argument passed (opentracing#226) * Make ChildOf not fail if there is a null argument passed * Moved tracer test to where it belongs. Fixed typo. * Use correct reference in Javadoc (opentracing#230) * MockTracer use text map propag in the default constructor (opentracing#179) * Implement a simpler Span propagation without lifetime handling. (opentracing#188) - Scope replaces ActiveSpan, without sharing a common ancestor with Span. - ScopeManager replaces ActiveSpanSource. - No reference-count to handle Span's lifetime. - A simple thread-local based ScopeManager. * travis publish.sh allow releases from branches (opentracing#191) * Travis allow release from non master branches (opentracing#192) * Travis allow release from non master branches Publis script compares remote branch with current checkout. This passes travis_branch into git checkout command so it will compare the same branches. * Fix comments * Travis publish script, remove -RC on git checkout (opentracing#193) * Update the Travis script to allow publishing from v.0.0.0 branches. (opentracing#195) Thing is, we cannot publish from 0.0.0-style branches as they are excluded, based on the current global Travis configuration, thus we try to publish from branches following a v.0.0.0 style, if any. * Readme document release process for release candidates (opentracing#198) * Readme document release process for release candidates * Adjust publish.sh to work with release from master branch * Add Examples for async use cases (opentracing#197) * Add examples for async as test cases This includes execution flow similar to: * Actor ask/tell * Promises with callbacks * Work interleaved on a thread using suspend/resume. The implementations of these execution models are obviously very simplistic, but intended to emphasize the tracing aspect. * Update README files to reflect the Scope concept. (opentracing#217) * Let Tracer.activeSpan() be a shorthand for ScopeManager's active Span. (opentracing#228) * Let Tracer.activeSpan() be a shorthand for ScopeManager's active Span. * Document the null case for Tracer.activeSpan(). * Have Tracer.activeSpan() return null. * Remove @link for Tracer.activeSpan(). 'Nested' links do not happen to exist under javadoc, so Tracer.scopeManager().active().span() is now not a link. * Change BINARY to be resizable and stream-oriented. (opentracing#223) * Change BINARY to be resizable and stream-oriented. * Abstract the Binary adapter and have an Adapters class to return it. * Remove the isInbound/isOutbound methods from BinaryAdapter. * Make Binary use the Channel paradigm for injection/extraction * Have binary methods in Adapters named after inject/extract. * Add a BINARY propagator for MockTracer. * Throw RuntimeException in case of errors during BINARY's injection. * Put braces around if-blocks for Adapters/BinaryAdapter. * Use verbose messages for null parameters in Adapters. * SpanBuilder deprecate startManual (opentracing#225) * SpanBuilder deprecate startManual * Fix review comments * Remove default finish behaviour for `activate()` (opentracing#219) * Do not auto finish on scope.close * Fix review comments * Fix review comments * Add explanatory statement about not auto-finishing * Define only activate(s, bool) * Use the parameterless startActive() in a forgotten test in MockTracerTest.

From 0.30.0 to 0.31.0 they have made some changes, https://github.com/opentracing/opentracing-java/pull/251/files Highlights are : - opentracing/opentracing-java#219 - a new method in Tracer, activeSpan - SpanBuilder's startActive changed to startActive(boolean finishSpanOnClose) Last but not least, found that someone has created a compatibility layer for 0.30.0 API, more info: opentracing/opentracing-java#221 They finally decided to create it as an external library https://github.com/opentracing/opentracing-java-v030/, which is already on maven central https://mvnrepository.com/artifact/io.opentracing/opentracing-v030/0.0.4

Do not auto finish on scope.close

cd26a39

yurishkuro reviewed Nov 29, 2017

View reviewed changes

Fix review comments

1bb2e14

objectiser reviewed Nov 29, 2017

View reviewed changes

Fix review comments

be7ac02

objectiser mentioned this pull request Nov 29, 2017

Should 'ignoreActiveSpan' be changed to 'ignoreScope' #222

Closed

yurishkuro previously approved these changes Nov 30, 2017

View reviewed changes

pavolloffay mentioned this pull request Nov 30, 2017

SpanBuilder deprecate startManual #225

Merged

Add explanatory statement about not auto-finishing

8cb95fb

Define only activate(s, bool)

6423db2

pavolloffay changed the title ~~Do not auto finish on scope.close~~ Remove default finish behaviour for activate() Dec 1, 2017

malafeev approved these changes Dec 7, 2017

View reviewed changes

yurishkuro reviewed Dec 7, 2017

View reviewed changes

README.md

// Do things.

} catch {

Tags.ERROR.set(scope.span(), true);

Copy link

Member

yurishkuro Dec 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scope.span() is inaccessible here according to the comment

yurishkuro reviewed Dec 7, 2017

View reviewed changes

yurishkuro approved these changes Dec 7, 2017

View reviewed changes

Merge branch 'v0.31.0' into scope-do-not-close-by-default

1a1a0ec

tedsuo merged commit 39036f9 into opentracing:v0.31.0 Dec 15, 2017

pavolloffay mentioned this pull request Dec 15, 2017

Fix readme and docs about scope manager #232

Merged

yacota mentioned this pull request Feb 11, 2018

#issue 376 : problems with transitive dependencies from jaeger stagemonitor/stagemonitor#379

Merged

		@@ -53,15 +53,15 @@ public void activateSpan() throws Exception {
		}

		// Make sure the Span got finish()ed.

Remove default finish behaviour for activate() #219

Remove default finish behaviour for activate() #219

Conversation

pavolloffay commented Nov 15, 2017

coveralls commented Nov 15, 2017

carlosalberto commented Nov 20, 2017 • edited Loading

carlosalberto commented Nov 28, 2017

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 29, 2017

objectiser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Nov 29, 2017

coveralls commented Nov 29, 2017

tedsuo commented Nov 29, 2017

tylerbenson commented Nov 30, 2017

yurishkuro commented Nov 30, 2017

coveralls commented Nov 30, 2017

pavolloffay commented Nov 30, 2017

tedsuo commented Nov 30, 2017

pavolloffay commented Dec 1, 2017

coveralls commented Dec 1, 2017

tylerbenson commented Dec 1, 2017

carlosalberto commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Dec 11, 2017

coveralls commented Dec 15, 2017

Remove default finish behaviour for `activate()` #219

Remove default finish behaviour for `activate()` #219

carlosalberto commented Nov 20, 2017 •

edited

Loading