feat(store/v2): basic metrics #18529
Conversation
WalkthroughThe changes introduce a new Changes
Assessment against linked issues
TipsChat with CodeRabbit Bot (
|
|
@alexanderbez your pull request is missing a changelog! |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- store/metrics/metrics.go (1 hunks)
- store/root/store.go (8 hunks)
- store/root/store_test.go (2 hunks)
Additional comments: 10
store/root/store_test.go (2)
11-16: The import statement for the
metricspackage has been added correctly to support the new metrics functionality in the test suite. This change aligns with the pull request's goal of integrating metrics into theRootStore.33-39: The
RootStoreis now being initialized with ametrics.NoOpMetrics{}instance. This is a good practice for tests where metrics collection is not needed, as it avoids the overhead of the metrics collection process. However, ensure that theNewfunction's signature has been updated to accept this new parameter and that all calls toNewthroughout the codebase have been updated accordingly.store/root/store.go (8)
12-17: The import statements are correctly updated to include the new
metricspackage. This is necessary for the new functionality being added to theRootStore.58-64: The addition of the
telemetryfield to theStorestruct is consistent with the goal of integrating metrics into theRootStore. However, ensure that all instances ofStoreare now initialized with a validmetrics.StoreMetricsimplementation to avoid nil pointer dereferences when callings.telemetry.MeasureSince.58-74: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [66-88]
The
Newfunction has been correctly updated to accept ametrics.StoreMetricsparameter and initialize thetelemetryfield of theStorestruct. This change is necessary for the new metrics functionality.
166-171: The use of
defer s.telemetry.MeasureSince("root_store", "query")is appropriate for measuring the duration of theQuerymethod. However, ensure that theMeasureSincemethod is designed to handle cases where thetelemetryfield might be aNoOpMetricsinstance, which should result in a no-op.212-217: The
LoadLatestVersionmethod now includes a call tos.telemetry.MeasureSinceto track the time taken by the method. This is consistent with the goal of adding metrics to the store operations.223-226: The
LoadVersionmethod is correctly instrumented with a call tos.telemetry.MeasureSinceto measure its execution time. This is in line with the changes made to other methods for metrics collection.290-297: The
WorkingHashmethod is instrumented with a call tos.telemetry.MeasureSinceto measure its execution time. This is in line with the changes made to other methods for metrics collection.319-323: The
Commitmethod is instrumented with a call tos.telemetry.MeasureSinceto measure its execution time. Ensure that theworkingHashis indeed set before callingCommitto avoid runtime errors due to the precondition check.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- store/root/store.go (6 hunks)
Additional comments: 6
store/root/store.go (6)
163-168: The
Queryfunction has been correctly updated to include thetelemetry.MeasureSincecall for metrics collection. This is a good use of deferred execution to ensure that the measurement is taken regardless of where the function returns.209-214: The
LoadLatestVersionfunction has been correctly updated to include thetelemetry.MeasureSincecall for metrics collection. This is consistent with the changes made to theQueryfunction and follows the same pattern of using deferred execution.220-223: The
LoadVersionfunction has been correctly updated to include thetelemetry.MeasureSincecall for metrics collection. This is consistent with the changes made to the other functions and follows the same pattern of using deferred execution.289-294: The
WorkingHashfunction has been correctly updated to include thetelemetry.MeasureSincecall for metrics collection. This is consistent with the changes made to the other functions and follows the same pattern of using deferred execution.315-320: The
Commitfunction has been correctly updated to include thetelemetry.MeasureSincecall for metrics collection. This is consistent with the changes made to the other functions and follows the same pattern of using deferred execution.292-294: The
WorkingHashfunction checks ifs.workingHashisniland if so, it callss.writeSC(). However, there is a potential issue here. Ifs.writeSC()fails, thes.workingHashwill remainnil, which could lead to unexpected behavior on subsequent calls toWorkingHashorCommit. It would be safer to ensure thats.workingHashis only set ifs.writeSC()succeeds.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (3)
- store/metrics/metrics.go (1 hunks)
- store/root/store.go (8 hunks)
- store/root/store_test.go (1 hunks)
Additional comments: 12
store/metrics/metrics.go (3)
11-14: The
StoreMetricsinterface is well-defined and follows the principle of least privilege by exposing only the necessary methodMeasureSince.38-42: Ensure that the underlying
metrics.MeasureSinceWithLabelsfunction can handle emptykeyswithout issues.21-35: Returning
Metricsby value inNewMetricsis acceptable as it contains only a slice, which is a reference type.store/root/store.go (9)
5-8: The import of the "time" package is appropriate for the telemetry functionality added.
63-64: The addition of the "telemetry" field to the "Store" struct aligns with the pull request's goal of adding metrics.
67-73: The updated "New" function signature correctly includes the "metrics.StoreMetrics" parameter.
167-170: Telemetry measurements have been correctly added to the "Query" method.
216-219: Telemetry measurements have been correctly added to the "LoadLatestVersion" method.
230-233: Telemetry measurements have been correctly added to the "LoadVersion" method.
302-305: Telemetry measurements have been correctly added to the "WorkingHash" method.
331-334: Telemetry measurements have been correctly added to the "Commit" method.
329-339: The documentation for the "Commit" function has been updated to reflect the requirement of calling "WorkingHash" before "Commit", addressing the previous comment.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- store/metrics/metrics.go (1 hunks)
- store/root/store.go (8 hunks)
- store/root/store_test.go (1 hunks)
Additional comments: 8
store/root/store.go (8)
5-8: The addition of the "time" package is appropriate for the telemetry functionality added to various methods.
59-64: The new "telemetry" field in the "Store" struct aligns with the changes outlined in the summary.
67-72: The updated function signature of "New" to include "metrics.StoreMetrics" is consistent with the changes described.
171-174: The explicit nil check for the "telemetry" field before calling "MeasureSince" is consistent with the user's preference to avoid unnecessary operations.
220-223: The explicit nil check for the "telemetry" field before calling "MeasureSince" in "LoadLatestVersion" is consistent with the user's preference.
234-237: The explicit nil check for the "telemetry" field before calling "MeasureSince" in "LoadVersion" is consistent with the user's preference.
306-309: The explicit nil check for the "telemetry" field before calling "MeasureSince" in "WorkingHash" is consistent with the user's preference.
335-338: The explicit nil check for the "telemetry" field before calling "MeasureSince" in "Commit" is consistent with the user's preference.
Description
Closes: #18463
Measure time of core operations in
RootStore.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
Enhancements
Tests