[Merged by Bors] - added debounce to log

[eserilev]

Issue Addressed

#4259

Proposed Changes

debounce spammy Unable to send message to the beacon processor log messages

Additional Info

We could potentially debounce other logs that have the potential to be "spammy".

After some feedback we decided to additionally add the following change:

create a newtype wrapper around mpsc::Sender<BeaconWorkEvent<T>>. When there is an error on the try_send method on the wrapper, we increase a counter metric with one label per work type.

[AgeManning]

This seems fine to me, however CI is failing. Could you run cargo fmt to get CI to pass.

[michaelsproul]

Looks good to me but I wouldn't mind input from e.g. @paulhauner on whether he thinks it's OK that we don't de-bounce the log messages by work_type. It could be kinda useful to get one new message for each work_type, but then again, if the node is overwhelmed maybe it doesn't matter too much what is hammering it.

I think if we did want to differentiate by work_type we'd need multiple latches, which might be more trouble than it's worth.

[paulhauner]

@paulhauner on whether he thinks it's OK that we don't de-bounce the log messages by work_type

I like this PR but I also agree with Michael's point. Separately, I've been keen to see some metrics about which work events are being dropped by the BeaconProcessor since I don't think we have anything like that atm. Doing that would mitigate the issue of the log debounce; we'd get less logs but be able to see which message types are being dropped by looking at metrics.

To ensure we're always logging dropped messages, I don't think it would suffice to just add a metric where we're debouncing these logs since there are multiple places where this beacon_processor_send is used. I think the easiest way to ensure we catch all uses would be to create a newtype wrapper around the mpsc::Sender<BeaconWorkEvent<T>> BeaconProcessor. When there is an error on the try_send method on the wrapper, it could increase a counter metric with one label per work type. Similar to this:

lighthouse/beacon_node/network/src/beacon_processor/mod.rs

Lines 1170 to 1173 in c547a11

	metrics::inc_counter_vec(
	&metrics::BEACON_PROCESSOR_WORK_EVENTS_RX_COUNT,
	&[event.work.str_id()],
	);

I'm open to doing this change separately if it's too much work for you at the moment @eserilev ☺️ No pressure.

[eserilev]

To ensure we're always logging dropped messages, I don't think it would suffice to just add a metric where we're debouncing these logs since there are multiple places where this beacon_processor_send is used. I think the easiest way to ensure we catch all uses would be to create a newtype wrapper around the mpsc::Sender<BeaconWorkEvent<T>> BeaconProcessor. When there is an error on the try_send method on the wrapper, it could increase a counter metric with one label per work type. Similar to this:

I like this idea, I can definitely include these changes as part of this PR. Thanks for the feedback

[eserilev]

@paulhauner I made some changes here that hopefully should reflect the discussions above. It's ready for another review when you get the chance. Thanks!

[eserilev]

I should also bounce error logs by work type, I'll push up those changes shortly

[eserilev]

Looks good to me but I wouldn't mind input from e.g. @paulhauner on whether he thinks it's OK that we don't de-bounce the log messages by work_type. It could be kinda useful to get one new message for each work_type, but then again, if the node is overwhelmed maybe it doesn't matter too much what is hammering it.

I think if we did want to differentiate by work_type we'd need multiple latches, which might be more trouble than it's worth.

you're right, we would need additional latches to debounce log by work type. That might make things messy. Since we are now increasing a counter metric per work type, that may be sufficient enough?

[paulhauner]

I think this is great as is! Thanks @eserilev!

[eserilev]

sorry, there was a linting issue that i just noticed on the new BeaconProcessorSend newtype wrapper impl

pub fn try_send(&self, message: WorkEvent<T>) -> Result<(), TrySendError<WorkEvent<T>>>

 the `Err`-variant is at least 256 bytes
    |
    = help: try reducing the size of `tokio::sync::mpsc::error::TrySendError<beacon_processor::WorkEvent<T>>`, for example by boxing large elements or replacing it with `Box<tokio::sync::mpsc::error::TrySendError<beacon_processor::WorkEvent<T>>>`

I resolved by Boxing TrySendError. I also had to 'unbox' the match statement in router.rs

[paulhauner]

I resolved by Boxing TrySendError. I also had to 'unbox' the match statement in router.rs

Thanks, I'm happy with this! I've refactored the WorkEvent struct in #4435, so I don't think it's worth digging deeper here to shrink that struct.

[paulhauner]

bors r+

[bors]

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• arbitrary-check
• beacon-chain-tests
• cargo-audit
• cargo-fmt
• cargo-udeps
• cargo-vendor
• check-benchmarks
• check-msrv
• clippy
• compile-with-beta-compiler
• debug-tests-ubuntu
• dockerfile-ubuntu
• doppelganger-protection-test
• ef-tests-ubuntu
• eth1-simulator-ubuntu
• execution-engine-integration-ubuntu
• merge-transition-ubuntu
• no-eth1-simulator-ubuntu
• op-pool-tests
• release-tests-ubuntu
• release-tests-windows
• slasher-tests
• state-transition-vectors-ubuntu
• syncing-simulator-ubuntu