-
Notifications
You must be signed in to change notification settings - Fork 984
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
Add a LongTaskTimer for every Observation in TimerObservationHandler #3215
Add a LongTaskTimer for every Observation in TimerObservationHandler #3215
Conversation
This way handlers will be able to handle tasks that can potentially take long differently (e.g.: tracking active tasks while they are active)
...er-core/src/main/java/io/micrometer/core/instrument/observation/TimerObservationHandler.java
Show resolved
Hide resolved
...crometer-samples-core/src/main/java/io/micrometer/core/samples/ObservationHandlerSample.java
Outdated
Show resolved
Hide resolved
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 like this. I think it's worthwhile in most cases to have a LTT along with a Timer to see metrics on in-flight tasks. In theory, if something being timed has a timeout that's less than the step interval - meaning an active task would never be timed across two step intervals, then I guess the LTT becomes less interesting. As long as the overhead of the LTT isn't a problem, though, I think it is fine.
The difference in tags is a potential point of confusion. We should document it in the JavaDoc perhaps.
.lowCardinalityKeyValue("staticTag", "42").start(); | ||
|
||
// created after start, LongTaskTimer won't have it | ||
observation.lowCardinalityKeyValue("dynamicTag", "24"); |
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.
In general that shouldn't really happen. We will recommend to the users that they create a non started observation, and always use a key value provider (I think that in extremely rare cases they could set key value pairs manually)
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.
The KeyValueProvider
is the same case since it is queried at stop
when you have all the details in the context and the LongTaskTimer
is created at start
.
This PR changes the behavior of
TimerObservationHandler
so that it will always create an additionalLongTaskTimer
(on top of theTimer
) so that the active task can be observed too on-the-fly.There is a caveat/issue with this: since the
KeyValuesProvider
is used atstop
, none of those tags will be added to theLongTaskTimer
. Also, the tags that are added to theObservation
afterstart
won't be added to theLongTaskTimer
./cc @cppwfs