Missing consensus telemetry events from FinalizationConsumer and CommunicatorConsumer

[AlexHentschel]

Metrika messaged us via slack saying that there is some lack of data in Mainnet 24:

none of the consensus nodes have the “proposer” label anymore?

Digging into the current telemetry events, I found that starting in Mainnet 24, the TelemetryConsumer captures

• none of the events emitted via the FinalizationConsumer
• none of the events emitted via the CommunicatorConsumer

Digging into the code, I found that we are not subscribing the TelemetryConsumer to the respective events (neither does the TelemetryConsumer state that it implements the consumer interface for the respective events).

This PR fixes the missing subscription problem.

Back-ported to v0.32 branch: #5021

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b05bb9c) 56.23% compared to head (68cf90e) 56.23%.
Report is 20 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5020   +/-   ##
=======================================
  Coverage   56.23%   56.23%           
=======================================
  Files         969      969           
  Lines       90362    90417   +55     
=======================================
+ Hits        50813    50848   +35     
- Misses      35764    35777   +13     
- Partials     3785     3792    +7     

Flag	Coverage Δ
unittests	56.23% <ø> (+<0.01%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[AlexHentschel]

This is a repeat of #4518, which I obviously forgot to merge 😭

I am trying to address Yurii's suggestion from an earlier PR here here. In a nutshell, Yurii's suggestion was to introduce a TelemetryConsumer interface that bundles the respective dependencies

type TelemetryConsumer interface {
	ParticipantConsumer
	CommunicatorConsumer
	FinalizationConsumer
	VoteCollectorConsumer
	TimeoutCollectorConsumer
}

I tried this and ran into the following challenges:

• We could combine the interfaces

   ParticipantConsumer
   CommunicatorConsumer
   FinalizationConsumer

  as these are all sub-interfaces of the hotstuff.Consumer interface where the Distributor provides the respective pub-sub implementation.
• However, the VoteCollectorConsumer and TimeoutCollectorConsumer interfaces are separate (neither part of hotstuff.Consumer interface nor distributed by Distributor)

I feel that implementing the suggestion would entail possibly wide-spread code changes:

1. VoteCollector and TimeoutCollector produce events that the EventLoop ingests. Therefore, I would be inclined to keep the interfaces separate and not merge them into

   flow-go/consensus/hotstuff/consumer.go

   Lines 347 to 351 in ccb723f

   	type Consumer interface {
   	FollowerConsumer
   	CommunicatorConsumer
   	ParticipantConsumer
   	}

2. Given that the TelemetryConsumer would ingest events from Consumer, as well as VoteCollectorConsumer and TimeoutCollectorConsumer, I would be inclined to to add a new Top-Level interface.

@durkmurder I have tried to address your suggestion in PR #5022. But it entails a lot of changes. Personally, I feel the code becomes a bit cleaner, but that might be very well my personal perception (implementor bias).

[durkmurder]

@durkmurder I have tried to address your suggestion in PR #5022. But it entails a lot of changes. Personally, I feel the code becomes a bit cleaner, but that might be very well my personal perception (implementor bias).

I think you are right it's not worth to merge {Vote, Timeout}CollectorConsumer with Consumer interfaces, current design works good if we don't forget to subscribe everything(but that's a well known problem for pub-sub systems). Everything else looks great. Thanks for the fix.

[durkmurder]

🚢