Skip to content

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.

@fstab fstab requested a review from a team May 26, 2024 15:07
@codecov
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.

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

5 participants