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

Add prometheus exporter #619

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

albertored
Copy link
Contributor

@albertored albertored commented Aug 29, 2023

First draft of the Prometheus exporter to collect some feedback.

Open points:

  1. At the moment it is embedded in the opentelemetry_experimental app to speed up development, should it be a separate app?

  2. As suggested in the spec I used a reader and an exporter but from the user point of view the exporter is masked and its config parameters are on the reader, for example:

    config :opentelemetry_experimental,
      readers: [
        %{
          module: :otel_metric_reader_prometheus,
          config: %{
            add_scope_info: true,
            add_total_suffix: true,
            endpoint_port: 3000
          }
        }
      ]

    default_temporality_mapping, export_interval_ms and exporter options on the reader are overridden, should we emit a warning if the user configures them?

  3. The encoding part works but it can be improved for sure, I still need to go through that module again and refactor it. Anyway, any suggestion is welcomed.

  4. The otel_metric_reader_prometheus is in reality a supervisor that starts the reader itself and the httpd server if needed. The httpd server is started only if an endpoint_port is given to the configuration. We can maybe have a more explicit configuration field for this?

  5. If the httpd server is not used it should be possible for the user to get the metrics in prometheus format so that he can expose them. This can be done by calling the otel_metric_reader_prometheus:collect/1 function but it needs the reader pid as an argument, not so user-friendly.

  6. How to implement the same behavior of erlang:float_to_binary(2.0, [short]) that was added in OTP 25? (tests failing for this reason)

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.98%. Comparing base (8f01a9a) to head (af3826b).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #619      +/-   ##
==========================================
- Coverage   74.48%   67.98%   -6.51%     
==========================================
  Files          56       61       +5     
  Lines        1862     2152     +290     
==========================================
+ Hits         1387     1463      +76     
- Misses        475      689     +214     
Flag Coverage Δ
api 58.84% <ø> (-14.67%) ⬇️
elixir 17.32% <ø> (?)
erlang 74.35% <ø> (-0.14%) ⬇️
exporter 67.47% <ø> (ø)
sdk 78.69% <ø> (-0.01%) ⬇️
zipkin 54.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eproxus
Copy link

eproxus commented Oct 4, 2023

I'm very interested in use case 5, since we already have our own web server and do not want to export an additional port.

@albertored
Copy link
Contributor Author

I'm very interested in use case 5, since we already have our own web server and do not want to export an additional port.

Yea, I think this will be the most common use case, we need to find a way to make this easy for users

@starbelly
Copy link

I am also interested in use case 5. Will evaluate switching to otel collector but for now this would go in nicely on my current project 😄

@tsloughter
Copy link
Member

@albertored are you able to update this PR? I'd like to get prometheus support in so we can get the technical committee to sign off on our metrics implementation so we can go GA.

@albertored
Copy link
Contributor Author

@tsloughter I'll try this weekend hoping things have not changed too much. Unfortunately in the last months I was not able to follow the project with constancy

@RoadRunnr
Copy link
Contributor

@albertored I've rebased and modified your version a bit main...travelping:opentelemetry-erlang:feature/prometheus-exporter-for-upstream

The reader logic can now be used from a cowboy handler as well (sample included).

@albertored
Copy link
Contributor Author

@tsloughter rebased and pushed with additions from @RoadRunnr, we should still resolve the points in my first comment before merging

@albertored
Copy link
Contributor Author

PS: this commit is causing failures in the pipeline since maps:iterator/2 was introduced in erlang 26. I don't know if there is an efficient way of doing it without that function (I can only think about converting map to list and order it before folding). If not we should revert that commit

@RoadRunnr
Copy link
Contributor

PS: this commit is causing failures in the pipeline since maps:iterator/2 was introduced in erlang 26. I don't know if there is an efficient way of doing it without that function (I can only think about converting map to list and order it before folding). If not we should revert that commit

The ordering is only used for the tests to get defined ordering for comparing the results. It is therefore not to critical that they are super efficient. The simple sort of the map keys would be enough. I can supply a patch if you want?

we should still resolve the points in my first comment before merging

point 5 (or use case 5) should be solved. A sample cowboy handler is included.

The encoding part works but it can be improved for sure.

I do have additional changes pending in travelping/opentelemetry-erlang@bbcfe2f...0d5f677

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants