-
Notifications
You must be signed in to change notification settings - Fork 180
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
inmem: allow streaming of metrics as intervals complete #125
Conversation
3d15a58
to
7713fa0
Compare
These functions were only called in a single place, and repeated some of the same logic. This change is made in preparation for streaming metrics.
7713fa0
to
b1da2f8
Compare
b1da2f8
to
980bc52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, this is a big improvement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a cool! One Q inline but I can merge this if it's not an issue!
This will allow `consul debug` to include all the metrics, instead of missing many metrics.
980bc52
to
056fff3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as it is here.
Do you want to make the encoder passing change @dnephin? I'm happy to merge as-is but let me know if you want to get that in first.
I pushed another commit which reduces the interface that I think this is ready for merge now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. I like the interface.
Related to one of the items in hashicorp/consul#10320
We use the
InMemSink
to capture metrics for the debug archive, but the previous implementation resulted in us missing a lot of metrics because only 1/3 of the intervals were captured. Attempting to request the metrics at each interval is a challenge (maybe impossible), so instead this PR adds for an HTTP handler that sends metrics each time an interval completes.