-
Notifications
You must be signed in to change notification settings - Fork 650
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 args to serve application behind a route prefix/external URL #1335
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: kwilt <[email protected]>
Supporting exporter-toolkit pull request: prometheus/exporter-toolkit#283 |
we need a new version from the https://github.com/prometheus/exporter-toolkit before we can get this merged |
Signed-off-by: Sebastian Schubert <[email protected]>
I have released an update to the exporter-toolkit. (#1360) |
Needs some |
Signed-off-by: Richard Hartmann <[email protected]>
pprof profile - endpoint
|
The pprof endpoints are not configurable easily. We would have to custom register our own handlers. We basically need to completely re-work how we handle the web endpoint for exporters to handle this better. Really, I don't think we should be doing this at all. This is too much extra boilerplate and makes our exporters inconsistent. The fact that this has been hand-implemented by the blackbox exporter, and this PR is a mess of additional code, shows that we should not be doing this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is far too much boilerplate as-is. I will not merge this until it is simpler for exporter maintainers to add universally across the ecosystem.
Signed-off-by: kwilt <[email protected]>
I have attempted to do this in this pull request prometheus/exporter-toolkit#293 |
f3db7b9
to
0d9c3b4
Compare
@@ -50,6 +51,8 @@ var ( | |||
concurrency = kingpin.Flag("snmp.module-concurrency", "The number of modules to fetch concurrently per scrape").Default("1").Int() | |||
debugSNMP = kingpin.Flag("snmp.debug-packets", "Include a full debug trace of SNMP packet traffics.").Default("false").Bool() | |||
expandEnvVars = kingpin.Flag("config.expand-environment-variables", "Expand environment variables to source secrets").Default("false").Bool() | |||
externalURL = kingpin.Flag("web.external-url", "The URL under which snmp exporter is externally reachable (for example, if snmp exporter is served via a reverse proxy). Used for generating relative and absolute links back to snmp exporter itself. If the URL has a path portion, it will be used to prefix all HTTP endpoints served by snmp exporter. If omitted, relevant URL components will be derived automatically.").PlaceHolder("<url>").String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these flags be part of github.com/prometheus/exporter-toolkit/web/kingpinflag
?
This is a duplicate of #1328
Sorry for the duplicate. Didn't realize deleting my branch would auto-close the pull request. Anyways... 🤦♂️
Add feature to serve application behind a route prefix or external URL by supplying args --web.external-url/--web.route-prefix, in the same way as blackbox_exporter or other exporters.
I am also submitting a pull request for exporter-toolkit so that this will actually work. Currently you cannot serve the landing page on anything other than root ("/"), so the tests are going to fail initially.
I would consider these additional args to be "must-have" features to bring snmp_exporter closer to feature parity with the other exporters. I needed to serve this exporter behind a prefix like I'm doing with the other exporters, so I figured I would make an attempt at this update myself, as it does appear to be something other people have been wanting for quite some time. See: #648