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

Expose prometheus metrics strictly on '/metrics'-endpoint #907

Conversation

Philippus
Copy link
Contributor

@Philippus Philippus commented Nov 24, 2020

Applying this PR will make sure the prometheus metrics are exposed strictly on the '/metrics'-endpoint. Prometheus only scrapes that endpoint so it makes sense to restrict it.
We had a situation where, through a temporary misconfiguration, a request for a css-file was sent to the metrics server on port 9095. Since the metrics are sent for all requests and sent with a 200 OK status code, this invalid css-file ends up being served and being cached in the browser, leading to serious issues. It's better to send a 404-error when something else than metrics is requested.

@SimunKaracic
Copy link
Contributor

Hey, I'll give this a look soon, but until then you can rebase on top of master, there's a fix for github actions there!

@Philippus Philippus force-pushed the expose-metrics-only-on-metrics-endpoint branch from da9e5eb to ddc9ebd Compare November 24, 2020 12:15
@SimunKaracic
Copy link
Contributor

This is a breaking change, so we'll need to go about this a bit more carefully.

Let's add a setting for the endpoint, which has a default of *.
This helps in two ways:

  1. Default behaviour is identical to the current one
  2. You can now set it to whatever you want, including "metrics"

How does that sound?

@Philippus
Copy link
Contributor Author

There's already the kamon.prometheus.embedded-server.impl-setting, this is what we're using right now as a workaround with the changes to the server implemented as StrictEmbeddedHttpServer. So maybe it's overkill to offer another setting. We could also add the strict version as 'sun_strict' and deprecate the existing one.

@SimunKaracic
Copy link
Contributor

SimunKaracic commented Dec 4, 2020

I've studied the Prometheus documentation a bit more, and it looks like we can just merge this as is.

I'll ponder it some more over the weekend, but I expect to just push the big green button on Monday

@SimunKaracic SimunKaracic merged commit f7ae498 into kamon-io:master Dec 8, 2020
@SimunKaracic
Copy link
Contributor

Thanks for the contrib! 🎉

@Philippus Philippus deleted the expose-metrics-only-on-metrics-endpoint branch December 8, 2020 11:40
@Scarysize
Copy link

Scarysize commented Feb 5, 2021

Just to notify you: This change broke stuff. I'm wondering why this was merged. Is there something we missed on the prometheus side of things that could have prevent the breakage?

We got a Nginx server running in front of the Kamon HTTP endpoint. The prometheus scrape job is set up to scrape "/service/metrics" on the instance, the Nginx server does a proxy pass to the local Kamon endpoint at /. After the update the prometheus metrics weren't available at / anymore.

@SimunKaracic
Copy link
Contributor

Ah, there's nothing you could have done :(

I should have anticipated this, my bad.

By default, prometheus goes looking for the /metrics endpoint, so I expected it to just work ™.
We narrowed it down to only listening on that endpoint, because the old behavior of just accepting all requests to that port was not exactly the optimal solution.
I'll be more careful in the future 👼

Did you fix the issue? You should be able to just change proxy pass to /metrics

@Scarysize
Copy link

Yes, we fixed this by changing the proxy pass to /metrics. But this only worked well because older kamon versions also respond on that path ;)

@SimunKaracic
Copy link
Contributor

So you're telling me that we broke backward compatibility, but older versions were forward compatible? :D

@Scarysize
Copy link

Yes, it would have been a big pain to proxy pass to a different path based on the kamon version a specific service is using. Now the services using the previous kamon version can be scraped like those using the new version.

Maybe it would be helpful to at least update the release notes to reflect the breaking change more prominently?

@tcoville-coveo
Copy link

Same here, this is blocking us from upgrading at the moment. Any chance to have a configurable option in the future ?

@SimunKaracic
Copy link
Contributor

Yup, we'll push that out asap

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.

4 participants