Backport HotStuff view metric fixes from v0.22-mainnet
#1477
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1477 +/- ##
==========================================
- Coverage 55.12% 55.11% -0.02%
==========================================
Files 517 517
Lines 32330 32333 +3
==========================================
- Hits 17823 17819 -4
- Misses 12116 12122 +6
- Partials 2391 2392 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
cmd/scaffold.go
Outdated
| HotstuffViewFun: func() (uint64, error) { | ||
| return 0, nil | ||
| }, |
There was a problem hiding this comment.
If we set it in the else of the following if, wouldn't that let us return a non-nil error for non-consensus nodes that would say "error: you're looking at something that only makes sense for a consensus node!" ...?
There was a problem hiding this comment.
The result seems to be the same (see here) but putting some context in the error is definitely clearer 👍
| // we can exclude metrics from non-consensus nodes by checking if the value is above 0. | ||
| // consensus nodes will start this metrics value with 0 as well, and won't report, but that's | ||
| // OK, because their hotstuff view will quickly go up and start reporting. | ||
| if hotstuffCurView > 0 { |
There was a problem hiding this comment.
Thanks @jordanschalm for fixing this.
I removed the label Role, because only consensus nodes will report this methods, not even collection nodes.
I've tested with localnet, and it worked.
I tested last time as well, I thought it worked, but actually I didn't run the localnet long enough to catch the bug.
…com:onflow/flow-go into jordan/backport-hotstuff-view-metrics-fixes
|
bors merge |
1477: Backport HotStuff view metric fixes from `v0.22-mainnet` r=jordanschalm a=jordanschalm Fixes two issues with HotStuff view metric from Mainnet deployment branch: * `HotstuffViewFun` unset caused panic on non-consensus nodes * `hotstuffCurView` metric missing `role` label Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Kay-Zee <kan@axiomzen.co> Co-authored-by: Leo Zhang (zhangchiqing) <zhangchiqing@gmail.com> Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
|
Build failed: |
|
bors merge |
Fixes two issues with HotStuff view metric from Mainnet deployment branch:
HotstuffViewFununset caused panic on non-consensus nodeshotstuffCurViewmetric missingrolelabel