compactor: add downsample duration histogram #4551
Conversation
| m.downsampleDuration = promauto.With(reg).NewHistogram(prometheus.HistogramOpts{ | ||
| Name: "thanos_compact_downsample_duration_seconds", | ||
| Help: "Duration of downsample runs", | ||
| Buckets: []float64{1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192}, |
There was a problem hiding this comment.
Maybe we can start from a larger number. Please note that the time includes the block download and index verification steps so small buckets doesn't make sense here.
There was a problem hiding this comment.
Hello - that turn around was blazing fast!
I have no reservations in making the starting intervals larger - I was merely trying to replicate the same intervals used in the prometheus_tsdb_compaction_duration_seconds_bucket metric currently being exposed by the compactor.
If I'm not mistaken the begin variable is updated to time.Now() once the block has been downloaded and the index has been verified: https://github.com/thanos-io/thanos/blob/main/cmd/thanos/downsample.go#L326
So what this should measure is the time taken to open the block and run the downsample.Downsample method.
There was a problem hiding this comment.
Yes, you are right! Sorry, the Github code review just showed the begin := time.Now() at the beginning so I misunderstood it.
But it would be better to double-check the buckets. Also might be good to have a label for different resolution. 5m block and 1h block are different.
There was a problem hiding this comment.
Love it - I was thinking the same. I was originally conflicted because the compaction duration histogram has no labels (like compaction level) so I thought might be better to conform to the standard. But the resolution level would result in something more meaningful.
I'm also thinking of adding histograms for overall block upload and download times as well (in another PR). Having resolution/compaction level on that may be beneficial as well.
| @@ -346,6 +361,7 @@ func processDownsampling(ctx context.Context, logger log.Logger, bkt objstore.Bu | |||
|
|
|||
| level.Info(logger).Log("msg", "downsampled block", | |||
There was a problem hiding this comment.
Might be good to initialize a variable for passed_time above this line so that we don't have to call time.Since(begin) multiple times.
Signed-off-by: Anugrah Vijay <[email protected]>
|
@yeya24 I realized that I signed off the previous commit with the wrong email 🤦 Updating. |
|
Actually it seems I didn't update my git config for my fork to use my personal email. I can run a git filter-branch script to update historic commits to use the correct author email (though this will change commit history) or create a new PR. Any preference? |
It is fine to create another pr. Just do whichever is better for you. Thanks for the contribution. |
|
Closing this PR in favor of: #4552 which has appropriate signing. |
Changes
Verification