Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Remove default finish behaviour for activate() #219

Merged
merged 6 commits into from
Dec 15, 2017

Conversation

pavolloffay
Copy link
Member

For more see #189 (comment)

@coveralls
Copy link

Coverage Status

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
Copy link
Collaborator

carlosalberto commented Nov 20, 2017

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
Copy link
Collaborator

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

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

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

README.md Outdated
//
// If we create async work, `scope.span()` allows us to pass the `Span` along as well.
} catch {
Tags.ERROR.set(span(), true);
Copy link
Member

Choose a reason for hiding this comment

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

Was this meant to call scope.span() ?

README.md Outdated
@@ -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()`.
Copy link
Member

Choose a reason for hiding this comment

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

, only to restore previous scope

?

README.md Outdated
} finally {
scope.deactivate();
//
// If we create async work, `scope.span()` allows us to pass the `Span` along as well.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

* // 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
Copy link
Member

Choose a reason for hiding this comment

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

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

@coveralls
Copy link

Coverage Status

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.

Copy link
Contributor

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

Looking good - just some minor rewording.

README.md Outdated
@@ -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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

"starts a Scope..."

README.md Outdated
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

README.md Outdated
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

README.md Outdated
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

"is a Scope..."

README.md Outdated
@@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

could be :)

* // (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
Copy link
Contributor

Choose a reason for hiding this comment

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

"Span finishes automatically..."

@@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indentation

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

// Make sure the Span got finish()ed.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@pavolloffay
Copy link
Member Author

@yurishkuro @objectiser thanks, changes applied.

@coveralls
Copy link

Coverage Status

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
Copy link
Member

tedsuo commented Nov 29, 2017

Any objections to merging this at this point?

yurishkuro
yurishkuro previously approved these changes Nov 30, 2017
@tylerbenson
Copy link

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.

@yurishkuro yurishkuro dismissed their stale review November 30, 2017 05:37

change my mind

@yurishkuro
Copy link
Member

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
Copy link

Coverage Status

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
Copy link
Member Author

@tylerbenson I have added more comments in 8cb95fb

@tedsuo
Copy link
Member

tedsuo commented Nov 30, 2017

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
Copy link
Member Author

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
Copy link

Coverage Status

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
Copy link

@pavolloffay can you update the PR description too please?

@pavolloffay pavolloffay changed the title Do not auto finish on scope.close Remove default finish behaviour for activate() Dec 1, 2017
@carlosalberto
Copy link
Collaborator

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.

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

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

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

Choose a reason for hiding this comment

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

might be good to add a catch clause with a comment

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

@@ -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}
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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).
Copy link
Member

Choose a reason for hiding this comment

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

  • not just startActive but start as well.
  • no need for parentheses around (rather than at buildSpan)

@carlosalberto
Copy link
Collaborator

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
Copy link

Coverage Status

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.

@tedsuo tedsuo merged commit 39036f9 into opentracing:v0.31.0 Dec 15, 2017
carlosalberto pushed a commit to carlosalberto/opentracing-java that referenced this pull request Dec 15, 2017
* 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)
carlosalberto added a commit to carlosalberto/opentracing-java that referenced this pull request Dec 17, 2017
* 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.
yacota added a commit to yacota/stagemonitor that referenced this pull request Feb 11, 2018
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants