fix(metrics): prevent native metrics from loading when not needed#7942
fix(metrics): prevent native metrics from loading when not needed#7942pabloerhard wants to merge 4 commits intomasterfrom
Conversation
Overall package sizeSelf size: 5.49 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.0.1 | 82.56 kB | 817.39 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: ccb057f | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
BenchmarksBenchmark execution time: 2026-04-08 13:53:11 Comparing candidate commit ccb057f in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 230 metrics, 30 unstable metrics. |
| nativeMetrics = null | ||
| // Only load native metrics when at least one native-backed feature is enabled. | ||
| // This avoids loading native code unnecessarily, which can crash in some environments. | ||
| if (trackEventLoop || trackGc) { |
There was a problem hiding this comment.
While that makes sense, it hinders our ability to debug issues unless it's paired with a new option to override this behaviour. Because of this, I would say to add the option in this PR.
There was a problem hiding this comment.
In this case, should we keep the trackEventLoop || trackGc flag? Since nativeMetrics does more than just event loop and GC metrics, and both of those can be disabled while native metrics still provide some type of value, I don’t see why we should keep using these flags as part of the conditional (This seems like a design mistake on my part from the beginning)
I think it should only use the option to decide wether to load the native add on or not, and not the paired condition.
There was a problem hiding this comment.
Hmm yeah I guess for now let's remove the optimization and focus only on the option, we can always re-evaluate later.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7942 +/- ##
=========================================
Coverage ? 74.24%
=========================================
Files ? 769
Lines ? 36080
Branches ? 0
=========================================
Hits ? 26788
Misses ? 9292
Partials ? 0 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
We should be aware that the monitorEventLoopDelay is somewhat broken in Node.js and even with our fix, it reports bad data. It is not a 1-to-1 fallback due to that and I am hesitant of doing this.
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
Adding the configuration, seems like an easy fix on our side. Being a user, I would probably never use it, if I do not have a strong need for it. Did this come up as a user request somewhere?
The only reason to not use the build-in methods is the broken Node.js behavior. So my suggestion is: let us fix that in Node.js get it backported, if possible (that might not be possible due to the drastic changed output with a fix) and to change the behavior as soon as that is the case.
| // Using no-gc prevents the native gc metrics from being tracked. Not | ||
| // passing any options means all metrics are tracked. | ||
| // TODO: This is a workaround. We should find a better solution. |
There was a problem hiding this comment.
This should be kept in place until it is changed in @datadog/native-metrics :)
There was a problem hiding this comment.
(Using this thread to respond to the above comment) This is more of a user need than a user request per se. We do have a follow-up Q2 task to fix and backport this to Node.js :)
3 participants
What does this PR do?
This PR prevents native runtime metrics from loading when event loop metrics and gc metrics are disabled, falling back to the JS implementation with out loading the native module.
Motivation
Additional Notes