Add cadence mtric collection on empty block as well #589
Conversation
WalkthroughA new line of code has been added to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
| @@ -161,6 +161,7 @@ func (e *Engine) processEvents(events *models.CadenceEvents) error { | |||
| err, | |||
| ) | |||
| } | |||
| e.collector.CadenceHeightIndexed(events.CadenceHeight()) | |||
There was a problem hiding this comment.
it seems that blocks.SetLatestCadenceHeight() is only called when the events is empty. If we ever end up with a large string of evm blocks in a row, this would never get saved, possibly resulting in the node having to reindex all of the block if it crash. Should we always set it?
There was a problem hiding this comment.
We actually do set the latest indexed Cadence height, but we do so every time we store an EVM block, in: https://github.com/onflow/flow-evm-gateway/blob/main/storage/pebble/blocks.go#L99-L105.
Calling blocks.SetLatestCadenceHeight() when the events is empty, is just an optimization from the early days, where there was no 1:1 mapping between EVM <-> Flow blocks.
Right now, the only way to have no EVM block for a Flow block, is when the system chunk transaction fails to run(https://github.com/onflow/flow-go/blob/4668c5d997b1e40af65e9a9510e3a3654763e330/fvm/blueprints/scripts/systemChunkTransactionTemplate.cdc#L22-L25). There was one such incident a couple weeks ago, but now we have some backup mechanisms in place, to guard against that.
![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)
services/ingestion/engine.go (1)
Line range hint
153-157: Minor: Standardize error message formattingFor consistency in error message formatting, consider using the same style throughout the method. Currently, there's a slight difference in how errors are formatted. For example:
// Current return fmt.Errorf( "failed to update to latest cadence height: %d, during events ingestion: %w", events.CadenceHeight(), err, ) // vs // Current return fmt.Errorf("failed to index block %d event: %w", events.Block().Height, err)Consider standardizing to the multi-line format for improved readability:
// Suggested return fmt.Errorf( "failed to index block %d event: %w", events.Block().Height, err, )This is a minor nitpick and doesn't affect functionality.
Also applies to: 176-180
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- services/ingestion/engine.go (1 hunks)
🔇 Additional comments not posted (2)
services/ingestion/engine.go (2)
Line range hint
1-300: Summary: PR objectives met, code change looks goodThe implemented change successfully addresses the PR objective of adding cadence metric collection for empty blocks. The addition is well-placed and consistent with the existing code structure.
A minor suggestion was made to standardize error message formatting for improved consistency, but this doesn't affect the functionality of the code.
Overall, the changes look good and ready for merging, pending any additional reviews or tests as per your team's process.
164-164: LGTM! Metric collection added for empty blocks.The addition of
e.collector.CadenceHeightIndexed(events.CadenceHeight())for empty events (heartbeat events) aligns well with the PR objective. This ensures that cadence metrics are collected consistently, even when there's no associated EVM block.To ensure consistency, let's verify that
CadenceHeightIndexedis called in all relevant places:✅ Verification successful
Verification Complete:
CadenceHeightIndexedis consistently called inprocessEvents.The
CadenceHeightIndexedmethod is appropriately invoked within theprocessEventsmethod inservices/ingestion/engine.go. No inconsistencies or additional unexpected usages were found in other parts of the codebase, ensuring that cadence metrics are collected as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of CadenceHeightIndexed # Test: Search for all occurrences of CadenceHeightIndexed echo "Occurrences of CadenceHeightIndexed:" rg --type go "CadenceHeightIndexed" -A 3 # Test: Verify that CadenceHeightIndexed is called in all processEvents scenarios echo "\nVerifying CadenceHeightIndexed calls in processEvents:" rg --type go "func \(e \*Engine\) processEvents" -A 50 | rg "CadenceHeightIndexed"Length of output: 1657
Closes: #???
Description
Related to #571
I missed collecting metrics when the cadence block does not include a evm block.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes