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 port option to exporter middleware #199

Merged
merged 1 commit into from
Feb 23, 2021

Conversation

tragiclifestories
Copy link
Contributor

If the port option is set, all requests for /metrics on other ports will be forwarded to the app. If it is unset or nil, or the ports match, export things as usual.

Allows separate mounting of metrics and main app to enforce different security setups etc.

@coveralls
Copy link

coveralls commented Jul 30, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 42860ce on gocardless:exporter-port into 8b20201 on prometheus:master.

@dmagliola
Copy link
Collaborator

I've been thinking about this again, and while my posture has always been that we shouldn't permit much configurability of these middlewares, encouraging implementers to use these as a template and make their own version instead...

I have realized that in that "policy" I've been conflating both middlewares unfairly.
I still think that we shouldn't allow much configurability of the the Collector middleware, as it gets messy quite quickly, and the changes will be quite specific to each user of the client.

However, I no longer think this should apply to the Exporter. Or at least it doesn't need to as much.
This change is a great example of adding quite useful functionality, that probably many users will need, without really adding much complexity. I think we should embrace these.

I'm going to go through other closed PRs to see if this new thinking applied to any of them, and I closed them when I maybe shouldn't have.

I'm sorry @tragiclifestories that we took forever to respond to this. This sort of landed in the middle of a wider conversation we've been having for a while, and it got bundled with a whole category of changes that I now realize it doesn't really belong with. My bad!

If you rebase this to add the "signoff", I'd be happy to merge this.
Not sure if @Sinjo has a different opinion.

@tragiclifestories
Copy link
Contributor Author

Fuck, I'd completely forgotten about this. Will do in the new year. Merry Xmas!

If the port option is set, all requests for /metrics on other ports will
be forwarded to the app. If it is unset or nil, or the ports match,
export things as usual.

Allows separate mounting of metrics and main app to enforce different
security setups etc.

Signed-off-by: James Turley <jamesturley@gocardless.com>
@tragiclifestories
Copy link
Contributor Author

Had 5 spare minutes and a hangover, so done this now.

Copy link
Member

@Sinjo Sinjo left a comment

Choose a reason for hiding this comment

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

+1 on this. Let's get it in and add it to the docs in a subsequent PR.

@Sinjo Sinjo merged commit 1c8e407 into prometheus:master Feb 23, 2021
@Sinjo Sinjo deleted the exporter-port branch February 23, 2021 17:48
Envek added a commit to yabeda-rb/yabeda-prometheus that referenced this pull request Nov 8, 2022
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