Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'beacon_' prefix to PeerDAS metrics names #6537

Open
wants to merge 3 commits into
base: unstable
Choose a base branch
from

Conversation

KatyaRyazantseva
Copy link

Issue Addressed

This PR addresses issue #6018, the list of the PeerDAS metrics.

Proposed Changes

Added beacon_ prefix to metrics names to align with PeerDAS metrics specs.

@chong-he chong-he added the ready-for-review The code is ready for review label Oct 25, 2024
@jimmygchen jimmygchen added the das Data Availability Sampling label Nov 1, 2024
@@ -1812,7 +1812,7 @@ pub static KZG_VERIFICATION_BATCH_TIMES: LazyLock<Result<Histogram>> = LazyLock:
pub static KZG_VERIFICATION_DATA_COLUMN_SINGLE_TIMES: LazyLock<Result<Histogram>> =
LazyLock::new(|| {
try_create_histogram_with_buckets(
"kzg_verification_data_column_single_seconds",
"beacon_kzg_verification_data_column_single_seconds",
"Runtime of single data column kzg verification",
Copy link
Member

Choose a reason for hiding this comment

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

Note that we also have similar metrics (e.g. kzg_verification_single_seconds) for Deneb blobs, but we can probably leave them as it is, as they will eventually be replaced by the PeerDAS ones.

"Time taken to reconstruct columns",
)
});
pub static DATA_AVAILABILITY_RECONSTRUCTED_COLUMNS: LazyLock<Result<IntCounter>> =
LazyLock::new(|| {
try_create_int_counter(
"data_availability_reconstructed_columns_total",
"beacon_data_availability_reconstructed_columns_total",
"Total count of reconstructed columns",
)
});
Copy link
Member

Choose a reason for hiding this comment

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

Could you also update the 4 data column metrics below to include the prefix too?

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.

Thanks @KatyaRyazantseva, the prefix make sense, just one minor comment here to update a few other related metrics.

@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 Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
das Data Availability Sampling waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants