Improves the ThreadLocalAccessor story of continuing scopes #3731
Conversation
marcingrzejszczak
force-pushed
the
continue_scope_1_10
branch
2 times, most recently
from
e1c0e90
to
c1e1212
Compare
micrometer-observation/src/main/java/io/micrometer/observation/SimpleObservation.java
Outdated
Show resolved
Hide resolved
micrometer-observation/src/main/java/io/micrometer/observation/SimpleObservation.java
Outdated
Show resolved
Hide resolved
micrometer-observation/src/main/java/io/micrometer/observation/SimpleObservation.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public void makeCurrent() { |
There was a problem hiding this comment.
I'd add a comment that this logic is justified only under the circumstance that before it is executed, a reset was called immediately before.
micrometer-observation/src/main/java/io/micrometer/observation/SimpleObservation.java
Show resolved
Hide resolved
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
| @@ -47,6 +47,8 @@ class SimpleObservation implements Observation { | |||
|
|
|||
| private final Collection<ObservationFilter> filters; | |||
|
|
|||
| private final ThreadLocal<Scope> scopeThreadLocal = new ThreadLocal<>(); | |||
There was a problem hiding this comment.
ThreadLocalUsage: ThreadLocals should be stored in static fields
ℹ️ Expand to see all @sonatype-lift commands
You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.
| Command | Usage |
|---|---|
@sonatype-lift ignore |
Leave out the above finding from this PR |
@sonatype-lift ignoreall |
Leave out all the existing findings from this PR |
@sonatype-lift exclude <file|issue|path|tool> |
Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file |
Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.
Help us improve LIFT! (Sonatype LiftBot external survey)
Was this a good recommendation for you? Answering this survey will not impact your Lift settings.
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]
| @@ -47,6 +47,8 @@ class SimpleObservation implements Observation { | |||
|
|
|||
| private final Collection<ObservationFilter> filters; | |||
|
|
|||
| private final ThreadLocal<Scope> enclosingScopeThreadLocal = new ThreadLocal<>(); | |||
There was a problem hiding this comment.
ThreadLocalUsage: ThreadLocals should be stored in static fields
ℹ️ Expand to see all @sonatype-lift commands
You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.
| Command | Usage |
|---|---|
@sonatype-lift ignore |
Leave out the above finding from this PR |
@sonatype-lift ignoreall |
Leave out all the existing findings from this PR |
@sonatype-lift exclude <file|issue|path|tool> |
Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file |
Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.
Help us improve LIFT! (Sonatype LiftBot external survey)
Was this a good recommendation for you? Answering this survey will not impact your Lift settings.
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]
marcingrzejszczak
force-pushed
the
continue_scope_1_10
branch
from
aab85b4
to
c733ea2
Compare
The Scope#makeCurrent method is called e.g. via ObservationThreadLocalAccessor#restore(Observation). In that case, we're calling ObservationThreadLocalAccessor#reset() first, and we're closing all the scopes, HOWEVER those are called on the Observation scope that was present there in thread local at the time of calling the method, NOT on the scope that we want to make current (that one can contain some leftovers from previous scope openings like creation of e.g. Brave scope in the TracingContext that is there inside the Observation's Context. When we want to go back to the enclosing scope and want to make that scope current we need to be sure that there are no remaining scoped objects inside Observation's context. This is why BEFORE rebuilding the scope structure we need to notify the handlers to clear them first (again this is a separate scope to the one that was cleared by the reset method) via calling ObservationHandler#onScopeReset(Context).
When we reset a scope, we don't want to close it thus we don't want to remove any enclosing scopes. With this logic we reset any existing scopes on reset by calling the handler + when we make an enclosing scope current we will remove it from the list of enclosing scopes
@chemicL