Closed shaleynikov closed 5 months ago
So the current issue is:
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:
PeriodType
of Period
(or wrong Period
value, compare to wall profile here https://gist.github.com/shaleynikov/a56656df9b2f9f097c3565abc37df9db)value
field with value of [0, 10000]
. I'm not totally sure if it's incorrectMaybe i'm missing something important about cpu
profile
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
Idle on walltime profiler was intentionally removed by DataDog
What's the status of this PR? It looks good to me and I'd be keen to see this released
btw period bug was fixed https://github.com/DataDog/pprof-nodejs/pull/61 should be included in the next 1.2.0 and we will not need fixes here
both wall and cpu right now write to our ".cpu" application, we may want to change that
you said it's possible to run both wall and cpu, can you check? this tests says it shouldn't be possible https://github.com/pyroscope-io/pyroscope-nodejs/blob/b69e80ebd3973cdead6637ad981e52d62a7757a5/test/profiler.test.ts#L67
Also if you could bump the major version, since technically there's a breaking change (startCpuProfiling actually starts >cpu< profiling :P )
+1 for major bump because I will probably need to make some api async
you said it's possible to run both wall and cpu, can you check? this tests says it shouldn't be possible
the tests says it should not be possible, but at the same time I dont see any expected failure/rejection in the test itself and also in the code
I will debug it more precisely, thanks.
It would be also nice to have a unified logic for scheduling and canceling profiling between cpu and wall and maybe heap, and make the profiler implementation plugfable
Cause right now we have to reimplement setTimeout and all that stuff and also it is already different between cpu and wall
Not sure if it is hard and or possible, will try to look into it as well
It does allow both cpu https://flamegraph.com/share/5d0b6890-b745-11ed-8c0d-0a1586f30030 and wall https://flamegraph.com/share/73af49fe-b745-11ed-8c0d-0a1586f30030 simultaneously
I will just change the title for the testcase
I gave up on unification of scheduling/stopping logic in this PR, I believe it is better to do in a separate PR
@eh-am ptal I made some changes
Apart from what we already talked in slack:
I also noticed that we are not cleaning the CWD from the name: https://github.com/pyroscope-io/pyroscope-nodejs/blob/09d3f4d1b77d8640181e871db0fa3d60919e4a7d/src/index.ts#L123-L127
@shaleynikov, I guess you need to sign the CLA? as per https://github.com/grafana/pyroscope-nodejs/pull/19#issuecomment-1525208071
What is holding this back ?
please merge this, the old version is giving audit issues.
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 :pray:
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 https://github.com/notaphplover/one-game/pull/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.
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.
@korniltsev it would be my pleasure, I will submit the PR in the upcoming days
@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 :)
Proof of concept of CPU nodejs profiler with tags support
Usage examples are here https://github.com/pyroscope-io/pyroscope/commit/1ddf73bea8e609d4e4de702d61d84f6e0e28782b
TODO: