[CORE-354] - Add tracking for market availability in order to alert in datadog #706
Conversation
CORE-354 monitor unavailable symbols from exchanges
once this is done, we can reduce sensitivity of see thread |
WalkthroughThe changes introduce telemetry tracking for market availability on exchanges in the price fetcher daemon. A new function 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: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/daemons/pricefeed/client/price_fetcher/price_fetcher.go (4 hunks)
- protocol/lib/metrics/constants.go (1 hunks)
Files skipped from review due to trivial changes (2)
- protocol/daemons/pricefeed/client/price_fetcher/price_fetcher.go
- protocol/lib/metrics/constants.go
protocol/daemons/pricefeed/client/price_fetcher/price_fetcher.go
Outdated
Show resolved
Hide resolved
| pf.writeToBufferedChannel(exchangeId, price, err) | ||
| } | ||
|
|
||
| // Emit metrics on this exchange's market availability. | ||
| for marketId, available := range availableMarkets { |
There was a problem hiding this comment.
For my understanding what is the cadence of this logic? Is it per runSubTask run? How often is it?
There was a problem hiding this comment.
It's run once every time a price fetcher makes a query.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/daemons/pricefeed/client/price_fetcher/price_fetcher.go (4} hunks)
Additional comments: 4
protocol/daemons/pricefeed/client/price_fetcher/price_fetcher.go (4)
- 3-9: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [6-10]
The import of the
telemetrypackage is new. Ensure that this package is installed and available in the project dependencies.
187-206: The
emitMarketAvailabilityMetricsfunction is a new addition. It emits telemetry data about the availability of a market on an exchange. The function takes an exchange ID, market ID, and a boolean indicating availability as parameters. It uses thetelemetry.IncrCounterWithLabelsfunction to increment a counter with specific labels. This function seems to be well-structured and follows good practices.264-277: The code now emits telemetry data when a query fails. It iterates over all market IDs and calls
emitMarketAvailabilityMetricswithfalsefor availability. This is a good practice as it provides visibility into market availability during failures. However, ensure that theemitMarketAvailabilityMetricsfunction handles errors properly and does not cause therunSubTaskfunction to fail.306-315: The code now tracks which markets were available when queried. It uses a map to store the availability of each market and emits telemetry data for each market. This is a good practice as it provides visibility into market availability. However, ensure that the
emitMarketAvailabilityMetricsfunction handles errors properly and does not cause therunSubTaskfunction to fail.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/daemons/pricefeed/client/price_fetcher/price_fetcher.go (4} hunks)
- protocol/lib/metrics/constants.go (2} hunks)
Files skipped from review due to trivial changes (1)
- protocol/lib/metrics/constants.go
Additional comments: 4
protocol/daemons/pricefeed/client/price_fetcher/price_fetcher.go (4)
6-8: The new imports
github.com/cosmos/cosmos-sdk/telemetryandmath/randare introduced to support telemetry metrics and random sampling respectively. Ensure that these packages are used appropriately in the code.188-207: The new function
emitMarketAvailabilityMetricsis introduced to emit telemetry metrics that track the availability of a market when queried on an exchange. The function takes inexchangeId,id(marketId), andavailableas parameters. It uses thetelemetry.IncrCounterWithLabelsfunction from thecosmos-sdk/telemetrypackage to increment a counter with labels. The labels include thePricefeedDaemon,PriceFetcherQueryForMarket, andsuccess(which can be eithermetrics.Successormetrics.Errordepending on theavailableparameter). The function seems to be well-structured and logically sound.263-284: The
runSubTaskfunction is updated to include telemetry metrics. If an error occurs during the price fetching process, the function will report all markets as unavailable. It also creates a mapavailableMarketsto track which markets were available when queried. Ensure that these changes do not affect the overall functionality of therunSubTaskfunction.312-323: The
runSubTaskfunction is further updated to mark a market as available in theavailableMarketsmap if a price is successfully fetched for it. It also emits metrics on the exchange's market availability according to the sampling rate. These changes seem to be logically sound and should improve the system's ability to monitor and alert on market availability issues.
| telemetry.IncrCounterWithLabels( | ||
| []string{ | ||
| metrics.PricefeedDaemon, | ||
| metrics.PriceFetcherQueryForMarket, |
There was a problem hiding this comment.
Nit: perhaps update the metrics name with a suffix like _sampled so it's clear that this is a sampled metric
…n datadog (#706) (#712) Co-authored-by: Crystal Lemire <[email protected]>
Changelist
Track availability of market symbols in each exchange for alerting purposes.
Test Plan
Existing tests, this is a telemetry change.
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.