feat: eventbus: log error on slow consumers #3031
Conversation
Slow consumers can stall libp2p in hard to debug ways. For example, a slow consumer of Identify events can prevent NewStreams from opening. In some cases we do want this back-pressure, but in other cases it is a bug in the user's application (or even in go-libp2p). The recommended approach has been to use metrics with Grafana and Prometheus to identify these issues. In practice, that's been a non-trivial task for users to setup. A simple log would help identify these issues. Note that the codepath is relatively unchanged in the normal case. Only if a producer is stalled will the extra work be put in. Co-authored-by: Wondertan <[email protected]>
|
Yes, I will try it. |
p2p/host/eventbus/basic.go
Outdated
| select { | ||
| case sink.ch <- evt: | ||
| default: | ||
| n.slowConsumerTimer = emitAndLogError(n.slowConsumerTimer, wildcardType, evt, sink) | ||
| } | ||
| } | ||
| n.RUnlock() |
There was a problem hiding this comment.
This won't work with an RLock, right? We need Lock since we're updating state on n
I think it's fine to create the timer right when we create the node. I don't think there are that many nodes in total.
There was a problem hiding this comment.
I didn't realize this was an RLock. Thanks for highlighting that. I've updated the code to set the timer within a lock. It's technically possible to allocate two or more stopped timers, but that should be fine as it'll get cleaned up by the GC.
I think it's fine to create the timer right when we create the node.
I considered this, but there's no way to create a stopped timer. You must create an armed one. So if you are going through the trouble of setting one, you may as well use it.
There was a problem hiding this comment.
I'm fine with the current state of the code.
The way I create a stopped timer is to create one for MaxInt64 and then Reset. Not resetting such a timer is also fine.
Slow consumers can stall libp2p in hard to debug ways. For example, a slow consumer of Identify events can prevent NewStreams from opening. In some cases we do want this back-pressure, but in other cases it is a bug in the user's application (or even in go-libp2p). The recommended approach has been to use metrics with Grafana and Prometheus to identify these issues. In practice, that's been a non-trivial task for users to setup. A simple log would help identify these issues.
Note that the codepath is relatively unchanged in the normal case. Only if a producer is stalled will the extra work be put in.
Closes #2361 . See my comment #2983 (comment) for why this is brought up again.
@vyzo Could you please try to repro your issue on this branch?