-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Observe instantaneous events #3247
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
Observe instantaneous events #3247
Conversation
# Conflicts: # micrometer-core/src/test/java/io/micrometer/core/instrument/binder/okhttp3/OkHttpMetricsEventListenerTest.java # micrometer-observation/src/main/java/io/micrometer/observation/ObservationHandler.java
# Conflicts: # micrometer-observation/src/main/java/io/micrometer/observation/Observation.java
micrometer-observation/src/main/java/io/micrometer/observation/Observation.java
Show resolved
Hide resolved
# Conflicts: # micrometer-core/src/test/java/io/micrometer/core/instrument/binder/okhttp3/OkHttpMetricsEventListenerTest.java
micrometer-observation/src/main/java/io/micrometer/observation/Observation.java
Show resolved
Hide resolved
micrometer-observation/src/main/java/io/micrometer/observation/Observation.java
Show resolved
Hide resolved
shakuzen
left a comment
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.
Looks good. I just left one question about the naming of the counter in the default meter handler.
| public boolean supportsContext(Observation.Context context) { | ||
| return true; | ||
| public void onEvent(Observation.Event event, Observation.Context context) { | ||
| Counter.builder(event.getName()).tags(createTags(context)).register(meterRegistry).increment(); |
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.
Since the event is scoped to the Observation, I wonder if the Counter name should somehow include both. observation name + event name, perhaps? I'm not sure how this looks given typical use cases, though, which is probably the best gauge of what makes sense. Unless events are supposed to be standalone things that just happen to occur during an observation?
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.
It makes a lot of sense to me. Since we are attaching an Event to an Observation, I think it is a fair assumption that they belong together.
I can imagine a use-case where Events are just happening standalone, not connected to an Observation but to cover that use-case, I would use a different component (if we want to cover that at all).
A real-life example I can think of is applying the same business logic for two different endpoints or sources: let's say we are accepting messages over HTTP and AMQP but the workflow/business logic is the same for both.
This PR makes possible to signal instantaneous events that will be sent to handlers so they can record these
Events.A few notes, questions:
Eventsdon't haveKeyValues but handlers have access to theContext. Is there a use-case for being able to attachKeyValues to events?Eventscan be extended so arbitrary fields can be added (adding data is also possible to the Context).Events are not available from theContextand event history is not preserved (in theContext). Both should be simple to implement though.Eventand theContexttoo:onEvent(Observation.Event event, Observation.Context context).Contextbut that has a few consequences:Eventor theEventhistory should be available from theContextwhich would make it bigger and it can be problematic if concurrent events are possible.Counterbut adding events/annotations to theSpanshould be simple:ObservationHandlerSamplefor an example, you should see theCounter(custom.event) and a log entry simulating logging/tracing scenarios:[...] i.m.o.ObservationTextPublisher - EVENT - event.name='custom.event' [...]fyi:
TimerObservationHandlerwas renamed toDefaultMeterObservationHandlerbecause it can createCounters not onlyTimers.