feat(general): add stats to maintenance run - CompactSingleEpoch #4941
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4941 +/- ##
==========================================
+ Coverage 75.86% 78.00% +2.14%
==========================================
Files 470 542 +72
Lines 37301 31309 -5992
==========================================
- Hits 28299 24424 -3875
+ Misses 7071 4843 -2228
- Partials 1931 2042 +111 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds observability statistics for the CompactSingleEpoch maintenance task to help users understand repository behavior and troubleshoot maintenance operations. It introduces a new stats structure to track compaction metrics including blob count, size, and epoch information.
Key changes:
- Creates a new
CompactSingleEpochStatsstruct to capture compaction metrics - Updates the
MaybeCompactSingleEpochmethod to return statistics instead of just an error - Adds comprehensive test coverage for the new stats functionality
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
repo/maintenancestats/stats_compact_single_epoch.go |
New stats struct with JSON serialization and human-readable summary methods |
repo/maintenancestats/builder_test.go |
Test cases for stats serialization/deserialization |
repo/maintenancestats/builder.go |
Adds support for the new stats type in the builder pattern |
repo/maintenance/maintenance_run.go |
Updates maintenance runners to capture and handle returned stats |
internal/epoch/epoch_manager_test.go |
Updates existing tests to handle new return signature and validate stats |
internal/epoch/epoch_manager.go |
Core implementation changes to calculate and return compaction statistics |
| } | ||
|
|
||
| result.CurrentEpoch++ | ||
| result.CurrentEpoch = cs.WriteEpoch + 1 |
There was a problem hiding this comment.
This change appears unrelated to the CompactSingleEpoch stats feature. The original result.CurrentEpoch++ was incrementing from the current value, but now it's being set to cs.WriteEpoch + 1 which could produce different behavior. This change should either be reverted or moved to a separate PR with proper justification.
| result.CurrentEpoch = cs.WriteEpoch + 1 | |
| result.CurrentEpoch++ |
There was a problem hiding this comment.
Copilot's comment makes sense, it is orthogonal to the changes in this PR.
However, we'll merge it with this PR to avoid an additional roundtrip.
There was a problem hiding this comment.
Yes, this is irrelevant to the current changes. This is for the comment #4937 (comment) of another PR.
It should be included, but it would be better if it was in a separate PR. Will do it next time. And since the PR has been merged, we don't need to do any further thing.
| Epoch: uncompacted, | ||
| } | ||
|
|
||
| contentlog.Log1(ctx, e.log, "starting single-epoch compaction for epoch", result) |
There was a problem hiding this comment.
The logging call is passing result (a *CompactSingleEpochStats struct) as the log parameter, but the original code passed logparam.Int(\"uncompacted\", uncompacted). This will likely not log the epoch number properly. Consider changing to logparam.Int(\"uncompacted\", uncompacted) or implement a proper logging format for the stats struct.
| contentlog.Log1(ctx, e.log, "starting single-epoch compaction for epoch", result) | |
| contentlog.Log1( | |
| ctx, | |
| e.log, | |
| "starting single-epoch compaction for epoch", | |
| logparam.Int("epoch", result.Epoch), | |
| logparam.Int("compactedBlobCount", result.CompactedBlobCount), | |
| logparam.Int64("compactedBlobSize", result.CompactedBlobSize), | |
| ) |
There was a problem hiding this comment.
There is no information lose -- all the information has been included in result.
julio-lopez
left a comment
There was a problem hiding this comment.
Let's merge and iterate.
A followup PR will fix the field names.
Maintenance is critical for healthy of the repository.
On the other hand, Maintenance is complex, because it runs multiple sub tasks each may generate different results according to the maintenance policy. The results may include deleting/combining/adding data/metadata to the repository.
It is worthy to add more observability for these tasks for below reasons:
There will be a serial of PRs to add observability info for each sub task.
The current PR add the stats info for CompactSingleEpoch sub task.