[profiling] Add experimental CPU profiler #3895

nsavoire · 2023-12-20T10:37:45Z

What does this PR do?

Motivation

Additional Notes

Security

Datadog employees:

If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

github-actions · 2023-12-20T10:38:30Z

Overall package size

Self size: 5.83 MB
Deduped: 61.48 MB
No deduping: 62.23 MB

Dependency sizes

name	version	self size	total size
@datadog/native-iast-taint-tracking	1.6.4	16.43 MB	16.44 MB
@datadog/native-appsec	6.0.0	14.48 MB	14.48 MB
@datadog/pprof	5.0.0	9.59 MB	10.44 MB
protobufjs	7.2.5	2.77 MB	6.56 MB
@datadog/native-iast-rewriter	2.2.2	2.29 MB	2.37 MB
@opentelemetry/core	1.14.0	872.87 kB	1.47 MB
@datadog/native-metrics	2.0.0	898.77 kB	1.3 MB
@opentelemetry/api	1.4.1	780.32 kB	780.32 kB
import-in-the-middle	1.7.2	45.99 kB	709.38 kB
pprof-format	2.0.7	588.12 kB	588.12 kB
msgpack-lite	0.1.26	201.16 kB	281.59 kB
opentracing	0.14.7	194.81 kB	194.81 kB
semver	7.5.4	93.4 kB	123.8 kB
@datadog/sketches-js	2.1.0	109.9 kB	109.9 kB
lodash.sortby	4.7.0	75.76 kB	75.76 kB
lru-cache	7.14.0	74.95 kB	74.95 kB
ipaddr.js	2.1.0	60.23 kB	60.23 kB
ignore	5.2.4	51.22 kB	51.22 kB
int64-buffer	0.1.10	49.18 kB	49.18 kB
istanbul-lib-coverage	3.2.0	29.34 kB	29.34 kB
lodash.uniq	4.5.0	25.01 kB	25.01 kB
tlhunter-sorted-set	0.1.0	24.94 kB	24.94 kB
limiter	1.1.5	23.17 kB	23.17 kB
dc-polyfill	0.1.2	22.77 kB	22.77 kB
retry	0.13.1	18.85 kB	18.85 kB
lodash.kebabcase	4.1.1	17.75 kB	17.75 kB
node-abort-controller	3.1.1	16.89 kB	16.89 kB
lodash.pick	4.4.0	16.33 kB	16.33 kB
jest-docblock	29.7.0	8.99 kB	12.76 kB
crypto-randomuuid	1.0.0	11.18 kB	11.18 kB
path-to-regexp	0.1.7	6.78 kB	6.78 kB
koalas	1.0.2	6.47 kB	6.47 kB
methods	1.1.2	5.29 kB	5.29 kB
module-details-from-path	1.0.3	4.47 kB	4.47 kB

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

codecov · 2023-12-20T10:39:53Z

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (cfc2436) 84.69% compared to head (4aa5fbf) 84.69%.

Files	Patch %	Lines
packages/dd-trace/src/profiling/profilers/wall.js	52.94%	8 Missing ⚠️
...ackages/dd-trace/src/profiling/profilers/events.js	0.00%	5 Missing ⚠️
...ackages/dd-trace/src/profiling/profilers/shared.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3895   +/-   ##
=======================================
  Coverage   84.69%   84.69%           
=======================================
  Files         238      238           
  Lines       10156    10176   +20     
  Branches       33       33           
=======================================
+ Hits         8602     8619   +17     
- Misses       1554     1557    +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pr-commenter · 2024-01-03T16:36:28Z

Benchmarks

Benchmark execution time: 2024-01-08 14:12:30

Comparing candidate commit 4aa5fbf in PR branch nsavoire/PROF-8804-collect-cpu-profile with baseline commit cfc2436 in branch master.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 258 metrics, 7 unstable metrics.

scenario:plugin-graphql-with-depth-off-18

🟩 max_rss_usage [-102.648MB; -96.032MB] or [-10.960%; -10.253%]

szegedi · 2024-01-04T10:38:45Z

packages/dd-trace/src/profiling/profiler.js

-    const end = new Date()
+    const end = Date.now()


Is this okay? As far as I can tell, we use Date objects for start and end throughout. We could consistently use millis everywhere, but that'd be a larger change.

I messed up my commits, fixed.

szegedi · 2024-01-04T10:45:05Z

packages/dd-trace/src/profiling/profilers/events.js

@@ -171,10 +171,11 @@ class EventsProfiler {
  stop () {
    if (this._observer) {
      this._observer.disconnect()
+      this._observer = undefined


With this, we could also move the .observe call into the if statement in the start () method.

szegedi · 2024-01-04T10:50:40Z

packages/dd-trace/src/profiling/profilers/events.js

    }
  }

-  profile (startDate, endDate) {
+  profile ({ startDate, endDate, restart = true }) {


I wanted to ask what's the value in passing an object here, and what's the value in having a default for restart – if the only call site is in profiler.js and it always passes all three values then just keeping 3-args would suffice. Except if you want to avoid specifying all args in wall and space profilers, in which case this might make sense. I'm still unclear on what does the default buy us.

Well, my initial thought was that since wall and space profilers did not use startDate/endDate, it would be nice to not specify these parameters for them and at the same time make the API flexible.
But since this can be considered as an internal API than we can change at will, I guess we can pass 3 arguments as well.

But having 3 args, implies updating tests with things like profiler.profile(undefined, undefined, true)

I could swap the args to avoid that:
profile (restart, startDate, endDate)

szegedi · 2024-01-04T10:52:50Z

packages/dd-trace/src/profiling/profilers/wall.js

@@ -267,30 +290,16 @@ class NativeWallProfiler {
    return labels
  }

-  profile () {
-    return this._stop(true)
+  profile ({ restart = true } = {}) {


I like { restart = true} = {}. This is fun :-) OTOH, we don't use = {} in the events profiler, and I'm unclear on where would we call this method without arguments. Is it from tests?

szegedi · 2024-01-08T13:57:37Z

packages/dd-trace/src/profiling/exporters/file.js

@@ -2,6 +2,7 @@

 const fs = require('fs')
 const { promisify } = require('util')
+const { threadId } = require('node:worker_threads')


Won't node: again cause trouble for Node < 14.18?

Woops, fixed

Upon process exit, profiler collects profiles a last time and was restarting all profilers before stopping them. Avoid this useless restart by changing the profiler API and make `profile()` methode take a restart argument.

szegedi

🚢 🚢

* Add CPU profiler * Avoid restarting profiling for last collect Upon process exit, profiler collects profiles a last time and was restarting all profilers before stopping them. Avoid this useless restart by changing the profiler API and make `profile()` methode take a restart argument. * Bump pprof-nodejs version

nsavoire force-pushed the nsavoire/PROF-8804-collect-cpu-profile branch from 5e2735e to 37ad59c Compare January 3, 2024 16:28

nsavoire added the semver-minor label Jan 3, 2024

szegedi reviewed Jan 4, 2024

View reviewed changes

nsavoire force-pushed the nsavoire/PROF-8804-collect-cpu-profile branch 4 times, most recently from 7fda2e6 to d250c7e Compare January 5, 2024 15:25

nsavoire added feature-request profiling labels Jan 5, 2024

nsavoire force-pushed the nsavoire/PROF-8804-collect-cpu-profile branch from d611230 to af2b748 Compare January 8, 2024 09:43

nsavoire requested a review from szegedi January 8, 2024 10:02

nsavoire force-pushed the nsavoire/PROF-8804-collect-cpu-profile branch from af2b748 to 5012a60 Compare January 8, 2024 12:58

nsavoire marked this pull request as ready for review January 8, 2024 13:08

nsavoire requested a review from a team as a code owner January 8, 2024 13:08

szegedi reviewed Jan 8, 2024

View reviewed changes

nsavoire added 3 commits January 8, 2024 15:04

Add CPU profiler

735aeca

Avoid restarting profiling for last collect

bab46f9

Upon process exit, profiler collects profiles a last time and was restarting all profilers before stopping them. Avoid this useless restart by changing the profiler API and make `profile()` methode take a restart argument.

Bump pprof-nodejs version

4aa5fbf

nsavoire force-pushed the nsavoire/PROF-8804-collect-cpu-profile branch from 5012a60 to 4aa5fbf Compare January 8, 2024 14:04

szegedi approved these changes Jan 8, 2024

View reviewed changes

nsavoire merged commit 5c019ea into master Jan 8, 2024
109 of 110 checks passed

nsavoire deleted the nsavoire/PROF-8804-collect-cpu-profile branch January 8, 2024 14:16

This was referenced Jan 9, 2024

v5.0.0-proposal #3920

Closed

V5.0.0 proposal #3949

Closed

v5.0.0 #3950

Merged

v5.0.0 #3957

Merged

szegedi mentioned this pull request Jan 15, 2024

V4.24.0 proposal #3966

Merged

szegedi mentioned this pull request Jan 15, 2024

V3.45.0 proposal #3967

Merged

erikvanbrakel mentioned this pull request Jul 5, 2024

[DEVX-3565] add neo4j plugin for v4.41 mazedesignhq/dd-trace-js#15

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[profiling] Add experimental CPU profiler #3895

[profiling] Add experimental CPU profiler #3895

nsavoire commented Dec 20, 2023 •

edited

Loading

github-actions bot commented Dec 20, 2023 •

edited

Loading

codecov bot commented Dec 20, 2023 •

edited

Loading

pr-commenter bot commented Jan 3, 2024 •

edited

Loading

szegedi Jan 4, 2024

nsavoire Jan 5, 2024

szegedi Jan 4, 2024

szegedi Jan 4, 2024

nsavoire Jan 8, 2024

nsavoire Jan 8, 2024

nsavoire Jan 8, 2024

szegedi Jan 4, 2024

szegedi Jan 8, 2024

nsavoire Jan 8, 2024

szegedi left a comment

[profiling] Add experimental CPU profiler #3895

[profiling] Add experimental CPU profiler #3895

Conversation

nsavoire commented Dec 20, 2023 • edited Loading

What does this PR do?

Motivation

Additional Notes

Security

github-actions bot commented Dec 20, 2023 • edited Loading

Overall package size

Dependency sizes

codecov bot commented Dec 20, 2023 • edited Loading

Codecov Report

pr-commenter bot commented Jan 3, 2024 • edited Loading

Benchmarks

scenario:plugin-graphql-with-depth-off-18

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szegedi left a comment

Choose a reason for hiding this comment

nsavoire commented Dec 20, 2023 •

edited

Loading

github-actions bot commented Dec 20, 2023 •

edited

Loading

codecov bot commented Dec 20, 2023 •

edited

Loading

pr-commenter bot commented Jan 3, 2024 •

edited

Loading