-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Polishing Observation API #3425
Polishing Observation API #3425
Conversation
…e in the ObservationRegistry
and make getError return Throwable (not Optional<Throwable>)
default <T extends Observation.Context> Observation createNotStarted( | ||
@Nullable ObservationConvention<T> customConvention, @NonNull ObservationConvention<T> defaultConvention, | ||
@NonNull Supplier<T> contextSupplier, @NonNull ObservationRegistry registry) { | ||
default <T extends Observation.Context> Observation observation(@Nullable ObservationConvention<T> customConvention, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss if we want to rename this class.
@shakuzen Did you get some feedback on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we rename existing observation
to createNotStarted
?
Either way, it is better to be unified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yepp, that's why the let's discuss comment.
I think:
- If we want to keep the class name as-is: it is better to rename everything to
createNotStarted
- If we want to rename the class to
ObservationDocumentation
(or similar): it is better to rename everything toobservation
- If we want to keep changes minimal: what I did
I think the best would be # 2 but it is also the most invasive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ObservationDocumentation
better expresses what the interface is for and makes it clear that it is not an Observation
itself. The feedback I got so far was that they agreed with that. I think it will be easier to consider that change in a separate pull request, though, so we don't block making the rest of these changes.
micrometer-observation/src/main/java/io/micrometer/observation/ObservationRegistry.java
Outdated
Show resolved
Hide resolved
micrometer-observation/src/main/java/io/micrometer/observation/ObservationRegistry.java
Outdated
Show resolved
Hide resolved
micrometer-observation/src/main/java/io/micrometer/observation/aop/ObservedAspect.java
Outdated
Show resolved
Hide resolved
This reverts commit 18b7b5c.
micrometer-observation/src/main/java/io/micrometer/observation/Observation.java
Outdated
Show resolved
Hide resolved
BTW if we're chaning the |
micrometer-observation/src/main/java/io/micrometer/observation/ObservationRegistry.java
Outdated
Show resolved
Hide resolved
micrometer-observation/src/main/java/io/micrometer/observation/Observation.java
Outdated
Show resolved
Hide resolved
micrometer-observation/src/main/java/io/micrometer/observation/ObservationRegistry.java
Show resolved
Hide resolved
|
The return type is no longer an Optional, but the Throwable directly or null. See micrometer-metrics/micrometer#3425
DocumentedObservation.createNotStarted
->observation
(to make it consistent with others)ObservationRegistry.observationConvention
: removed varargsgetObservationConvention
andisObservationEnabled
package-private in theObservationRegistry
(should not be used)getError
returnThrowable