core/consensus: add histogram metric for consensus duration #1526
Conversation
Codecov ReportBase: 54.21% // Head: 54.30% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1526 +/- ##
==========================================
+ Coverage 54.21% 54.30% +0.08%
==========================================
Files 152 153 +1
Lines 19379 19414 +35
==========================================
+ Hits 10506 10542 +36
+ Misses 7450 7449 -1
Partials 1423 1423
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
core/consensus/component.go
Outdated
| @@ -301,6 +301,11 @@ func (c *Component) propose(ctx context.Context, duty core.Duty, value proto.Mes | |||
| Receive: t.recvBuffer, | |||
| } | |||
|
|
|||
| startTime := time.Now() | |||
| defer func() { | |||
There was a problem hiding this comment.
Use time.Since not time.Until, and use more concise defer pattern
defer func(t0 time.Time() {
consensusDuration.WithLabelValues(duty.Type.String()).Observe(time.Since(to)).Seconds())
}(time.Now())
dB2510
force-pushed
the
dhruv/consensusdelay
branch
from
6d612a8
to
57f8f9d
Compare
core/consensus/component.go
Outdated
| @@ -54,7 +54,7 @@ func Protocols() []protocol.ID { | |||
| type subscriber func(ctx context.Context, duty core.Duty, value proto.Message) error | |||
|
|
|||
| // newDefinition returns a qbft definition (this is constant across all consensus instances). | |||
| func newDefinition(nodes int, subs func() []subscriber) qbft.Definition[core.Duty, [32]byte] { | |||
| func newDefinition(nodes int, subs func() []subscriber, startTime time.Time) qbft.Definition[core.Duty, [32]byte] { | |||
There was a problem hiding this comment.
read the newDefinition godoc, this still instruments the wrong thing.
dB2510
force-pushed
the
dhruv/consensusdelay
branch
from
3f2d21a
to
ad6ee79
Compare
core/consensus/component.go
Outdated
| if err != nil && !isContextErr(err) { | ||
| err = qbft.Run[core.Duty, [32]byte](ctx, def, qt, duty, peerIdx, hash) | ||
| if isContextErr(err) { | ||
| consensusTimeout.WithLabelValues(duty.String()).Inc() |
There was a problem hiding this comment.
think about this one some more
There was a problem hiding this comment.
increase consensus timeout specifically for "context deadline exceeded" and not for both "context deadline exceeded" and "context canceled"?
There was a problem hiding this comment.
nope. The Run function only exits when the context is closed. So the context is always closed, does that mean we always timeout?
There was a problem hiding this comment.
We timeout when context is closed but we haven't DECIDED
dB2510
added
the
merge when ready
Adds a histogram metric to report consensus duration for each consensus instance.
category: feature
ticket: #1247