main: start the exporter through the toolkit bootstrap package - #1368
main: start the exporter through the toolkit bootstrap package#1368nicolastakashi wants to merge 1 commit into
Conversation
f22182d to
fd8421c
Compare
dd6b2ca to
2e9ea79
Compare
The entrypoint hand-rolls what exporter-toolkit/bootstrap now provides: the web.telemetry-path / web.listen-address flags, promslog flag wiring, version output, the landing page, and the ListenAndServe call. Keeping a private copy is how exporters drift apart in flags and startup behavior. Move exporter construction into a metrics handler factory, which runs after flags are parsed, and register the multi-target /probe endpoint and the net/http/pprof and expvar (/debug/) handlers as bootstrap routes so they keep working on the mux bootstrap owns. Two flags come with bootstrap and are wired to real behavior here: --web.max-requests limits in-flight scrapes, and --web.disable-exporter-metrics drops the Go and process collectors. exporter-toolkit is bumped to v0.19.0, which ships the route-registration support this depends on (prometheus/exporter-toolkit#430). PG_EXPORTER_WEB_TELEMETRY_PATH keeps working, but bootstrap dropped env-var support for --web.telemetry-path, so instead of relocating the metrics endpoint the handler is additionally served at the env var's path (with a deprecation warning) for a compatibility window. Fatal startup errors are logged through the configured logger once one exists, falling back to stderr only for pre-parse failures. --dumpmaps and runtime shutdown are handled from main rather than exiting from inside the bootstrap callback. Signed-off-by: Nicolas Takashi <nicolas.tcs@hotmail.com>
c6c8dab to
37fa135
Compare
ArthurSens
left a comment
ArthurSens
left a comment
There was a problem hiding this comment.
I think this is also a great opportunity to add a big warning in the changelog about the deprecation of env vars!
| if !b.DisableExporterMetrics { | ||
| registry.MustRegister( | ||
| prometheuscollectors.NewGoCollector(), | ||
| prometheuscollectors.NewProcessCollector(prometheuscollectors.ProcessCollectorOpts{}), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Hmmm, I'm just seeing this now. It looks weird that Bootstrap owns the self-metrics toggle but has absolutely no control over it. I wonder if this even is something that the exporter toolkit can control 🤔
If we compare what this boolean does here with what it does in node exporter, in node exporter we also have HTTP metrics since it uses promhttp.InstrumentMetricsHander
Version collector is also about the exporter itself and is not gated by the boolean
There was a problem hiding this comment.
Yup good catch, I moved version collector under the flag for now. Longer term I think this belongs in bootstrap itself, it owns the registry and gates self-metrics directly instead of just handing exporters a bool, happy to open that as a follow-up on exporter-toolkit if you're up for it wdyt?
There was a problem hiding this comment.
I don't know if that's possible though. If it is, then great...let's do it
| } | ||
| http.Handle("/", landingPage) | ||
| } | ||
| b.HandleFunc("/probe", handleProbe(logger, authHandler, cfg)) |
There was a problem hiding this comment.
--web.max-requests works for /metrics but doesn't wrap /probe
There was a problem hiding this comment.
Yeah, makes sense, maybe we should have exporter toolkit enforce web.max-requests on every route, wdyt? if you agree I'd make it on the exporter instead of rebuilding it here.
There was a problem hiding this comment.
I think for now we can just do it here. I'm not sure if it's safe to do that, we can make this move later :)
| // net/http/pprof and the transitively-imported "expvar" package both | ||
| // register handlers under /debug/ on the default mux. |
There was a problem hiding this comment.
this comment seems unnecessary
3 participants
Summary
Starts
postgres_exporteron the shared exporter startup path by replacing the hand-rolledmain()block withexporter-toolkit/bootstrap, mirroring prometheus/node_exporter#3660.The entrypoint currently reimplements what
bootstrapprovides: the--web.telemetry-pathflag,kingpinflag/promslog wiring, version output, landing page construction, and theweb.ListenAndServecall. Keeping a private copy is how exporters drift apart in flags and startup behavior.Two notes for review:
--dumpmapsexits from inside the handler factory, since it is an early-exit path andbootstraphas no pre-serve hookCloseServers()now runs afterRun()returns instead of viadefer; the olddefernever actually ran, because every exit path went throughos.Exit