Improves the ThreadLocalAccessor story of continuing scopes by marcingrzejszczak · Pull Request #3731 · micrometer-metrics/micrometer (original) (raw)
A general bug
label
[![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4)
[
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
izeye added a commit to izeye/micrometer that referenced this pull request
shakuzen pushed a commit that referenced this pull request
shakuzen added a commit to shakuzen/micrometer that referenced this pull request
getEnclosingScope() was added in 1.10.6 (PR micrometer-metrics#3731) so that ObservationThreadLocalAccessor.restore(Observation) could obtain the observation's enclosing scope and call makeCurrent() before setValue().
In 1.10.x (PR micrometer-metrics#3831, NullObservation) the restore logic was changed to use the registry's current scope and getPreviousObservationScope() instead. Since then, getEnclosingScope() has not been used by any production code; the only remaining callers are tests in ObservationThreadLocalAccessorTests.
Deprecating the method is a first step toward removing it and the lastScope field in SimpleObservation, which will reduce per-observation overhead (ConcurrentHashMap field and scope bookkeeping on open/close).
shakuzen added a commit to shakuzen/micrometer that referenced this pull request
makeCurrent() was added in 1.10.6 (PR micrometer-metrics#3731) so that ObservationThreadLocalAccessor.restore(Observation) could re-apply an observation's scope when restoring: it called value.getEnclosingScope() then enclosingScope.makeCurrent() before setValue(value).
In 1.10.x (PR micrometer-metrics#3831, NullObservation) the restore logic was changed to use the registry's current scope and closeCurrentScope() instead. Since then, makeCurrent() has not been used by any production code; the only remaining caller is the test CurrentObservationTest.nestedScopes_makeCurrent().
Deprecating the method aligns with the getEnclosingScope() deprecation and allows future removal of the scope-rebuild logic in SimpleScope that exists only to support makeCurrent().
jonatan-ivanov pushed a commit that referenced this pull request
- Deprecate ObservationView.getEnclosingScope()
getEnclosingScope() was added in 1.10.6 (PR #3731) so that ObservationThreadLocalAccessor.restore(Observation) could obtain the observation's enclosing scope and call makeCurrent() before setValue().
In 1.10.x (PR #3831, NullObservation) the restore logic was changed to use the registry's current scope and getPreviousObservationScope() instead. Since then, getEnclosingScope() has not been used by any production code; the only remaining callers are tests in ObservationThreadLocalAccessorTests.
Deprecating the method is a first step toward removing it and the lastScope field in SimpleObservation, which will reduce per-observation overhead (ConcurrentHashMap field and scope bookkeeping on open/close).
- Deprecate Scope.makeCurrent()
makeCurrent() was added in 1.10.6 (PR #3731) so that ObservationThreadLocalAccessor.restore(Observation) could re-apply an observation's scope when restoring: it called value.getEnclosingScope() then enclosingScope.makeCurrent() before setValue(value).
In 1.10.x (PR #3831, NullObservation) the restore logic was changed to use the registry's current scope and closeCurrentScope() instead. Since then, makeCurrent() has not been used by any production code; the only remaining caller is the test CurrentObservationTest.nestedScopes_makeCurrent().
Deprecating the method aligns with the getEnclosingScope() deprecation and allows future removal of the scope-rebuild logic in SimpleScope that exists only to support makeCurrent().
- Deprecate Scope.reset() and ObservationHandler.onScopeReset()
Scope.reset() was used by ObservationThreadLocalAccessor until 1.10.x (PR #3831, NullObservation). That flow called reset() to clear thread locals before making another scope current. After #3831, OTLA uses closeCurrentScope() with scope.close() instead, so Scope.reset() is no longer used by any production code; the only remaining caller is ObservationValidatorTests.scopeResetAfterStopShouldBeValid().
ObservationHandler.onScopeReset(Context) is only invoked when Scope.reset() or Scope.makeCurrent() runs. Both are now deprecated and unused in production, so the callback is deprecated as well. Handlers should use onScopeClosed for cleanup when a scope is closed.
Deprecating these aligns with deprecating getEnclosingScope() and Scope.makeCurrent() and allows future removal of the reset/makeCurrent-related code paths.
etrandafir93 pushed a commit to etrandafir93/micrometer that referenced this pull request
- Deprecate ObservationView.getEnclosingScope()
getEnclosingScope() was added in 1.10.6 (PR micrometer-metrics#3731) so that ObservationThreadLocalAccessor.restore(Observation) could obtain the observation's enclosing scope and call makeCurrent() before setValue().
In 1.10.x (PR micrometer-metrics#3831, NullObservation) the restore logic was changed to use the registry's current scope and getPreviousObservationScope() instead. Since then, getEnclosingScope() has not been used by any production code; the only remaining callers are tests in ObservationThreadLocalAccessorTests.
Deprecating the method is a first step toward removing it and the lastScope field in SimpleObservation, which will reduce per-observation overhead (ConcurrentHashMap field and scope bookkeeping on open/close).
- Deprecate Scope.makeCurrent()
makeCurrent() was added in 1.10.6 (PR micrometer-metrics#3731) so that ObservationThreadLocalAccessor.restore(Observation) could re-apply an observation's scope when restoring: it called value.getEnclosingScope() then enclosingScope.makeCurrent() before setValue(value).
In 1.10.x (PR micrometer-metrics#3831, NullObservation) the restore logic was changed to use the registry's current scope and closeCurrentScope() instead. Since then, makeCurrent() has not been used by any production code; the only remaining caller is the test CurrentObservationTest.nestedScopes_makeCurrent().
Deprecating the method aligns with the getEnclosingScope() deprecation and allows future removal of the scope-rebuild logic in SimpleScope that exists only to support makeCurrent().
- Deprecate Scope.reset() and ObservationHandler.onScopeReset()
Scope.reset() was used by ObservationThreadLocalAccessor until 1.10.x (PR micrometer-metrics#3831, NullObservation). That flow called reset() to clear thread locals before making another scope current. After micrometer-metrics#3831, OTLA uses closeCurrentScope() with scope.close() instead, so Scope.reset() is no longer used by any production code; the only remaining caller is ObservationValidatorTests.scopeResetAfterStopShouldBeValid().
ObservationHandler.onScopeReset(Context) is only invoked when Scope.reset() or Scope.makeCurrent() runs. Both are now deprecated and unused in production, so the callback is deprecated as well. Handlers should use onScopeClosed for cleanup when a scope is closed.
Deprecating these aligns with deprecating getEnclosingScope() and Scope.makeCurrent() and allows future removal of the reset/makeCurrent-related code paths.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})