From 07179c452310b5f25e68d27e8cbb5962438076d4 Mon Sep 17 00:00:00 2001 From: Dhruv Bodani Date: Fri, 7 Oct 2022 12:11:17 +0530 Subject: [PATCH 1/2] skip negative participation reporting --- core/tracker/tracker.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/core/tracker/tracker.go b/core/tracker/tracker.go index 406489963..55ecb05f2 100644 --- a/core/tracker/tracker.go +++ b/core/tracker/tracker.go @@ -421,16 +421,13 @@ func newParticipationReporter(peers []p2p.Peer) func(context.Context, core.Duty, unexpectedEventsCounter.WithLabelValues(peer.Name).Inc() } else { absentPeers = append(absentPeers, peer.Name) - participationGauge.WithLabelValues(duty.Type.String(), peer.Name).Set(0) } } if fmt.Sprint(prevAbsent[duty.Type]) != fmt.Sprint(absentPeers) { if len(absentPeers) == 0 { log.Info(ctx, "All peers participated in duty") - } else if len(absentPeers) == len(peers) { - log.Info(ctx, "No peers participated in duty") - } else { + } else if len(absentPeers) != len(peers) { log.Info(ctx, "Not all peers participated in duty", z.Any("absent", absentPeers)) } } From 2c67051a3a652ac0f7a4b3e012ca1ace1a047d1e Mon Sep 17 00:00:00 2001 From: Dhruv Bodani Date: Fri, 7 Oct 2022 16:03:17 +0530 Subject: [PATCH 2/2] cleanup --- core/tracker/tracker.go | 18 ++++++++---- core/tracker/tracker_internal_test.go | 40 +++++++++++++++++++++++---- 2 files changed, 48 insertions(+), 10 deletions(-) diff --git a/core/tracker/tracker.go b/core/tracker/tracker.go index 55ecb05f2..707fc69ff 100644 --- a/core/tracker/tracker.go +++ b/core/tracker/tracker.go @@ -148,7 +148,7 @@ type Tracker struct { failedDutyReporter func(ctx context.Context, duty core.Duty, failed bool, component component, reason string) // participationReporter instruments duty peer participation. - participationReporter func(ctx context.Context, duty core.Duty, participatedShares map[int]bool, unexpectedPeers map[int]bool) + participationReporter func(ctx context.Context, duty core.Duty, failed bool, participatedShares map[int]bool, unexpectedPeers map[int]bool) } // New returns a new Tracker. The deleter deadliner must return well after analyser deadliner since duties of the same slot are often analysed together. @@ -195,7 +195,7 @@ func (t *Tracker) Run(ctx context.Context) error { // Analyse peer participation participatedShares, unexpectedShares := analyseParticipation(duty, t.events) - t.participationReporter(ctx, duty, participatedShares, unexpectedShares) + t.participationReporter(ctx, duty, failed, participatedShares, unexpectedShares) case duty := <-t.deleter.C(): delete(t.events, duty) } @@ -406,11 +406,16 @@ func isParSigEventExpected(duty core.Duty, pubkey core.PubKey, allEvents map[cor // newParticipationReporter returns a new participation reporter function which logs and instruments peer participation // and unexpectedPeers. -func newParticipationReporter(peers []p2p.Peer) func(context.Context, core.Duty, map[int]bool, map[int]bool) { +func newParticipationReporter(peers []p2p.Peer) func(context.Context, core.Duty, bool, map[int]bool, map[int]bool) { // prevAbsent is the set of peers who didn't participate in the last duty per type. prevAbsent := make(map[core.DutyType][]string) - return func(ctx context.Context, duty core.Duty, participatedShares map[int]bool, unexpectedShares map[int]bool) { + return func(ctx context.Context, duty core.Duty, failed bool, participatedShares map[int]bool, unexpectedShares map[int]bool) { + if len(participatedShares) == 0 && !failed { + // Ignore participation metrics and log for noop duties (like DutyAggregator) + return + } + var absentPeers []string for _, peer := range peers { if participatedShares[peer.ShareIdx()] { @@ -421,13 +426,16 @@ func newParticipationReporter(peers []p2p.Peer) func(context.Context, core.Duty, unexpectedEventsCounter.WithLabelValues(peer.Name).Inc() } else { absentPeers = append(absentPeers, peer.Name) + participationGauge.WithLabelValues(duty.Type.String(), peer.Name).Set(0) } } if fmt.Sprint(prevAbsent[duty.Type]) != fmt.Sprint(absentPeers) { if len(absentPeers) == 0 { log.Info(ctx, "All peers participated in duty") - } else if len(absentPeers) != len(peers) { + } else if len(absentPeers) == len(peers) { + log.Info(ctx, "No peers participated in duty") + } else { log.Info(ctx, "Not all peers participated in duty", z.Any("absent", absentPeers)) } } diff --git a/core/tracker/tracker_internal_test.go b/core/tracker/tracker_internal_test.go index f192325c7..81c5e5860 100644 --- a/core/tracker/tracker_internal_test.go +++ b/core/tracker/tracker_internal_test.go @@ -53,7 +53,9 @@ func TestTrackerFailedDuty(t *testing.T) { tr := New(analyser, deleter, []p2p.Peer{}, 0) tr.failedDutyReporter = failedDutyReporter - tr.participationReporter = func(_ context.Context, _ core.Duty, _ map[int]bool, _ map[int]bool) {} + tr.participationReporter = func(_ context.Context, _ core.Duty, failed bool, _ map[int]bool, _ map[int]bool) { + require.True(t, failed) + } go func() { for _, td := range testData { @@ -91,6 +93,9 @@ func TestTrackerFailedDuty(t *testing.T) { tr := New(analyser, deleter, []p2p.Peer{}, 0) tr.failedDutyReporter = failedDutyReporter + tr.participationReporter = func(_ context.Context, _ core.Duty, failed bool, _ map[int]bool, _ map[int]bool) { + require.False(t, failed) + } go func() { for _, td := range testData { @@ -353,9 +358,10 @@ func TestTrackerParticipation(t *testing.T) { count int lastParticipation map[int]bool ) - tr.participationReporter = func(_ context.Context, actualDuty core.Duty, actualParticipation map[int]bool, _ map[int]bool) { + tr.participationReporter = func(_ context.Context, actualDuty core.Duty, failed bool, actualParticipation map[int]bool, _ map[int]bool) { require.Equal(t, testData[count].duty, actualDuty) require.True(t, reflect.DeepEqual(actualParticipation, expectedParticipationPerDuty[testData[count].duty])) + require.False(t, failed) if count == 2 { // For third duty, last Participation should be equal to that of second duty. @@ -424,10 +430,11 @@ func TestUnexpectedParticipation(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) tr := New(analyser, deleter, peers, 0) - tr.participationReporter = func(_ context.Context, duty core.Duty, participatedShares map[int]bool, unexpectedPeers map[int]bool) { + tr.participationReporter = func(_ context.Context, duty core.Duty, failed bool, participatedShares map[int]bool, unexpectedPeers map[int]bool) { require.Equal(t, d, duty) require.True(t, reflect.DeepEqual(unexpectedPeers, map[int]bool{unexpectedPeer: true})) require.True(t, reflect.DeepEqual(participatedShares, participation)) + require.True(t, failed) cancel() } @@ -463,7 +470,7 @@ func TestDutyRandaoUnexpected(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) tr := New(analyser, deleter, peers, 0) - tr.participationReporter = func(_ context.Context, duty core.Duty, participatedShares map[int]bool, unexpectedPeers map[int]bool) { + tr.participationReporter = func(_ context.Context, duty core.Duty, failed bool, participatedShares map[int]bool, unexpectedPeers map[int]bool) { if duty.Type == core.DutyProposer { return } @@ -471,6 +478,7 @@ func TestDutyRandaoUnexpected(t *testing.T) { require.Equal(t, dutyRandao, duty) require.True(t, reflect.DeepEqual(unexpectedPeers, unexpected)) require.True(t, reflect.DeepEqual(participatedShares, participation)) + require.True(t, failed) cancel() } @@ -509,12 +517,13 @@ func TestDutyRandaoExpected(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) tr := New(analyser, deleter, peers, 0) - tr.participationReporter = func(_ context.Context, duty core.Duty, participatedShares map[int]bool, unexpectedPeers map[int]bool) { + tr.participationReporter = func(_ context.Context, duty core.Duty, failed bool, participatedShares map[int]bool, unexpectedPeers map[int]bool) { if duty.Type == core.DutyProposer { return } require.Equal(t, dutyRandao, duty) + require.True(t, failed) require.True(t, reflect.DeepEqual(unexpectedPeers, unexpected)) require.True(t, reflect.DeepEqual(participatedShares, participation)) @@ -739,4 +748,25 @@ func TestAnalyseDutyFailedAgg(t *testing.T) { require.Equal(t, sigAgg, comp) require.Equal(t, msgSigAgg, msg) }) + + t.Run("no aggregator found", func(t *testing.T) { + allEvents := make(map[core.Duty][]event) + allEvents[dutyAgg] = append(allEvents[dutyAgg], event{ + duty: dutyAgg, + component: scheduler, + }) + allEvents[dutyPrepAgg] = append(allEvents[dutyPrepAgg], event{ + duty: dutyPrepAgg, + component: sigAgg, + }) + allEvents[dutyAtt] = append(allEvents[dutyAtt], event{ + duty: dutyAtt, + component: sigAgg, + }) + + failed, comp, msg := analyseDutyFailed(dutyAgg, allEvents) + require.False(t, failed) + require.Equal(t, fetcher, comp) + require.Equal(t, "", msg) + }) }