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

Use default sample rate of 100 per second for CPU #3342

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

simonswine
Copy link
Contributor

@simonswine simonswine commented Jun 6, 2024

This fixes the default sample rate for profiling and clarfies the field value. Relates to #3340.

@@ -82,7 +82,7 @@ type FlamebearerMetadataV1 struct {
Format string `json:"format"`
// Name of the spy / profiler used to generate the profile, if any.
SpyName string `json:"spyName"`
// Sample rate at which the profiler was operating.
// Sample rate at which the profiler was operating. Note: This unit is not related to the Units field and is measured in samples per second.
SampleRate uint32 `json:"sampleRate"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not too sure if this is the right type and definition. I think it would be better if we would signal the sampleRate per SampleUnit. But for this we would need to change the field to be a float.

The default 100 Hz, would become 0.00001 per nanosecond. Any insights that you have on this @korniltsev

This fixes the default sample rate for profiling and clarfies the field
value. Relates to grafana#3340.
@simonswine simonswine force-pushed the 20240606_use-default-cpu-sample-rate branch from dcf5123 to 81c66e7 Compare August 21, 2024 14:03
@simonswine simonswine marked this pull request as ready for review August 21, 2024 14:03
@simonswine simonswine requested a review from a team as a code owner August 21, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant