Khalil/5766 improve epochs metrics #1185

kc1116 · 2021-08-22T20:23:37Z

This PR adds the following metrics

consensus_compliance_current_epoch_final_view : the final view of the current epoch with the greatest counter
consensus_compliance_current_dkg_phase1_final_view: the final view of phase 1 of the current epochs DKG with the greatest counter
consensus_compliance_current_dkg_phase2_final_view: the final view of phase 2 of the current epochs DKG with the greatest counter
consensus_compliance_current_dkg_phase3_final_view: the final view of phase 3 of the current epochs DKG with the greatest counter

codecov-commenter · 2021-08-23T14:30:42Z

Codecov Report

Merging #1185 (313c006) into master (0e5b3f3) will increase coverage by 0.01%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master    #1185      +/-   ##
==========================================
+ Coverage   55.69%   55.70%   +0.01%     
==========================================
  Files         482      482              
  Lines       29584    29632      +48     
==========================================
+ Hits        16476    16507      +31     
- Misses      10858    10868      +10     
- Partials     2250     2257       +7

Flag	Coverage Δ
unittests	`55.70% <60.00%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
state/protocol/badger/state.go	`55.95% <60.00%> (+0.15%)`	⬆️
...sus/approvals/assignment_collector_statemachine.go	`42.30% <0.00%> (-7.70%)`	⬇️
...ngine/common/synchronization/finalized_snapshot.go	`66.66% <0.00%> (-5.13%)`	⬇️
engine/consensus/dkg/reactor_engine.go	`75.38% <0.00%> (-0.59%)`	⬇️
module/dkg/controller.go	`68.79% <0.00%> (ø)`
fvm/bootstrap.go	`73.99% <0.00%> (+0.58%)`	⬆️
module/signature/threshold.go	`76.47% <0.00%> (+1.47%)`	⬆️
cmd/bootstrap/utils/key_generation.go	`46.66% <0.00%> (+2.22%)`	⬆️
module/signature/signer_store.go	`50.00% <0.00%> (+50.00%)`	⬆️

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 0e5b3f3...313c006. Read the comment docs.

jordanschalm

👌 Looks good. Once we extend the tests to cover the epoch 1->2 transition, this is good to go for me.

module/metrics/compliance.go

state/protocol/badger/state.go

jordanschalm · 2021-08-23T20:17:20Z

state/protocol/badger/mutator_test.go

@@ -470,6 +470,20 @@ func TestExtendEpochTransitionValid(t *testing.T) {
 		metrics.On("CurrentEpochPhase", initialPhase).Once()
 		metrics.On("CommittedEpochFinalView", finalView).Once()

+		metrics.On("CurrentEpochFinalView", finalView).Once()
+
+		currentDKGPhase1FinalView, err := initialCurrentEpoch.DKGPhase1FinalView()


Likewise here, can use

flow-go/state/protocol/convert.go

Line 150 in 3d3a19b

func DKGPhaseViews(epoch Epoch) (phase1FinalView uint64, phase2FinalView uint64, phase3FinalView uint64, err error) {

jordanschalm · 2021-08-23T20:17:32Z

state/protocol/badger/state_test.go

@@ -47,6 +47,19 @@ func TestBootstrapAndOpen(t *testing.T) {
 		complianceMetrics.On("CommittedEpochFinalView", finalView).Once()
 		complianceMetrics.On("CurrentEpochCounter", counter).Once()
 		complianceMetrics.On("CurrentEpochPhase", phase).Once()
+		complianceMetrics.On("CurrentEpochFinalView", finalView).Once()
+
+		currentDKGPhase1FinalView, err := epoch.DKGPhase1FinalView()


Likewise here, can use

flow-go/state/protocol/convert.go

Line 150 in 3d3a19b

func DKGPhaseViews(epoch Epoch) (phase1FinalView uint64, phase2FinalView uint64, phase3FinalView uint64, err error) {

jordanschalm · 2021-08-23T20:17:41Z

state/protocol/badger/state_test.go

+		require.NoError(t, err)
+		complianceMetrics.On("CurrentEpochFinalView", currentEpochFinalView).Once()
+
+		currentDKGPhase1FinalView, err := committedPhaseSnapshot.Epochs().Current().DKGPhase1FinalView()


Likewise here, can use

flow-go/state/protocol/convert.go

Line 150 in 3d3a19b

func DKGPhaseViews(epoch Epoch) (phase1FinalView uint64, phase2FinalView uint64, phase3FinalView uint64, err error) {

jordanschalm · 2021-08-23T20:22:04Z

state/protocol/badger/mutator_test.go

+
+		currentDKGPhase3FinalView, err := initialCurrentEpoch.DKGPhase3FinalView()
+		require.NoError(t, err)
+		metrics.On("CurrentDKGPhase3FinalView", currentDKGPhase3FinalView).Once()


Nice 👍

Could we also assert that the metrics are updated correctly after the epoch transition. Can use the same pattern as is used for checking current epoch counter on lines 697/702 below:

flow-go/state/protocol/badger/mutator_test.go

Lines 695 to 705 in a603c08

// expect epoch transition once we finalize block 9

consumer.On("EpochTransition", epoch2Setup.Counter, block9.Header).Once()

metrics.On("CurrentEpochCounter", epoch2Setup.Counter)

err = state.Finalize(block8.ID())

require.NoError(t, err)

err = state.Finalize(block9.ID())

require.NoError(t, err)

metrics.AssertCalled(t, "CurrentEpochCounter", epoch2Setup.Counter)

consumer.AssertCalled(t, "EpochTransition", epoch2Setup.Counter, block9.Header)

metrics.AssertExpectations(t)

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

zhangchiqing

Looks good, thanks for adding the metrics. Only left a minor comment.

zhangchiqing · 2021-08-24T20:10:36Z

state/protocol/badger/state.go

+	state.metrics.CurrentEpochFinalView(currentEpochFinalView)
+
+	dkgPhase1FinalView, dkgPhase2FinalView, dkgPhase3FinalView, err := protocol.DKGPhaseViews(snap.Epochs().Current())
+	if err != nil {
+		return fmt.Errorf("could not get dkg phase final view: %w", err)
+	}
+
+	state.metrics.CurrentDKGPhase1FinalView(dkgPhase1FinalView)
+	state.metrics.CurrentDKGPhase2FinalView(dkgPhase2FinalView)
+	state.metrics.CurrentDKGPhase3FinalView(dkgPhase3FinalView)


can we report all these metrics in one call?

Better that all these metrics are updated atomically, since they are all pointing at the "current" epoch.

What do you mean by in one call , In a single function call ?

jordanschalm

🎸

kc1116 added 2 commits August 20, 2021 17:42

add metrics for dkg phases 1-3 final view and current epoch final view

9454ade

update mocks, extend test case to cover new metrics

e41e46c

kc1116 requested review from zhangchiqing and jordanschalm August 22, 2021 20:23

kc1116 and others added 3 commits August 23, 2021 09:42

Update state_test.go

85d22c6

update TestBootstrapAndOpen_EpochCommitted to cover new metrics

b11e5de

Merge branch 'master' into khalil/5766-improve-epochs-metrics

6ae24c1

Merge branch 'master' into khalil/5766-improve-epochs-metrics

cefbeeb

jordanschalm reviewed Aug 23, 2021

View reviewed changes

kc1116 and others added 6 commits August 24, 2021 09:36

Update module/metrics/compliance.go

8046de0

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

Update module/metrics/compliance.go

8180890

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

Update state/protocol/badger/state.go

b555a56

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

Update state/protocol/badger/state.go

fbbef2a

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

check metrics after epoch transition

ba2cb21

lint fix

313c006

kc1116 requested a review from jordanschalm August 24, 2021 19:19

zhangchiqing approved these changes Aug 24, 2021

View reviewed changes

jordanschalm approved these changes Aug 24, 2021

View reviewed changes

Merge branch 'master' into khalil/5766-improve-epochs-metrics

ed87165

kc1116 merged commit 814b004 into master Aug 25, 2021

kc1116 deleted the khalil/5766-improve-epochs-metrics branch August 25, 2021 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Khalil/5766 improve epochs metrics #1185

Khalil/5766 improve epochs metrics #1185

kc1116 commented Aug 22, 2021

codecov-commenter commented Aug 23, 2021 •

edited

jordanschalm left a comment

jordanschalm Aug 23, 2021

jordanschalm Aug 23, 2021

jordanschalm Aug 23, 2021

jordanschalm Aug 23, 2021

zhangchiqing left a comment

zhangchiqing Aug 24, 2021

kc1116 Aug 24, 2021

zhangchiqing Aug 24, 2021

jordanschalm left a comment

	// expect epoch transition once we finalize block 9
	consumer.On("EpochTransition", epoch2Setup.Counter, block9.Header).Once()
	metrics.On("CurrentEpochCounter", epoch2Setup.Counter)
	err = state.Finalize(block8.ID())
	require.NoError(t, err)
	err = state.Finalize(block9.ID())
	require.NoError(t, err)
	metrics.AssertCalled(t, "CurrentEpochCounter", epoch2Setup.Counter)
	consumer.AssertCalled(t, "EpochTransition", epoch2Setup.Counter, block9.Header)

	metrics.AssertExpectations(t)

Khalil/5766 improve epochs metrics #1185

Khalil/5766 improve epochs metrics #1185

Conversation

kc1116 commented Aug 22, 2021

codecov-commenter commented Aug 23, 2021 • edited

Codecov Report

jordanschalm left a comment

Choose a reason for hiding this comment

jordanschalm Aug 23, 2021

Choose a reason for hiding this comment

jordanschalm Aug 23, 2021

Choose a reason for hiding this comment

jordanschalm Aug 23, 2021

Choose a reason for hiding this comment

jordanschalm Aug 23, 2021

Choose a reason for hiding this comment

zhangchiqing left a comment

Choose a reason for hiding this comment

zhangchiqing Aug 24, 2021

Choose a reason for hiding this comment

kc1116 Aug 24, 2021

Choose a reason for hiding this comment

zhangchiqing Aug 24, 2021

Choose a reason for hiding this comment

jordanschalm left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 23, 2021 •

edited