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

Make /metrics the only Prometheus metrics endpoint #6476

Merged
merged 3 commits into from
May 29, 2024

Conversation

fstab
Copy link
Member

@fstab fstab commented May 26, 2024

Currently, the PrometheusHttpServer serves Prometheus metrics on any endpoint:

  • /metrics
  • /liveness
  • /my/pets/123
  • /
  • ...

This leads to issues (see #6071), and is uncommon in the Prometheus ecosystem.

For reference, I tested the default behavior of a couple of popular official Prometheus exporters:

The behavior of all of them is:

  • The default handler /* serves a static HTML page with some info on the exporter, and with a link to /metrics
  • The /metrics handler serves the metrics.

This is also the default behavior of the HTTPServer of the Prometheus Java client library.

This PR makes opentelemetry-java use the default behavior. Metrics are only served on /metrics. The default handler serves a static HTML page with a link to /metrics.

Note

This differs from the suggestion in #6071:

  • Support explicit endpoint for Prometheus exporter. #6071 suggests to respond with HTTP 404 on /*. However, this is not what other exporters in the Prometheus ecosystem do, so I'd just stick with HTTP 200 to follow the principle of least surprise.
  • Support explicit endpoint for Prometheus exporter. #6071 suggests to make the endpoint configurable. I don't have a strong opinion on this, and I'm happy to make it configurable. However, I don't see a good use case for configuring a non-default endpoint. I think we should only add API to configure it if there is a use case for it.

Signed-off-by: Fabian Stäber <fabian@fstab.de>
Copy link

codecov bot commented May 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.87%. Comparing base (0f99d70) to head (34b3d38).

Current head 34b3d38 differs from pull request most recent head a2f71a6

Please upload reports for the commit a2f71a6 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #6476   +/-   ##
=========================================
  Coverage     90.86%   90.87%           
  Complexity     6169     6169           
=========================================
  Files           678      678           
  Lines         18511    18506    -5     
  Branches       1818     1818           
=========================================
- Hits          16820    16817    -3     
+ Misses         1154     1153    -1     
+ Partials        537      536    -1     

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

@jkwatson
Copy link
Contributor

Since this is a breaking behavioral change, we'll need to be prepared to roll it back in a patch release if we get complaints. Or, update it to make it easily configurable. I think it is unlikely that anyone is actually scraping anything but /metrics, but breaking those people without a quick workaround would be developer/SRE-unfriendly behavior, which we'd definitely like to avoid.

I'll approve, but I think we also want @jack-berg and probably @trask on board with this breaking change before we make it official.

@@ -84,7 +83,6 @@ public static PrometheusHttpServerBuilder builder() {
.port(port)
.executorService(executor)
.registry(prometheusRegistry)
.defaultHandler(new MetricsHandler(prometheusRegistry))
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a configuration option for this to PrometheusHttpServerBuilder:

public PrometheuesHttpServerBuilder setDefaultHandler(com.sun.net.httpserver.HttpHandler defaultHandler)

This will allow users to depend on this behavior to restore the current behavior (although it requires programmatic configuration) while aligning with standard prometheus client library behavior.

Copy link
Member

Choose a reason for hiding this comment

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

I pushed a commit to add this but it broke the animalsniffer check because com.sun.net.httpserver.HttpHandler isn't available in the android API. Prometheus exporter doesn't need to support android environments, and doesn't work on them currently despite animalsniffer passing (animalsniffer apparently can't detect usage of unsupported APIs in dependencies).

I opened #6478 to separately address this. Can merge that, then merge main into this branch and merge this PR.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

🤞

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

🤘🏻

@jack-berg jack-berg merged commit 7da7037 into open-telemetry:main May 29, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants