Skip to content
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

Backport HotStuff view metric fixes from v0.22-mainnet #1477

Merged
merged 8 commits into from
Oct 23, 2021
6 changes: 6 additions & 0 deletions cmd/scaffold.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ func (fnb *FlowNodeBuilder) EnqueueNetworkInit() {
}
return head.Height, nil
},
HotstuffViewFun: nil, // set in next code block, depending on role
}

// only consensus roles will need to report hotstuff view
Expand All @@ -191,6 +192,11 @@ func (fnb *FlowNodeBuilder) EnqueueNetworkInit() {

return curView, nil
}
} else {
// non-consensus will not report any hotstuff view
pingProvider.HotstuffViewFun = func() (uint64, error) {
return 0, fmt.Errorf("non-consensus nodes do not report hotstuff view in ping")
}
}

streamFactory, err := p2p.LibP2PStreamCompressorFactoryFunc(fnb.BaseConfig.LibP2PStreamCompression)
Expand Down
21 changes: 14 additions & 7 deletions module/metrics/ping.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func NewPingCollector() *PingCollector {
Namespace: namespaceNetwork,
Subsystem: subsystemGossip,
Help: "the hotstuff current view",
}, []string{LabelNodeID, LabelNodeAddress, LabelNodeRole, LabelNodeInfo}),
}, []string{LabelNodeID, LabelNodeAddress, LabelNodeInfo}),
}

return pc
Expand Down Expand Up @@ -65,10 +65,17 @@ func (pc *PingCollector) NodeInfo(node *flow.Identity, nodeInfo string, version
LabelNodeVersion: version}).
Set(float64(sealedHeight))

pc.hotstuffCurView.With(prometheus.Labels{
LabelNodeID: node.NodeID.String(),
LabelNodeAddress: node.Address,
LabelNodeInfo: nodeInfo,
}).
Set(float64(hotstuffCurView))
// we only need this metrics from consensus nodes.
// since non-consensus nodes will report this metrics as well, and the value will always be 0,
// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image

I tested last time as well, I thought it worked, but actually I didn't run the localnet long enough to catch the bug.

pc.hotstuffCurView.With(prometheus.Labels{
LabelNodeID: node.NodeID.String(),
LabelNodeAddress: node.Address,
LabelNodeInfo: nodeInfo,
}).
Set(float64(hotstuffCurView))
}
}