Skip to content

Conversation

ackintosh
Copy link
Member

Issue Addressed

N/A

Proposed Changes

Removed metrics that were defined but not used anywhere.

Comment on lines -1693 to -1699
pub static DATA_COLUMNS_SIDECAR_PROCESSING_SUCCESSES: LazyLock<Result<IntCounter>> =
LazyLock::new(|| {
try_create_int_counter(
"beacon_data_column_sidecar_processing_successes_total",
"Number of data column sidecars verified for gossip",
)
});
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: another metric with the same name beacon_data_column_sidecar_processing_successes_total exists:

pub static DATA_COLUMN_SIDECAR_PROCESSING_SUCCESSES: LazyLock<Result<IntCounter>> =
LazyLock::new(|| {
try_create_int_counter(
"beacon_data_column_sidecar_processing_successes_total",
"Number of data column sidecars verified for gossip",
)
});

@ackintosh ackintosh marked this pull request as ready for review January 19, 2025 00:37
@ackintosh ackintosh added the ready-for-review The code is ready for review label Jan 19, 2025
*/
pub static BLOCK_AVAILABILITY_DELAY: LazyLock<Result<IntGauge>> = LazyLock::new(|| {
try_create_int_gauge(
"block_availability_delay",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this sounds like a useful metric that we should use

Copy link
Member

Choose a reason for hiding this comment

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

We do have this:

pub static BEACON_BLOCK_DELAY_AVAILABLE_SLOT_START: LazyLock<Result<IntGauge>> =
LazyLock::new(|| {
try_create_int_gauge(
"beacon_block_delay_available_slot_start",
"Duration between the time that block became available and the start of the slot.",
)
});

Copy link
Member

Choose a reason for hiding this comment

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

oh nvm just read the comments below!

pub static DATA_AVAILABILITY_OVERFLOW_STORE_CACHE_SIZE: LazyLock<Result<IntGauge>> =
LazyLock::new(|| {
try_create_int_gauge(
"data_availability_overflow_store_cache_size",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have DATA_AVAILABILITY_OVERFLOW_MEMORY_BLOCK_CACHE_SIZE and DATA_AVAILABILITY_OVERFLOW_MEMORY_STATE_CACHE_SIZE, so ok to remove

"beacon_sync_contribution_processing_signature_seconds",
"Time spent on the signature verification of sync contribution processing",
)
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok to delete for me

"beacon_update_head_seconds",
"Time taken to update the canonical head",
)
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have FORK_CHOICE_TIMES which cover what we may be interested in, ok to remove

"beacon_fork_choice_set_head_lag_times",
"Time taken between finding the head and setting the canonical head value",
)
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't sound too useful, ok to remove

"beacon_block_processing_signature_seconds",
"Time spent doing signature verification for a block.",
)
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we are interested in this, ok to remove

@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 Jan 22, 2025
@ackintosh
Copy link
Member Author

@dapplion I've re-added block_availability_delay in 2ffb739. I'm not very familiar with the code related to the beacon_chain, so I would appreciate it if you could review this. 🙏

@ackintosh ackintosh added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jan 24, 2025
"block_availability_delay",
"Duration between start of the slot and the time at which all components of the block are available.",
)
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually reviewed current unstable and we have BEACON_BLOCK_DELAY_AVAILABLE_SLOT_START which tracks exactly this but with more precision. Ok to delete BLOCK_AVAILABILITY_DELAY

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Feb 7, 2025
mergify bot added a commit that referenced this pull request Feb 7, 2025
mergify bot added a commit that referenced this pull request Feb 7, 2025
@mergify mergify bot merged commit 7408719 into sigp:unstable Feb 7, 2025
31 checks passed
@ackintosh ackintosh deleted the remove-unused-metrics branch February 7, 2025 20:56
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.

3 participants