feat(controller): enable pprof profiling support #3769
Conversation
Signed-off-by: John Wood <[email protected]>
cmd/rollouts-controller/main.go
Outdated
| @@ -283,6 +291,7 @@ func newCommand() *cobra.Command { | |||
| command.Flags().IntVar(&klogLevel, "kloglevel", 0, "Set the klog logging level") | |||
| command.Flags().IntVar(&metricsPort, "metricsport", controller.DefaultMetricsPort, "Set the port the metrics endpoint should be exposed over") | |||
| command.Flags().IntVar(&healthzPort, "healthzPort", controller.DefaultHealthzPort, "Set the port the healthz endpoint should be exposed over") | |||
| command.Flags().IntVar(&pprofPort, "pprof-port", controller.DefaultPProfPort, "Set the port the pprof endpoint should be exposed over") | |||
There was a problem hiding this comment.
In my mind as a user, I would like to be able to overwrite the port in case I'm using the default debug port 6060 elsewhere. I will defer on a maintainer's judgement for what pattern makes the most sense in the project given this is creating two flags for one feature.
There was a problem hiding this comment.
I am kind of a fan of passing in a single flag of such that we can do 0.0.0.0:6060, :6060 or localhost:6062 . The main reason for this is not all pods can you exec into etc and it might be nice to be a be able to remotely pull pprof files if configured to do so.
There was a problem hiding this comment.
In terms of wording, a boolean is much easier to convey --enable-pprof=true vs. --pprof-port being the boolean toggle and the config for the address. Do you have any ideas on how we can convey that?
I see two options with tradeoffs:
- Having
--pprof-portresolve to an address doesn't make it clear that this is disabled by default (maybe the help text is what we need to clarify here). - Setting
--enable-pprofequal to an address instead of a boolean feels strange to me. Maybe something like--enable-pprof-server-addressis a (verbose) happy medium?
There was a problem hiding this comment.
Yea, thats hard. We could stick with the status quo with the code base and be a little less secure which uses 0.0.0.0 by default and then just have the enable and port flag.
I don't like the above due to the security concerns around the data available on the pprof endpoints. So with that said, I think let's go verbos with --enable-pprof-server-address or --enable-pprof-address I think I am impartial on weather server is there or not, thoughts?
There was a problem hiding this comment.
I'm fine with both. I think the latter is more simple so I will probably go with that. Give me a moment and I can put in some changes.
There was a problem hiding this comment.
I pushed up a commit to consolidate the two config vars. My remaining question is what you want to do about the default value? We obviously don't want to enable profiling for every user so I don't know know a way around "" as the default unless you have suggestions?
There was a problem hiding this comment.
What you did for default is perfect, this LGTM thanks for adding this.
Signed-off-by: John Wood <[email protected]>
johnmwood
force-pushed
the
add-profiling
branch
from
672f43a
to
0c8dc15
Compare
Published E2E Test Results 4 files 4 suites 3h 29m 17s ⏱️ For more details on these failures, see this check. Results for commit 691ab58. ♻️ This comment has been updated with latest results. |
Published Unit Test Results2 257 tests 2 257 ✅ 2m 58s ⏱️ Results for commit 691ab58. ♻️ This comment has been updated with latest results. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3769 +/- ##
==========================================
+ Coverage 78.19% 83.95% +5.75%
==========================================
Files 157 162 +5
Lines 22507 18508 -3999
==========================================
- Hits 17599 15538 -2061
+ Misses 3995 2107 -1888
+ Partials 913 863 -50 ☔ View full report in Codecov by Sentry. |
Signed-off-by: John Wood <[email protected]>
johnmwood
force-pushed
the
add-profiling
branch
from
2583b63
to
691ab58
Compare
|
This PR enables pprof profiling on the Argo Rollouts controller via the
--enable-pprofflag.Issue: #3757
Checklist:
"fix(controller): Updates such and such. Fixes #1234".