src: add event loop based metrics into node#62935
Conversation
edb66ea to
e7093c8
Compare
6f5cdca to
bc0a59c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62935 +/- ##
==========================================
- Coverage 89.65% 89.64% -0.02%
==========================================
Files 708 708
Lines 220413 220546 +133
Branches 42275 42290 +15
==========================================
+ Hits 197607 197702 +95
- Misses 14658 14675 +17
- Partials 8148 8169 +21
🚀 New features to boost your workflow:
|
bc0a59c to
27ead1e
Compare
add a samplePerIteration option to monitorEventLoopDelay that records event loop delay from libuv event loop iterations instead of the existing timer interval sampler. The default remains the interval based, uses of monitorEventLoopDelay() will still behave the same unless the samplePerIteration options is passed through. Signed-off-by: Pablo Erhard Hernandez <pablo.erhardhernandez@datadoghq.com>
Add test coverage for the per iteration event loop delay histogram start and stop callbacks Use disitinct debug tracking keys for the event loop delay histogram callbacks.
Use the super Close method instead of duplicating code
27ead1e to
3b21617
Compare
Added extra test to add coverage of the overall functionality of the new ELD histogram.
Modified docs to be on par with the changes and modifications made.
The C++ class `ELDHistogram` is the per-iteration sampling implementation that sits next to `IntervalHistogram` (the timer-based one). Its name collided with the JS exposed `ELDHistogram` class that `monitorEventLoopDelay()` returns regardless of sampling mode, making the layering confusing: "ELDHistogram" was both the generic JS concept and the name of one specific C++ backing.
align perf_hooks docs after renaming internal ELDHistogram to IterationHistogram
71f121f to
88fce8d
Compare
Added shared templates for duplicated start/stop logic in IntevalHistogram and IterationHistogram.
50f8eab to
29b708d
Compare
BridgeAR
left a comment
There was a problem hiding this comment.
LGTM, while I would still like to get another LG who works on CPP frequently
| @@ -1997,9 +2006,10 @@ added: v11.10.0 | |||
|
|
|||
| The standard deviation of the recorded event loop delays. | |||
|
|
|||
| ## Class: `IntervalHistogram extends Histogram` | |||
| ## Class: `ELDHistogram extends Histogram` | |||
There was a problem hiding this comment.
Why the rename? the intent here was for it to be named generically so that it could potentially be used for things other than just event loop delay monitoring.
There was a problem hiding this comment.
The rename was meant to avoid documenting that monitorEventLoopDelay() strictly returns an IntervalHistogram, this because with the new samplePerIteration option the native backing is no longer always interval based. Leaving the documented type as IntervalHistogram felt misleading considering the same API can be backed by either an iteration or interval based sampling.
My first approach was to document that monitorEventLoopDelay() returns {IntervalHistogram|IterationHistogram}, but that felt redundant considering both modes expose the same JS API shape and only differ in how samples are collected. That said, maybe this approach is a better option if we want to maintain IntervalHistogram as a generic documented type while also introducing IterationHistogram.
Adds an opt in into monitorEventLoopDelay(), this option adds support for per iteration sampling using libuv hooks, while preserving the existing timer based approach.
When the new samplePerIteration option is set to true, monitorEventLoopDelay() samples directly from event loop iterations instead of using the interval timer. When samplePerIteration is omitted or false, the existing interval based implementation remains unchanged, including resolution handling and the older interval based trace counters.
Motivation
The interval based implementation has two limitations that the new option addresses:
This is an alternative implementation to the one in #62934
This PR directly addresses the following issue #56064