Add Scope closing validation to ObservationValidator#7253
Add Scope closing validation to ObservationValidator#7253jonatan-ivanov merged 3 commits intomicrometer-metrics:mainfrom
Conversation
shakuzen
left a comment
There was a problem hiding this comment.
Great work. Thank you for this. Looks good to me.
...er-observation-test/src/main/java/io/micrometer/observation/tck/TestObservationRegistry.java
Outdated
Show resolved
Hide resolved
...meter-observation-test/src/main/java/io/micrometer/observation/tck/ObservationValidator.java
Outdated
Show resolved
Hide resolved
...meter-observation-test/src/main/java/io/micrometer/observation/tck/ObservationValidator.java
Show resolved
Hide resolved
...-observation-test/src/test/java/io/micrometer/observation/tck/ObservationValidatorTests.java
Show resolved
Hide resolved
|
|
||
| private final Map<String, Set<String>> lowCardinalityKeysByObservationName; | ||
|
|
||
| private final Deque<Context> globalScopeStack; |
There was a problem hiding this comment.
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?
...meter-observation-test/src/main/java/io/micrometer/observation/tck/ObservationValidator.java
Outdated
Show resolved
Hide resolved
5ce0a26 to
3017abe
Compare
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]>
|
@seonghyeoklee Thank you very much! |
|
After merging this in I realized something. Doing this: 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();
} |
|
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. |
…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]>
Summary
TestObservationRegistry:Capabilityenum values and builder methods, keeping them opt-in to avoid breaking reactive contextsChanges
ObservationValidator: Track scope state (thread IDs via per-contextScopeState, global scope stack for LIFO ordering). Validate ononScopeClosed, clear ononScopeReset.TestObservationRegistry: AddSCOPES_SHOULD_BE_CLOSED_IN_REVERSE_ORDER_OF_OPENINGandSCOPES_SHOULD_BE_OPENED_AND_CLOSED_ON_THE_SAME_THREADcapabilities 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 passesscopeClosedInWrongOrderShouldBeInvalid- Wrong order detectedscopeClosedOnSameThreadShouldBeValid- Same thread passesscopeClosedOnDifferentThreadShouldBeInvalid- Different thread detectedscopeValidationNotEnabledByDefault- No validation without opt-innestedScopesOnSameObservationClosedInCorrectOrderShouldBeValid- Both validations togetherObservationValidatorTestspassCloses gh-6329