Skip to content

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Oct 26, 2024

Issue Addressed

Our current metrics to track the result of block imports are incomplete. A block import can be triggered by any block component, both from gossip and RPC. We should metric the AvailabilityProcessingStatus of block, blob, and custody column import to count all possible import events.

Also, I would like to track all errors that can occur during import. For example BlockError::DuplicateFullyImported, which show the efficiency of sync.

Proposed Changes

Track the return of all process_* functions with the same helper register_process_result_metrics. Track the result FullyImported / MissingComponents / Error labeled by block component and provenance.

Additional Info

This PR has no breaking changes w.r.t. current metrics. However, it fixes the following metric:

  • beacon_processor_rpc_block_imported_total: This metric is incorrect. Blocks with blobs synced by lookup sync are never tracked, as the block is always imported first and blobs second.

@dapplion dapplion added the ready-for-review The code is ready for review label Oct 26, 2024
@dapplion dapplion force-pushed the beacon-processor-error-metric branch from 92e801b to dc81bbd Compare October 26, 2024 13:44
LazyLock::new(|| {
try_create_int_counter_vec(
"beacon_processor_import_errors_total",
"Total number of block components verified",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be Total number of block components that was not verified

Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, just a nit on the metric description and some merge conflicts, otherwise looks good!

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Dec 13, 2024
@dapplion dapplion force-pushed the beacon-processor-error-metric branch from dc81bbd to 77e8e8b Compare December 14, 2024 06:40
@dapplion dapplion requested a review from jimmygchen December 14, 2024 12:28
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thanks!

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Dec 16, 2024
@jimmygchen
Copy link
Member

@mergify queue

Copy link

mergify bot commented Dec 16, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at c92c07f

@mergify mergify bot merged commit c92c07f into sigp:unstable Dec 16, 2024
29 checks passed
@dapplion dapplion deleted the beacon-processor-error-metric branch January 24, 2025 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants