-
Notifications
You must be signed in to change notification settings - Fork 61
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
[FEAT] Adding TTL to cleanup old metrics #61
base: master
Are you sure you want to change the base?
Conversation
9917f90
to
ec64fb9
Compare
Hi @nicolastakashi |
Hey @shlompy thanks for helping me and sorry for the late reply, I was on holiday. |
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.
I read through this change. It looks plausible, however (a) I don't run this code and (b) I don't think Weaveworks run this code either, since Weave Cloud was shut down.
I don't work for Weaveworks any more, so not going to say what is best for the project in future.
I see @shlompy suggested a fix, which does not appear to be in this PR. Please can you clarify whether it was accepted or rejected. Either way, I would suggest a test showing that the problem @shlompy reported does not occur.
histogram_bucket{le="+Inf"} 4 | ||
histogram_sum{} 2.5 | ||
histogram_count{} 1 | ||
histogram_bucket{le="1"} 0 %[1]d |
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.
All these changes to add %[1]d
to metric output are mysterious to me. Perhaps caused by an upstream change?
I prefer when all changes are explained by the commit message.
I'm taking over this PR #45