[CLOB-1014] [CLOB-1004] Unify metrics library, un-modularize metrics keys #792
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| package metrics | ||
|
|
||
| import ( | ||
| "time" | ||
|
|
||
| gometrics "github.com/armon/go-metrics" | ||
| "github.com/cosmos/cosmos-sdk/telemetry" | ||
| ) | ||
|
|
||
| // This file provides a main entrypoint for logging in the v4 protocol. | ||
| // TODO(CLOB-1013) Drop both metrics libraries above for a library | ||
| // that supports float64 (i.e hashicorp go-metrics) | ||
|
|
||
| type Label = gometrics.Label | ||
|
|
||
| // IncrCounterWithLabels 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). | ||
| 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). | ||
| func SetGauge(key string, val float32) { | ||
| telemetry.SetGaugeWithLabels([]string{key}, val, []gometrics.Label{}) | ||
| } | ||
|
|
||
| // AddSampleWithLabels provides a wrapper functionality for emitting a sample | ||
| // metric with the provided labels. | ||
| func AddSampleWithLabels(key string, val float32, labels ...gometrics.Label) { | ||
| gometrics.AddSampleWithLabels( | ||
| []string{key}, | ||
| val, | ||
| labels, | ||
| ) | ||
| } | ||
|
|
||
| // AddSample provides a wrapper functionality for emitting a sample | ||
| // metric. | ||
| func AddSample(key string, val float32) { | ||
| gometrics.AddSampleWithLabels( | ||
| []string{key}, | ||
| val, | ||
| []gometrics.Label{}, | ||
| ) | ||
| } | ||
|
|
||
| // ModuleMeasureSince provides a wrapper functionality for emitting a time measure | ||
| // metric with global labels (if any). | ||
| // Please try to use `AddSample` instead. | ||
| // TODO(CLOB-1022) Roll our own calculations for timing on top of AddSample instead | ||
| // of using MeasureSince. | ||
| 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 labels. Global labels are not included in this metric. | ||
| // Please try to use `AddSampleWithLabels` instead. | ||
| // TODO(CLOB-1022) Roll our own calculations for timing on top of AddSample instead | ||
| // of using MeasureSince. | ||
| 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..., | ||
| ), | ||
| ) | ||
jiajames marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,42 @@ | ||||
| // nolint:lll | ||||
| package metrics | ||||
|
|
||||
| // Metrics Keys Guidelines | ||||
| // 1. Be wary of length | ||||
| // 2. Prefix by module | ||||
| // 3. Suffix keys with a unit of measurement | ||||
| // 4. Delimit with '_' | ||||
| // 5. Information such as callback type should be added as tags, not in key names. | ||||
jiajames marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
| // Example: clob_place_order_count, clob_msg_place_order_latency_ms, clob_operations_queue_length | ||||
| // clob_expired_stateful_orders_count, clob_processed_orders_ms_total | ||||
|
|
||||
| // Clob Metrics Keys | ||||
| const ( | ||||
| // Stats | ||||
jiajames marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
| ClobExpiredStatefulOrders = "clob_expired_stateful_order_removed" | ||||
| ClobPrepareCheckStateCannotDeleverageSubaccount = "clob_prepare_check_state_cannot_deleverage_subaccount" | ||||
| ClobDeleverageSubaccountTotalQuoteQuantums = "clob_deleverage_subaccount_total_quote_quantums" | ||||
| ClobDeleverageSubaccount = "clob_deleverage_subaccount" | ||||
| LiquidationsPlacePerpetualLiquidationQuoteQuantums = "liquidations_place_perpetual_liquidation_quote_quantums" | ||||
| LiquidationsLiquidationMatchNegativeTNC = "liquidations_liquidation_match_negative_tnc" | ||||
| 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
Suggested change
|
||||
| ClobMev = "clob_mev" | ||||
|
|
||||
| // Samples | ||||
| ClobDeleverageSubaccountTotalQuoteQuantumsDistribution = "clob_deleverage_subaccount_total_quote_quantums_distribution" | ||||
| DeleveragingPercentFilledDistribution = "deleveraging_percent_filled_distribution" | ||||
| ClobDeleveragingNumSubaccountsIteratedCount = "clob_deleveraging_num_subaccounts_iterated_count" | ||||
| ClobDeleveragingNonOverlappingBankrupcyPricesCount = "clob_deleveraging_non_overlapping_bankruptcy_prices_count" | ||||
| 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_distribution" | ||||
| LiquidationsPlacePerpetualLiquidationQuoteQuantumsDistribution = "liquidations_place_perpetual_liquidation_quote_quantums_distribution" | ||||
|
|
||||
| // Measure Since | ||||
| ClobOffsettingSubaccountPerpetualPosition = "clob_offsetting_subaccount_perpetual_position" | ||||
| MevLatency = "mev_latency" | ||||
| ) | ||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This comment is a bit confusing to me, since
AddSampleWithLabelsis not time related. Are you saying that its preferred to pass the calculating timing intoAddSampleWithLabelsinstead of using this?Also,
AddSampleWithLabelsalso usesgometrics, so wouldn't that have missing global labels too?Uh oh!
There was an error while loading. Please reload this page.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://linear.app/dydx/issue/CLOB-1022/%5Bp2%5D-create-our-own-measuresince-method-in-our-metrics-library
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.
nit: add TODO to code 😄