eventbus: add metrics #2038

sukunrt · 2023-02-01T11:42:37Z

Related to: #2020

sukunrt · 2023-02-01T11:43:50Z

You'll need to change

"datasource": {
                                "type": "prometheus",
                                "uid": "aMik1H0Vz"
                        },

datasource.uid to your prometheus datasource uid.

p2p/host/eventbus/basic_metrics.go

marten-seemann · 2023-02-02T21:37:58Z

p2p/host/eventbus/basic_metrics.go

+	subscriberQueueLength = prometheus.NewHistogramVec(
+		prometheus.HistogramOpts{
+			Namespace: metricNamespace,
+			Name:      "subscriber_queue_length",
+			Help:      "Subscriber queue length",
+			Buckets:   prometheus.ExponentialBuckets(1.0, 2.0, 10),
+		},
+		[]string{"subscriber_name"},
+	)
+	subscriberQueueFull = prometheus.NewGaugeVec(
+		prometheus.GaugeOpts{
+			Namespace: metricNamespace,
+			Name:      "subscriber_queue_full",
+			Help:      "Subscriber Queue completely full",
+		},
+		[]string{"subscriber_name"},
+	)
+	subscriberEventQueued = prometheus.NewCounterVec(
+		prometheus.CounterOpts{
+			Namespace: metricNamespace,
+			Name:      "subscriber_event_queued",
+			Help:      "Event Queued for subscriber",
+		},
+		[]string{"subscriber_name"},
+	)


I'd like to avoid having subscribers pass in a name. Can we just make this per event?

But the queues are per subscriber and not per event.
We can remove the subscriber metrics and just track whether for a given event type one of the subscriber queues was full. Should I do that for now?

Out of curiosity, what's the issue with subscribers passing in a name?

marten-seemann · 2023-02-02T21:41:13Z

p2p/host/eventbus/basic.go

@@ -57,12 +67,22 @@ func NewBus() event.Bus {
 	}
 }

+func NewBusWithMetrics() event.Bus {


This works totally fine, but we usually use the option pattern:

type Option func(*basicBus) error func NewBus(opts ...Option) event.Bus

will fix. This would also be backwards compatible. nice :)

This is what we did for the swarm: https://github.com/libp2p/go-libp2p/blob/master/p2p/net/swarm/swarm.go#L74-L79

fixed. Not returning error for now since there's only one option that can't error.

marten-seemann · 2023-02-03T02:51:30Z

p2p/host/eventbus/basic_metrics.go

+	subscriberEventQueued.WithLabelValues(name).Inc()
+}
+
+func stripPrefix(s string) string {


Nit: https://pkg.go.dev/strings#TrimPrefix

sukunrt · 2023-02-03T04:51:48Z

@marten-seemann Addressed review comments. Removed custom passed in names from all subscribers for now. they are set to the filename. If you want I'll add them back.

marten-seemann · 2023-02-03T05:18:35Z

p2p/host/eventbus/basic_metrics.go

+	notificationTime = prometheus.NewHistogramVec(
+		prometheus.HistogramOpts{
+			Namespace: metricNamespace,
+			Name:      "notification_time_microseconds",


There's a convention in Prometheus to use SI units: https://prometheus.io/docs/practices/naming/. So this should be notification_time_seconds.

Thinking about this a bit more, this measurement doesn't seem as useful any more:

If all channels don't block, this takes basically no time at all, we're just adding an event to every channel.

If a channel blocks because the subscriber is not reading from the channel, it will block indefinitely, and then this event won't be emitted.

I suggest remote it.

marten-seemann · 2023-02-03T05:19:11Z

p2p/host/eventbus/basic_metrics.go

+	numSubscribers = prometheus.NewGaugeVec(
+		prometheus.GaugeOpts{
+			Namespace: metricNamespace,
+			Name:      "num_subscribers",


Suggested change

Name: "num_subscribers",

Name: "subscribers_total",

In Grafana, this would look pretty cool as a Gauge:

marten-seemann · 2023-02-03T05:27:04Z

p2p/host/eventbus/grafana-dashboards/eventbus.json

+            "uid": "${DS_PROMETHEUS}"
+          },
+          "editorMode": "builder",
+          "expr": "sum by(event) (rate(libp2p_eventbus_events_emitted_total[1m]))",


It's better to use the $__rate_interval variable instead of hardcoding a value. Also, I don't think you need the sum. This configuration should work:

removed hardcoding

sukunrt · 2023-02-03T07:01:58Z

@marten-seemann addressed review comments.

sukunrt · 2023-02-03T08:29:55Z

made subscriber queue a state timeline

MarcoPolo · 2023-02-04T00:52:11Z

p2p/host/eventbus/basic_metrics.go

+var initMetricsOnce sync.Once
+
+func initMetrics() {
+	prometheus.MustRegister(eventsEmitted, totalSubscribers, subscriberQueueLength, subscriberQueueFull, subscriberEventQueued)


Let's have this accept a registerer function and not register with the default registerer. Example: https://github.com/libp2p/go-libp2p/blob/36db69d65efc54ee5e028a02c6cefd82dd985f71/p2p/host/resource-manager/obs/stats.go#LL143C6-L143C14.

By default we can still use the default registerer, but there should be a way to override that.

fixed. I'm using

type MetricsTracerOption = func(*metricsTracerSetting) func NewMetricsTracer(opts ...MetricsTracerOption) MetricsTracer

MarcoPolo · 2023-02-04T01:04:18Z

p2p/host/eventbus/basic_metrics.go

+		},
+		[]string{"event"},
+	)
+	subscriberQueueLength = prometheus.NewHistogramVec(


Why is this a histogram? I think this should be a gauge. What do you think was the value in having this be a histogram?

If it's a gauge, the promql query also becomes simpler and correct. I think the current query in the grafana dashboard is wrong. It's showing you the increase in queue length. If you're full, you'll see that number go to 0 since it's not increasing.

I wanted to look at things like 90%ile queue length. I guess we can ignore that since if the consumer is slow the queue will get full eventually. Having a gauge alerts us to increasing queue length which should be enough.

I didn't realise the current query would go to 0, I thought sum / count would stay constant. I just checked, it does go to 0 😅

marten-seemann

I added two minor changes to this PR:

Removing the version number from the subscriber event name. This makes it more pleasant to view in Grafana.
A number of minor tweaks to the Grafana dashboard.

I think this is ready to go now. Thanks to @sukunrt for doing all the heavy lifting here!

This has been addressed.

sukunrt mentioned this pull request Feb 1, 2023

eventbus: expose metrics #2020

Closed

marten-seemann reviewed Feb 1, 2023

View reviewed changes

p2p/host/eventbus/basic_metrics.go Outdated Show resolved Hide resolved

sukunrt marked this pull request as ready for review February 2, 2023 14:11

marten-seemann reviewed Feb 2, 2023

View reviewed changes

p-shahi changed the title ~~add metrics to eventbus~~ eventbus: add metrics Feb 2, 2023

marten-seemann mentioned this pull request Feb 3, 2023

autonat (?): node frequently changes its mind about its reachability status #2046

Closed

marten-seemann reviewed Feb 3, 2023

View reviewed changes

sukunrt added 12 commits February 3, 2023 10:24

add events emitted metric to eventbus

12a3a37

properly export grafana dashboard

4dbcbf5

Add eventbus subscriber and notification time metrics

8fa00a2

expose name in subscription

6a0025b

use atomic.AddInt64 for backwards compatibility

18351f9

make metrics optin

b0f7fc0

update dashboard json

be314c4

follow options constructor pattern for eventbus

f61cd4e

remove unused type

791f746

Review comments

8513178

remove event name

99392b0

remove whitespace diff

27ea103

marten-seemann reviewed Feb 3, 2023

View reviewed changes

address review changes. remove notification time

96ee6b2

sukunrt force-pushed the eventbus-metrics branch from bae8856 to 96ee6b2 Compare February 3, 2023 06:57

fix subscribers count

58d7c8d

make subscriber queue state timeline

df7d6de

MarcoPolo previously requested changes Feb 4, 2023

View reviewed changes

allow custom registerer. Make queue length gauge

33dcbdf

sukunrt requested review from MarcoPolo and marten-seemann and removed request for MarcoPolo and marten-seemann February 4, 2023 04:20

marten-seemann added 2 commits February 5, 2023 10:34

eventbus: remove package version from subscriber metrics

05f0194

eventbus: minor dashboard tweaks

1897d9e

marten-seemann approved these changes Feb 4, 2023

View reviewed changes

marten-seemann mentioned this pull request Feb 4, 2023

feat(eventbus): warn applications whenever events aren't read #2026

Closed

marten-seemann linked an issue Feb 4, 2023 that may be closed by this pull request

eventbus: expose metrics #2020

Closed

marten-seemann merged commit 3598171 into libp2p:master Feb 4, 2023

marten-seemann mentioned this pull request Feb 5, 2023

swarm: emit PeerConnectedness event from swarm instead of from hosts #1574

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eventbus: add metrics #2038

eventbus: add metrics #2038

sukunrt commented Feb 1, 2023

sukunrt commented Feb 1, 2023

marten-seemann Feb 2, 2023

sukunrt Feb 3, 2023 •

edited

Loading

marten-seemann Feb 2, 2023

sukunrt Feb 3, 2023

marten-seemann Feb 3, 2023

sukunrt Feb 3, 2023

marten-seemann Feb 3, 2023

sukunrt Feb 3, 2023

sukunrt commented Feb 3, 2023

marten-seemann Feb 3, 2023

marten-seemann Feb 3, 2023

sukunrt Feb 3, 2023

marten-seemann Feb 3, 2023

marten-seemann Feb 3, 2023

sukunrt Feb 3, 2023

marten-seemann Feb 3, 2023

sukunrt Feb 3, 2023

sukunrt commented Feb 3, 2023

sukunrt commented Feb 3, 2023

MarcoPolo Feb 4, 2023

sukunrt Feb 4, 2023

MarcoPolo Feb 4, 2023

sukunrt Feb 4, 2023

marten-seemann left a comment

eventbus: add metrics #2038

eventbus: add metrics #2038

Conversation

sukunrt commented Feb 1, 2023

sukunrt commented Feb 1, 2023

Choose a reason for hiding this comment

sukunrt Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukunrt commented Feb 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukunrt commented Feb 3, 2023

sukunrt commented Feb 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marten-seemann left a comment

Choose a reason for hiding this comment

sukunrt Feb 3, 2023 •

edited

Loading