Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenTelemetry: implement a mapping from ZIO Metrics Summary to OTEL Histogram #821

Open
grouzen opened this issue Apr 19, 2024 · 8 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@grouzen
Copy link
Contributor

grouzen commented Apr 19, 2024

This functionality wasn't implemented in #801 as it is tricky to do.

As a reference you can look at the https://github.com/micrometer-metrics/micrometer library. It does exactly this for OTEL integration.

@grouzen grouzen added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Apr 19, 2024
@IvanFinochenko
Copy link
Contributor

Hi @grouzen
What do you think can we use OpenTelemetryMeterRegistry and DistributionSummary.Builder for this issue?

@IvanFinochenko
Copy link
Contributor

I have researched some libraries and find this OpenTelemetryMeterRegistry
It has DistributionSummary, but methods for building OpenTelemetryMeterRegistry are not flexible.
I will try to create a PR with method builder taking OpenTelemetry Meter

@grouzen
Copy link
Contributor Author

grouzen commented May 3, 2024

Hi @IvanFinochenko!
Sorry, don't have enough time at the moment to check this.
Regarding OpenTelemetryMeterRegistry - I don't think we need it since we maintain our own registry for our instruments. We just need to "copy" the way they map DistributedSummary to OTEL Histogram. The problem is the parts of the mapping algorithm are scattered all over the micrometer's codebase so it is tricky to understand it as a whole.

@grouzen
Copy link
Contributor Author

grouzen commented May 3, 2024

@IvanFinochenko
We need to implement https://github.com/zio/zio-telemetry/blob/series/2.x/opentelemetry/src/main/scala/zio/telemetry/opentelemetry/metrics/internal/OtelMetricListener.scala#L40.
For this, we need to use registry.getHistogram and transform the payload of the ZIO Metrics Summary into OTEL Metrics Histogram. Hope it makes sense.

@grouzen
Copy link
Contributor Author

grouzen commented May 4, 2024

@IvanFinochenko, we could do a pairing session in Discord, for example, if you want.

@IvanFinochenko
Copy link
Contributor

@grouzen it is a good idea. I need a little more time to dive deeper under hood of micrometer and I'll write you.

@andrzejressel
Copy link
Contributor

So I also took a look at this and I found following things:

  • Summary is marked as deprecated/do not use. It is supported directly with Protobufs, but not in Java API
  • Micrometer does not use OTEL Java library - so they can just push summaries without issues.
  • Summaries are not really transferrable to histograms - the issue with summaries is that their timeline is weird - they are not just values they are precomputed values.

I personally see 2 solutions

  1. Push to Pull

Currently in zio-telemetry metrics are added directly to Java API counterparts. Instead of doing that we could utilize MetricProducer to instead pull metrics from inside ZIO-Metrics.

  1. Deprecate summary in ZIO

From my understanding OTEL is like a consorcium of companies that does telemetry stuff. If they decided that summary metric is deprecated, should we still have it?

@grouzen
Copy link
Contributor Author

grouzen commented Jul 30, 2024

@andrzejressel Hey! I want to clarify this one. From your reply, I understood you are talking about adding support for OTEL Summary, which is legacy. This particular issue is about mapping ZIO metrics Summary to OTEL Histogram because OTEL Summary is deprecated, as stated above.
That's precisely what the micrometer library does internally. It also avoids dealing with OTEL Summary and uses quite complex logic to map summaries to histograms for the OTEL case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants