Make per-meter OTLP configuration more flexible #6102
Conversation
shakuzen
force-pushed
the
mo-flavor-mo-problems
branch
5 times, most recently
from
0e8c3ae to
a1f1416
Compare
...entations/micrometer-registry-otlp/src/main/java/io/micrometer/registry/otlp/OtlpConfig.java
Outdated
Show resolved
Hide resolved
Allow users to configure in more ways than an exact match on the meter name.
shakuzen
force-pushed
the
mo-flavor-mo-problems
branch
from
a1f1416 to
2f2eadc
Compare
| * @see #histogramFlavor() | ||
| */ | ||
| default HistogramFlavor histogramFlavorPerMeter(Meter.Id id) { | ||
| return histogramFlavorPerMeter().getOrDefault(id.getName(), histogramFlavor()); |
There was a problem hiding this comment.
As mentioned offline, I think we should make a type for the function that is this method's signature and make it configurable on the registry Builder rather than in the OtlpConfig. The reason is to keep logic in the registry and config in the config interface - this feels a bit too much like logic for how the config is used. Probably it would be most usable for most people for the default implementation to copy the longest starts-with match wins behavior they have in Spring Boot for some similar types of configuration properties. If that were the case, I suspect the vast majority of users would not want to customize the logic anyway, so the fact it needs to be done via the builder is not much of an issue. Thoughts?
| * @see OtlpConfig#histogramFlavorPerMeter() | ||
| * @see OtlpConfig#histogramFlavor() | ||
| */ | ||
| HistogramFlavor getHistogramFlavor(OtlpConfig config, Meter.Id id); |
There was a problem hiding this comment.
Should we make this and MaxBucketsPerMeterLookup#getMaxBuckets @Nullable?
In the current case the lookup should provide a fallback, if we make it nullable we can do that for the user.
There was a problem hiding this comment.
I think allowing nulls and falling back to the default value makes sense.
There was a problem hiding this comment.
I've updated the signature along with incorporating this change such that the mapping function should not deal with the default value other than to return null. This feels cleaner than the expectation of implementations calling specific methods on OtlpConfig
|
Final change: remove the API to customize the lookup logic and just change the logic so it is more usable, which solves the original ask without adding more API that doesn't seem necessary (yet). |
|
Thanks to @lenin-jaganathan for the offline review of these changes. |
The previous logic for matching required the exact meter name, which was straightforward but cumbersome for configuring groups of meters. This changes the default matching for the per-meter config to use the longest dot-separated segments match.
Resolves #6099