Let Tracer.activeSpan() be a shorthand for ScopeManager's active Span. by carlosalberto · Pull Request #228 · opentracing/opentracing-java

carlosalberto · 2017-12-01T07:25:24Z

This is a small PR intended to address #227 as a way to provide a shorthand for getting the active span from a Tracer.

One alternative would have been to have Tracer.activeScope() but that would only save us from typing scopeManager(), so not much save here.

coveralls · 2017-12-01T07:27:16Z

Coverage increased (+0.2%) to 78.832% when pulling 44dad4e on carlosalberto:tracer_active_span into b9feb48 on opentracing:v0.31.0.

sjoerdtalsma · 2017-12-01T09:15:53Z

    ScopeManager scopeManager();

+    /**
+     * @return the active {@link Span}, if any. This is a shorthand for {@link Tracer#scopeManager()#active()#span()}.


Please also document the null check.
Otherwise people may either think this method could be unsafe to call in absence of active scope, or (worse) may think it's safe to unconditionally dereference active() scope in any situation.

Good catch, will do.

pavolloffay · 2017-12-01T09:13:43Z


+    @Override
+    public Span activeSpan() {
+        return NoopSpan.INSTANCE;


IIRC in 0.30.0 this returned null which could cause NPEs

It's probably better to return something.

Agree - although it might also be good to have some way to determine if there is an active span, in case an instrumentation wants to avoid doing unnecessary work. Possibly an additional method on the ScopeManager.hasActiveSpan()?

good point.

Possibly an additional method on the ScopeManager.hasActiveSpan()?

This seems like a duplicate of Span activeSpan().

It's related to #134. Maybe it's better to return null and in future add a convenient Optional-like method.

Or we could make noop to use span manager and return what is actually there.

coveralls · 2017-12-01T22:14:12Z

Coverage increased (+0.2%) to 78.832% when pulling a8e4b88 on carlosalberto:tracer_active_span into b9feb48 on opentracing:v0.31.0.

carlosalberto · 2017-12-01T22:15:15Z

Updated this PR - added a null case note for Tracer.activeSpan() and now NoopTracer.activeSpan() is returning null as well. @sjoerdtalsma @pavolloffay @objectiser

sjoerdtalsma · 2017-12-04T07:35:47Z

    ScopeManager scopeManager();

+    /**
+     * @return the active {@link Span}. This is a shorthand for {@link Tracer#scopeManager()#active()#span()},


Just curious: Do these @link javadoc tags actually work?

'Nested' links do not happen to exist under javadoc, so Tracer.scopeManager().active().span() is now not a link.

coveralls · 2017-12-04T17:48:07Z

Coverage increased (+0.2%) to 78.832% when pulling 5a6af78 on carlosalberto:tracer_active_span into b9feb48 on opentracing:v0.31.0.

carlosalberto · 2017-12-04T17:49:27Z

@sjoerdtalsma Oh, indeed. Removed the link for Tracer.scopeManager().active().span() (we have a @link to ScopeManager.active() there already, anyway).

carlosalberto · 2017-12-06T09:00:58Z

@sjoerdtalsma @pavolloffay @objectiser Any objections to have this PR merged?

sjoerdtalsma · 2017-12-06T09:40:18Z

@carlosalberto I have no objections

objectiser · 2017-12-06T12:05:50Z

@carlosalberto No objections.

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.

@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.

Let Tracer.activeSpan() be a shorthand for ScopeManager's active Span.

44dad4e

objectiser approved these changes Dec 1, 2017

View reviewed changes

sjoerdtalsma approved these changes Dec 1, 2017

View reviewed changes

pavolloffay approved these changes Dec 1, 2017

View reviewed changes

carlosalberto added 2 commits December 1, 2017 14:36

Document the null case for Tracer.activeSpan().

7bbf3d0

Have Tracer.activeSpan() return null.

a8e4b88

sjoerdtalsma approved these changes Dec 4, 2017

View reviewed changes

Remove @link for Tracer.activeSpan().

5a6af78

'Nested' links do not happen to exist under javadoc, so Tracer.scopeManager().active().span() is now not a link.

malafeev approved these changes Dec 6, 2017

View reviewed changes

tedsuo merged commit d8f3384 into opentracing:v0.31.0 Dec 6, 2017

carlosalberto deleted the tracer_active_span branch December 17, 2017 23:48

carlosalberto mentioned this pull request Jan 8, 2018

Should we add Tracer.activeSpan()? #227

Closed

Conversation

carlosalberto commented Dec 1, 2017

Uh oh!

coveralls commented Dec 1, 2017

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!

coveralls commented Dec 1, 2017

Uh oh!

carlosalberto commented Dec 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 4, 2017

Uh oh!

carlosalberto commented Dec 4, 2017

Uh oh!

carlosalberto commented Dec 6, 2017

Uh oh!

sjoerdtalsma commented Dec 6, 2017

Uh oh!

objectiser commented Dec 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants