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 additional PeerDAS metrics (#6018) #6248

Draft
wants to merge 9 commits into
base: das
Choose a base branch
from

Conversation

KatyaRyazantseva
Copy link

@KatyaRyazantseva KatyaRyazantseva commented Aug 9, 2024

Issue Addressed

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

Proposed Changes

Added the following metrics:

Custody:

  • Total count of columns in custody (counter)

Gossipsub:

  • Number of bytes received from each topic (deduplicated) (counter)

Libp2p:

  • Number of requests received (counter)
  • Number of responses received (counter)

@CLAassistant
Copy link

CLAassistant commented Aug 9, 2024

CLA assistant check
All committers have signed the CLA.

@KatyaRyazantseva KatyaRyazantseva marked this pull request as draft August 9, 2024 15:25
@KatyaRyazantseva
Copy link
Author

Renamed topic_msg_recv_bytes metric to topic_msg_recv_bytes_unfiltered. It's kind of a breaking change but I guess metrics names should be put in order or we will have _unfiltered/_filtered mess. It's up to you, please share your feedback

@@ -1160,6 +1160,40 @@ impl<E: EthSpec> Network<E> {
peer_id: PeerId,
response: Response<E>,
) -> Option<NetworkEvent<E>> {
match &response {
Response::Status(_) => {
metrics::inc_counter_vec(&metrics::TOTAL_RPC_RESPONSES_RECEIVED, &["status"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can derive IntoStaticStr for Response and just do a single

metrics::inc_counter_vec(&metrics::TOTAL_RPC_RESPONSES_RECEIVED, &[response.into()])

Copy link
Author

Choose a reason for hiding this comment

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

done

@jimmygchen jimmygchen added the das Data Availability Sampling label Aug 12, 2024
};

custody_subnet_count.saturating_mul(spec.data_columns_per_subnet())
}
Copy link
Member

Choose a reason for hiding this comment

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

import_all_data_columns isn't available here, but I've added this function in another PR, feel free to copy it over if you'd like to test this:

https://github.com/sigp/lighthouse/pull/6255/files#diff-eeea69875a56959be32e581165eace1a25fd517afd98fe0576372db24e0fd9a5R128-R132

Copy link
Author

Choose a reason for hiding this comment

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

done

StatusMessage::from_ssz_bytes(decoded_buffer)?,
))),
SupportedProtocol::StatusV1 => {
metrics::inc_counter_vec_by(&metrics::TOTAL_RPC_REQUESTS_BYTES_RECEIVED, &["status"], decoded_buffer.len() as u64);
Copy link
Author

Choose a reason for hiding this comment

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

if this is the right spot for this metric, I will add another ones here: by_range, by_root, etc.

@KatyaRyazantseva
Copy link
Author

@jxs could you please review the libp2p metrics we've discussed?

@KatyaRyazantseva
Copy link
Author

Added Req/Resp metrics:

  • Number of requests sent (counter)
  • Number of requests bytes sent (counter)
  • Number of responses sent (counter)
  • Number of responses bytes sent (counter)
  • Number of requests bytes received (counter)
  • Number of responses bytes received (counter)

@mergify mergify bot deleted the branch sigp:das August 27, 2024 04:10
@mergify mergify bot closed this Aug 27, 2024
@michaelsproul michaelsproul reopened this Aug 27, 2024
@michaelsproul
Copy link
Member

Please update to point at unstable by either:

  1. Rebasing on unstable (if your branch has a small number of commits that are easy to tease out), or
  2. Merging origin/das into this PR: git fetch origin; git merge origin/das. This will result in the smallest number of conflict resolutions and is better for branches that already contain merge commits or have extensive history.

net::IpAddr,
task::{Context, Poll},
time::Duration,
cmp::{max, Ordering}, collections::{BTreeSet, HashMap, HashSet, VecDeque}, fmt, io::Read, net::IpAddr, task::{Context, Poll}, time::Duration
Copy link
Member

Choose a reason for hiding this comment

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

you might want to run make cargo-fmt locally to format these with cargo fmt

pub static CUSTODY_COLUMNS_COUNT: LazyLock<Result<IntGauge>> = LazyLock::new(|| {
try_create_int_gauge(
"beacon_custody_columns_count_total",
"Total count of columns in custody",
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth clarifying whether this number refers to the number of columns in custody (up to 128), or the total number of columns in custody within the data availability boundary. I'm guessing this means the latter?

Copy link
Author

Choose a reason for hiding this comment

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

It would be the second, it would seesaw as new columns come into custody and old ones are pruned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
das Data Availability Sampling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants