Skip to content

Remove unused metrics #6817

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 7, 2025
Merged

Remove unused metrics #6817

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86c65d6
Remove unused metrics
ackintosh Jan 18, 2025
2ffb739
Re-added block_availability_delay and utilized it
ackintosh Jan 24, 2025
4d06b67
Revert "Re-added block_availability_delay and utilized it"
ackintosh Jan 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 0 additions & 55 deletions beacon_node/beacon_chain/src/metrics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,6 @@ pub static BLOCK_PROCESSING_COMMITTEE: LazyLock<Result<Histogram>> = LazyLock::n
"Time spent building/obtaining committees for block processing.",
)
});
pub static BLOCK_PROCESSING_SIGNATURE: LazyLock<Result<Histogram>> = LazyLock::new(|| {
try_create_histogram(
"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

pub static BLOCK_PROCESSING_CORE: LazyLock<Result<Histogram>> = LazyLock::new(|| {
try_create_histogram(
"beacon_block_processing_core_seconds",
Expand Down Expand Up @@ -591,12 +585,6 @@ pub static FORK_CHOICE_WRITE_LOCK_AQUIRE_TIMES: LazyLock<Result<Histogram>> = La
exponential_buckets(1e-3, 4.0, 7),
)
});
pub static FORK_CHOICE_SET_HEAD_LAG_TIMES: LazyLock<Result<Histogram>> = LazyLock::new(|| {
try_create_histogram(
"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

pub static BALANCES_CACHE_HITS: LazyLock<Result<IntCounter>> = LazyLock::new(|| {
try_create_int_counter(
"beacon_balances_cache_hits_total",
Expand Down Expand Up @@ -651,12 +639,6 @@ pub static DEFAULT_ETH1_VOTES: LazyLock<Result<IntCounter>> = LazyLock::new(|| {
/*
* Chain Head
*/
pub static UPDATE_HEAD_TIMES: LazyLock<Result<Histogram>> = LazyLock::new(|| {
try_create_histogram(
"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

pub static HEAD_STATE_SLOT: LazyLock<Result<IntGauge>> = LazyLock::new(|| {
try_create_int_gauge(
"beacon_head_state_slot",
Expand Down Expand Up @@ -1547,20 +1529,6 @@ pub static SYNC_CONTRIBUTION_PROCESSING_APPLY_TO_OP_POOL: LazyLock<Result<Histog
"Time spent applying a sync contribution to the block inclusion pool",
)
});
pub static SYNC_CONTRIBUTION_PROCESSING_SIGNATURE_SETUP_TIMES: LazyLock<Result<Histogram>> =
LazyLock::new(|| {
try_create_histogram(
"beacon_sync_contribution_processing_signature_setup_seconds",
"Time spent on setting up for the signature verification of sync contribution processing"
)
});
pub static SYNC_CONTRIBUTION_PROCESSING_SIGNATURE_TIMES: LazyLock<Result<Histogram>> =
LazyLock::new(|| {
try_create_histogram(
"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


/*
* General Sync Committee Contribution Processing
Expand Down Expand Up @@ -1690,13 +1658,6 @@ pub static DATA_COLUMN_SIDECAR_GOSSIP_VERIFICATION_TIMES: LazyLock<Result<Histog
"Full runtime of data column sidecars gossip verification",
)
});
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",
)
});
Comment on lines -1693 to -1699
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:

lighthouse/beacon_node/beacon_chain/src/metrics.rs

Lines 1647 to 1653 in 86c65d6

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",
)
});


pub static BLOBS_FROM_EL_HIT_TOTAL: LazyLock<Result<IntCounter>> = LazyLock::new(|| {
try_create_int_counter(
Expand Down Expand Up @@ -1873,15 +1834,6 @@ pub static BLOCK_PRODUCTION_BLOBS_VERIFICATION_TIMES: LazyLock<Result<Histogram>
)
},
);
/*
* Availability related metrics
*/
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:

lighthouse/beacon_node/beacon_chain/src/metrics.rs

Lines 1390 to 1396 in a4e3f36

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!

"Duration between start of the slot and the time at which all components of the block are available.",
)
});

/*
* Data Availability cache metrics
Expand All @@ -1900,13 +1852,6 @@ pub static DATA_AVAILABILITY_OVERFLOW_MEMORY_STATE_CACHE_SIZE: LazyLock<Result<I
"Number of entries in the data availability overflow state memory cache.",
)
});
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

"Number of entries in the data availability overflow store cache.",
)
});
pub static DATA_AVAILABILITY_RECONSTRUCTION_TIME: LazyLock<Result<Histogram>> =
LazyLock::new(|| {
try_create_histogram(
Expand Down
Loading