compactor: adds downsample duration histogram

[vanugrah]

Signed-off-by: Anugrah Vijay [email protected]

• I added CHANGELOG entry for this change.
• Change is not relevant to the end user.

Changes

• Registers and implements downsampleDuration histogram.
• New var for downsample duration to avoid recomputing time.Since across logs/metric.

Verification

• Local validation of compactor with local minIO bucket

[vanugrah]

Based on @yeya24 's feedback on the previous PR I will be:

• Adding the downsample resolution as label to the histogram.
• Increasing the starting bucket size.

[vanugrah]

With respect to the histogram buckets, we could get rid of the initial few buckets and make it 64, 128, 256, 512, 1024, 2048, 4096, 8192 or we could define more user friendly intervals such as 1m, 5m, 15m, 30m, 1h, 2h, 4h. WDYT?

[yeya24]

Anything you still want to update? @vanugrah If not please mark this pr ready for review. 😄

[vanugrah]

Bucket ranges? Currently it mimics the compactors geometric series: 64, 128, 256, 512, 1024, 2048, 4096, 8192.
But changing that to recognizable intervals may be more user friendly: 1m, 5m, 15m, 30m, 60m, 120m, 240m.

[yeya24]

Bucket ranges? Currently it mimics the compactors geometric series: 64, 128, 256, 512, 1024, 2048, 4096, 8192.
But changing that to recognizable intervals may be more user friendly: 1m, 5m, 15m, 30m, 60m, 120m, 240m.

Latter sounds better.

[yeya24]

LGTM. Thanks for the contribution.

[vanugrah]

😄 Thanks for the speedy feedback!

[vanugrah]

So for flakey e2e tests do we just rerun?

[yeya24]

So for flakey e2e tests do we just rerun?

I restarted it and it is passing now

[vanugrah]

Wooot 🎉 Thanks for your help @yeya24 . More PR's soon to come.

[GiedriusS]

With --wait doesn't this mean that we will have infinitely different groups? 🤔 Perhaps a histogram over all durations would be more suitable i.e. drop the group label?

[vanugrah]

Hello! 👋

Based on the DefaultGroupKey it looks like the two parts of the label are the resolution and a hash of the meta labels. So if I'm not mistaken the cardinality would increase by num_buckets x 3 (1 for each resolution) for each unique set of meta labels, i.e. each unique source of blocks. So theoretically yes there is no upper bound on groups given that it correlates with things uploading blocks. But in practice, I haven't heard of more than a few thousand block sources for a single bucket, even on the larger side. Though, this is definitely just a heuristic.

Another point to consider is that the other downsample metrics already use the group key as a label: https://github.com/thanos-io/thanos/blob/main/cmd/thanos/downsample.go#L46-L53

And we already initialize the metric for all groups:
https://github.com/thanos-io/thanos/blob/main/cmd/thanos/downsample.go#L117-L119

So we're not necessarily introducing anything new to risk unbounded cardinality and we'd be able to align labels with the existing metrics.

Still wrapping my head around the compactor so let me know if I've misunderstood anything 😄

[vanugrah]

@GiedriusS Any thoughts?

[vanugrah]

@yeya24 Your review was marked stale because I had to resolve a merge conflict on the Changelog. Care to re-review?

[yeya24]

LGTM. Let's merge it.