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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use 99hz as default profiling sample rate #2007

Merged
merged 1 commit into from
Apr 26, 2022

Conversation

Qard
Copy link
Collaborator

@Qard Qard commented Apr 25, 2022

Avoid power of ten sample rates. This is to avoid bias toward things like timers which are often defined at power of ten intervals. Probably won't actually make much difference with Node.js because timers are best-effort timing through the event loop, but there's no reason not to do this. 馃し馃徎

@Qard Qard added the profiling label Apr 25, 2022
@Qard Qard requested a review from a team as a code owner April 25, 2022 17:14
@Qard Qard force-pushed the non-power-of-ten-profiling-sample-rate branch from f209ac4 to ea33e7d Compare April 25, 2022 17:17
rochdev
rochdev previously approved these changes Apr 25, 2022
PyvesB
PyvesB previously approved these changes Apr 26, 2022
Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

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

Thanks for addressing this!

@Qard Qard dismissed stale reviews from PyvesB and rochdev via 33f9de2 April 26, 2022 16:38
@Qard Qard force-pushed the non-power-of-ten-profiling-sample-rate branch from ea33e7d to 33f9de2 Compare April 26, 2022 16:38
@Qard
Copy link
Collaborator Author

Qard commented Apr 26, 2022

Force pushed to re-run the CI. Hopefully npm is in a better state this time...

@Qard Qard force-pushed the non-power-of-ten-profiling-sample-rate branch from 33f9de2 to e2188a8 Compare April 26, 2022 16:45
@rochdev rochdev merged commit cf1de86 into master Apr 26, 2022
@rochdev rochdev deleted the non-power-of-ten-profiling-sample-rate branch April 26, 2022 17:10
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.

None yet

3 participants