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

Allow clearing the current Scope. #313

Open
wants to merge 2 commits into
base: v0.32.0
Choose a base branch
from
Open

Allow clearing the current Scope. #313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
18933ff
Allow clearing the currently active Scope.
carlosalberto Oct 18, 2018
e9868ee
Add a note on ThreadLocal not calling remove() for the clear().
carlosalberto Oct 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions opentracing-api/src/main/java/io/opentracing/ScopeManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,16 @@ public interface ScopeManager {
*/
Span activeSpan();

/**
* Clear the current active state, setting both {@link #active()} and
* {@link #activeSpan()} to null for the current context (usually a thread).
*
* <p>
* This method can be called to discard any previously leaked {@link Scope} objects
* that were unintentionally left active.
*/
void clear();
Copy link

Choose a reason for hiding this comment

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

Is there any difference in expected behavior between this calling

activate(null)

Copy link
Contributor

Choose a reason for hiding this comment

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

Some implementations may keep a stack of activated scopes. My understanding is that clear() would clear the whole stack and activate would push another element on the stack.

Not quite sure but clear() may have slightly different semantics depending on whether the ScopeManager maintains a stack of Scopes or only holds the currently active scope.

Copy link
Contributor

@sjoerdtalsma sjoerdtalsma Oct 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

👍 My context manager -by default- maintains a stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the OT, but I'm thinking about doing the same. What are your reasons for that? Is your context manager on GitHub?

Copy link

Choose a reason for hiding this comment

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

Care to share (@sjoerdtalsma @felixbarny) about the benefits and tradeoffs with actively managing a stack vs a simple reference to the current which thus has reference to its' parent.

Copy link
Contributor

@sjoerdtalsma sjoerdtalsma Oct 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Since you ask: My context manager is 'pluggable' and has no real opinion on stacking or not. The default context uses a stack so you can easily change e.g. the current Locale just in a small piece of code and have everything else the way it was. Current supported contexts include MDC, Spring security context, Locale, originating ServletRequest, and OpenTracing Span. You can plug additional types via a ServiceLoader SPI.
For many situations, having a Closeable context that can be nested is the 'easiest' for users (as it returns things to the way they were). For OpenTracing Spans however, I just delegate to the configured ScopeManager.
More info: https://github.com/talsma-ict/context-propagation
For questions, feel free to ping me at gitter, always curious what others think.

@tylerbenson Sorry, just re-read your question (sorry for the late reply)

a simple reference to the current which thus has reference to its' parent

--> This forms what I meant by stack, it virtually forms a 'stack' of contexts this way (opposed to keeping only the current value)

Copy link
Contributor

@sjoerdtalsma sjoerdtalsma Oct 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Tradeoffs for stacking:
➕ Support nesting
➕ User experience: 'return things they way they were' / 'works as expected' in many cases
➖ Obviously a bigger chance of bigger memory leaks if open-close semantics are not correctly used.
➖ More memory consumption / garbage collecting

Copy link
Contributor

@felixbarny felixbarny Oct 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

More memory consumption / garbage collecting

Not sure if that is true. The stack won't be collected unless the thread terminates.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a good idea. It is a hack, but a similar hack can be achieved by allowing a closure that clears a scope. ex passing a null tracecontext to the thing that scopes the active span

Copy link
Contributor

Choose a reason for hiding this comment

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

the benefit of passing null is you can still provide for "descoping that null" which is in easiest form returning a noop scope object when it was already null.


/**
* @deprecated use {@link #activate(Span)} instead.
* Set the specified {@link Span} as the active instance for the current
Expand Down
4 changes: 4 additions & 0 deletions opentracing-noop/src/main/java/io/opentracing/noop/NoopScopeManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ public Span activeSpan() {
return NoopSpan.INSTANCE;
}

@Override
public void clear() {
}

static class NoopScopeImpl implements NoopScopeManager.NoopScope {
@Override
public void close() {}
Expand Down
7 changes: 7 additions & 0 deletions opentracing-util/src/main/java/io/opentracing/util/AutoFinishScopeManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,11 @@ public Span activeSpan() {
AutoFinishScope scope = tlsScope.get();
return scope == null ? null : scope.span();
}

@Override
public void clear() {
// Set a null value instead of calling remove(),
// to prevent unnecessary allocation of new ThreadLocal-related objects.
tlsScope.set(null);
sjoerdtalsma marked this conversation as resolved.
Show resolved Hide resolved
}
}
7 changes: 7 additions & 0 deletions opentracing-util/src/main/java/io/opentracing/util/ThreadLocalScopeManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,11 @@ public Span activeSpan() {
Scope scope = tlsScope.get();
return scope == null ? null : scope.span();
}

@Override
public void clear() {
// Set a null value instead of calling remove(),
// to prevent unnecessary allocation of new ThreadLocal-related objects.
tlsScope.set(null);
sjoerdtalsma marked this conversation as resolved.
Show resolved Hide resolved
}
}
14 changes: 14 additions & 0 deletions opentracing-util/src/test/java/io/opentracing/util/AutoFinishScopeManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,18 @@ public void activateSpan() throws Exception {
Scope missingSpan = source.active();
assertNull(missingSpan);
}

@Test
public void clear() throws Exception {
Span span = mock(Span.class);

Scope scope = source.activate(span);
assertNotNull(scope);
assertNotNull(source.active());
assertNotNull(source.activeSpan());

source.clear();
assertNull(source.active());
assertNull(source.activeSpan());
}
}
14 changes: 14 additions & 0 deletions opentracing-util/src/test/java/io/opentracing/util/ThreadLocalScopeManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,4 +114,18 @@ public void dontFinishSpanNoClose() throws Exception {
assertNull(source.active());
assertNull(source.activeSpan());
}

@Test
public void clear() throws Exception {
Span span = mock(Span.class);

Scope scope = source.activate(span);
assertNotNull(scope);
assertNotNull(source.active());
assertNotNull(source.activeSpan());

source.clear();
assertNull(source.active());
assertNull(source.activeSpan());
}
}