Event loop lag should be a summary, not a gauge

[dhet]

Hi,
as stated in the title, I believe the event loop lag metric should be a summary, not a gauge. The reason is that event loop lag can fluctuate quite substantially in a very short time. Think of a web server: when a web server handles a request, and we take a lag sample at exactly that moment then the lag value is is probably high. If we take a sample in idle state on the other hand, lag should be close to zero. Now when we plot the gauge metric in a line chart we can see seemingly random lag spikes. The spike might be there or not depending on whether a sample was taken during a request or during idle time. A summary would produce much more meaningful results as it records not only a single point in time but also considers all previous samples.

Have a look at the official docs: monitorEventLoopDelay also uses a histogram.

I can open a PR if you want, otherwise #278 would be a good opportunity to make this change.

[dhet]

This is what it looks like currently:

Quite random I would say

Here's what I think it should look like (Ignore the .999 percentile. Those are outliers):

[sam-github]

Shouldn't it be a histogram?

[siimon]

Yeah, histogram is the way to go

[dhet]

Care to explain why? Isn't a summary just a histogram with percentiles as buckets?

[sam-github]

Histograms have fixed buckets, with counts in them, so its possible to sum the histograms across all the instances (pods for example). Summaries the data is analyzed on the fly, each pod has different values for the percentiles, and they can't be summed. If you google around you'll find a bunch of info on this. Summaries look great until someone tries to aggregate into a dashboard, then it goes badly.

There is a case to be made that the user should be able to provide either a histogram or summary object to be used for the observation of the values (go does this with some metrics it collects), but I haven't seen the Node.js client do this, so if there is only one, it should be histogram.

[zbjornson]

The reason is that event loop lag can fluctuate quite substantially in a very short time. Think of a web server: when a web server handles a request, and we take a lag sample at exactly that moment then the lag value is is probably high. If we take a sample in idle state on the other hand, lag should be close to zero.

The advanced event loop monitoring added in v12.0.0 (#278) directly exposes the underlying histogram from Node.js v11.10.0+, which fixes this specific problem.

As far as the metric type, I don't think we can convert this into a Prometheus Histogram because the data we get from Node.js is the "processed histogram" -- i.e. "lag at Nth percentile" whereas we'd need "counts in bin LE=0.05". I think @dhet is correct to suggest a Summary for that reason, but @sam-github is correct about Summaries being a pain to work with and aggregate.

I'm inclined to leave this as-is unless someone suggests otherwise.

[anthonyalayo]

Maybe once native histograms are on the main branch, that would be the best solution?

https://github.com/prometheus/prometheus/milestone/10