[Consensus] add hotstuff view to ping route #1462

zhangchiqing · 2021-10-07T19:03:43Z

For https://github.com/dapperlabs/flow-go/issues/5920

jordanschalm · 2021-10-07T20:01:13Z

module/metrics.go

-	// NodeInfo tracks the software version and sealed height of a node
-	NodeInfo(node *flow.Identity, nodeInfo string, version string, sealedHeight uint64)
+	// NodeInfo tracks the software version, sealed height and hotstuff view of a node
+	NodeInfo(node *flow.Identity, nodeInfo string, version string, sealedHeight uint64, hotstuffChain string, hotstuffCurView uint64)


It doesn't seem like hotstuffChain argument is included in any of the implementations (eg here)

jordanschalm · 2021-10-07T20:03:12Z

network/message/ping.proto

@@ -8,4 +8,5 @@ message PingRequest {
 message PingResponse {
    string version = 1;     // node software version
    uint64 blockHeight = 2; // latest sealed block height
+    uint64 hotstuffView = 3; // latest hotstuff cur view


Do we need to regenerate pb.go files as well?

jordanschalm · 2021-10-07T21:04:11Z

network/message/Makefile

 # go get github.com/gogo/protobuf/protoc-gen-gofast
+# brew install protobuf


We should be able to put at least the go get under make install-tools.

I don't think we assume people have brew installed anywhere else though, so adding that could be breaking.

Here's what grpc recommends for protoc installation:
https://grpc.io/docs/protoc-installation/

zhangchiqing · 2021-10-07T21:52:28Z

Tested with localnet and it worked.

zhangchiqing · 2021-10-07T21:56:11Z

integration/localnet/Makefile

-	docker-compose -f docker-compose.metrics.yml up -d --remove-orphans
-	DOCKER_BUILDKIT=1 COMPOSE_DOCKER_CLI_BUILD=1 docker-compose -f docker-compose.nodes.yml up --remove-orphans --build -d


This is useful, however, I had to remove it, because the --remove-orphans will stop the containers created by docker-compose -f docker-compose.metrics.yml

Kay-Zee · 2021-10-07T21:56:35Z

module/metrics/ping.go

+	pc.hotstuffCurView.With(prometheus.Labels{
+		LabelNodeID:      node.NodeID.String(),
+		LabelNodeAddress: node.Address,
+		LabelNodeRole:    node.Role.String(),


is it worth only reporting if it's collection/consensus? so we don't have a ton of zeros in our metrics?

Already did. Only consensus will report this metrics.
I was thinking to report view for collection in the future, but it's a bit more complex, because we would need to also report chain ID.

For now, I think consensus hotstuff view is enough.

I could remove this label as well

codecov-commenter · 2021-10-08T14:02:54Z

Codecov Report

Merging #1462 (4c3798d) into master (adcc2f3) will decrease coverage by 0.00%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##           master    #1462      +/-   ##
==========================================
- Coverage   55.30%   55.29%   -0.01%     
==========================================
  Files         516      516              
  Lines       32168    32181      +13     
==========================================
+ Hits        17791    17795       +4     
- Misses      11990    11999       +9     
  Partials     2387     2387

Flag	Coverage Δ
unittests	`55.29% <25.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/scaffold.go	`0.91% <0.00%> (-0.02%)`	⬇️
engine/access/ping/engine.go	`0.00% <0.00%> (ø)`
network/p2p/ping.go	`51.06% <50.00%> (-1.21%)`	⬇️
engine/collection/synchronization/engine.go	`63.97% <0.00%> (+1.07%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adcc2f3...4c3798d. Read the comment docs.

vishalchangrani · 2021-10-08T19:37:04Z

module/metrics.go

-	// NodeInfo tracks the software version and sealed height of a node
-	NodeInfo(node *flow.Identity, nodeInfo string, version string, sealedHeight uint64)
+	// NodeInfo tracks the software version, sealed height and hotstuff view of a node
+	NodeInfo(node *flow.Identity, nodeInfo string, version string, sealedHeight uint64, hotstuffCurView uint64)


@zhangchiqing - will all node type report this? or only consensus nodes

Only Consensus nodes will report metrics. See this screenshot

See the logic control here

zhangchiqing · 2021-10-08T21:06:44Z

bors merge

bors · 2021-10-08T21:16:40Z

Canceled.

zhangchiqing · 2021-10-08T22:05:35Z

bors merge

zhangchiqing · 2021-10-08T22:22:21Z

    core_rapid_test.go:157: 
        	Error Trace:	core_rapid_test.go:157
        	            				statemachine.go:78
        	            				engine.go:276
        	            				engine.go:158
        	            				engine.go:94
        	            				core_rapid_test.go:174
        	Error:      	Should be true
        	Test:       	TestRapidSync
        	Messages:   	Height 861ff6f1 was expected to be Received (or filled through a same-height block) and is Queued

was flakey

bors · 2021-10-08T22:45:45Z

Build succeeded:

zhangchiqing requested review from vishalchangrani and yhassanzadeh13 as code owners October 7, 2021 19:03

zhangchiqing requested review from jordanschalm, AlexHentschel and Kay-Zee and removed request for yhassanzadeh13 October 7, 2021 19:03

jordanschalm reviewed Oct 7, 2021

View reviewed changes

jordanschalm approved these changes Oct 7, 2021

View reviewed changes

zhangchiqing commented Oct 7, 2021

View reviewed changes

Kay-Zee reviewed Oct 7, 2021

View reviewed changes

zhangchiqing requested review from Kay-Zee and huitseeker October 8, 2021 16:22

yhassanzadeh13 approved these changes Oct 8, 2021

View reviewed changes

vishalchangrani reviewed Oct 8, 2021

View reviewed changes

vishalchangrani approved these changes Oct 8, 2021

View reviewed changes

add hotstuff view to ping route

4c3798d

zhangchiqing force-pushed the leo/add-hotstuff-view-to-ping branch from 4bce670 to 4c3798d Compare October 8, 2021 21:16

zhangchiqing mentioned this pull request Oct 8, 2021

v0.22 mainnet add hotstuff view #1470

Merged

bors bot merged commit 08b9420 into master Oct 8, 2021

bors bot deleted the leo/add-hotstuff-view-to-ping branch October 8, 2021 22:45

zhangchiqing mentioned this pull request Oct 13, 2021

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Consensus] add hotstuff view to ping route #1462

[Consensus] add hotstuff view to ping route #1462

zhangchiqing commented Oct 7, 2021

jordanschalm Oct 7, 2021

jordanschalm Oct 7, 2021

zhangchiqing Oct 7, 2021

jordanschalm Oct 7, 2021

huitseeker Oct 8, 2021

zhangchiqing commented Oct 7, 2021

zhangchiqing Oct 7, 2021

Kay-Zee Oct 7, 2021

zhangchiqing Oct 7, 2021

zhangchiqing Oct 7, 2021

codecov-commenter commented Oct 8, 2021 •

edited

vishalchangrani Oct 8, 2021

zhangchiqing Oct 8, 2021

zhangchiqing commented Oct 8, 2021

bors bot commented Oct 8, 2021

zhangchiqing commented Oct 8, 2021

zhangchiqing commented Oct 8, 2021

bors bot commented Oct 8, 2021

		# go get github.com/gogo/protobuf/protoc-gen-gofast
		# brew install protobuf

		docker-compose -f docker-compose.metrics.yml up -d --remove-orphans
		DOCKER_BUILDKIT=1 COMPOSE_DOCKER_CLI_BUILD=1 docker-compose -f docker-compose.nodes.yml up --remove-orphans --build -d

[Consensus] add hotstuff view to ping route #1462

[Consensus] add hotstuff view to ping route #1462

Conversation

zhangchiqing commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangchiqing commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 8, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangchiqing commented Oct 8, 2021

bors bot commented Oct 8, 2021

zhangchiqing commented Oct 8, 2021

zhangchiqing commented Oct 8, 2021

bors bot commented Oct 8, 2021

codecov-commenter commented Oct 8, 2021 •

edited