Add metric for indexed cadence block height #571
Conversation
WalkthroughThe changes introduce a new method, Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
metrics/collector.go (1)
58-61: LGTM with a minor suggestion: Cadence block height gauge initializationThe initialization of the
cadenceBlockHeightgauge is correct and consistent with other metrics. It's properly added to themetricsslice for registration.Consider slightly modifying the help text to be more specific:
- Help: "Current Cadence block height", + Help: "Latest indexed Cadence block height",This change would more accurately reflect that it's tracking the latest indexed height rather than the current network height.
Also applies to: 84-84
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- metrics/collector.go (6 hunks)
- metrics/nop.go (1 hunks)
- services/ingestion/engine.go (1 hunks)
Additional comments not posted (6)
metrics/nop.go (1)
18-18: LGTM! The new method is correctly implemented.The addition of
CadenceHeightIndexed(uint64)to thenopCollectorstruct is consistent with the existing implementation and aligns with the PR objectives. The empty implementation is correct for a no-op collector.Let's verify that the
Collectorinterface has been updated to include this new method:Verification successful
Verified that the
Collectorinterface includesCadenceHeightIndexed(uint64). No issues found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the Collector interface includes the CadenceHeightIndexed method # Test: Search for the Collector interface definition rg --type go -A 20 'type Collector interface'Length of output: 1224
metrics/collector.go (4)
16-16: LGTM: New method added to track Cadence block heightThe addition of
CadenceHeightIndexed(height uint64)to theCollectorinterface is well-placed and consistent with existing methods. This change aligns with the PR objective to introduce a metric for tracking Cadence block height.
30-30: LGTM: New gauge added for Cadence block heightThe addition of
cadenceBlockHeight prometheus.Gaugeto theDefaultCollectorstruct is appropriate. It uses the correct Prometheus type for tracking a single numerical value and is well-placed among other related metrics.
130-132: LGTM: Implementation of CadenceHeightIndexed methodThe implementation of
CadenceHeightIndexedis correct and consistent with similar methods in the struct. It properly updates thecadenceBlockHeightgauge with the provided height value.
Line range hint
1-162: Summary: Cadence block height metric successfully implementedThe changes to add a metric for tracking the indexed Cadence block height have been implemented correctly and consistently throughout the file. The new
CadenceHeightIndexedmethod andcadenceBlockHeightgauge are well-integrated into the existingCollectorinterface andDefaultCollectorstruct.The implementation aligns with the PR objectives and follows the established patterns in the codebase. A minor suggestion was made to improve the help text for the new gauge, but overall, the changes are approved and ready for integration.
services/ingestion/engine.go (1)
209-209: LGTM! Verify theCadenceHeightIndexedmethod implementation.The addition of
e.collector.CadenceHeightIndexed(events.CadenceHeight())aligns well with the PR objective of adding a metric for indexed cadence block height. This change enhances the monitoring capabilities by tracking both EVM and Cadence block heights.To ensure the completeness of this change, please run the following script to verify the implementation of the
CadenceHeightIndexedmethod in theCollectorinterface and its concrete implementations:This script will help ensure that the
CadenceHeightIndexedmethod is properly defined in theCollectorinterface and implemented in the necessary structs.Verification successful
Verified:
CadenceHeightIndexedmethod is properly defined and implemented.All necessary implementations are in place, and no outstanding issues were found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of CadenceHeightIndexed method # Test 1: Check if CadenceHeightIndexed is defined in the Collector interface echo "Checking Collector interface:" rg --type go "type\s+Collector\s+interface" -A 10 | rg "CadenceHeightIndexed" # Test 2: Look for implementations of CadenceHeightIndexed echo "Checking for implementations:" rg --type go "func\s+\(\w+\s+\*\w+\)\s+CadenceHeightIndexed" # Test 3: Check for any TODOs or FIXMEs related to CadenceHeightIndexed echo "Checking for TODOs or FIXMEs:" rg --type go "(TODO|FIXME).*CadenceHeightIndexed"Length of output: 626
| @@ -206,6 +206,7 @@ func (e *Engine) processEvents(events *models.CadenceEvents) error { | |||
| } | |||
|
|
|||
| e.collector.EVMHeightIndexed(events.Block().Height) | |||
| e.collector.CadenceHeightIndexed(events.CadenceHeight()) | |||
There was a problem hiding this comment.
This processEvents method has a case where it can return early, if a Flow block has no EVM-related events:
flow-evm-gateway/services/ingestion/engine.go
Lines 155 to 157 in 7f8a5d5
Maybe we should move the tracking of
CadenceHeightIndexed before that condition, if we want to keep track of the fact that we received a response from the event-streaming API.Right now we use a
heartbeatInterval of 1, so we should get a response for every block.
There was a problem hiding this comment.
Good point @m-Peter
I think we should change flow EVM GW to expect one evm block for one cadence block, now we kinda generalize the idea so there could be none blocks etc, I think this loose requirement can create more issues than it will solve, if we, one day change this mapping 1:1 we should update the gateway then. I don't think it's still worth generalizing at this point. So this PR can be accepted in the current shape but follow-up PR must be made to make ingesting more strict imho.
Oops. I was too fast on the merge and didn't see your comment. I opened #589 to address it. |
Closes: #570
Description
Add metric for latest cadence block height
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes