Properly clears the scopes #2280
Conversation
| final brave.propagation.CurrentTraceContext delegate; | ||
|
|
||
| public BraveCurrentTraceContext(brave.propagation.CurrentTraceContext delegate) { | ||
| this.delegate = delegate; | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
@Override isn't needed anymore?
| /** | ||
| * Brave implementation of a {@link CurrentTraceContext}. | ||
| * | ||
| * @author Marcin Grzejszczak |
There was a problem hiding this comment.
Should not we have this, since it is a public class?
| return new BraveTraceContext(context); | ||
| CurrentTraceContext.Scope previous = this.scopes.get(); | ||
| CurrentTraceContext.Scope current = new BraveScope(this.delegate.newScope(BraveTraceContext.toBrave(context))); | ||
| return new RevertingScope(this, previous, current); |
There was a problem hiding this comment.
If RevertingScope deals with the scopes ThreadLocal, would not it be better to let it get the previous one on it's own instead of passing it? That way all logic that touched scopes would be in the same place.
There was a problem hiding this comment.
Can you elaborate? I don't think i follow
There was a problem hiding this comment.
Actually now i wonder if we shouldn't do it on the level of sleuth api rather than Braves. The same way we do it with micrometer tracing...
There was a problem hiding this comment.
I meant moving this:
CurrentTraceContext.Scope previous = this.scopes.get();Into the ctor of RevertingScope but maybe we can also move current:
CurrentTraceContext.Scope current = new BraveScope(this.delegate.newScope(BraveTraceContext.toBrave(context)));There was a problem hiding this comment.
We can't move all of that into reverting scpoe cause there's one reverting scope but two delegates (maybeScope, newScope). I can improve this constructor however.
We also can't move the code to sleuth's api cause there are no implementations there - just interfaces. It has to stay here & in OTel.
marcingrzejszczak
force-pushed
the
clearing_scopes
branch
from
7b4eef0 to
1bcf14b
Compare
without this fix when calling `maybeScope(null)` or `withSpan(null)` or `newScope(null)` we ended up with creating a null scope that would allocate additional resources and not clear things. with this fix we're clearing all the scopes and removing thread locals
marcingrzejszczak
force-pushed
the
clearing_scopes
branch
from
1bcf14b to
dde5b8e
Compare
without this fix when calling
maybeScope(null)orwithSpan(null)ornewScope(null)we ended up with creating a null scope that would allocate additional resources and not clear things.with this fix we're clearing all the scopes and removing thread locals