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
Abstraction for instrumentating with a Timer or Observation (optionally) #3357
Conversation
…tion (optionally) We have a lot of existing instrumentations with Timer that are good candidates to be rewritten with the Observation API for added extensibility. However, we cannot break compatibility for existing users by requiring an ObservationRegistry for each instrumentation. We will want to add an option to configure an ObservationRegistry on the builder for the instrumentation or as an additional constructor if there isn't a builder. Instrumentation should still work as before without providing an ObservationRegistry. This can be handled in each instrumentation, but this added class ObservationOrTimerCompatibleInstrumentation is intended to make it easier to support this case where instrumentation may be done with a Timer or Observation depending on whether an ObservationRegistry is available. By having a common abstraction for this, we can hopefully make supporting Observation in our existing instrumentations easier, and also avoid adding overhead to instrumentation by avoiding instantiating things for Observation when a Timer is used (and the opposite situation).
...a/io/micrometer/core/instrument/observation/ObservationOrTimerCompatibleInstrumentation.java
Show resolved
Hide resolved
...a/io/micrometer/core/instrument/observation/ObservationOrTimerCompatibleInstrumentation.java
Show resolved
Hide resolved
...a/io/micrometer/core/instrument/observation/ObservationOrTimerCompatibleInstrumentation.java
Outdated
Show resolved
Hide resolved
* @param <RES> type of the response | ||
*/ | ||
public <RES> void setResponse(RES response) { | ||
if (observationRegistry.isNoop() || !(context instanceof RequestReplySenderContext)) { |
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.
What about RequestReplyReceiverContext
? We have the response there too?
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.
Good catch. I had only tried this with the Apache HTTP client instrumentation, which is why I probably only thought of this. I expect we'll catch other things and add to it as we use this in more places.
Do you think it makes sense to extract the response-related methods to an interface so we could check for that here and just cast to that?
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.
Sure, that's a good idea
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'll follow-up with this in a separate pull request. I've left a TODO comment for now.
We have a lot of existing instrumentations with Timer that are good candidates to be rewritten with the Observation API for added extensibility. However, we cannot break compatibility for existing users by requiring an ObservationRegistry for each instrumentation. We will want to add an option to configure an ObservationRegistry on the builder for the instrumentation or as an additional constructor if there isn't a builder. Instrumentation should still work as before without providing an ObservationRegistry. This can be handled in each instrumentation, but this added class ObservationOrTimerCompatibleInstrumentation is intended to make it easier to support this case where instrumentation may be done with a Timer or Observation depending on whether an ObservationRegistry is available. By having a common abstraction for this, we can hopefully make supporting Observation in our existing instrumentations easier, and also avoid adding overhead to instrumentation by avoiding instantiating things for Observation when a Timer is used (and the opposite situation).