OTel Metrics Micrometer MeterProvider #71
Conversation
HaloFour
force-pushed
the
micrometer-metricsprovider
branch
from
229de3c to
ad3fd2f
Compare
HaloFour
force-pushed
the
micrometer-metricsprovider
branch
from
ad3fd2f to
f7ef8c9
Compare
There was a problem hiding this comment.
Two things come to mind looking at this:
- It looks like the async instruments from OTEL may need more machinery to work correctly here.
- What's the cost of registering counters + things w/ labels in micrometer? The current mechanism to allow Baggage labels in metric attributes may come with large performance costs.
| @Override | ||
| public BoundDoubleCounter bind(Attributes attributes) { | ||
| Tags tags = TagUtils.attributesToTags(attributes); | ||
| Counter counter = factory.get().tags(tags).register(state.meterRegistry()); |
There was a problem hiding this comment.
How expensive is it for OTEL's API to constantly register counters in the hot path?
There was a problem hiding this comment.
Internally Micrometer keeps a map of every instrumented by Meter.Id which is comprised of the name and the tags. We could add a separate map here as I am doing with gauges. But without Attributes implementing hashcode/equals it seems that I can't avoid conversion to Tags prior to looking up the instrument from the map which I presume is probably the most expensive part of this operation.
There was a problem hiding this comment.
the Attributes better have a well-behaved equals/hashcode or things will be very bad.
There was a problem hiding this comment.
ArrayBackedAttributes looks like it does, but AttibutesMap does not. It seems the latter is only used by SpanBuilder so perhaps it's safe to assume that any attributes passed to these APIs can be used as a key.
There was a problem hiding this comment.
AttributesMap is a HashMap so it should be fine.
There was a problem hiding this comment.
but yes, these APIs won't get getting the attributes from the internals of the span, I don't think.
| } | ||
|
|
||
| @Override | ||
| public void buildWithCallback(Consumer<ObservableDoubleMeasurement> callback) { |
There was a problem hiding this comment.
This implementation means the async measurements are recorded ONCE and then never again.
There was a problem hiding this comment.
Thanks for the heads up, that's my misunderstanding of the OTel API. To confirm, callback is then called once per export?
There was a problem hiding this comment.
Yeah. The idea is that anytime you export, this callback should be called and you synthesize your metrics from it right then.
There was a problem hiding this comment.
Hm, ok, this might create an interesting chicken/egg scenario. Each invocation of that callback could potentially observe metrics for a different set of attributes/tags that might correspond to instruments that would have to be created in Micrometer.
I see, that was my misunderstanding. I assumed that the callback would hold onto the observer and would continually publish results.
Every permutation of tags is a separate instrument. As of today Micrometer has no concept of exemplars. |
Baggage != exemplars. baggage is the ability to attach labels (dynamically) via distributed context propagation. |
Right, in that case we can just treat them like |
|
Just curious, is there a reason to have an SDK that implements the OTel metrics API using micrometer, instead of the other way around, a micrometer registry that writes to the OTel metrics API? Is it to allow OTel users to have access to all of micrometer registry implementations? If the collector can fill that gap, the reverse may be more valuable by opening up cool features like exemplars and baggage to micrometer users (at least I think it'd be possible). Of course it could be good to have both, just wondering. |
My use case is that we already have a series of Spring apps that make fairly extensive use of Micrometer and they report to a custom metrics exporter that only has Micrometer bindings. Having a binding from OTel to Micrometer would make it easier to adopt components that make use of OTel metrics (e.g. |
|
Hi @HaloFour - I think this was implemented by @mateuszrzeszutek now Should we go ahead and close this PR? |
This PR seems to implement the reverse bridge: OTel -> Micrometer. Anyway -- is this still valid/needed? |
|
Personally, anyone using Micrometer today is more likely to want to stick with the Micrometer API, and would be more interested in a OTeL Registry for Micrometer as an alternative output mechanism. What are your thoughts @jonatan-ivanov? |
|
I believe that the bridge is still needed as there will be projects that need to make use of both facades and Micrometer is already more established, especially in Spring Boot applications. But I think that this PR draft specifically took a naive approach which won't work for a proper bridge, so I'd be fine closing this PR as long as an issue remains open to explore this space. |
|
@kenfinnigan Since this is bridging the OTel Metrics API to the Micrometer API, I guess this would be useful for those users who want to use the OTel Metrics API but also want to use a backend that is not supported by OTel but supported by Micrometer. The OTel registry/OTLP support in Micrometer I guess is the other way: people want to use the Micrometer API but they also want to use OTLP (fyi: we are planning to add OTLP support in our next feature release). |
|
Sorry I had forgotten that this is the reverse direction. Though I tend to agree with @kenfinnigan that there probably wouldn't be that much usage of this, and it is a fairly complex beast. If someone wants to continue with this PR we could still consider it but maybe it's ok to go ahead and close for now.
@jonatan-ivanov @shakuzen Just checking whether this is still needed given we have the OTel API MeterRegistry now? I guess the use cases are the same, but the OTel API version will have more features (e.g., integrate with other OTel instrumentation, especially tracing to get exemplars) and don't know of any drawback though there may be. |
|
Already implemented via #328. |
First pass at implementing a
MeterProviderthat wraps MicrometerMeterRegistry.