feat(sync): bifurcation for syncTarget #219
Conversation
cristaloleg
requested review from
tzdybal,
renaynay,
vgonkivs,
gupadhyaya and
walldiss
as code owners
cristaloleg
force-pushed
the
sync/bifurcation
branch
from
93d28e6
to
f9d2daf
Compare
There was a problem hiding this comment.
more comments for tests would be helpful - and I think there's some deduplication that can be done throughout the tests if possible ideally but that's just a nice to have.
TODO:
- some sort of metric reported if verifySkipping fails
- reject networkHead in case verifySkipping fails (log it aggressively w/ a WARN and report as metric)
- change name of err message
| @@ -71,6 +72,16 @@ func newMetrics() (*metrics, error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| failedAgainstSubjHead, err := meter.Int64Counter( | |||
There was a problem hiding this comment.
IMO I feel like we'd need more context here to make this metric valuable (e.g. when it failed, what heights, etc)
Bc just the number by itself won't be really helpful to diagnose where / how the problem occurred.
I'm in favour of leaving this metric out for now and adding it back later in a meaningful way. This really rarely happens anyway
There was a problem hiding this comment.
So...adding attributes like height will make it more valuable?
Also, e.g. when it failed what exactly? I assume you're not talking about time when it happened but the place? Probably this should be reflected in metric's name, do you have a better naming for this?
There was a problem hiding this comment.
You'd need to know:
height/hash of subjective head when this occurred, + height/hash of failed network head and probably timestamp as well
Fixes #217