-
Notifications
You must be signed in to change notification settings - Fork 99
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
Track delta metrics between scrapes #168
Track delta metrics between scrapes #168
Conversation
Signed-off-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>
…ounter store Signed-off-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>
Signed-off-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>
…as flag is respected Signed-off-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>
Signed-off-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>
Signed-off-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>
dc1fcba
to
ffb190b
Compare
Thanks, I'll take a look at this soon. To start, this needs a DCO sign-off. You can use |
Signed-off-by: Kyle Eckhart <kgeckhart@users.noreply.github.com>
b26820c
to
aefd076
Compare
Hello @SuperQ / @kgeckhart, this looks like it solves a really important issue with the exporter where DELTA metrics seem pretty much unusable in certain cases. Can I ask what is stopping it from being merged and if there is anything I can do to speed that up? This issue is making monitoring of some of our GCP dependencies a lot harder as the data is split into 2 places. |
Hi @ahjmorton, I'll defer to @SuperQ for the review + merge side of things. I can say we have been running this version in production for a little over three weeks. Our largest exporter is tracking ~500k metrics without any noticeable issues. |
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.
Minor nits, but otherwise LGTM.
CC @igorwwwwwwwwwwwwwwwwwwww, this may interest you. |
cc @T4cC0re |
Co-authored-by: Ben Kochie <superq@gmail.com> Signed-off-by: kgeckhart <kgeckhart@users.noreply.github.com>
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
This PR introduces support for aggregating DELTA metrics in-memory. This implementation is somewhat based on #167 but with extra functionality which is intended to solve the problem identified here, #167 (comment).
I chose to inject interfaces for the delta and distribution stores at the Collector level to try to keep library friendliness intact, #157
I did as much as I could to explain the solution and the tradeoffs with aggregating deltas in this manner in the README.md. The bulk of the changes are in
monitoring_metrics.go
where I tried to obfuscate as much of the delta logic as possible.Should hopefully resolve #116 and #25