Clean up old metrics using metrics timestamp #45
Conversation
mrzacarias
force-pushed
the
issue44_mz_cleanup_old_metrics
branch
from
f3ad0ac to
25f0fb1
Compare
mrzacarias
force-pushed
the
issue44_mz_cleanup_old_metrics
branch
from
25f0fb1 to
fa3fefb
Compare
|
Thanks for this - sorry nobody responded before now. |
There was a problem hiding this comment.
The main functionality you've added seems fine, but there are other changes in this PR that either need better justification, or removal.
Doing the cleanup, including copying all the metrics, on every scrape could get expensive, but probably not noticeable for modest usage.
| cors := flag.String("cors", "*", "The 'Access-Control-Allow-Origin' value to be returned.") | ||
| pushPath := flag.String("push-path", "/metrics/", "HTTP path to accept pushed metrics.") | ||
| timeToLiveMs := flag.Int64("time-to-live-ms", 3600000, "How long unmerged metrics will live, in milliseconds (default 1h)") |
There was a problem hiding this comment.
This is a little user-hostile; flag.Duration would allow specifying as 1h, 10m, etc.
| @@ -259,12 +301,15 @@ func (a *aggate) handler(w http.ResponseWriter, r *http.Request) { | |||
| } | |||
|
|
|||
| func main() { | |||
| listen := flag.String("listen", ":80", "Address and port to listen on.") | |||
| listen := flag.String("listen", ":8080", "Address and port to listen on.") | |||
There was a problem hiding this comment.
This would be a breaking change for users, and unrelated to the description. Please remove from this PR.
| } else if c.err1.Error() != err.Error() { | ||
| t.Fatalf("Expected %s, got %s", c.err1, err) | ||
| t.Fatalf("Expected '%s', got '%s'", c.err1, err) |
| a := newAggate() | ||
| a := newAggate(3600000) | ||
| if c.b != "" { | ||
| c.a, c.b, c.want = fmt.Sprintf(c.a, now), fmt.Sprintf(c.b, now), fmt.Sprintf(c.want, now) |
There was a problem hiding this comment.
Where is this checked or relied on? I can't see that this test makes any use of the timestamp.
|
Hey @bboreham and @mrzacarias is there anything that I help you to merge this? |
|
Do you want to take over the PR and resolve the comments I made? |
|
Hi @bboreham, sure! |
|
Hi, @bboreham sorry for the bother. |
|
Hi @bboreham friendly reminder, it would be fantastic to have this feature available. |
|
I have commented on #61. I don't have write access to this repo so can't close this PR. |
|
Hello. Is there an ETA on when this feature might be available? Thank you |
Issue: #44
There's no need to keep the metrics always there on PAG, as they are constantly scraped and stored on Prometheus or Cortex long living storage. The attempt to fix the MEM/CPU performance issues generated by this is:
This was tested at EverQuote with excellent results:
