ConsensusLatency: Add consensus latency metrics

[hanish520]

No description provided.

[meling]

Minor changes suggested @hanish520

[meling]

@AlanRostem Could you take a look at this and provide some guidance on possible issues related to migration to the new package structure PR, or other suggested improvements.

[AlanRostem]

Please see my review comments... Merging package-restructuring into this branch is necessary to apply the changes I requested.

[AlanRostem]

What I understand is that Block is 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?

[meling]

The Block is sent on the wire like this:

message Block {
  bytes Parent = 1;
  QuorumCert QC = 2;
  uint64 View = 3;
  bytes Command = 4;
  uint32 Proposer = 5;
}

So this ts field 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 timestamp

[AlanRostem]

I see. So the timestamp is recorded here:

hotstuff/block.go

Line 30 in 04497a2

ts: time.Now(),

but NewBlock is used to convert from proto to the Block:

hotstuff/internal/proto/hotstuffpb/convert.go

Line 149 in 461e32c

return hotstuff.NewBlock(

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?

[hanish520]

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.

[AlanRostem]

What do you mean with network simulation mode? Is this done with hotstuff run locally?

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

[meling]

@AlanRostem My idea was to merge this into master and then merge master into package-restructuring. Do you see any issues with that?

[AlanRostem]

@AlanRostem My idea was to merge this into master and then merge master into package-restructuring. Do you see any issues with that?

Ah, I misunderstood. Yes this is doable as long as we address the changes I requested upon merging master into package-restructuring.

Though, I am still wondering about the block timestamps I mentioned in my other comment...

Dismissing my review since I will address them myself when merging into package-restructuring