-
Notifications
You must be signed in to change notification settings - Fork 347
Have profilers emit settings and metrics into event.json
#6597
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6597 +/- ##
==========================================
- Coverage 83.80% 83.55% -0.25%
==========================================
Files 490 490
Lines 20653 20674 +21
==========================================
- Hits 17308 17275 -33
- Misses 3345 3399 +54 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Overall package sizeSelf size: 12.65 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.7.0 | 35.02 MB | 35.02 MB | | @datadog/native-appsec | 10.2.1 | 20.64 MB | 20.65 MB | | @datadog/native-iast-taint-tracking | 4.0.0 | 11.72 MB | 11.73 MB | | @datadog/pprof | 5.11.1 | 9.96 MB | 10.34 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.5.4 | 2.95 MB | 5.73 MB | | @datadog/wasm-js-rewriter | 4.0.1 | 2.85 MB | 3.58 MB | | @datadog/native-metrics | 3.1.1 | 1.02 MB | 1.43 MB | | @opentelemetry/api | 1.9.0 | 1.22 MB | 1.22 MB | | jsonpath-plus | 10.3.0 | 617.18 kB | 1.08 MB | | import-in-the-middle | 1.14.4 | 123.18 kB | 851.76 kB | | lru-cache | 10.4.3 | 804.3 kB | 804.3 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | source-map | 0.7.6 | 185.63 kB | 185.63 kB | | pprof-format | 2.2.1 | 163.06 kB | 163.06 kB | | @datadog/sketches-js | 2.1.1 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 7.0.5 | 63.38 kB | 63.38 kB | | istanbul-lib-coverage | 3.2.2 | 34.37 kB | 34.37 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.3 | 23.74 kB | 23.74 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | mutexify | 1.4.0 | 5.71 kB | 8.74 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | module-details-from-path | 1.0.4 | 3.96 kB | 3.96 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2025-10-10 13:26:28 Comparing candidate commit c09e149 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1606 metrics, 64 unstable metrics. |
495c192
to
d318f4d
Compare
// transpose e.g. info.wall.settings to info.settings.wall. That way | ||
// we have functional groupings first, then profiler type. | ||
infos.settings[type] = info.settings | ||
delete info.settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems dangerous to modify the passed object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair. It happens infrequently enough that it's not necessary to optimize for this.
delete info.settings | ||
// take over rest of info without transposition | ||
if (Object.keys(info).length > 0) { | ||
infos[type] = info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No functional grouping here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought not, the rest are typically metrics, so they'll end up as
wall.totalAsyncContextCount
. Of course, we could embed this into another level, e.g. metrics
so then it'd become wall.metrics.totalAsyncContextCount
and we could then transpose that to metrics.wall.totalAsyncContextCount
. FWIW, we don't even need to do the whole transposition, it just adds complexity; I originally wanted a top-level settings
key because that's how e.g. Ruby does it, so I wanted to keep it a bit consistent with it:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree better be consistent with other langages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually now reworked it so that settings are constructed by config.js
and embedded in system info by top-level profiler inprofiler.js
and very little is left in individual profiler facets, just computed settings and metrics, and for those I don't bother with transposition anymore. I think it's much better this way, but I needed you asking to nudge me into rethinking it :-)
d318f4d
to
590378f
Compare
590378f
to
98b25d8
Compare
98b25d8
to
c09e149
Compare
What does this PR do?
Profiler uploads contain a file
event.json
that has a section for system info. The contents of system info are often language specific. So far we had the agent exporter set some system-specific values in it, but we want to generalize it a bit so that both the profiler config as well as individual profiler facets (wall, heap, events) can contribute values there as well. The config contributes its own settings, while the facets contribute either metrics or computed settings.In this iteration, the wall profiler contributes metrics for tracking the usage of async sample contexts when using
AsyncContextFrame
on Node.js 24+.Additionally, the wall profiler now emits telemetry about the async sample contexts as well. This telemetry is defined in https://github.com/DataDog/dd-go/pull/190144
The PR contains commits that also make fields private in various touched classes. I also noticed while examining candidate for emitted settings that the stack depth in
space.js
will always be the hardcoded 64 as there's no external way to set it. Hardcoding it allows us to presume the value in the backend, which has some special handling for truncated frames.Motivation
Having more information in uploaded profiles about the system configuration + having insight into the use of async sample contexts.
Plugin Checklist
Additional Notes
Jira: PROF-12666