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

feat(net): Metered senders #726

Merged
merged 12 commits into from
Jan 18, 2023
Merged

feat(net): Metered senders #726

merged 12 commits into from
Jan 18, 2023

Conversation

akirillo
Copy link
Contributor

@akirillo akirillo commented Jan 5, 2023

Resolves #140, intended to build on top of #707

@akirillo akirillo added C-enhancement New feature or request A-observability Related to tracing, metrics, logs and other observability tools A-networking Related to networking in general labels Jan 5, 2023
@akirillo akirillo self-assigned this Jan 5, 2023
@akirillo
Copy link
Contributor Author

akirillo commented Jan 6, 2023

@onbjerg would appreciate a quick glance at my current approach here to address #140!
Should I also add a BandwidthMeter a la #707 so that we can gauge / count outbound bandwidth from the sender?

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2023

Codecov Report

Merging #726 (c2b31f5) into main (75fab00) will increase coverage by 0.28%.
The diff coverage is 36.36%.

@@            Coverage Diff             @@
##             main     #726      +/-   ##
==========================================
+ Coverage   73.55%   73.83%   +0.28%     
==========================================
  Files         280      284       +4     
  Lines       29898    30157     +259     
==========================================
+ Hits        21991    22267     +276     
+ Misses       7907     7890      -17     
Flag Coverage Δ
unit-tests 73.83% <36.36%> (+0.28%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
crates/metrics/common/src/metered_sender.rs 0.00% <0.00%> (ø)
crates/metrics/common/src/lib.rs 100.00% <100.00%> (ø)
crates/net/common/src/bandwidth_meter.rs 94.70% <100.00%> (ø)
crates/net/network/src/listener.rs 77.27% <0.00%> (-6.82%) ⬇️
crates/transaction-pool/src/test_utils/mock.rs 53.11% <0.00%> (-6.60%) ⬇️
crates/primitives/src/chain.rs 90.81% <0.00%> (-2.60%) ⬇️
crates/primitives/src/transaction/mod.rs 88.57% <0.00%> (-1.27%) ⬇️
bin/reth/src/config.rs 62.00% <0.00%> (-1.27%) ⬇️
crates/transaction-pool/src/pool/txpool.rs 58.87% <0.00%> (-0.50%) ⬇️
crates/net/eth-wire/src/disconnect.rs 83.45% <0.00%> (-0.49%) ⬇️
... and 33 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@akirillo akirillo marked this pull request as ready for review January 7, 2023 00:11
@mattsse mattsse requested a review from onbjerg January 7, 2023 10:18
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Wrapping around the type and tracking makes sense, do we intend to integrate this anywhere in this PR?

Sender,
};

/// Network throughput metrics
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not limited to network, we can use this for any bounded channel that we want to monitor.
so perhaps we move this to a separate crate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it to common/metrics/metered_sender.rs, that work?

@akirillo
Copy link
Contributor Author

Wrapping around the type and tracking makes sense, do we intend to integrate this anywhere in this PR?

Am happy to! Would appreciate any pointers on where this would be the highest signal, maybe in the Senders used in the discv4 crate?

@onbjerg onbjerg added the S-blocked This cannot more forward until something else changes label Jan 11, 2023
@onbjerg
Copy link
Collaborator

onbjerg commented Jan 11, 2023

Marked this as blocked on #785

@akirillo
Copy link
Contributor Author

Note to self: add docs to metrics.md after #785 is merged

@gakonst
Copy link
Member

gakonst commented Jan 14, 2023

Unblocked @akirillo we got #785 in now!

@akirillo
Copy link
Contributor Author

making use of dynamic scope now - any suggestions on where to integrate this metric? which senders do we care the most about? @gakonst

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

This probably should be under crates/metrics right?

@mattsse when u get a chance if you can point at some senders that would benefit from this

@akirillo akirillo changed the title feat(net): Throughput metering metrics feat(net): Metered senders Jan 17, 2023
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

looks good - we should identify where we have senders and wrap them around this in a followup

@gakonst gakonst merged commit 5d45325 into main Jan 18, 2023
@gakonst gakonst deleted the andrew/throughput-metering-metrics branch January 18, 2023 01:41
literallymarvellous pushed a commit to literallymarvellous/reth that referenced this pull request Feb 5, 2023
literallymarvellous pushed a commit to literallymarvellous/reth that referenced this pull request Feb 5, 2023
literallymarvellous pushed a commit to literallymarvellous/reth that referenced this pull request Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Related to networking in general A-observability Related to tracing, metrics, logs and other observability tools C-enhancement New feature or request S-blocked This cannot more forward until something else changes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add metered channel abstractions
5 participants