-
Notifications
You must be signed in to change notification settings - Fork 68
ConsensusLatency: Add consensus latency metrics #187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| package metrics | ||
|
|
||
| import ( | ||
| "time" | ||
|
|
||
| "github.com/relab/hotstuff" | ||
| "github.com/relab/hotstuff/eventloop" | ||
| "github.com/relab/hotstuff/logging" | ||
| "github.com/relab/hotstuff/metrics/types" | ||
| "github.com/relab/hotstuff/modules" | ||
| ) | ||
|
|
||
| func init() { | ||
hanish520 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| RegisterReplicaMetric("consensus-latency", func() any { | ||
| return &ConsensusLatency{} | ||
| }) | ||
| } | ||
|
|
||
| // ConsensusLatency processes consensus latency measurements and writes them to the metrics logger. | ||
| type ConsensusLatency struct { | ||
| metricsLogger Logger | ||
| id hotstuff.ID | ||
| wf Welford | ||
| } | ||
|
|
||
| // InitModule gives the module access to the other modules. | ||
| func (lr *ConsensusLatency) InitModule(mods *modules.Core) { | ||
hanish520 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| var ( | ||
| eventLoop *eventloop.EventLoop | ||
| logger logging.Logger | ||
| opts *modules.Options | ||
| ) | ||
|
|
||
| mods.Get( | ||
| &lr.metricsLogger, | ||
| opts, | ||
| &eventLoop, | ||
| &logger, | ||
| ) | ||
|
|
||
| lr.id = opts.ID() | ||
| eventLoop.RegisterHandler(hotstuff.ConsensusLatencyEvent{}, func(event any) { | ||
| latencyEvent := event.(hotstuff.ConsensusLatencyEvent) | ||
| lr.addLatency(latencyEvent.Latency) | ||
| }) | ||
|
|
||
| eventLoop.RegisterHandler(types.TickEvent{}, func(event any) { | ||
| lr.tick(event.(types.TickEvent)) | ||
| }, eventloop.Prioritize()) | ||
|
|
||
| logger.Info("Consensus Latency metric enabled") | ||
| } | ||
|
|
||
| // AddLatency adds a latency data point to the current measurement. | ||
| func (lr *ConsensusLatency) addLatency(latency time.Duration) { | ||
| millis := float64(latency) / float64(time.Millisecond) | ||
| lr.wf.Update(millis) | ||
| } | ||
|
|
||
| func (lr *ConsensusLatency) tick(_ types.TickEvent) { | ||
| mean, variance, count := lr.wf.Get() | ||
| event := &types.LatencyMeasurement{ | ||
| Event: types.NewReplicaEvent(uint32(lr.id), time.Now()), | ||
| Latency: mean, | ||
| Variance: variance, | ||
| Count: count, | ||
| } | ||
| lr.metricsLogger.Log(event) | ||
| lr.wf.Reset() | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I understand is that
Blockis sent over the wire. So, if a remote replica created this block, won't the latency be incorrect since the nodes don't have perfectly synchronized time?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
Blockis sent on the wire like this:So this
tsfield is not sent over the wire; I believe it is a local-only timestamp.@hanish520 Please confirm my understanding. I also suggest to rename the field to
timestampThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. So the timestamp is recorded here:
hotstuff/block.go
Line 30 in 04497a2
but
NewBlockis used to convert from proto to theBlock:hotstuff/internal/proto/hotstuffpb/convert.go
Line 149 in 461e32c
As I understand, it measures "local commit latency". I.e., it measures the latency between block arrival and commit if the replica is an acceptor. For leader, this latency measurement is very tiny, no? @hanish520 is this the intention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This metrics is used to measure the commit latency at the leader when used in the network simulation mode. This metrics may not make sense in the normal mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean with network simulation mode? Is this done with
hotstuff runlocally?Otherwise I understand, and I would suggest a different way to do this: put timestamps in propose and commit events instead of the block. Then in the
ConsensusLatency, you can filter blocks that were committed/proposed by other replicas than yourself. This way it could work regardless of the mode.I can come up with some code for this soon