Add metric to track the Flow total supply on each block - #949
Conversation
📝 WalkthroughWalkthroughAdds a Prometheus gauge Changes
Sequence Diagram(s)sequenceDiagram
participant Ingest as Ingestion Engine
participant Collector as Collector (interface)
participant Default as DefaultCollector
participant Prom as Prometheus
Ingest->>Collector: FlowTotalSupply(totalSupply *big.Int)
Collector->>Default: FlowTotalSupply(totalSupply)
Default->>Default: convert big.Int -> float64 (check precision)
Default-->>Prom: Set gauge `flow_total_supply` value
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @metrics/collector.go:
- Around line 259-262: In DefaultCollector.FlowTotalSupply, add a nil check for
the totalSupply parameter to avoid panics and replace the blind
totalSupply.Float64() call with a safe conversion that checks the returned
Accuracy and handles overflow/precision loss: convert the *big.Int to a
big.Float (e.g., big.NewFloat(0).SetInt(totalSupply)), call bigFloat.Float64()
and inspect the accuracy (big.Exact/Above/Below) or detect +Inf; if nil or
conversion is inaccurate/overflowing, log or clamp to a sensible value (or skip
updating) instead of silently setting an imprecise metric when calling
c.flowTotalSupply.Set.
In @services/ingestion/engine.go:
- Line 215: The call passes events.Block().TotalSupply (a *big.Int) directly
into e.collector.FlowTotalSupply which can panic when TotalSupply is nil; add a
nil guard before calling FlowTotalSupply (e.g., check events.Block() != nil &&
events.Block().TotalSupply != nil) and only call e.collector.FlowTotalSupply
when TotalSupply is non-nil, or alternatively update metrics/collector.go's
FlowTotalSupply to defensively handle a nil *big.Int by treating it as zero or
returning early—modify either the call site in engine.go (around
e.collector.FlowTotalSupply) or the FlowTotalSupply method to handle nil inputs.
🧹 Nitpick comments (1)
metrics/collector.go (1)
90-93: Consider clarifying the help text.The help text "Total supply of native EVM FLOW token at a given time" contains "native EVM FLOW token" which might be clearer as "FLOW tokens in EVM" or simply "EVM FLOW supply".
📝 Suggested help text improvement
var flowTotalSupply = prometheus.NewGauge(prometheus.GaugeOpts{ Name: prefixedName("flow_total_supply"), - Help: "Total supply of native EVM FLOW token at a given time", + Help: "Total supply of FLOW tokens in EVM at a given time", })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
metrics/collector.gometrics/nop.goservices/ingestion/engine.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-18T19:26:37.579Z
Learnt from: peterargue
Repo: onflow/flow-evm-gateway PR: 617
File: api/stream.go:62-67
Timestamp: 2024-10-18T19:26:37.579Z
Learning: In the `flow-evm-gateway` project, within the Go file `api/stream.go`, the `prepareBlockResponse` method includes the Bloom filter as the field `LogsBloom` in the returned `Block` struct.
Applied to files:
services/ingestion/engine.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (2)
metrics/nop.go (1)
4-4: LGTM!The no-op implementation correctly satisfies the updated
Collectorinterface with the appropriate signature.Also applies to: 28-28
metrics/collector.go (1)
111-111: LGTM!The gauge is correctly registered in the metrics slice, added to the interface, included in the struct, and properly initialized in the constructor.
Also applies to: 129-129, 151-151, 176-176
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
metrics/collector.go (1)
90-93: Clarify the unit of measurement in the help text.The help text should specify whether
flow_total_supplyrepresents the smallest unit (similar to wei in Ethereum) or whole FLOW tokens. This clarity will help operators correctly interpret the metric values and set appropriate alert thresholds.📝 Suggested help text improvement
var flowTotalSupply = prometheus.NewGauge(prometheus.GaugeOpts{ Name: prefixedName("flow_total_supply"), - Help: "Total supply of FLOW tokens in EVM at a given time", + Help: "Total supply of FLOW tokens in EVM at a given time (in smallest unit)", })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
metrics/collector.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (6)
metrics/collector.go (6)
5-5: LGTM!The
math/bigimport is appropriate for handling large FLOW token supply values that may exceed standard integer types.
111-111: LGTM!The
flowTotalSupplymetric is correctly added to the registration list.
129-129: LGTM!The method signature correctly uses
*big.Intto handle potentially large FLOW token supply values.
135-135: LGTM!Adding the
loggerfield toDefaultCollectorenables proper logging of precision loss warnings in theFlowTotalSupplymethod. The initialization inNewCollectoris correct.Also applies to: 163-163
153-153: LGTM!The
flowTotalSupplyfield and its initialization follow the established pattern for metric collection in this codebase.Also applies to: 179-179
262-273: Verify that float64 precision is sufficient for the expected total supply values.The conversion from
*big.Inttofloat64will lose precision for values exceeding 2^53 (~9 × 10^15). If the total supply is measured in the smallest unit (similar to wei in Ethereum), values may frequently exceed this threshold, leading to:
- Inaccurate absolute values in the metric
- Potential issues detecting small fluctuations
- Misleading data for alerting purposes
The nil check and precision loss warning are good defensive measures. However, consider whether:
- The expected magnitude of
totalSupplyvalues will fit within float64 precision- Converting to a larger unit (e.g., whole FLOW tokens) before the float64 conversion would preserve sufficient precision for your alerting needs
- The current precision is adequate for detecting abnormal fluctuations (the stated goal in issue #947)
Could you verify the typical range of
totalSupplyvalues and confirm that float64 precision is acceptable for your monitoring and alerting requirements?
janezpodhostnik
left a comment
janezpodhostnik
left a comment
There was a problem hiding this comment.
Looks good.
One Q: whats the mechanism behind events.Block().TotalSupply how do we get that?
@janezpodhostnik The
|
3f59858 to
603c1b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
metrics/collector.go (1)
262-273: Consider adding the value to the precision loss warning for debugging.The implementation correctly handles nil values and logs a warning when precision is lost. However, for debugging purposes, including the actual
totalSupplyvalue in the warning would help identify when/where precision loss occurs.💡 Suggested enhancement
floatTotalSupply, accuracy := totalSupply.Float64() if accuracy != big.Exact { - c.logger.Warn().Msg("precision loss when casting total supply to float") + c.logger.Warn(). + Str("totalSupply", totalSupply.String()). + Msg("precision loss when casting total supply to float") }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
metrics/collector.gometrics/nop.goservices/ingestion/engine.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-07T01:35:09.751Z
Learnt from: peterargue
Repo: onflow/flow-evm-gateway PR: 772
File: services/requester/keystore/key_store.go:50-62
Timestamp: 2025-03-07T01:35:09.751Z
Learning: In the flow-evm-gateway codebase, panics are acceptable in scenarios where immediate detection of critical bugs is desired during development and testing, particularly for invariant violations that should never occur in a correctly functioning system (e.g., when a key is available but locked in the keystore implementation).
Applied to files:
services/ingestion/engine.go
🧬 Code graph analysis (1)
services/ingestion/engine.go (2)
eth/types/types.go (1)
Block(457-478)models/block.go (1)
Block(45-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (4)
metrics/nop.go (1)
4-4: LGTM!The no-op implementation correctly satisfies the
Collectorinterface with an empty method body, and themath/bigimport is properly added for the*big.Intparameter type.Also applies to: 28-28
metrics/collector.go (2)
90-93: LGTM!The gauge is properly defined with a clear help text indicating the unit (wei). The naming convention follows the existing pattern with the
prefixedNamefunction.
129-129: LGTM!The interface method, struct fields, and constructor initialization are all consistent with the existing patterns in the codebase. The logger is appropriately added to support the precision warning.
Also applies to: 135-136, 153-153, 163-163, 179-179
services/ingestion/engine.go (1)
210-213: LGTM!The
TotalSupplyfield is properly defined on the Block model as*big.Int, and the metric implementation correctly handles nil values. The metric emission is correctly placed after successful event processing and follows the existing pattern.
2 participants
Closes: #947
Description
Tracking this field from the
EVM.BlockExecutedevent payload, can be useful and will allow us to create alerts for abnormal fluctuations on thetotalSupplyvalue.For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.