feat: tracks block part size in consensus block parts table #1067
Conversation
| schema.WriteBlockPart(conR.traceClient, rs.Height, rs.Round, peer.ID(), part.Index, schema.TransferTypeUpload) | ||
| schema.WriteBlockPart(conR.traceClient, rs.Height, | ||
| rs.Round, peer.ID(), part.Index, | ||
| schema.TransferTypeUpload, int64(parts.Size())) |
There was a problem hiding this comment.
To the reviewers: While an alternative approach could involve utilizing a helper function to measure the part size, it's worth noting that the Size() method within the protobuf message type Part already accomplishes the same task.
staheri14
changed the title
| }) | ||
| } | ||
|
|
||
| const ( | ||
| // BlockTable is the name of the table that stores metadata about consensus blocks. | ||
| // following schema: | ||
| // | ||
| // | time | height | timestamp | | ||
| // | time | height | unix_millisecond_timestamp | tx_count | square_size | block_size | proposer | last_commit_round | |
There was a problem hiding this comment.
consensus/reactor.go
Outdated
| part, err := msg.Part.ToProto() // this conversion is only needed to get the part size | ||
| // consistent with how it is measured in the gossipDataRoutine |
There was a problem hiding this comment.
[uber nit] since this comment is split across multiple lines, proposal to do something like this so that it's clear both lines apply to the code line on 341:
// The .ToProto conversion is only needed to get the part size.
// This is consistent with how it is measured in the gossipDataRoutine.
part, err := msg.Part.ToProto()There was a problem hiding this comment.
Incorporated you suggestion a82c574
| partSize := -1 | ||
| if err != nil { | ||
| partSize = part.Size() | ||
| } |
There was a problem hiding this comment.
[not blocking] Does this intentionally avoid checking for an error?
There was a problem hiding this comment.
Yes, it is intentional, because the earlier conversion (.toProto()) is merely for measuring the size and tracing it in influx DB, which is not a critical part of the consensus operation. Nevertheless, it is not expected to get any error.
There was a problem hiding this comment.
Yes, it is intentional, because the earlier conversion (.toProto()) is merely for measuring the size and tracing it in influx DB, which is not a critical part of the consensus operation.
Makes sense! Since it is not critical to the consensus operation, I think this could not propagate the error and log it instead but this feels kinda hacky:
part, err := msg.Part.ToProto()
if err != nil {
// log the error
} else {
schema.WriteBlockPart(conR.traceClient, msg.Height, msg.Round, e.Src.ID(), msg.Part.Index, schema.TransferTypeDownload, int64(part.Size()))
}I'm not familiar with this code so defer to what you already have.
|
We do have something similar that already exists: Lines 85 to 96 in 8f3e050 |
Nice, thanks for mentioning it! 👍 very helpful |
|
Converting this PR to draft as it seems we can derive data transmission rate using the already available Prometheus metrics. |
|
@evan-forbes The traffic rate analysis has been successfully conducted utilizing Prometheus metrics. Given this, do you think it is would be useful to monitor the block part sizes in InfluxDB? If not, I can proceed to close this pull request (we can always reopen it later if necessary). |
|
Closing it for now, may reopen it in the future if need be. |
Description
Part of #1055. This allows us to calculate the data transmission rate (bytes per second) incurred in the consensus reactor.