opentelemetry-erlang icon indicating copy to clipboard operation
opentelemetry-erlang copied to clipboard

Add prometheus exporter

Open albertored opened this issue 1 year ago • 10 comments

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)

albertored avatar Aug 29 '23 16:08 albertored

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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%) :arrow_down:
elixir 17.32% <ø> (?)
erlang 74.35% <ø> (-0.14%) :arrow_down:
exporter 67.47% <ø> (ø)
sdk 78.69% <ø> (-0.01%) :arrow_down:
zipkin 54.16% <ø> (ø)

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Aug 29 '23 16:08 codecov[bot]

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.

eproxus avatar Oct 04 '23 06:10 eproxus

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

albertored avatar Oct 04 '23 07:10 albertored

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 😄

starbelly avatar Oct 05 '23 23:10 starbelly

@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.

tsloughter avatar Feb 26 '24 11:02 tsloughter

@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

albertored avatar Feb 28 '24 09:02 albertored

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

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

RoadRunnr avatar Mar 13 '24 15:03 RoadRunnr

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

albertored avatar Mar 25 '24 08:03 albertored

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

albertored avatar Mar 25 '24 08:03 albertored

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 https://github.com/travelping/opentelemetry-erlang/compare/bbcfe2f6370c89fcd8c7ed5981fd8c171aeff4d0...0d5f677ccc64a069274689aeeee70663acb41eed

RoadRunnr avatar Mar 25 '24 09:03 RoadRunnr