Add ObservationValidator #5300
Add ObservationValidator #5300
Conversation
shakuzen
left a comment
shakuzen
left a comment
There was a problem hiding this comment.
Looks good. I think it'd be good to have some test cases of expected instrumentation patterns and assert that there is no invalid message - we want to make sure we don't introduce false positive validation issues.
jonatan-ivanov
force-pushed
the
observation-validator
branch
from
d365754 to
bc2a260
Compare
|
Added some tests to cover those cases too. |
| private final Predicate<Context> supportsContextPredicate; | ||
|
|
||
| ObservationValidator() { | ||
| this(validationResult -> LOGGER.warn(validationResult.toString())); |
There was a problem hiding this comment.
I approved and merged, but I wonder if it would be better to throw an exception by default (what gets used with TestObservationRegistry). I worry that a log message in tests will very easily be missed. The things we're checking for here should plainly be instrumentation bugs and so unless there's a bug in this validator, I'm not sure it should be a problem that an exception is thrown. Thoughts?
There was a problem hiding this comment.
I agree. The validator is installed by default, but I can't think of a case where duplicate stops calls is not considered as a bug in the instrumentation. If somehow this is expected, people should overrride the consumer and log things.
There was a problem hiding this comment.
If somehow this is expected, people should overrride the consumer and log things.
The issue with this is that the validator was intentionally made package private so it is all kept as an implementation detail. As things are, there's no option for users to override the consumer. We could make that public API so it is possible, but we'll have to make sure the API is right before going GA.
There was a problem hiding this comment.
I think we have a few options to make this more flexible:
- We can make the API public and add an overload to the test registry to inject handlers
- We can make only the consumer part public (+ the class that is received by the consumer) and add an overload to the test registry to inject the consumer (I don't think there is a huge difference between this and the previous one)
- We can also add an overload to the test registry with a flag to not register the validator
There was a problem hiding this comment.
I think my preference would be to switch it to throw an exception and not make anything configurable until we get feedback from a user why the validator's assumptions about correct instrumentation are wrong for their use case. Catching and ignoring the exception thrown in a test is always a workaround on a case-by-case basis. The challenge, as always, is getting enough feedback during the milestone phase before releasing GA, but it's also good for us to understand such use cases since I think we are lacking imagination to come up with them.
There was a problem hiding this comment.
👍🏼 In order to do that, we need to surface at least an exception so that users can handle it as needed: #5307
3 participants
Closes gh-5239