Skip to content

Add Scope closing validation to ObservationValidator#7253

Merged
jonatan-ivanov merged 3 commits intomicrometer-metrics:mainfrom
seonghyeoklee:gh-6329
Mar 12, 2026
Merged

Add Scope closing validation to ObservationValidator#7253
jonatan-ivanov merged 3 commits intomicrometer-metrics:mainfrom
seonghyeoklee:gh-6329

Conversation

@seonghyeoklee
Copy link
Copy Markdown
Contributor

Summary

  • Add two new opt-in validation capabilities for TestObservationRegistry:
    • LIFO scope ordering: Validates that scopes are closed in reverse order of opening
    • Same-thread validation: Validates that scopes are opened and closed on the same thread
  • Both validations are gated behind new Capability enum values and builder methods, keeping them opt-in to avoid breaking reactive contexts

Changes

  • ObservationValidator: Track scope state (thread IDs via per-context ScopeState, global scope stack for LIFO ordering). Validate on onScopeClosed, clear on onScopeReset.
  • TestObservationRegistry: Add SCOPES_SHOULD_BE_CLOSED_IN_REVERSE_ORDER_OF_OPENING and SCOPES_SHOULD_BE_OPENED_AND_CLOSED_ON_THE_SAME_THREAD capabilities with builder methods.
  • ObservationValidatorTests: Add tests for valid LIFO ordering, invalid LIFO ordering, same-thread valid, different-thread invalid, not enabled by default, and nested scopes.

Test plan

  • scopeClosedInCorrectOrderShouldBeValid - LIFO ordering passes
  • scopeClosedInWrongOrderShouldBeInvalid - Wrong order detected
  • scopeClosedOnSameThreadShouldBeValid - Same thread passes
  • scopeClosedOnDifferentThreadShouldBeInvalid - Different thread detected
  • scopeValidationNotEnabledByDefault - No validation without opt-in
  • nestedScopesOnSameObservationClosedInCorrectOrderShouldBeValid - Both validations together
  • All existing ObservationValidatorTests pass

Closes gh-6329

Copy link
Copy Markdown
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

Great work. Thank you for this. Looks good to me.

@shakuzen shakuzen requested a review from jonatan-ivanov March 11, 2026 02:48

private final Map<String, Set<String>> lowCardinalityKeysByObservationName;

private final Deque<Context> globalScopeStack;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think about renaming this to something that reflects that this is a collection of contexts that are "in scope"? For example scopedContexts of contextsInScope?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Renamed to scopedContexts in 3017abe.

@seonghyeoklee seonghyeoklee force-pushed the gh-6329 branch 2 times, most recently from 5ce0a26 to 3017abe Compare March 12, 2026 00:39
Add two new opt-in validation capabilities for TestObservationRegistry:

- SCOPES_SHOULD_BE_CLOSED_IN_REVERSE_ORDER_OF_OPENING: Validates that
  scopes are closed in LIFO order. If scope A is opened then scope B,
  scope B must be closed before scope A.

- SCOPES_SHOULD_BE_OPENED_AND_CLOSED_ON_THE_SAME_THREAD: Validates that
  a scope is closed on the same thread it was opened on.

Both validations are opt-in via the TestObservationRegistryBuilder to
avoid breaking existing tests in reactive contexts where scope handling
may follow different patterns.

Closes micrometer-metricsgh-6329

Signed-off-by: seonghyeoklee <[email protected]>
- Change @SInCE to 1.17.0 for new builder methods
- Add @author Seonghyeok Lee to ObservationValidator
- Fix error message string concatenation
- Rename globalScopeStack to scopedContexts
- Track openingThreadIds only when SAME_THREAD capability is enabled
- Add tests for multiple scope open/close exercising the Deque

Signed-off-by: seonghyeoklee <[email protected]>
@jonatan-ivanov
Copy link
Copy Markdown
Member

@seonghyeoklee Thank you very much!

@jonatan-ivanov jonatan-ivanov merged commit f524cb7 into micrometer-metrics:main Mar 12, 2026
11 checks passed
@jonatan-ivanov
Copy link
Copy Markdown
Member

jonatan-ivanov commented Mar 12, 2026

After merging this in I realized something. Doing this:

scope1.open
scope2.open
scope1.close
scope2.close

is valid if scope1 and scope2 are "siblings" i.e.: they run on different threads and observations in-parallel (e.g.: they belong to two different http requests).

Currently the validator ignores this and handles scopes "globally". Not sure how big of an issue this is.

Click to reveal test to reproduce this
@Test
void siblings() throws InterruptedException {
    TestObservationRegistry registry = TestObservationRegistry.builder()
        .validateScopesClosedInReverseOrderOfOpening(true)
        .build();
    registry.observationConfig().observationHandler(new ObservationTextPublisher());

//        Forcing the following order but scope 1 and scope2 are siblings
//            - scope1.open
//            - scope2.open
//            - scope1.close
//            - scope2.close

    CountDownLatch scopeOneOpenLatch = new CountDownLatch(1);
    CountDownLatch scopeTwoOpenLatch = new CountDownLatch(1);
    CountDownLatch scopeOneCloseLatch = new CountDownLatch(1);

    AtomicReference<Exception> error = new AtomicReference<>();
    Thread thread1 = new Thread(() -> {
        Observation observation1 = Observation.start("one", registry);
        try {
            Scope scope1 = observation1.openScope();
            scopeOneOpenLatch.countDown();
            scopeTwoOpenLatch.await();
            scope1.close();
            scopeOneCloseLatch.countDown();
        }
        catch (Exception e) {
            error.set(e);
        }
        finally {
            scopeOneCloseLatch.countDown();
        }
    });
    Thread thread2 = new Thread(() -> {
        try {
            scopeOneOpenLatch.await();
            Observation observation2 = Observation.start("two", registry);
            Scope scope2 = observation2.openScope();
            scopeTwoOpenLatch.countDown();
            scopeOneCloseLatch.await();
            scope2.close();
            observation2.stop();
        }
        catch (Exception e) {
            // noop
        }
    });

    thread1.start();
    thread2.start();
    thread1.join();
    thread2.join();

    assertThat(error.get()).isNull();
}

@shakuzen
Copy link
Copy Markdown
Member

I suspect that scenario should be fairly common and we should fix that. Let's open an issue to track it, and ideally fix it before RC1.

etrandafir93 pushed a commit to etrandafir93/micrometer that referenced this pull request Mar 21, 2026
…ics#7253)

Add two new opt-in validation capabilities for TestObservationRegistry:

- SCOPES_SHOULD_BE_CLOSED_IN_REVERSE_ORDER_OF_OPENING: Validates that
  scopes are closed in LIFO order. If scope A is opened then scope B,
  scope B must be closed before scope A.

- SCOPES_SHOULD_BE_OPENED_AND_CLOSED_ON_THE_SAME_THREAD: Validates that
  a scope is closed on the same thread it was opened on.

Both validations are opt-in via the TestObservationRegistryBuilder to
avoid breaking existing tests in reactive contexts where scope handling
may follow different patterns.

Closes micrometer-metricsgh-6329

Signed-off-by: seonghyeoklee <[email protected]>
Co-authored-by: Jonatan Ivanov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validate Observation.Scope closing when using TestObservationRegistry

3 participants