-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add partition length metrics to Olric #3053
Conversation
WalkthroughThe recent changes involve enhancing the distributed cache metrics by introducing two new gauge metrics: one for the length of cache partitions and another for the length of backup cache partitions. These metrics are added to the Changes
Poem
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- pkg/dist-cache/dist-cache-metrics.go (3 hunks)
- pkg/dist-cache/dist-cache.go (1 hunks)
- pkg/metrics/schema.go (1 hunks)
Files skipped from review due to trivial changes (1)
- pkg/metrics/schema.go
Additional comments: 8
pkg/dist-cache/dist-cache-metrics.go (4)
22-23: The addition of
PartitionsLength
andBackupPartitionsLength
to theDistCacheMetrics
struct aligns with the PR's objective to introduce new metrics for tracking the number of keys and their backups across partitions. This change appears to be correctly implemented.69-76: The initialization of
PartitionsLength
andBackupPartitionsLength
in thenewDistCacheMetrics
function is consistent with the initialization of other metrics in this function. The metric names and help descriptions are set appropriately.92-93: The update to the
allMetrics
method to includePartitionsLength
andBackupPartitionsLength
ensures that these new metrics will be collected and exposed by Prometheus. This change is correctly implemented.66-79: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [19-95]
It is recommended to test the new metrics in a playground or test environment to ensure they are being collected and reported as expected. This would involve deploying the changes and verifying the metrics through Prometheus queries.
pkg/dist-cache/dist-cache.go (4)
- 151-171: The implementation of the new metrics within the
scrapeMetrics
method appears to correctly accumulate the lengths of partitions and backup partitions and sets the gauge metrics accordingly. However, there are a few points to consider:
Error Handling: The error handling for the metric retrieval or creation logs a debug message and continues. This is consistent with the existing pattern in the method. However, it might be beneficial to consider if this is the desired behavior or if any additional action should be taken upon failure to retrieve or create metrics.
Performance: The loop to calculate the lengths of partitions and backup partitions is straightforward and should perform well given that the number of partitions is not excessively large. If the number of partitions could be large, it might be worth considering if there's a more efficient way to retrieve these statistics from Olric directly.
Best Practices: The code is clear and follows the existing structure of the
scrapeMetrics
method. It's good to see that the new metrics are consistent with the existing metrics in terms of naming and handling.Overall, the changes seem to align with the PR objectives and are implemented correctly. It's important to ensure that the new metrics are documented and that any necessary updates to related documentation or dashboards are made to reflect these new metrics.
151-171: Please ensure that the new metrics are thoroughly tested in a development or staging environment to verify their correctness and to ensure that they do not introduce any unexpected behavior.
151-171: Consider if there are any concurrency concerns with the new metrics. Since
scrapeMetrics
may be called concurrently, ensure that the metric updates are thread-safe. The Prometheus client library typically handles concurrency internally, but it's good to confirm that this is the case for the used metric types.151-171: Please verify that the
DistCacheMetrics
struct and thenewDistCacheMetrics
andallMetrics
methods withindist-cache-metrics.go
have been updated to include and initialize the new metrics as described in the PR objectives and AI-generated summaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- pkg/metrics/schema.go (1 hunks)
Files skipped from review due to trivial changes (1)
- pkg/metrics/schema.go
Description of change
This can be used to track number of keys and their backups in Olric.
Checklist
Summary by CodeRabbit
New Features
Refactor