Skip to content

servenv: provide a global flag for profiling#7496

Merged
systay merged 1 commit intovitessio:masterfrom
vmg:vmg/pprof
Feb 18, 2021
Merged

servenv: provide a global flag for profiling#7496
systay merged 1 commit intovitessio:masterfrom
vmg:vmg/pprof

Conversation

@vmg
Copy link
Collaborator

@vmg vmg commented Feb 16, 2021

Description

The current servenv package provides some knobs to enable some of the profiling options in the Go runtime (but not all of them). In order to fully cover all the use cases that we'll require for profiling Vitess, I've decided to implement an aggregated -pprof flag with a comprehensive syntax, instead adding more one-off flags to servenv. The new flag handles all the existing profile options + all the others that are available in the Go runtime, and allows configuring the output path of the profiles.

We're hoping to wire up this new flag to @frouioui's new benchmarking implementation.

Related Issue(s)

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build/CI
  • VTAdmin

Signed-off-by: Vicent Marti <vmg@strn.cat>
Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Very nice! Do you see a case where one may want to profile more than one thing at the same time?

(PR is approved)

@systay systay merged commit 5c886b3 into vitessio:master Feb 18, 2021
@systay systay deleted the vmg/pprof branch February 18, 2021 08:32
@vmg
Copy link
Collaborator Author

vmg commented Feb 18, 2021

Do you see a case where one may want to profile more than one thing at the same time?

I intentionally designed the API so that no more than one profiling mode can be enabled at the same time to prevent contamination/noise. This may have been overly restrictive. I'll revisit the API if we find the the future that running several profiles at once saves us time and doesn't cost accuracy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants