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 metric 'Number of bytes received from each topic (deduplicated)' #6431

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

Conversation

KatyaRyazantseva
Copy link

Issue Addressed

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

Proposed Changes

  • Changed topic_msg_recv_bytes metric into topic_msg_recv_bytes_unfiltered. This can be a critical change, but needed to put metrics in order.
  • Implemented topic_msg_recv_bytes as deduplicated.

@chong-he chong-he added ready-for-review The code is ready for review das Data Availability Sampling labels Sep 25, 2024
@jimmygchen jimmygchen self-assigned this Oct 1, 2024
pub(crate) fn get_size(&self) -> usize {
self.source
.as_ref()
.map_or(0, |m| 1 + sizeof_len(m.to_bytes().len()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

The PeerID has a fixed length by definition, we could just precompute it

@@ -1837,7 +1837,7 @@ where

// Record the received message with the metrics
if let Some(metrics) = self.metrics.as_mut() {
metrics.msg_recvd(&message.topic);
metrics.msg_recvd(&message.topic, message.get_size());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you use get_size() and above we use raw_protobuf_len what motivates the difference?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should use raw_protobuf_len to obtain the byte length of received message. Message is transformed data from RawMessage and is missing some fields compared to RawMessage, so message.get_size() returns a smaller byte size than the actually received bytes.

Copy link
Member

Choose a reason for hiding this comment

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

yeah the get_size function is unrequired we can just use raw_protobuf_len

@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
Comment on lines +86 to +88
pub fn hash_byte_len(&self) -> usize {
self.hash.as_bytes().len()
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: we are only using this once and it's a one liner, no need for a function to abstract the one liner

@@ -1837,7 +1837,7 @@ where

// Record the received message with the metrics
if let Some(metrics) = self.metrics.as_mut() {
metrics.msg_recvd(&message.topic);
metrics.msg_recvd(&message.topic, message.get_size());
Copy link
Member

Choose a reason for hiding this comment

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

yeah the get_size function is unrequired we can just use raw_protobuf_len

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.

6 participants