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

Adding Pprof register method #1885

Merged
merged 3 commits into from
Apr 5, 2023
Merged

Adding Pprof register method #1885

merged 3 commits into from
Apr 5, 2023

Conversation

ramiro-gamarra
Copy link
Contributor

Reason for Change:

This PR enables pprof endpoints to be registered on the multi tenancy path.

Issue Fixed:

Requirements:

Notes:

@ramiro-gamarra ramiro-gamarra requested a review from a team as a code owner April 3, 2023 17:55
@ramiro-gamarra ramiro-gamarra requested review from thatmattlong and removed request for a team April 3, 2023 17:55
timraymond
timraymond previously approved these changes Apr 3, 2023
Copy link
Member

@timraymond timraymond left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me

@rbtr
Copy link
Contributor

rbtr commented Apr 3, 2023

is it feasible/should we make this dynamically registerable? if we're trying to catch a memory leak, it might be nice to be able to turn the pprof handlers on, take the heap dump, and turn them back off without restarting the process.

@ramiro-gamarra
Copy link
Contributor Author

ramiro-gamarra commented Apr 3, 2023

is it feasible/should we make this dynamically registerable? if we're trying to catch a memory leak, it might be nice to be able to turn the pprof handlers on, take the heap dump, and turn them back off without restarting the process.

I believe we can still attach routes to the mux even after the server has started listening. Are you proposing:

  1. client calls a CNS endpoint to enable pprof handlers, client calls pprof endpoint, client calls CNS endpoint to turn off pprof handlers.
  2. client calls a CNS endpoint to collect pprof data, where CNS can internally use the pprof package without needing handlers.
  3. something else.

I think while we decide on a more dynamic approach, it might be simpler to suggest to teams that use multi tenancy to run CNS with this turned on, since they own the infra and there should be less risk for abuse of these endpoints.

@ramiro-gamarra ramiro-gamarra enabled auto-merge (squash) April 3, 2023 23:30
@timraymond
Copy link
Member

@rbtr @ramiro-gamarra Why do we need to turn the pprof handlers on and off dynamically? There's no performance penalty until you actually collect a profile.

@rbtr
Copy link
Contributor

rbtr commented Apr 4, 2023

but there is significant performance penalty when you do so best practice is to not leave pprof on in prod where someone could naively hit a dump endpoint and freeze the process for a minute

@ramiro-gamarra ramiro-gamarra merged commit 6048ba2 into Azure:master Apr 5, 2023
@ramiro-gamarra ramiro-gamarra deleted the and-pprof-for-all branch April 5, 2023 15:45
@tamilmani1989 tamilmani1989 added the cns Related to CNS. label Apr 5, 2023
rbtr pushed a commit that referenced this pull request Sep 8, 2023
* adding pprof register method that can be invoked in multiple paths

* fixing lint issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cns Related to CNS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants