Add block/tx indexed metrics and spork client logging #595
Conversation
|
Warning Rate limit exceeded@janezpodhostnik has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request enhance the functionality of the metrics collector by introducing a new method for tracking EVM transactions and updating existing methods to increment counters accordingly. 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 (
|
![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 (5)
metrics/nop.go (1)
20-20: LGTM! Consider adding a comment for clarity.The new
EVMTransactionIndexedmethod is consistent with the existing no-op implementation pattern and aligns well with the PR objectives for adding transaction indexed metrics.Consider adding a brief comment to explain the purpose of the
intparameter, e.g.:// EVMTransactionIndexed records the number of EVM transactions indexed func (c *nopCollector) EVMTransactionIndexed(count int) {}This would improve code readability and maintain consistency with any existing documentation practices in the project.
services/requester/cross-spork_client_test.go (1)
45-47: LGTM: Updatedaddmethod calls with logger parameter.The changes correctly implement the new method signature for
add, including theloggerparameter. The order of client additions remains unchanged, which is crucial for testing the sorting functionality.Consider extracting the
loggerparameter into a variable for better readability:- require.NoError(t, clients.add(logger, client2)) - require.NoError(t, clients.add(logger, client3)) - require.NoError(t, clients.add(logger, client1)) + addClient := func(client Client) { + require.NoError(t, clients.add(logger, client)) + } + addClient(client2) + addClient(client3) + addClient(client1)This change would make the test more concise and easier to maintain if additional parameters are added in the future.
metrics/collector.go (1)
153-154: LGTM with a minor suggestion: Update EVMHeightIndexed method.The addition of
c.evmBlockIndexedCounter.Inc()correctly increments the counter for each indexed block. This change enhances the tracking capabilities as intended.Consider updating the method name to reflect its dual purpose, e.g.,
EVMHeightIndexedAndCounted. Alternatively, add a comment explaining that this method both updates the current height and increments the total block count.services/requester/cross-spork_client.go (1)
Line range hint
31-46: LGTM! Consider adding error logging.The changes to the
addmethod improve observability by adding logging for the spork client's first and last heights. This aligns well with the PR objectives.Consider adding error logging before returning errors. For example:
if err != nil { + logger.Error().Err(err).Msg("failed to get latest block header") return fmt.Errorf("could not get latest height using the spork client: %w", err) }This would provide more context in case of failures.
services/ingestion/engine.go (1)
209-211: Consider grouping related metric collectionsThe addition of the EVM transaction indexing metric enhances the system's observability without introducing complexity or altering the existing logic. It aligns well with the file's purpose and structure.
For improved readability and maintainability, consider grouping related metric collections together. You might want to move the new
EVMTransactionIndexedcall next to theEVMHeightIndexedcall:e.collector.EVMHeightIndexed(events.Block().Height) e.collector.EVMTransactionIndexed(len(events.Transactions())) e.collector.CadenceHeightIndexed(events.CadenceHeight())This grouping would make it easier to understand and maintain related metrics in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- metrics/collector.go (6 hunks)
- metrics/nop.go (1 hunks)
- services/ingestion/engine.go (1 hunks)
- services/requester/cross-spork_client.go (3 hunks)
- services/requester/cross-spork_client_test.go (2 hunks)
🔇 Additional comments (14)
metrics/nop.go (1)
Line range hint
1-24: Overall, the changes look good and align with the PR objectives.The addition of the
EVMTransactionIndexedmethod to thenopCollectorstruct is consistent with the existing implementation pattern and supports the goal of adding transaction indexed metrics. The change is minimal, focused, and doesn't introduce any issues.services/requester/cross-spork_client_test.go (3)
37-37: LGTM: Logger initialization added.The addition of the
loggervariable usingzerolog.Nop()is appropriate for test scenarios. This change aligns with the updated method signature ofaddmentioned in the summary.
62-62: LGTM: Test case name improved andaddmethod calls updated.The changes in this test case are appropriate:
- The test case name has been corrected from "not-continues" to "not-continuous", improving clarity.
- The
clients.add()calls have been correctly updated with the newloggerparameter, consistent with the changes in the first test case.These modifications enhance the test's readability and align with the updated method signature.
Also applies to: 68-69
Line range hint
37-69: Summary: Successful integration of logger parameter in CrossSporkClients tests.The changes in this file successfully integrate the new
loggerparameter into theCrossSporkClientstests. Key points:
- A
loggervariable is properly initialized usingzerolog.Nop().- All
clients.add()calls are updated to include theloggerparameter.- The test case for non-continuous validation has an improved name.
These modifications align well with the PR objectives of enhancing logging capabilities. The tests now accurately reflect the updated method signature of the
addfunction, ensuring that the CrossSporkClient's behavior with the new logging feature is properly validated.To ensure consistency across the codebase, let's verify the usage of the updated
addmethod:This script will help identify any locations where the
addmethod might still be called without the logger parameter, ensuring complete implementation of the logging enhancement.metrics/collector.go (7)
18-18: LGTM: New method for tracking indexed EVM transactions.The addition of
EVMTransactionIndexed(count int)to theCollectorinterface is well-designed and aligns with the PR objectives. It allows for efficient tracking of multiple indexed transactions at once.
33-34: LGTM: New counters for EVM block and transaction indexing.The addition of
evmBlockIndexedCounterandevmTxIndexedCounterto theDefaultCollectorstruct is appropriate. These prometheus counters will effectively track the cumulative number of indexed blocks and transactions, enhancing the monitoring capabilities as per the PR objectives.
71-79: LGTM: Proper initialization of new counters.The initialization of
evmBlockIndexedCounterandevmTxIndexedCounteris well-implemented. The metric names and help descriptions are clear, consistent, and follow Prometheus naming conventions.
99-100: LGTM: New counters added to metrics slice.The
evmBlockIndexedCounterandevmTxIndexedCounterare correctly added to the metrics slice. This ensures they will be properly registered with Prometheus for monitoring.
116-117: LGTM: New counters properly assigned in DefaultCollector initialization.The
evmBlockIndexedCounterandevmTxIndexedCounterare correctly assigned to the corresponding struct fields in theDefaultCollectorinitialization. This ensures that these counters can be used within the collector's methods.
156-157: LGTM: Efficient implementation of EVMTransactionIndexed method.The
EVMTransactionIndexedmethod is implemented correctly, usingAdd(float64(count))to increment the counter by the given number of transactions. This approach is efficient and aligns well with the PR objectives for tracking indexed transactions.
Line range hint
1-180: Overall assessment: Well-implemented enhancements to metrics collection.The changes in this file successfully implement the new metrics for EVM block and transaction indexing, aligning perfectly with the PR objectives. The code is well-structured, follows Go best practices, and integrates smoothly with the existing codebase. The new counters and methods enhance the monitoring capabilities of the Flow EVM Gateway, particularly for block and transaction processing.
A minor suggestion was made to improve the clarity of the
EVMHeightIndexedmethod, but this doesn't impact the functionality. Great job on these improvements!services/requester/cross-spork_client.go (2)
121-121: LGTM! Consistent with theaddmethod update.The change to pass the
loggerto theaddmethod is consistent with the updated method signature and ensures proper logging for each past spork client.
Line range hint
1-224: Overall, the changes effectively improve logging capabilities.The modifications to this file successfully implement the PR objectives by:
- Adding logging capabilities to the
addmethod ofsporkClients.- Ensuring consistent usage of the logger throughout the file.
These changes enhance the observability of spork client operations, particularly during the addition of new spork clients, without altering the core functionality. This improvement will aid in monitoring and debugging block and transaction processing within the system.
services/ingestion/engine.go (1)
209-209: LGTM: Metric addition aligns with PR objectivesThe added line
e.collector.EVMTransactionIndexed(len(events.Transactions()))successfully implements the PR objective of adding transaction indexed metrics. This change enhances the observability of the EVM Gateway by tracking the number of indexed transactions, complementing the existing metrics for EVM and Cadence heights.The placement and implementation are consistent with the existing code structure and other metric collection calls. This addition will provide valuable insights into the transaction processing within the system.
janezpodhostnik
requested review from
janezpodhostnik
and removed request for
sideninja
janezpodhostnik
requested review from
zhangchiqing and
ramtinms
as code owners
Closes: #???
Description
Add metrics for block and transaction indexing. Also add logging for the spork client block ranges on startup.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes
Tests