OTLP metrics sender interface #5691
Conversation
shakuzen
force-pushed
the
feat/otlp-metric-sender
branch
from
327e017 to
ef01385
Compare
shakuzen
changed the title
Keeps the API more generic without a dependency on API from the opentelemetry-proto-java project. Also adds a Builder to avoid the need for more public constructors that may change in the future.
There was a problem hiding this comment.
Thank you for the pull request @kasparkivistik. I've modified things a bit - I removed the API dependency on opentelemetry-proto-java by taking a byte array instead of ExportMetricsServiceRequest in the OtlpMetricsSender. I've also added an argument for headers. I opted for adding a Builder rather than increase the number of public constructors. Otherwise, things are mostly the same as you initially proposed. Do you think this will work for your purposes and other people's needs on this? I think we may try to get this into our milestone release today to start getting feedback on it, with the caveat that the API may still change or be reverted entirely before GA.
|
|
||
| // intentionally not public while we incubate this concept | ||
| // if we want to use this in other registries, it should move to micrometer-core and become public API | ||
| interface MetricsSender { |
There was a problem hiding this comment.
This is just to demonstrate what this might look like if we try to expand its scope outside of the OTLP registry.
Hi! Thanks for revising the PR. The proposed change looks solid, I can try it out :) |
| this.registry = new OtlpMeterRegistry(otlpConfig(), this.clock, | ||
| new NamedThreadFactory("otlp-metrics-publisher"), this.mockHttpSender); | ||
| OtlpMetricsSender metricsSender = new OtlpHttpMetricsSender(mockHttpSender, config); | ||
| this.registry = OtlpMeterRegistry.builder(config).clock(clock).metricsSender(metricsSender).build(); |
There was a problem hiding this comment.
One thing I worry about that is demonstrated here is how it's possible to pass a different instance of OtlpConfig to the OtlpHttpMetricsSender and the OtlpMeterRegistry (or Builder). It would be better if such a misconfiguration weren't possible, but we can think on how to improve this in the next milestone rather than block merging this for early feedback, I think.
| ----- | ||
|
|
||
| You can also provide a custom implementation of `OtlpMetricsSender` that does not use HTTP at all. | ||
| For instance, if you made a gRPC implementation, you could configure it in the following way. |
There was a problem hiding this comment.
I'm not sure what the right wording would be but from this it sounds like to me that gRPC does not use HTTP (it does use HTTP/2).
| * @param metricsData encoded batch of metrics | ||
| * @param headers metadata to send as headers with the metrics data | ||
| */ | ||
| void send(byte[] metricsData, Map<String, String> headers); |
There was a problem hiding this comment.
I think encoding the data should be the responsibility of the sender, e.g.: users should be able to provide a sender that uses json and another one that use protobuf. But we can discuss it and change it in the next milestone if this is valid.
There was a problem hiding this comment.
Encoding and sending are separate concerns. The existing HttpSender does not handle encoding, and encoding is specific to the backend/format, whereas a sender is re-usable across registries as is. I had a version of changes at one point that introduced encoding generically in PushMeterRegistry, but it felt like too much abstraction and too wide scope for the purpose of achieving this in the OTLP registry. We could add it just for the OTLP registry, but I was aiming to make the least amount of necessary change to achieve the actual use cases we have feedback requesting, which I didn't see any asking for e.g. JSON support. Keeping things as implementation details makes it easier for us to support it as well.
closes #5690