[CLOB-1014] [CLOB-1004] Unify metrics library, un-modularize metrics keys #792
Conversation
WalkthroughThe codebase underwent a significant refactoring of its metrics system, transitioning from the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- protocol/lib/metrics/constants.go (1 hunks)
- protocol/lib/metrics/lib.go (1 hunks)
- protocol/lib/metrics/metric_keys.go (1 hunks)
- protocol/x/clob/abci.go (3 hunks)
- protocol/x/clob/keeper/deleveraging.go (8 hunks)
Files skipped from review due to trivial changes (2)
- protocol/lib/metrics/constants.go
- protocol/lib/metrics/metric_keys.go
Additional comments: 12
protocol/x/clob/abci.go (3)
4-9: The removal of the import statement for "github.com/armon/go-metrics" indicates that the old metrics library is no longer being used. Ensure that all references to this library have been replaced throughout the entire codebase to avoid compilation errors and to maintain consistency in metrics collection.
62-69: The use of
metrics.IncrCounterWithLabelsto increment a counter for expired stateful orders is a good practice as it provides more detailed metrics with labels. However, ensure that theorderId.GetOrderIdLabels()function is correctly implemented and returns a set of labels that are meaningful for the metric being recorded. Also, verify that the metric keymetrics.ClobExpiredStatefulOrdersis correctly defined in themetric_keys.gofile.107-113: The use of
metrics.SetGaugeto set a gauge for the insurance fund balance is a good practice. It replaces the previous telemetry function and aligns with the new metrics system. Ensure that themetrics.GetMetricValueFromBigIntfunction correctly converts the insurance fund balance to a metric value and that the metric keymetrics.InsuranceFundBalanceis correctly defined in themetric_keys.gofile.protocol/x/clob/keeper/deleveraging.go (9)
3-16: The import section has been reorganized, and it's important to ensure that all the necessary packages are still imported and that there are no unused imports left. This is crucial for maintaining the cleanliness and efficiency of the code. Additionally, the removal of the
armon/go-metricspackage suggests that the codebase has fully transitioned to the new metrics system, so it's important to verify that all previous usages have been properly replaced.40-47: The use of
metrics.IncrCounterto increment a counter when a subaccount cannot be deleveraged is a good practice for monitoring and alerting. However, it's important to ensure that the metric keyClobPrepareCheckStateCannotDeleverageSubaccountis correctly defined in themetric_keys.gofile and that the metric is being used consistently across the codebase.64-70: The creation of labels for metrics is a good practice for providing more context in the reported data. However, it's important to ensure that the labels
PerpetualIdandIsLongare correctly defined and used consistently throughout the codebase. Additionally, the use ofdeltaQuantums.Sign() == -1to determine theIsLonglabel value assumes that a negative sign indicates a long position, which should be verified for correctness.76-83: The use of
metrics.IncrCounterWithLabelsto increment a counter with labels for the deleveraging operation is a good practice. It's important to ensure that the metric keyClobDeleverageSubaccountis correctly defined and that the labels used are consistent with the rest of the codebase. Additionally, the use of...to spread thelabelsslice is correct and idiomatic Go.85-100: The use of
metrics.IncrCounterWithLabelsandmetrics.AddSampleWithLabelsto record the total quote quantums involved in the deleveraging operation is a good practice. However, it's important to ensure that the metric keysClobDeleverageSubaccountTotalQuoteQuantumsandClobDeleverageSubaccountTotalQuoteQuantumsDistributionare correctly defined. Also, verify that the functionmetrics.GetMetricValueFromBigIntcorrectly convertsbig.Intvalues to the expected metric value type.105-113: The calculation of the percent filled for the deleveraging operation and recording it as a distribution using
metrics.AddSampleWithLabelsis a good practice for detailed metrics reporting. Ensure that the metric keyDeleveragingPercentFilledDistributionis correctly defined and that the labels used are consistent with the rest of the codebase.198-206: The use of
defer metrics.ModuleMeasureSinceto measure the time taken by theOffsetSubaccountPerpetualPositionfunction is a good practice for performance monitoring. However, it's important to ensure that the metric keyClobOffsettingSubaccountPerpetualPositionis correctly defined in themetric_keys.gofile and that the time measurement is being used consistently across the codebase.286-305: The recording of various counts related to the deleveraging process using
metrics.AddSampleWithLabelsis a good practice for detailed metrics reporting. Ensure that the metric keysClobDeleveragingNumSubaccountsIteratedCount,ClobDeleveragingNonOverlappingBankrupcyPricesCount, andClobDeleveragingNoOpenPositionOnOppositeSideCountare correctly defined and that the labels used are consistent with the rest of the codebase.422-436: The recording of the quantums deleveraged in quote quantums using
metrics.AddSampleWithLabelsis a good practice for detailed metrics reporting. Ensure that the metric keyClobDeleverageSubaccountFilledQuoteQuantumsis correctly defined and that the labels used are consistent with the rest of the codebase. Additionally, verify that the functionmetrics.GetMetricValueFromBigIntcorrectly convertsbig.Intvalues to the expected metric value type.
protocol/lib/metrics/lib.go
Outdated
| package metrics | ||
|
|
||
| import ( | ||
| "time" | ||
|
|
||
| gometrics "github.com/armon/go-metrics" | ||
| "github.com/cosmos/cosmos-sdk/telemetry" | ||
| ) | ||
|
|
||
| // main entrypoint for logging in the v4 protocol | ||
| // type Label struct { | ||
| // Name string | ||
| // Value string | ||
| // } | ||
|
|
||
| type Label = gometrics.Label | ||
|
|
||
| // IncrCounter provides a wrapper functionality for emitting a counter | ||
| // metric with global labels (if any) along with the provided labels. | ||
| func IncrCounterWithLabels(key string, val float32, labels ...Label) { | ||
| telemetry.IncrCounterWithLabels([]string{key}, val, labels) | ||
| } | ||
|
|
||
| // IncrCounter provides a wrapper functionality for emitting a counter | ||
| // metric with global labels (if any) along with the provided labels. | ||
| func IncrCounter(key string, val float32) { | ||
| telemetry.IncrCounterWithLabels([]string{key}, val, []gometrics.Label{}) | ||
| } | ||
|
|
||
| // Gauge provides a wrapper functionality for emitting a counter | ||
| // metric with global labels (if any) along with the provided labels. | ||
| func SetGaugeWithLabels(key string, val float32, labels ...gometrics.Label) { | ||
| telemetry.SetGaugeWithLabels([]string{key}, val, labels) | ||
| } | ||
|
|
||
| // Gauge provides a wrapper functionality for emitting a counter | ||
| // metric with global labels (if any) along with the provided labels. | ||
| func SetGauge(key string, val float32) { | ||
| telemetry.SetGaugeWithLabels([]string{key}, val, []gometrics.Label{}) | ||
| } | ||
|
|
||
| // Histogram provides a wrapper functionality for emitting a counter | ||
| // metric with global labels (if any) along with the provided labels. | ||
| func AddSampleWithLabels(key string, val float32, labels ...gometrics.Label) { | ||
| // TODO why the f is this a differnet library | ||
| gometrics.AddSampleWithLabels( | ||
| []string{key}, | ||
| val, | ||
| labels, | ||
| ) | ||
| } | ||
|
|
||
| // Histogram provides a wrapper functionality for emitting a counter | ||
| // metric with global labels (if any) along with the provided labels. | ||
| func AddSample(key string, val float32) { | ||
| // TODO why the f is this a differnet library | ||
| gometrics.AddSampleWithLabels( | ||
| []string{key}, | ||
| val, | ||
| []gometrics.Label{}, | ||
| ) | ||
| } | ||
|
|
||
| func ModuleMeasureSince(module string, key string, start time.Time) { | ||
| telemetry.ModuleMeasureSince( | ||
| module, | ||
| start, | ||
| key, | ||
| ) | ||
| } | ||
|
|
||
| // ModuleMeasureSinceWithLabels provides a short hand method for emitting a time measure | ||
| // metric for a module with a given set of keys and labels. | ||
| // NOTE: global labels are not included in this metric. | ||
| func ModuleMeasureSinceWithLabels( | ||
| module string, | ||
| keys []string, | ||
| start time.Time, | ||
| labels []gometrics.Label, | ||
| ) { | ||
| gometrics.MeasureSinceWithLabels( | ||
| keys, | ||
| start.UTC(), | ||
| append( | ||
| []gometrics.Label{telemetry.NewLabel(telemetry.MetricLabelNameModule, module)}, | ||
| labels..., | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
The code introduces a set of wrapper functions for emitting various types of metrics using the telemetry and gometrics libraries. However, there are a few issues and improvements that can be made:
- The comments for the
SetGaugeWithLabels,SetGauge,AddSampleWithLabels, andAddSamplefunctions incorrectly state that they are emitting a counter metric, when they should be emitting gauge and histogram metrics respectively. This could lead to confusion for developers reading the code. - The
TODOcomments on lines 45 and 56 indicate uncertainty about the use of different libraries for theAddSampleWithLabelsandAddSamplefunctions. It's important to resolve theseTODOitems before merging to ensure that there's a clear understanding of why different libraries are used and to maintain consistency across the codebase. - The
ModuleMeasureSinceWithLabelsfunction on lines 72-89 includes a note that global labels are not included in this metric. It's important to ensure that this is the intended behavior and that it's consistent with how other metrics are handled in the system. - The
ModuleMeasureSincefunction on lines 64-70 does not allow for labels, which might limit its usefulness in scenarios where labeled metrics are required. Consider adding an overload or a separate function that supports labels for this metric type.
Here are the proposed changes to address the incorrect comments:
- // Gauge provides a wrapper functionality for emitting a counter
+ // SetGaugeWithLabels provides a wrapper functionality for emitting a gauge
- // Gauge provides a wrapper functionality for emitting a counter
+ // SetGauge provides a wrapper functionality for emitting a gauge
- // Histogram provides a wrapper functionality for emitting a counter
+ // AddSampleWithLabels provides a wrapper functionality for emitting a histogram
- // Histogram provides a wrapper functionality for emitting a counter
+ // AddSample provides a wrapper functionality for emitting a histogramCommitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| package metrics | |
| import ( | |
| "time" | |
| gometrics "github.com/armon/go-metrics" | |
| "github.com/cosmos/cosmos-sdk/telemetry" | |
| ) | |
| // main entrypoint for logging in the v4 protocol | |
| // type Label struct { | |
| // Name string | |
| // Value string | |
| // } | |
| type Label = gometrics.Label | |
| // IncrCounter provides a wrapper functionality for emitting a counter | |
| // metric with global labels (if any) along with the provided labels. | |
| func IncrCounterWithLabels(key string, val float32, labels ...Label) { | |
| telemetry.IncrCounterWithLabels([]string{key}, val, labels) | |
| } | |
| // IncrCounter provides a wrapper functionality for emitting a counter | |
| // metric with global labels (if any) along with the provided labels. | |
| func IncrCounter(key string, val float32) { | |
| telemetry.IncrCounterWithLabels([]string{key}, val, []gometrics.Label{}) | |
| } | |
| // Gauge provides a wrapper functionality for emitting a counter | |
| // metric with global labels (if any) along with the provided labels. | |
| func SetGaugeWithLabels(key string, val float32, labels ...gometrics.Label) { | |
| telemetry.SetGaugeWithLabels([]string{key}, val, labels) | |
| } | |
| // Gauge provides a wrapper functionality for emitting a counter | |
| // metric with global labels (if any) along with the provided labels. | |
| func SetGauge(key string, val float32) { | |
| telemetry.SetGaugeWithLabels([]string{key}, val, []gometrics.Label{}) | |
| } | |
| // Histogram provides a wrapper functionality for emitting a counter | |
| // metric with global labels (if any) along with the provided labels. | |
| func AddSampleWithLabels(key string, val float32, labels ...gometrics.Label) { | |
| // TODO why the f is this a differnet library | |
| gometrics.AddSampleWithLabels( | |
| []string{key}, | |
| val, | |
| labels, | |
| ) | |
| } | |
| // Histogram provides a wrapper functionality for emitting a counter | |
| // metric with global labels (if any) along with the provided labels. | |
| func AddSample(key string, val float32) { | |
| // TODO why the f is this a differnet library | |
| gometrics.AddSampleWithLabels( | |
| []string{key}, | |
| val, | |
| []gometrics.Label{}, | |
| ) | |
| } | |
| func ModuleMeasureSince(module string, key string, start time.Time) { | |
| telemetry.ModuleMeasureSince( | |
| module, | |
| start, | |
| key, | |
| ) | |
| } | |
| // ModuleMeasureSinceWithLabels provides a short hand method for emitting a time measure | |
| // metric for a module with a given set of keys and labels. | |
| // NOTE: global labels are not included in this metric. | |
| func ModuleMeasureSinceWithLabels( | |
| module string, | |
| keys []string, | |
| start time.Time, | |
| labels []gometrics.Label, | |
| ) { | |
| gometrics.MeasureSinceWithLabels( | |
| keys, | |
| start.UTC(), | |
| append( | |
| []gometrics.Label{telemetry.NewLabel(telemetry.MetricLabelNameModule, module)}, | |
| labels..., | |
| ), | |
| ) | |
| } | |
| package metrics | |
| import ( | |
| "time" | |
| gometrics "github.com/armon/go-metrics" | |
| "github.com/cosmos/cosmos-sdk/telemetry" | |
| ) | |
| // main entrypoint for logging in the v4 protocol | |
| // type Label struct { | |
| // Name string | |
| // Value string | |
| // } | |
| type Label = gometrics.Label | |
| // IncrCounter provides a wrapper functionality for emitting a counter | |
| // metric with global labels (if any) along with the provided labels. | |
| func IncrCounterWithLabels(key string, val float32, labels ...Label) { | |
| telemetry.IncrCounterWithLabels([]string{key}, val, labels) | |
| } | |
| // IncrCounter provides a wrapper functionality for emitting a counter | |
| // metric with global labels (if any) along with the provided labels. | |
| func IncrCounter(key string, val float32) { | |
| telemetry.IncrCounterWithLabels([]string{key}, val, []gometrics.Label{}) | |
| } | |
| // SetGaugeWithLabels provides a wrapper functionality for emitting a gauge | |
| // metric with global labels (if any) along with the provided labels. | |
| func SetGaugeWithLabels(key string, val float32, labels ...gometrics.Label) { | |
| telemetry.SetGaugeWithLabels([]string{key}, val, labels) | |
| } | |
| // SetGauge provides a wrapper functionality for emitting a gauge | |
| // metric with global labels (if any) along with the provided labels. | |
| func SetGauge(key string, val float32) { | |
| telemetry.SetGaugeWithLabels([]string{key}, val, []gometrics.Label{}) | |
| } | |
| // AddSampleWithLabels provides a wrapper functionality for emitting a histogram | |
| // metric with global labels (if any) along with the provided labels. | |
| func AddSampleWithLabels(key string, val float32, labels ...gometrics.Label) { | |
| // TODO why the f is this a differnet library | |
| gometrics.AddSampleWithLabels( | |
| []string{key}, | |
| val, | |
| labels, | |
| ) | |
| } | |
| // AddSample provides a wrapper functionality for emitting a histogram | |
| // metric with global labels (if any) along with the provided labels. | |
| func AddSample(key string, val float32) { | |
| // TODO why the f is this a differnet library | |
| gometrics.AddSampleWithLabels( | |
| []string{key}, | |
| val, | |
| []gometrics.Label{}, | |
| ) | |
| } | |
| func ModuleMeasureSince(module string, key string, start time.Time) { | |
| telemetry.ModuleMeasureSince( | |
| module, | |
| start, | |
| key, | |
| ) | |
| } | |
| // ModuleMeasureSinceWithLabels provides a short hand method for emitting a time measure | |
| // metric for a module with a given set of keys and labels. | |
| // NOTE: global labels are not included in this metric. | |
| func ModuleMeasureSinceWithLabels( | |
| module string, | |
| keys []string, | |
| start time.Time, | |
| labels []gometrics.Label, | |
| ) { | |
| gometrics.MeasureSinceWithLabels( | |
| keys, | |
| start.UTC(), | |
| append( | |
| []gometrics.Label{telemetry.NewLabel(telemetry.MetricLabelNameModule, module)}, | |
| labels..., | |
| ), | |
| ) | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/lib/metrics/metric_keys.go (1 hunks)
- protocol/lib/metrics/util.go (2 hunks)
- protocol/x/clob/keeper/liquidations.go (6 hunks)
- protocol/x/clob/keeper/mev.go (9 hunks)
Additional comments: 16
protocol/lib/metrics/util.go (1)
- 3-8: The removal of the
timepackage import suggests that theModuleMeasureSinceWithLabelsfunction, which likely required time measurements, has been removed. Verify that this function is not used elsewhere in the codebase, or if it is, ensure that all references to it have been updated or replaced with an alternative.protocol/lib/metrics/metric_keys.go (1)
- 20-20: The
InsuranceFundBalancegauge has been mentioned as removed in the summary, but it is still present in the code. If it is indeed removed, this line should be deleted.- InsuranceFundBalance = "insurance_fund_balance"protocol/x/clob/keeper/liquidations.go (5)
27-33: The use of
metrics.AddSampleis correct for recording a metric sample. However, ensure that the metric keymetrics.LiquidationsLiquidatableSubaccountIdsCountis correctly defined in themetricspackage and that the metric system is properly initialized to handle this key.279-285: The labels for metrics are being set correctly. However, ensure that the
metrics.GetLabelForIntValueandmetrics.GetLabelForStringValuefunctions are properly implemented in themetricspackage and that they return the expected label format.293-301: The calculation of
percentFilledand the use ofmetrics.AddSampleWithLabelsare correct. Ensure that the metric keymetrics.LiquidationsPercentFilledDistributionis correctly defined in themetricspackage and that the metric system is properly initialized to handle this key with labels.320-334: The incrementing of the counter and adding a sample with labels are done correctly. However, ensure that the metric keys
metrics.LiqidationsPlacePerpetualLiquidationQuoteQuantumsandmetrics.LiquidationsPlacePerpetualLiquidationQuoteQuantumsDistributionare correctly defined in themetricspackage and that the metric system is properly initialized to handle these keys with labels.578-594: The incrementing of the counter with labels is done correctly. However, ensure that the metric key
metrics.LiquidationsLiquidationMatchNegativeTNCis correctly defined in themetricspackage and that the metric system is properly initialized to handle this key with labels. Also, ensure that thecallbackvariable is set to the correct value depending on the transaction type.protocol/x/clob/keeper/mev.go (9)
6-11: The removal of the
github.com/armon/go-metricsandgithub.com/cosmos/cosmos-sdk/telemetryimports is consistent with the change summary, which indicates a shift to a newmetricspackage. Ensure that no other parts of the codebase rely on these removed packages to avoid breaking changes.58-65: The
metrics.ModuleMeasureSincefunction is used to measure the time since a particular event, which is a new addition as per the change summary. It's important to ensure that themetricspackage'sModuleMeasureSincefunction is implemented correctly and that it provides the same or improved functionality compared to the deprecatedtelemetry.ModuleMeasureSince.101-107: The
metrics.IncrCounterfunction is used to increment a counter metric. This is part of the newmetricspackage and replaces the previous telemetry functionality. Ensure that themetrics.ClobMevErrorCountconstant is correctly defined in themetricspackage and that the counter increment behavior is as expected.119-125: The use of
metrics.IncrCounterhere is consistent with the new metrics collection strategy. Verify that the error handling logic is correctly capturing all relevant errors and that the counter is incremented appropriately in case of an error.140-146: Again,
metrics.IncrCounteris used for error handling. It's important to verify that the error handling is comprehensive and that the metric keymetrics.ClobMevErrorCountis being used consistently across the codebase for similar error scenarios.158-164: The
metrics.IncrCounterfunction is used for error handling. Ensure that the error handling is consistent and that the metric keymetrics.ClobMevErrorCountis being used correctly. It's also important to verify that the counter is incremented as expected in case of an error.234-241: The error handling for the failure to get the consensus round is correct, and the use of
metrics.IncrCounteris consistent with the new metrics collection strategy. Verify that the consensus round is a necessary metric and that the error handling is appropriate for this context.250-257: The error handling for the failure to get the proposer by consensus address is correct, and the use of
metrics.IncrCounteris consistent with the new metrics collection strategy. Verify that the proposer's consensus address is a necessary metric and that the error handling is appropriate for this context.300-314: The
metrics.SetGaugeWithLabelsfunction is used to set a gauge metric with labels. This is part of the newmetricspackage and replaces the previous telemetry functionality. Ensure that the metric keys and labels are correctly defined in themetricspackage and that the gauge is set as expected.
protocol/lib/metrics/metric_keys.go
Outdated
| ClobDeleveragingNoOpenPositionOnOppositeSideCount = "clob_deleveraging_no_open_position_on_opposite_side_count" | ||
| ClobDeleverageSubaccountFilledQuoteQuantums = "clob_deleverage_subaccount_filled_quote_quantums" | ||
| LiquidationsLiquidatableSubaccountIdsCount = "liquidations_liquidatable_subaccount_ids_count" | ||
| LiquidationsPercentFilledDistribution = "liquidations_percent_filled_distribbution" |
There was a problem hiding this comment.
There is a typo in the constant value liquidations_percent_filled_distribbution. It should be liquidations_percent_filled_distribution.
- LiquidationsPercentFilledDistribution = "liquidations_percent_filled_distribbution"
+ LiquidationsPercentFilledDistribution = "liquidations_percent_filled_distribution"Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| LiquidationsPercentFilledDistribution = "liquidations_percent_filled_distribbution" | |
| LiquidationsPercentFilledDistribution = "liquidations_percent_filled_distribution" |
protocol/lib/metrics/metric_keys.go
Outdated
| const ( | ||
| // Clob metric keys | ||
| // Currently Sorted by type of metric. | ||
| // TODO make sure the metric type is clear. |
There was a problem hiding this comment.
The TODO comment suggests that the metric type should be made clear. This should be addressed before finalizing the code to ensure that the metric types are indeed clear and understandable.
CLOB-1002 Enforce a specific logging library for all logs on the chain
Right now we have a few different logging functions, and it doesn't look like we've standardized our logging practices. It would be great to use a specific library for all logging needs, and find a way to enforce it across the stack |
jonfung-dydx
changed the title
jonfung-dydx
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/lib/metrics/lib.go (1 hunks)
- protocol/lib/metrics/metric_keys.go (1 hunks)
- protocol/x/clob/abci.go (3 hunks)
- protocol/x/clob/types/order_id.go (2 hunks)
Files skipped from review due to trivial changes (2)
- protocol/lib/metrics/metric_keys.go
- protocol/x/clob/abci.go
Additional comments: 3
protocol/x/clob/types/order_id.go (3)
6-10: The removal of the
github.com/armon/go-metricsimport aligns with the summary and the refactoring goal to consolidate metrics reporting.176-180: The change in the return type of the
GetOrderIdLabelsmethod from[]gometrics.Labelto[]metrics.Labelis consistent with the summary and the overall goal of standardizing the metrics system.176-180: Verify that all usages of the
GetOrderIdLabelsmethod have been updated to handle the new return type[]metrics.Label.
| // metric for a module with labels. Global labels are not included | ||
| // in this metric. | ||
| // Please try to use `AddSampleWithLabels` instead. | ||
| func ModuleMeasureSinceWithLabels( |
There was a problem hiding this comment.
This comment is a bit confusing to me, since AddSampleWithLabels is not time related. Are you saying that its preferred to pass the calculating timing into AddSampleWithLabels instead of using this?
Also, AddSampleWithLabels also uses gometrics, so wouldn't that have missing global labels too?
There was a problem hiding this comment.
Hmm i'm saying it's preferred to manually roll your own timing calculation instead of using this function.
Thinking about it more I can modify this later to add our own MeasureSince method that uses our entrypoint method under the hood.
IMO We should not be relying on specific metric logging library functionality such as this. It should be easy to swap out metrics for our system under the hood.
There was a problem hiding this comment.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/lib/metrics/lib.go (1 hunks)
- protocol/lib/metrics/metric_keys.go (1 hunks)
Additional comments: 2
protocol/lib/metrics/lib.go (1)
- 1-89: The code provided does not seem to reflect the PR objective of unifying metrics libraries and standardizing metric keys. The code still shows usage of both
gometricsandcosmos-sdk/telemetry, and there is no evidence of a singular entry point method for reporting metrics or new standardized metric keys. Please ensure that the code changes align with the PR objective and that the full scope of the refactoring is represented in the code provided.protocol/lib/metrics/metric_keys.go (1)
- 3-41: The metric keys are well-documented and follow the guidelines provided in the comments. The constants are grouped by type, which seems to be a deliberate choice to aid in the migration process as mentioned in previous comments. This organization may also help in maintaining clarity as the codebase evolves.
| ClobMevErrorCount = "clob_mev_error_count" | ||
|
|
||
| // Gauges | ||
| InsuranceFundBalance = "insurance_fund_balance" |
There was a problem hiding this comment.
The PR summary mentions the removal of outdated metrics code and dependencies, including the global variable "InsuranceFundBalance". However, this constant is still present in the code. Please verify if this is intentional or an oversight.
- InsuranceFundBalance = "insurance_fund_balance"Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| InsuranceFundBalance = "insurance_fund_balance" |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/keeper/liquidations.go (6 hunks)
Additional comments: 6
protocol/x/clob/keeper/liquidations.go (6)
30-33: The usage of
metrics.AddSampleto record the count of liquidatable subaccount IDs aligns with the PR objective of unifying metrics reporting.282-284: The usage of
metrics.Labelis correct and aligns with the PR objective of unifying metrics reporting.297-301: The usage of
metrics.AddSampleWithLabelsto record the percent filled of the liquidation is correct and aligns with the PR objective of unifying metrics reporting.323-327: The usage of
metrics.IncrCounterWithLabelsto record the volume of liquidation orders placed is correct and aligns with the PR objective of unifying metrics reporting.329-333: The usage of
metrics.AddSampleWithLabelsto record the distribution of liquidation orders placed is correct and aligns with the PR objective of unifying metrics reporting.582-593: The usage of
metrics.IncrCounterWithLabelsto record the negative TNC during liquidation matches is correct and aligns with the PR objective of unifying metrics reporting.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/lib/metrics/lib.go (1 hunks)
Additional comments: 2
protocol/lib/metrics/lib.go (2)
73-91: The summary states that the
ModuleMeasureSinceWithLabelsfunction has been removed, but it is still present in the hunk. Please verify if this function should be removed as part of the PR's objective to standardize metric reporting.40-58: The
AddSampleWithLabelsandAddSamplefunctions are still using thegometricspackage, which is inconsistent with the PR's objective to standardize metric reporting through thetelemetrypackage. Please verify if these functions should be updated to use thetelemetrypackage instead.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/lib/metrics/metric_keys.go (1 hunks)
- protocol/x/clob/keeper/deleveraging.go (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/lib/metrics/metric_keys.go
Additional comments: 8
protocol/x/clob/keeper/deleveraging.go (8)
3-16: The import section has been correctly updated to reflect the new metrics library usage, which is in line with the PR objectives.
42-47: The usage of
metrics.IncrCounterwith the new metric keymetrics.ClobPrepareCheckStateCannotDeleverageSubaccountis consistent with the PR's objective to standardize metric reporting.67-70: The creation of labels using
metrics.GetLabelForIntValueandmetrics.GetLabelForBoolValueis consistent with the PR's objective to standardize metric reporting.79-83: The use of
metrics.IncrCounterWithLabelsto record the status of the deleveraging operation with appropriate labels is consistent with the PR's objective to standardize metric reporting.109-113: The use of
metrics.AddSampleWithLabelsto record the percent filled of the deleveraging operation as a distribution is consistent with the PR's objective to standardize metric reporting.201-205: The removal of
ModuleMeasureSinceis consistent with the PR's objective to standardize metric reporting and remove un-modularized metric keys and functions.289-305: The use of
metrics.AddSampleWithLabelsto record various counts related to the deleveraging process is consistent with the PR's objective to standardize metric reporting.430-435: The use of
metrics.AddSampleWithLabelsto record the quantums deleveraged in quote quantums is consistent with the PR's objective to standardize metric reporting.
Provide a standard for metrics going forward.
First step to do this is to create a singular entrypoint method that we report metrics through.
We currently have multiple different ways we report telemetry, using these two libraries across all files in the codebase:
Barrel everything through metric functions in
metrics/lib.go.Files should be migrated one by one, so here's a start with migrating a few libraries.
There should be no change in how metrics are reported.
After everything goes through the singular entrypoint method, we can look at unifying the metrics libraries.
Also, un-modularize the metrics keys instead.
instead of passing in
[]string{"clob", "place_order", "count"}, we should just pass inmetrics.ClobPlaceOrderCountinstead. This will make our metrics and the naming system a lot more clear because of the 1:1 metric:name:dashboard ratio.Other files will need to be modified too later.