Add metrics for the events in the network output channels #5597

tomaka · 2020-04-09T10:04:45Z

There seems to be a high peak memory usage in the channels that send out network events to the rest of the subsystems.

This PR adds a metric for each possible event type in the channels.

I'm compiling Polkadot with this change to see how it goes.

tomaka · 2020-04-09T10:40:59Z

This seems to stay at 0 continuously on my local test (since we're polling every 5 seconds).
Does it make sense to use a Histogram here as well, like for the notifications queue?

twittner

This multi-staged setup is rather complicated. The code would benefit from an spmc channel I think but in the absence of it this must do.

arkpar · 2020-04-09T12:27:09Z

Looks good in general. The code seems to imply that the channels are always asynchronous. Is it absolutely guaranteed that there won't be a recursive deadlock on a mutex? E.g. unbounded_send that calls another unbounded_send?

tomaka · 2020-04-09T13:05:43Z

The code seems to imply that the channels are always asynchronous. Is it absolutely guaranteed that there won't be a recursive deadlock on a mutex?

It's impossible because any order sent to the network also goes through an unbounded channel, but I changed the implementation to only lock at the end and make this more fool-proof.

mxinden

I am a bit worried by the impact the single serialization point (lock) might have.

We don't depend on these metrics being consistent among each other. Recording metrics for a single event does not need to be atomic. Why not add an OutEventsMetrics struct in out_events.rs with Prometheus Gauges and Counters that would be registered through a registry passed to OutChannels::new. Using atomics instead of locks should reduce the contention and is (in this case) wait-free.

This multi-staged setup is rather complicated. The code would benefit from an spmc channel I think but in the absence of it this must do.

I agree with the complexity this pull request introduces. I have not done a lot of research. Is there no future-aware spmc or mpmc broadcast channel implementation out there? Wouldn't https://docs.rs/multiqueue/0.3.2/multiqueue/index.html do?

mxinden · 2020-04-09T14:09:06Z

client/network/src/service/out_events.rs

+/// Must be associated with an [`OutChannels`] before anything can be sent on it
+///
+/// > **Note**: Contrary to regular channels, this `Sender` is purposefully designed to not
+/// > implement the `Clone` trait. If someone adds a `#[derive(Clone)]` below, it is **wrong**.


Can you add in the note why you don't want multiple producers?

client/network/src/service/out_events.rs

tomaka · 2020-04-09T14:32:37Z

I totally disagree on the complexity. How is it complicated? You create a channel, then put the sender in a collection and send messages to all the senders in the collection at once.
It is exactly the same as what is being done in service.rs before this PR, except in a separate module.

tomaka · 2020-04-09T14:41:03Z

The code would benefit from an spmc channel I think but in the absence of it this must do.

Is there no future-aware spmc or mpmc broadcast channel implementation out there? Wouldn't https://docs.rs/multiqueue/0.3.2/multiqueue/index.html do?

That would mean tackling paritytech/polkadot-sdk#557 immediately, which is a different approach than what this PR does.

…hannels

tomaka · 2020-04-09T15:34:12Z

All right, I changed the PR to directly use Prometheus metrics. There is now a single Mutex that is locked only when pushing the sender in the collection and to clone an Arc.

client/network/src/service/out_events.rs

client/network/src/service.rs

client/network/src/service/out_events.rs

Co-Authored-By: Max Inden <mail@max-inden.de>

mxinden · 2020-04-09T16:47:00Z

client/network/src/service/out_events.rs

+				"sub_libp2p_out_events_num_channels",
+				"Number of internal active channels that broadcast network events",
+			)?, registry)?,
+			out_events_in: register(CounterVec::new(


Why not combine out_events_in and out_events_out into a single metric with a label direction and label values in and out?

mxinden · 2020-04-09T16:47:40Z

client/network/src/service/out_events.rs

+			)?, registry)?,
+			out_events_in: register(CounterVec::new(
+				Opts::new(
+					"sub_libp2p_out_events_notifications_opened_count",


I think this metric name is outdated, right? Shouldnt it be something like

Suggested change

"sub_libp2p_out_events_notifications_opened_count",

"sub_libp2p_out_events_notifications_count",

mxinden · 2020-04-09T16:48:10Z

client/network/src/service/out_events.rs

+			)?, registry)?,
+			out_events_out: register(CounterVec::new(
+				Opts::new(
+					"sub_libp2p_out_events_notifications_opened_count",


This metric name seems outdated, see same name above.

mxinden · 2020-04-09T16:48:55Z

client/network/src/service/out_events.rs

+			out_events_out: register(CounterVec::new(
+				Opts::new(
+					"sub_libp2p_out_events_notifications_opened_count",
+					"Number of events pending in the channels that broadcast network events"


Suggested change

"Number of events pending in the channels that broadcast network events"

"Number of events received from the channels that broadcast network events"

mxinden · 2020-04-09T16:49:37Z

client/network/src/service/out_events.rs

+
+struct Metrics {
+	// This list is ordered alphabetically
+	out_events_num_channels: Gauge<U64>,


why prefix each of the variable names with out_events?

client/network/service/out_events: Apply remaining suggestions

mxinden

Thanks for bearing with me!

Add metrics for the events in the network output channels

7ad9daa

tomaka added the A0-please_review Pull request needs code review. label Apr 9, 2020

tomaka requested review from twittner, mxinden and arkpar April 9, 2020 10:04

twittner approved these changes Apr 9, 2020

View reviewed changes

Documentation fixes

13c8c92

A couple fixes

33607a5

arkpar approved these changes Apr 9, 2020

View reviewed changes

Fix panic at destruction

ca87a4c

mxinden reviewed Apr 9, 2020

View reviewed changes

tomaka added 2 commits April 9, 2020 17:18

Merge remote-tracking branch 'upstream/master' into metrics-net-out-c…

318a87e

…hannels

Rework for direct Prometheus integration

0c5cef6

Don't lock on the Receiver

1c0a56e

tomaka requested a review from mxinden April 9, 2020 15:56

Another review address

d92feb8

mxinden reviewed Apr 9, 2020

View reviewed changes

tomaka and others added 3 commits April 9, 2020 18:40

Address review

d454b9a

Update client/network/src/service/out_events.rs

ebd9c16

Co-Authored-By: Max Inden <mail@max-inden.de>

Fix bad event name

07306db

mxinden reviewed Apr 9, 2020

View reviewed changes

tomaka and others added 3 commits April 9, 2020 18:51

Fix descriptions

1e61de6

Fix names

2c7b643

client/network/service/out_events: Apply remaining suggestions

f72863d

Merge pull request #3 from mxinden/metrics-net-out-channels

e99f029

client/network/service/out_events: Apply remaining suggestions

mxinden approved these changes Apr 9, 2020

View reviewed changes

tomaka added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Apr 9, 2020

tomaka merged commit 7f12a9f into paritytech:master Apr 9, 2020

tomaka deleted the metrics-net-out-channels branch April 9, 2020 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metrics for the events in the network output channels #5597

Add metrics for the events in the network output channels #5597

tomaka commented Apr 9, 2020 •

edited

Loading

tomaka commented Apr 9, 2020

twittner left a comment

arkpar commented Apr 9, 2020

tomaka commented Apr 9, 2020 •

edited

Loading

mxinden left a comment

mxinden Apr 9, 2020

tomaka commented Apr 9, 2020

tomaka commented Apr 9, 2020 •

edited

Loading

tomaka commented Apr 9, 2020 •

edited

Loading

mxinden Apr 9, 2020

mxinden Apr 9, 2020

mxinden Apr 9, 2020

mxinden Apr 9, 2020

mxinden Apr 9, 2020

mxinden left a comment

	"sub_libp2p_out_events_notifications_opened_count",
	"sub_libp2p_out_events_notifications_count",

	"Number of events pending in the channels that broadcast network events"
	"Number of events received from the channels that broadcast network events"

Add metrics for the events in the network output channels #5597

Add metrics for the events in the network output channels #5597

Conversation

tomaka commented Apr 9, 2020 • edited Loading

tomaka commented Apr 9, 2020

twittner left a comment

Choose a reason for hiding this comment

arkpar commented Apr 9, 2020

tomaka commented Apr 9, 2020 • edited Loading

mxinden left a comment

Choose a reason for hiding this comment

mxinden Apr 9, 2020

Choose a reason for hiding this comment

tomaka commented Apr 9, 2020

tomaka commented Apr 9, 2020 • edited Loading

tomaka commented Apr 9, 2020 • edited Loading

mxinden Apr 9, 2020

Choose a reason for hiding this comment

mxinden Apr 9, 2020

Choose a reason for hiding this comment

mxinden Apr 9, 2020

Choose a reason for hiding this comment

mxinden Apr 9, 2020

Choose a reason for hiding this comment

mxinden Apr 9, 2020

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

tomaka commented Apr 9, 2020 •

edited

Loading

tomaka commented Apr 9, 2020 •

edited

Loading

tomaka commented Apr 9, 2020 •

edited

Loading

tomaka commented Apr 9, 2020 •

edited

Loading