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

feat: added nodejs cpu profiler support #19

Merged
merged 46 commits into from
Apr 2, 2024
Merged

Conversation

shaleynikov
Copy link
Contributor

@shaleynikov shaleynikov commented Jul 25, 2022

Proof of concept of CPU nodejs profiler with tags support

screencast 2022-07-25 13-06-38

Usage examples are here grafana/pyroscope@1ddf73b

TODO:

  • Time value seems to have wrong cardinality
  • Fix tests & add more
  • Test static & dynamic tags
  • Update package documentation
  • Add tests for dynamic tags

src/cpu.ts Outdated
cpuProfiler.labels = { ...cpuProfiler.labels, [key]: value }
}

export function tagCall(
Copy link
Contributor

Choose a reason for hiding this comment

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

@petethepig what should this function be called to be consistent?

Copy link
Member

Choose a reason for hiding this comment

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

tagWrapper

@shaleynikov
Copy link
Contributor Author

So the current issue is:
Screenshot 2022-07-26 at 22 07 28

All timing values are below 0.01s

Here is a sample profile JSON of a sample nodejs rideapp loaded withload-generator.py
https://gist.github.com/shaleynikov/1870d4529a3c9ac982e0b2c47accfce5

I can see following problems:

  • wrong PeriodType of Period (or wrong Period value, compare to wall profile here https://gist.github.com/shaleynikov/a56656df9b2f9f097c3565abc37df9db)
  • there is an unexpected value field with value of [0, 10000]. I'm not totally sure if it's incorrect
  • I was expecting much more samples for 100hz probe during 10 seconds -- i've got near 30.

Maybe i'm missing something important about cpu profile

@shaleynikov
Copy link
Contributor Author

https://gist.github.com/shaleynikov/1870d4529a3c9ac982e0b2c47accfce5

Okay the issue I found is Period: 10 and period type is nanoseconds which is odd to me.
So what I did is created a PR for pprof package here. Going to confirm the issue with someone else and then send an issue to the origin git

@shaleynikov
Copy link
Contributor Author

shaleynikov commented Aug 16, 2022

Idle on walltime profiler was intentionally removed by DataDog

@shaleynikov shaleynikov marked this pull request as ready for review August 18, 2022 13:06
@ashtonsix
Copy link

ashtonsix commented Dec 14, 2022

What's the status of this PR? It looks good to me and I'd be keen to see this released

@korniltsev
Copy link
Collaborator

btw period bug was fixed DataDog/pprof-nodejs#61
should be included in the next 1.2.0 and we will not need fixes here

@korniltsev
Copy link
Collaborator

both wall and cpu right now write to our ".cpu" application, we may want to change that

src/cpu.ts Outdated
return
}

cpuProfilingTimer = setInterval(() => {
Copy link
Collaborator

@eh-am eh-am Feb 24, 2023

Choose a reason for hiding this comment

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

I am trying to run the profiler, while calculating fibonacci. This callback only gets called AFTER my fib code is ran, which is due to how js works: https://nodejs.org/en/docs/guides/dont-block-the-event-loop/#why-should-i-avoid-blocking-the-event-loop-and-the-worker-pool

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean, that's correct, the problem is that data is never flushed. Since when I call stop it simply cancels this setInterval, never getting the chance to upload the profile.

Copy link
Collaborator

Choose a reason for hiding this comment

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

stopCpuProfile not uploads last profile, should be better now

@CLAassistant
Copy link

CLAassistant commented Apr 27, 2023

CLA assistant check
All committers have signed the CLA.

@ben-xD
Copy link

ben-xD commented Jun 14, 2023

@shaleynikov, I guess you need to sign the CLA? as per #19 (comment)

@raphaelvigee
Copy link

What is holding this back ?

@Kidel
Copy link

Kidel commented Sep 27, 2023

please merge this, the old version is giving audit issues.

@mbombonato
Copy link

Hello guys! Any chances of this merge happening? I'm trying to implement Pyroscope for my company but I can't run this lib on NodeJS 18 and 20. I was hopping this PR could solve my issues because of the usage o @datadog/pprof.

Thanks!
/ping @petethepig 🙏

@notaphplover
Copy link
Contributor

This looks great, but @datadog/pprof@4 was already released, it would be great to update it to use that version. I honestly think this might take more time to be merged than the one I currently have, so I made notaphplover/one-game#701 heavily inspired on this PR in case someone is waiting for this to be merged and has not enough time to wait.

@korniltsev I don't mind giving you a hand with the update if that helps this PR to be merged. I would prefer having an official grafana library rather than a thousand forks.

@korniltsev
Copy link
Collaborator

We want to have this update but It seems like I'm not going to work on it in the near future.
@notaphplover If you could open a PR that would be great, I will be happy to review and merge. We can close this one in favor of a contribution.

@notaphplover
Copy link
Contributor

@korniltsev it would be my pleasure, I will submit the PR in the upcoming days

@notaphplover
Copy link
Contributor

@korniltsev I will be working on #39 to provide the datadog integration. There're multiple aspects to be discussed, but I think we will be able to do it :)

@petethepig petethepig merged commit d63816f into main Apr 2, 2024
4 checks passed
@petethepig petethepig deleted the feat/cpu-profiler-tags branch April 2, 2024 16:35
@korniltsev korniltsev restored the feat/cpu-profiler-tags branch April 11, 2024 08:01
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.