New approach to naming conventions #3255
Conversation
marcingrzejszczak
force-pushed
the
observationConvention
branch
from
465a5b6
to
336a7e9
Compare
| .collect(Collectors.toList()); | ||
| if (!observationConventions.isEmpty()) { | ||
| this.keyValuesProviders.addAll(observationConventions); | ||
| this.context.setName(observationConventions.get(0).getName()); |
There was a problem hiding this comment.
Can we do an isBlank check here? Let's say you don't want to provide a name so you return null or "".
| @Nullable Observation.KeyValuesProvider<OkHttpContext> keyValuesProvider) { | ||
| Observation.KeyValuesProvider<?> provider = null; | ||
| @Nullable Observation.ObservationConvention<OkHttpContext> customConvention) { | ||
| Observation.ObservationConvention<OkHttpContext> convention = null; | ||
| if (registry.isNoop()) { |
There was a problem hiding this comment.
I guess this if will be duplicated in a lot of Observation implementations. Could the NoopObservationRegistry return a NoopObservationConfig, which always return a NoopObservationConvention? This way you don't need the if in every implementation.
There was a problem hiding this comment.
Actually if registry is no-op we should return a no-op observation immediately - that's a good catch
There was a problem hiding this comment.
But should every instrumentation check if the registry is no-op here? The call to create later will return a no-op already if the registry is a no-op registry.
| @@ -67,7 +67,7 @@ public class OkHttpMetricsEventListener extends EventListener { | |||
|
|
|||
| private final ObservationRegistry observationRegistry; | |||
|
|
|||
| private final Observation.KeyValuesProvider<OkHttpContext> keyValuesProvider; | |||
| private final Observation.ObservationConvention<OkHttpContext> keyValuesProvider; | |||
There was a problem hiding this comment.
I would rename this field, as it's now a ObservationConvention.
| } | ||
|
|
||
| OkHttpMetricsEventListener(MeterRegistry registry, ObservationRegistry observationRegistry, | ||
| Observation.KeyValuesProvider<OkHttpContext> keyValuesProvider, String requestsMetricName, | ||
| Observation.ObservationConvention<OkHttpContext> keyValuesProvider, String requestsMetricName, |
| @@ -55,6 +55,32 @@ class SimpleObservation implements Observation { | |||
| .filter(handler -> handler.supportsContext(this.context)) | |||
| .collect(Collectors.toCollection(ArrayDeque::new)); | |||
| this.filters = registry.observationConfig().getObservationFilters(); | |||
| List<ObservationConvention<?>> observationConventions = registry.observationConfig().getObservationConventions() | |||
There was a problem hiding this comment.
When using the constructor with the ObservationConvention<?> parameter, only the given ObservationConvention is applied. In this case, ALL matching conventions are applied, but only the KeyValues and the first name.
I wonder if this is intended? Why doesn't this code use io.micrometer.observation.ObservationRegistry.ObservationConfig#getObservationConvention, which returns the first matching one? I must admit i'm a bit confused, so I can be totally wrong here.
There was a problem hiding this comment.
So I thought that the user might locally override the default behaviour by providing their own ObservationConvention. So IMO the flow could look like this (taken from OkHttpDocumentedObservation)
// If the user has explicitly provided a custom convention in the current instrumentation (e.g. `OkHttpBuilder`) then it will be picked
if (customConvention != null) {
convention = customConvention;
}
else {
// If there's nothing custom that has been locally picked, find a matching convention
// if there's no such matching convention pick the default one
convention = registry.observationConfig().getObservationConvention(okHttpContext,
new DefaultOkHttpObservationConvention(requestsMetricName));
}In this case, ALL matching conventions are applied, but only the KeyValues and the first name.
Yeah I had some ideas that you could create hierarchies of conventions but maybe let's start with simple solutions first. I will change this to pick the first matching one instead of picking all.
Also, when you explicitly provide a convention to create an observation then there's no point in searching for a matching one. When you don't provide a convention but you provide a default name then we will search for a matching convention. Does it make sense?
|
|
||
| private static String name(ObservationConvention<?> convention, Context context) { | ||
| if (!convention.supportsContext(context)) { | ||
| throw new IllegalStateException("Convention [" + convention + "] can't support context [" + context + "]"); |
with this change we're merging semantic name provider and key values provider into a single ObservationConvention construct users can register their conventions in the observationregistry configuration and then in the instrumentations ask for a registered convention (and provide a default if one is not registered)
marcingrzejszczak
force-pushed
the
observationConvention
branch
from
336a7e9
to
9953305
Compare
with this change we're merging semantic name provider and key values provider into a single ObservationConvention construct
users can register their conventions in the observationregistry configuration and then in the instrumentations ask for a registered convention (and provide a default if one is not registered)