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

[FEAT] Adding TTL to cleanup old metrics #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nicolastakashi
Copy link

I'm taking over this PR #45

@shlompy
Copy link

shlompy commented Sep 1, 2022

Hi @nicolastakashi
Raised a required fix into your fork, since current code doesn't add timestamp for new metrics, and they'll always get deleted when metrics are queried, unless they got updated before that:
nicolastakashi#1

@nicolastakashi
Copy link
Author

Hey @shlompy thanks for helping me and sorry for the late reply, I was on holiday.
regarding the change LGTM

Copy link
Contributor

@bboreham bboreham left a 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
Copy link
Contributor

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.

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.

3 participants