feat: bandwidth metering metrics #844

akirillo · 2023-01-13T01:38:00Z

Resolves #783, introduces a type that exposes metrics from an underlying BandwidthMeter

akirillo · 2023-01-13T01:38:42Z

Assumes the API introduced in #785

codecov-commenter · 2023-01-15T22:58:58Z

Codecov Report

Merging #844 (766297d) into main (b5dab61) will decrease coverage by 0.05%.
The diff coverage is 82.50%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #844      +/-   ##
==========================================
- Coverage   74.91%   74.87%   -0.05%     
==========================================
  Files         313      313              
  Lines       34108    34139      +31     
==========================================
+ Hits        25551    25560       +9     
- Misses       8557     8579      +22

Flag	Coverage Δ
unit-tests	`74.87% <82.50%> (-0.05%)`	⬇️

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

Impacted Files	Coverage Δ
crates/net/common/src/lib.rs	`100.00% <ø> (ø)`
crates/net/network/src/manager.rs	`47.98% <ø> (-0.03%)`	⬇️
crates/net/network/src/network.rs	`57.57% <ø> (+0.64%)`	⬆️
crates/net/network/src/session/handle.rs	`50.00% <ø> (ø)`
crates/net/common/src/metered_stream.rs	`81.86% <81.86%> (ø)`
crates/net/network/src/session/active.rs	`84.67% <100.00%> (-0.06%)`	⬇️
crates/net/network/src/session/mod.rs	`68.51% <100.00%> (-0.07%)`	⬇️
crates/net/eth-wire/src/capability.rs	`62.13% <0.00%> (-0.98%)`	⬇️
crates/stages/src/stages/sender_recovery.rs	`92.51% <0.00%> (-0.54%)`	⬇️
crates/net/network/src/peers/manager.rs	`83.39% <0.00%> (-0.19%)`	⬇️
... and 2 more

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

akirillo · 2023-01-16T00:38:34Z

Going to mark as blocked for now by #892

gakonst

What do we need to get this over the line?

crates/net/network/src/session/mod.rs

gakonst · 2023-01-23T16:04:03Z

As discussed we only want to be counting Ingress/Egress, and not KB/s or MB/s.

akirillo · 2023-01-23T16:38:00Z

As discussed we only want to be counting Ingress/Egress, and not KB/s or MB/s.

Ack, that's effectively what this does, will rename to make it clearer.

akirillo · 2023-01-23T19:14:40Z

As discussed we only want to be counting Ingress/Egress, and not KB/s or MB/s.

For viz: after discussing w/ @onbjerg, I realized I was overloading the term "metric." We don't want to expose bytes/s as an external metric (e.g. in Prometheus) but it will be helpful to have it in-process, picking up on that in #992

mattsse

lgtm

Wdyt @onbjerg ?

onbjerg

See my comment on the trait. Another question I have is what the difference is between NetworkIOMeter and MeteredStream? Could MeteredStream not tally this up itself, i.e. is it not possible to merge NetworkIOMeter and MeteredStream? If so, is there a reason not to?

crates/net/common/src/network_io_meter.rs

crates/net/network/src/session/mod.rs

akirillo · 2023-01-24T19:27:23Z

Another question I have is what the difference is between NetworkIOMeter and MeteredStream? Could MeteredStream not tally this up itself, i.e. is it not possible to merge NetworkIOMeter and MeteredStream? If so, is there a reason not to?

There is a slight difference - one NetworkIOMeter can be shared by multiple MeteredStreams. We don't currently do this, for now we're just doing per-stream metering, but it is possible to have multiple streams incrementing the same meter

onbjerg · 2023-01-24T19:36:55Z

I would prob rename NetworkIOMeter to MeteredStreamMetrics instead then and add a comment explaining that it can be shared between multiple streams to make it a bit clearer - allows us to potentially extract it later for non-network stream metering

akirillo · 2023-01-24T20:10:15Z

I would prob rename NetworkIOMeter to MeteredStreamMetrics instead then and add a comment explaining that it can be shared between multiple streams to make it a bit clearer - allows us to potentially extract it later for non-network stream metering

I'm being a bit pedantic, but I'm wary of calling it Metrics because it's a struct containing in-process measurements, we have a separate struct NetworkIOMeterMetrics which actually exposes metrics via metrics-derive.

You're right though that it's not just for network streams, perhaps I'll call it StreamMeter? lol
Or MeteredStreamCounts?

…ng shareability

…e counts for peer sessions

akirillo · 2023-01-26T19:58:52Z

@onbjerg does this line up w how you wanted discovery ingress/egress monitored?

mattsse

I think we need to be precise about what we actually want to

measure
expose via metric counter/gauge

Because right now I'm a little lost and not sure about either. But I'm strongly against instrumenting a TCP stream with metrics because this is likely more expensive that just incrementing an Atomic via relaxed for statistical purposes, like we're doing on main

is "total bytes read/written by all sessions combined" even useful?
what do we want/need from a single session in order to rate the quality of the session?

mattsse · 2023-01-26T21:36:29Z

crates/net/common/src/metered_stream.rs

+    /// The [`MeteredStreamCounts`] structs this can increment ingress / egress counts for,
+    /// with optional metrics associated with each.
+    meters: HashMap<&'a str, (MeteredStreamCounts, Option<MeteredStreamMetrics>)>,


what are we using this for?

E.g. here

// Attach meter for total across all sessions metered_stream.add_meter(SESSIONS_METER_NAME, metered_stream_counts); // Attach meter for this session metered_stream.add_meter("session", MeteredStreamCounts::default());

we meter ingress/egress both on the meter shared by all sessions, and on a meter just for the current session

I see, so this is basically just

struct MeteredStream { inner, total: MeteredStreamCounts, session: MeteredStreamCounts }

?

yup, i admittedly jumped the gun on flexibility so it can be any number of MeteredStreamCounts

mattsse · 2023-01-26T21:38:30Z

crates/net/common/src/metered_stream.rs

+    }
+}
+
+impl MeteredStream<'_, UdpSocket> {


hmm it is a bit odd that we implement something called Stream for a UdpSocket

since it only sends individual packets, perhaps counting bytes read/written is not really ideal

If we just want to count whole packets, we could make an extension to the MeteredSender, i.e. make it work over any Sink & have a similar MeteredReceiver that works over any Stream, and use a UdpFramed here to have a type that impls Sink + Stream.

Or, of course, we could make a new type here that simply counts whole packets, but FWIW the MeteredSender isn't used anywhere yet

akirillo · 2023-01-26T22:15:39Z

strongly against instrumenting a TCP stream with metrics

Yeah, we're not exposing metrics over the TCP streams in the peer sessions, just counting up via Atomics

is "total bytes read/written by all sessions combined" even useful?

Honestly, as something that we'd want to reference in code, I don't think so. But now the MeteredStream is sufficiently flexible to easily remove the meter across all sessions, and only keep the per-session one.

what do we want/need from a single session in order to rate the quality of the session?

As I'm thinking through #908, I really think it's a matter of throughput and latency. Latency is out of scope here, but for throughput we need a rate. The new approach I'm working on for #1002 actually does read the per-stream ingress/egress byte values to calculate this rate, for use in dropping peers

gakonst · 2023-03-17T00:08:46Z

Closing for now as we couldn't figure out a great way to integrate this at the time - think we'll revisit metrics in the coming weeks and we'll cherry-pick parts of this and the other PR then!

akirillo added 2 commits January 11, 2023 13:38

wip metrics structs

de360a7

wip; unsure about using Gauge vs Counter

6eb1abc

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 13, 2023

akirillo self-assigned this Jan 13, 2023

akirillo added the S-blocked This cannot more forward until something else changes label Jan 13, 2023

onbjerg changed the title ~~Bandwidth metering metrics~~ feat: bandwidth metering metrics Jan 13, 2023

akirillo added 4 commits January 13, 2023 16:15

using counters, made shareable metrics struct

57dd40f

tracking per-stream metrics

94346cb

Merge branch 'main' into andrew/bandwidth-metering-metrics

2c75b38

properly instantiating per-stream metrics

25c488f

inlining format! args

0286195

akirillo mentioned this pull request Jan 15, 2023

Ability to specify labels through metrics-derive #892

Closed

merged main

e188073

gakonst reviewed Jan 22, 2023

View reviewed changes

crates/net/network/src/session/mod.rs Outdated Show resolved Hide resolved

akirillo mentioned this pull request Jan 23, 2023

Expose bandwidth metering metrics #783

Closed

akirillo added 2 commits January 23, 2023 10:24

using ingress/egress naming, only counting per-stream

6e31821

using metric labels

8247bcf

akirillo marked this pull request as ready for review January 23, 2023 18:33

akirillo requested review from mattsse and Rjected as code owners January 23, 2023 18:33

akirillo added 2 commits January 23, 2023 10:34

merged w main

38276c4

renamed IngressEgress -> NetworkIO

1f101ed

mattsse requested a review from onbjerg January 24, 2023 17:26

mattsse approved these changes Jan 24, 2023

View reviewed changes

akirillo removed the S-blocked This cannot more forward until something else changes label Jan 24, 2023

onbjerg reviewed Jan 24, 2023

View reviewed changes

crates/net/common/src/network_io_meter.rs Outdated Show resolved Hide resolved

crates/net/network/src/session/mod.rs Outdated Show resolved Hide resolved

akirillo added 5 commits January 24, 2023 12:10

implementing AsMut instead of MeterableStream, added comment explaini…

7a793d7

…ng shareability

merged main

4deabef

renamed to MeteredStreamCounts, MeteredStreamMetrics

36bd98c

renamed to metered_stream.rs

665b1ec

renamed imports

7ec3a01

akirillo requested a review from onbjerg January 24, 2023 22:48

akirillo added 8 commits January 25, 2023 12:32

MeteredStream holds HashMap<&str, MeteredStreamCounts>, using multipl…

1b8c45c

…e counts for peer sessions

proper lifetime annotation

99422fa

merged w main

c054634

added impl MeteredStream<Arc<UdpSocket>>

6b138b2

using 'static for Discv4Service, using Arc<MeteredStream<...>>

2cf8eea

using MeteredStream<'static, ...> in SessionsManager

0a81ca3

added test, updated docs

4cd2f75

Merge branch 'main' into andrew/bandwidth-metering-metrics

280bc1b

mattsse reviewed Jan 26, 2023

View reviewed changes

akirillo added 2 commits January 26, 2023 15:16

removed UDP instrumentation, only metering per-session

766297d

merged w main

2656498

gakonst added the S-needs-investigation This issue requires detective work to figure out what's going wrong label Feb 9, 2023

gakonst closed this Mar 17, 2023

gakonst deleted the andrew/bandwidth-metering-metrics branch March 17, 2023 00:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: bandwidth metering metrics #844

feat: bandwidth metering metrics #844

akirillo commented Jan 13, 2023

akirillo commented Jan 13, 2023

codecov-commenter commented Jan 15, 2023 •

edited

Loading

akirillo commented Jan 16, 2023

gakonst left a comment

gakonst commented Jan 23, 2023

akirillo commented Jan 23, 2023

akirillo commented Jan 23, 2023

mattsse left a comment

onbjerg left a comment •

edited

Loading

akirillo commented Jan 24, 2023

onbjerg commented Jan 24, 2023

akirillo commented Jan 24, 2023

akirillo commented Jan 26, 2023

mattsse left a comment

mattsse Jan 26, 2023

akirillo Jan 26, 2023

mattsse Jan 26, 2023

akirillo Jan 26, 2023

mattsse Jan 26, 2023

akirillo Jan 26, 2023

akirillo commented Jan 26, 2023

gakonst commented Mar 17, 2023

feat: bandwidth metering metrics #844

feat: bandwidth metering metrics #844

Conversation

akirillo commented Jan 13, 2023

akirillo commented Jan 13, 2023

codecov-commenter commented Jan 15, 2023 • edited Loading

Codecov Report

akirillo commented Jan 16, 2023

gakonst left a comment

Choose a reason for hiding this comment

gakonst commented Jan 23, 2023

akirillo commented Jan 23, 2023

akirillo commented Jan 23, 2023

mattsse left a comment

Choose a reason for hiding this comment

onbjerg left a comment • edited Loading

Choose a reason for hiding this comment

akirillo commented Jan 24, 2023

onbjerg commented Jan 24, 2023

akirillo commented Jan 24, 2023

akirillo commented Jan 26, 2023

mattsse left a comment

Choose a reason for hiding this comment

mattsse Jan 26, 2023

Choose a reason for hiding this comment

akirillo Jan 26, 2023

Choose a reason for hiding this comment

mattsse Jan 26, 2023

Choose a reason for hiding this comment

akirillo Jan 26, 2023

Choose a reason for hiding this comment

mattsse Jan 26, 2023

Choose a reason for hiding this comment

akirillo Jan 26, 2023

Choose a reason for hiding this comment

akirillo commented Jan 26, 2023

gakonst commented Mar 17, 2023

codecov-commenter commented Jan 15, 2023 •

edited

Loading

onbjerg left a comment •

edited

Loading