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

Merged
merged 1 commit into from
Oct 23, 2024

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 module:sdk Features and improvements to the sdk module module:sdk:exporter 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.

Copy link
Contributor

@iRevive iRevive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@bio-aeon bio-aeon force-pushed the sdk-exporter/prometheus-exporter branch from 5066c4c to 4a3cd95 Compare October 22, 2024 19:07
@bio-aeon bio-aeon force-pushed the sdk-exporter/prometheus-exporter branch from 4a3cd95 to 554f43d Compare October 22, 2024 19:16
@iRevive iRevive merged commit f6d921d into typelevel:main Oct 23, 2024
8 of 10 checks passed
@lenguyenthanh
Copy link
Member

@iRevive could we have a release (snapshot is okay) for this? I'd like to try.

@iRevive
Copy link
Contributor

iRevive commented Oct 23, 2024

@lenguyenthanh definitely. The snapshot should be available today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:sdk:exporter Features and improvements to the sdk exporter module module:sdk 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