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

Metrics SDK: implement Prometheus exporter #799

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bio-aeon
Copy link
Contributor

@bio-aeon bio-aeon commented Oct 11, 2024

Resolves #751

A number of things to pay attention to:

  1. Exporter is implemented in the separate module sdk-exporter-prometheus.
  2. This PR doesn't implement Prometheus "created timestamps" feature, while OpenTelemetry Java does through using of Prometheus client.
  3. Maybe we need to discuss, what should config option look like, which allows to add resource attributes as metric attributes. Currently this PR doesn't contain this option. Or maybe we can implement it in a separate PR, since it's not mandatory according to the spec.
  4. The spec says that monotonic Sum with delta temporality should be converted to cumulative temporality, but I don't see any OpenTelemetry SDKs doing such a conversion for Prometheus exporter, they usually just exclude everything with delta temporality, which is also done in this PR.
  5. Instead of using ScalaCheck generators, this PR uses static "expected" values, because for me it looks like exactly for these tests the complexity of making generated samples ​​isn't much less than writing the exporter itself :) Test cases are inspired by opentelemetry-rust.

@bio-aeon bio-aeon force-pushed the sdk-exporter/prometheus-exporter branch 2 times, most recently from 1a603df to d526d43 Compare October 11, 2024 01:27
@iRevive
Copy link
Contributor

iRevive commented Oct 11, 2024

I will take a look over the weekend.

@bio-aeon bio-aeon force-pushed the sdk-exporter/prometheus-exporter branch 4 times, most recently from cf10610 to f024338 Compare October 11, 2024 23:47
build.sbt Outdated Show resolved Hide resolved
@iRevive
Copy link
Contributor

iRevive commented Oct 16, 2024

Maybe we need to discuss, what should config option look like, which allows to add resource attributes as metric attributes. Currently this PR doesn't contain this option. Or maybe we can implement it in a separate PR, since it's not mandatory according to the spec.

I would keep it for the separate PR.

The spec says that monotonic Sum with delta temporality should be converted to cumulative temporality, but I don't see any OpenTelemetry SDKs doing such a conversion for Prometheus exporter, they usually just exclude everything with delta temporality, which is also done in this PR.

I think it's good enough.

@iRevive
Copy link
Contributor

iRevive commented Oct 16, 2024

Excellent work.

We can move forward once we decide what to do with the default parameters.

@lenguyenthanh
Copy link
Member

lenguyenthanh commented Oct 16, 2024

this is excellent, can't wait to try it, thanks a lot for the work!

@iRevive iRevive added sdk module Features and improvements to the sdk module sdk exporter module Features and improvements to the sdk exporter module labels Oct 16, 2024
@bio-aeon
Copy link
Contributor Author

Thanks for the review and feedback. I'll deal with comments in the next few days.

@bio-aeon bio-aeon force-pushed the sdk-exporter/prometheus-exporter branch from f024338 to 5066c4c Compare October 19, 2024 17:55
@bio-aeon
Copy link
Contributor Author

@iRevive I made changes according to the comments, and I also realized that ideally attributes for scope/target info should also go through full preparation, escaping and so on, rather than just call .toString. So, finalized this too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sdk exporter module Features and improvements to the sdk exporter module sdk module Features and improvements to the sdk module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SDK metrics: Prometheus exporter
3 participants