MeterRegistry implements ObservationRegistry, static factory methods #2
Conversation
Keeps only one registry implementation while still providing a specific type for ObservationRegistry where Observation-specific concerns can be defined. Forces instantiation of Observation through its static methods so there is no direct instantiation by users. This provides fewer ways to do the same thing and exposes API, which should help guide users and hide implementation details.
| } | ||
|
|
||
| //TODO Want this under observationConfig but not sure that's possible | ||
| public void withTimerObservationHandler() { |
There was a problem hiding this comment.
Shouldn't this return ObservationConfig so that you can do this:
registry
.withTimerObservationHandler()
.observationHandler(new SampleHandler())
.observationPredicate(...)
There was a problem hiding this comment.
Ideally, yes. That was my intention, but I didn't see a practical way to do that without making it a method on ObservationConfig, which shouldn't directly know about Timer. I couldn't think of a way to add that only in MeterRegistry. We should definitely think of something better than what's there now. It was temporary since it was late and I was out of ideas but just wanted to demonstrate the general intention.
There was a problem hiding this comment.
To achieve what you mentioned, it works well enough to just return observationConfig after the current logic, but what I meant was that ideally the method itself would be nested under MeterRegistry#observationConfig() instead of a top-level method on MeterRegistry, I think. But that doesn't feel possible. So the question is perhaps are we okay with this (awkwardly) being a top-level method on MeterRegistry that returns the ObservationConfig? Another option is to put it under Config, but then that would be weird to return ObservationConfig from there.
| * @param context context | ||
| * @return this | ||
| */ | ||
| default Observation observation(String name, Observation.Context context) { |
There was a problem hiding this comment.
Should not we leave these so that you can do registry.observation(...) or Observation.createNotStarted(...)? Meters are similar from this perspective: registry.timer(...) and Timer.start(...).
There was a problem hiding this comment.
Good question. I was mirroring the API after Sample usage since Observation like Sample is not a Meter. While there is registry.timer(...) and Timer.start(...), the former returns (and registers) a Timer while the latter doesn't register anything and returns a Sample. So I thought it would be better to have only the latter. My hope was also that it makes it more clear that it's not registering or giving you a Meter because it's not a method on MeterRegistry. What do you think?
2 participants
Keeps only one registry implementation while still providing a specific type for ObservationRegistry where Observation-specific concerns can be defined. Forces instantiation of Observation through its static methods so there is no direct instantiation by users. This provides fewer ways to do the same thing and exposes API, which should help guide users and hide implementation details.