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.

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

Jump to bottom

fix: move bitswap msg counters out of encode/decode #374

Merged
merged 4 commits into from
Oct 20, 2022
Merged

fix: move bitswap msg counters out of encode/decode #374

merged 4 commits into from
Oct 20, 2022

Conversation

Arqu
Copy link
Collaborator

@Arqu Arqu commented Oct 20, 2022

No description provided.

@Arqu Arqu added metrics extracting quantified mesurements from iroh fix Fixes a bug labels Oct 20, 2022
@Arqu Arqu self-assigned this Oct 20, 2022
rklaehn
rklaehn reviewed Oct 20, 2022
View reviewed changes
Copy link
Contributor

@rklaehn rklaehn left a comment

Choose a reason for hiding this comment

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

So what was wrong before?

@Arqu
Copy link
Collaborator Author

Arqu commented Oct 20, 2022

So what was wrong before?

The counters were in the encode/decode functions which skewed results as we encode a lot more than we actually send.

@rklaehn
Copy link
Contributor

rklaehn commented Oct 20, 2022

The counters were in the encode/decode functions which skewed results as we encode a lot more than we actually send.

But that is also valuable information... Anyway, looks good.

dignifiedquire
dignifiedquire reviewed Oct 20, 2022
View reviewed changes
iroh-bitswap/src/lib.rs
@@ -287,6 +288,7 @@ impl<S: Store> Bitswap<S> {

fn receive_message(&self, peer: PeerId, message: BitswapMessage) {
inc!(BitswapMetrics::MessagesReceived);
record!(BitswapMetrics::MessageBytesIn, message.encoded_len() as u64);
Copy link
Contributor

Choose a reason for hiding this comment

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

this encodes the message again :/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ik :( Want me to extend BitswapMessage with a pre calculated len?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we could cache it?

@Arqu Arqu mentioned this pull request Feb 17, 2023
Open
@Arqu
Copy link
Collaborator Author

Arqu commented Oct 20, 2022

The counters were in the encode/decode functions which skewed results as we encode a lot more than we actually send.

But that is also valuable information... Anyway, looks good.

n0-computer/beetle#128

dignifiedquire
dignifiedquire approved these changes Oct 20, 2022
View reviewed changes
Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

lets get it in and worry about improving that encoding issue later

@Arqu Arqu merged commit 85d80b9 into main Oct 20, 2022
@Arqu Arqu deleted the arqu/fix_bitswap_msg_counters branch October 20, 2022 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rklaehn rklaehn rklaehn left review comments

@dignifiedquire dignifiedquire dignifiedquire approved these changes

Assignees

@Arqu Arqu

Labels
fix Fixes a bug metrics extracting quantified mesurements from iroh
Projects
iroh
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Arqu @rklaehn @dignifiedquire