register pprof endpoints for allocator #1408
Conversation
| @@ -78,6 +79,7 @@ func NewServer(log logr.Logger, allocator allocation.Allocator, discoveryManager | |||
| router.GET("/jobs", s.JobHandler) | |||
| router.GET("/jobs/:job_id/targets", s.TargetsHandler) | |||
| router.GET("/metrics", gin.WrapH(promhttp.Handler())) | |||
| registerPprof(router.Group("/debug/pprof/")) | |||
There was a problem hiding this comment.
Would it make sense to design the registration in a way that the end point can be switched on and off?
There was a problem hiding this comment.
is there any reason for it? E.g. perf penalty?
There was a problem hiding this comment.
As far as I'm aware, just the import has no impact on performance.
When I do see flags for controlling it, it's usually because it's run on a separate port, as the main handler is exposed to untrusted clients or has auth requirements.
The allocator runs with trusted clients, metrics are on the same listener, leading me to the conclusion that there's no need to hide it behind a flag.
|
Thanks for adding this in :) Could you fill out a changelog for this too please? |
|
i've added one |
.chloggen/allocator-pprof.yaml
Outdated
| component: target allocator | ||
|
|
||
| # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
| note: register pprof endpoints under /debug/pprof |
There was a problem hiding this comment.
Could you please start the sentence with a capital R ?
* register pprof endpoints for allocator * changelog entry * full sentence for changelog
These are generally useful for tracking down performance issues.
Allocator clients are generally trusted, these shouldn't need to be on a separate endpoint.