Skip to content
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

Reset internal histogram of monitorEventLoopDelay after each collect() invocation #459

Merged
merged 1 commit into from
Sep 19, 2021

Conversation

yarsky-tgz
Copy link
Contributor

This PR addresses problem described at #370.

I've read attentively that issue and propositions about the solution. Anyway after own investigation of the code I've decided that simple calling histogram.reset() after each collect() invocation will be simply enough.

Event loop lag metrics is important for us because we want to see how our application handles spikes and how internal event loop lag increases, to determine easier, there is hiding the bottleneck. Currently we just cannot see that, because nodejs_eventloop_lag_seconds represents just single measurement result (so it is pretty random, as noticed already at #309) and nodejs_eventloop_lag_mean_seconds shows mean value for overall process lifetime and I cannot imagine, how to use such information in our case and for what cases it can be useful at all.

If all collected now event loop lag values will represent only scrape interval period that will be simply enough for us and I think that for everyone too.

Copy link
Collaborator

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable, left some additional notes in #370 (comment).

I'll update the changelog separately.

Thanks for the contribution!

@zbjornson zbjornson merged commit 0f444cd into siimon:master Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants