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

Initial implementation of a no-op active Span. #320

Closed

Conversation

carlosalberto
Copy link
Collaborator

@carlosalberto carlosalberto commented Nov 16, 2018

Hey all,

I decided to test out, as a prototype, a no-op active Scope/Span for the current api. This had been suggested (or at least mentioned) in the past. And it could probably help with features such as #319

Advantages:

  • Never have to check for null on ScopeManager.active() nor ScopeManager.activeSpan().

Potential issues:

  • Tracers need to be aware of this feature - see the code in MockTracer which will check against NoopSpanContext, and which case it will ignore it as potential parent when creating new Spans.
  • Test writers might need to check against the same -, i.e. assertEquals(NoopScope.INSTANCE, scopeManager.active()) instead of assertNull(scopeManager.active()).

Thoughts?

@yurishkuro @felixbarny @tedsuo @sjoerdtalsma @tylerbenson @opentracing/opentracing-java-maintainers

PS - Did not update the testbed module. Will do that later if we decide to go on and merge this change ;)

@sjoerdtalsma
Copy link
Contributor

Conceptually, I would have a little trouble understanding what a no-op scope is. Is it a scope? Is it an empty scope? If so, would that be a better name? What is no-op scope precisely?

Ideally, I would prefer to receive an Optional result from the active.. methods. To me, it is much clearer about what is happening.

@sjoerdtalsma
Copy link
Contributor

Accidentally closed this pull request, sorry!

@sjoerdtalsma sjoerdtalsma reopened this Dec 6, 2018
@carlosalberto
Copy link
Collaborator Author

Hey @sjoerdtalsma

Is it a scope? Is it an empty scope? If so, would that be a better name? What is no-op scope precisely?

I'd say it's both an empty scope and an empty span, that basically do nothing. We could probably change the name?

The Optional is another possibility to consider, but we don't support Java 8 yet ;)

@carlosalberto
Copy link
Collaborator Author

Oh, btw, this is related ;) opentracing/specification#116

@carlosalberto
Copy link
Collaborator Author

After the lack of interest, I'm closing this PR. We can definitely re-open or create a new ticket if/when this feature is needed ;)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants