fix(metrics): prevent native metrics from loading when not needed#7942
Conversation
Overall package sizeSelf size: 5.48 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: 65b7c53 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
BenchmarksBenchmark execution time: 2026-04-13 14:47:18 Comparing candidate commit 65b7c53 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 232 metrics, 28 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 73.82% 73.82% -0.01%
==========================================
Files 773 773
Lines 35983 35985 +2
==========================================
+ Hits 26566 26567 +1
- Misses 9417 9418 +1 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
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
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 :)
BridgeAR
left a comment
There was a problem hiding this comment.
I am good with adding this by immediately marking it as deprecated and adding a comment about it being removed in future versions by switching everything to built-in as soon as it is fixed on Node.js side
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Tests failed on this commit 90415cf:
What to do next?
|
) * prevent native metrics from loading when not needed * added option to disableNative runtime metrics * added env and changed option name * added back comments * changed native metrics conditional * added native into config test * undo changes done by IDE and fixed tests * fix tests * set back to only using config
) * prevent native metrics from loading when not needed * added option to disableNative runtime metrics * added env and changed option name * added back comments * changed native metrics conditional * added native into config test * undo changes done by IDE and fixed tests * fix tests * set back to only using config
) * prevent native metrics from loading when not needed * added option to disableNative runtime metrics * added env and changed option name * added back comments * changed native metrics conditional * added native into config test * undo changes done by IDE and fixed tests * fix tests * set back to only using config
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