Retry service maintenance

[nlordell]

This PR changes the service maintenance component to retry updating to the latest block after some delay instead of waiting until a new block is observed.

This is done for two reasons:

1. In order to reduce alert noise because of maintenance update delays (I made sure to keep track of success and failure so we can setup dashboards and alerts for it).
2. Because we want maintenance to run so we are at the "latest" block always. Without retrying, we stay 12 seconds knowing that there is a new block and we have stale data before trying to update again. So, conceptually, retrying here is not so much of a sin.

Furthermore, in terms of the first point, changing the parameters of the alerts is not really possible. The issue is that the current alert measures how stale our data is (i.e. how far the last updated block is from the latest block). This PR proposes actively trying to make our data less stale (retrying to update to newer blocks when an update fails) and start measuring how often that has issues instead.

In general, I hate retry logic as it hides more problematic underlying issues... I'm not 100% convinced this is the right approach to go.

Test Plan

Added a unit test.

[MartinquaXD]

In general, I hate retry logic as it hides more problematic underlying issues... I'm not 100% convinced this is the right approach to go.

I think it's fine in this case because you are adding metrics so we should still be able to see that something is wrong.

[vkgnosis]

This is done in order to reduce alert noise because of maintenance update delays.

How often does this happen? The obvious knob to turn for this is to make the alert condition more lenient. Why not do that? Whether we should retry here should depend on whether it makes sense for this functionality, not the alert.

If a maintenance run takes 1 second and the time between blocks is 12 then it can make sense to do it.

[sunce86]

Lg.
After merging we can drop #590

[nlordell]

Whether we should retry here should depend on whether it makes sense for this functionality, not the alert.

So, this is the reason I'm not 100% convinced this is the way to go (as specified in the description).

Specifically, if we want maintenance to run so we are at the "latest" block always, then retrying makes sense. Otherwise, we stay 12 seconds knowing that there is a new block and we have stale data before trying to update again. So, conceptually, retrying here is not so much of a sin.

However, my worry is that we are hiding node issues with this too much.

Why not do that?

The issue is that the current alert measures how stale our data is (i.e. how far the last updated block is from the latest block). This PR proposes actively trying to make our data less stale (retrying to update to newer blocks when an update fails) and start measuring how often that has issues instead.

Does this make sense?

[vkgnosis]

Yeah makes sense. In the description it sounded like the only reason for this change was to reduce alert noise.

[nlordell]

In the description it sounded like the only reason for this change was to reduce alert noise.

Will update the PR description.

[MartinquaXD]

Just noticed that the ServiceMaintenance can maintain multiple different update tasks. If only a single task in the list failed to update all of the successful tasks would also get triggered again. I guess it would make sense to adapt the retry logic to only update the failed tasks until all tasks ran successfully for the given block.

[nlordell]

I guess it would make sense to adapt the retry logic to only update the failed tasks until all tasks ran successfully for the given block.

In theory, their updates should be no-ops (at least its the case for the event updaters).