storage: remove compaction_ratio metric from probe#29994
storage: remove compaction_ratio metric from probe#29994WillemKauf merged 1 commit intoredpanda-data:devfrom
storage: remove compaction_ratio metric from probe#29994Conversation
There was a problem hiding this comment.
Pull request overview
Reduces /metrics cardinality for storage compaction ratio by making the metric eligible for label aggregation across partitions (topic-level rollup) rather than emitting a distinct time series per partition.
Changes:
- Removed the non-aggregated
compaction_ratiometric registration. - Added a new aggregated metric
compaction_ratio_totalwith updated help text describing topic-level aggregation.
WillemKauf
force-pushed
the
compaction_ratio_metric_agg
branch
from
0d04d47 to
cf61deb
Compare
StephanDollberg
left a comment
StephanDollberg
left a comment
There was a problem hiding this comment.
Does it make more sense to calculate a ratio more directly at the topic level instead of doing the average of averages approach (weighted by bytes or something)?
to be honest this metric is already fairly approximate given its sliding window nature and the fact it has a sample size of 5 segments/operations. i don't believe it is a good indicator of "compactibility" and doing the low effort thing here probably isn't a huge deal. wdyt @dotnwat ? |
CI test resultstest results on build#82498
|
| sm::description( | ||
| "Sum of compaction ratios across segments in this partition. " | ||
| "When aggregate_metrics is enabled, this is summed across all " | ||
| "partitions in a topic; divide by the partition count to get " | ||
| "the average compaction ratio."), |
There was a problem hiding this comment.
I'm fine with this change, but just looking at how it's used (moving average of ratio computed with every segment rewrite), I'm wondering how useful it is in practice as is today (even without this aggregation).
If we're already going to need callers to do some math with this division, maybe we should instead expose some bytes_rewritten_in and bytes_rewritten_out and aggregate that, so that the collective compaction ratio is always bytes out / bytes in, whether aggregated or not.
There was a problem hiding this comment.
I'm wondering how useful it is in practice as is today
I don't think its all that useful. I was leaning towards fully removing it.
If we're already going to need callers to do some math with this division, maybe we should instead expose some bytes_rewritten_in and bytes_rewritten_out and aggregate that, so that the collective compaction ratio is always bytes out / bytes in, whether aggregated or not.
I'd prefer this change as well but it would require some additional state/book-keeping in disk_log_impl, and I wasn't sure that juice was worth the squeeze.
There was a problem hiding this comment.
+1 to removing it
additional state/book-keeping
FWIW the compaction_result (where we compute the compaction ratio today) seems to already have size_before and size_after. Presumably if we could just add these to a counter, or am I missing something?
There was a problem hiding this comment.
Ah i guess the issue is that we'd kind of want a rolling average of both of the counters in order to make sense of them over longer spans of time, though maybe for monitoring, even if we didn't have the rolling average we could do something like rate(size_before) / rate(size_after)
There was a problem hiding this comment.
Currently the compaction_ratio is represented as a moving_average with 5 samples in the disk_log_impl, so the book-keeping isn't just two size_ts or something like that
There was a problem hiding this comment.
Right, edited my message with another thought (using rates). If we do that, I don't think a moving average is necessary. Though I'm also not confident enough in my grafana fu to know that what i'm suggesting gets us something useful
There was a problem hiding this comment.
I'm leaning towards outright removing the metric and the moving average itself from disk_log_impl now. @dotnwat do you have any objections to this?
There was a problem hiding this comment.
I'll defer to your best judgement on if you think it's useful or not. Compaction ratio seems useful on the surface in that it tells us something about the data, but I think we lose that usefulness if we aggregate. So if the choice is between aggregate or drop it, dropping it is probably the right move.
However, if we kind of like knowing that it is there, we could expose it through the admin interface (for later inclusion in debug bundle), or we could log a summary of compation work (including ratio) after a compaction run at debug or trace level?
Although, maybe debug/trace also isn't useful in practice depending on when you'd want to know the compaction ratio 🤷
There was a problem hiding this comment.
or we could log a summary of compation work (including ratio) after a compaction run at debug or trace level?
we log some interesting compaction info at INFO level which (per segment) notes the number of batches processed, number of records discarded, etc. It's not a complete summary of the entire compaction run (which would likely be missed for long running compactions if logged at TRACE or DEBUG anyways) but it is pretty good information from which the compaction ratio can be extrapolated.
The cardinality of this metric is much too high, and its usefulness does not justify the cost. Aggregating it would also reduce its usefulness. We have plenty of log lines from which one could infer the "compactibility" of a `segment`/`log`/`topic`.
WillemKauf
force-pushed
the
compaction_ratio_metric_agg
branch
from
cf61deb to
88fa106
Compare
WillemKauf
changed the title
storage: aggregate compaction_ratio metric across partitionsstorage: remove compaction_ratio metric from probe
|
Force push to:
|
The cardinality of this metric is much too high, and its usefulness doesnot justify the cost. Aggregating it would also reduce its usefulness.
We have plenty of log lines from which one could infer the "compactibility" of a
segment/log/topic.Backports Required
Release Notes