Added metrics to track wiggles in polygon

[eastorski]

No description provided.

[shohamc1]

This can be merged with wiggle.go

[eastorski]

this file is rather a file containing block consumer metrics. I am planning some day move here not only wiggle related metrics but block delays. I would prefer keep all metrics of the package in one file if it is possible.

[shohamc1]

We have a file for block consumer metrics already - block_metrics.go.

[eastorski]

we probably would like to keep these metrics from block_metrics also in sync package, not somewhere else. I did not do this refactoring in this PR of course, however I would like to keep new metrics in more proper place from the very beginning.

[shohamc1]

No, it is incorrect to move those metrics into polygon/sync because they are shared by all chains. I would move those metrics into block_metrics.go since it would be helpful to find all block consumer metrics declared in one place.

[shohamc1]

To me it doesn't make sense for the data store to have this metric calculator - I think it should be in Connect in CanonicalChainBuilder.

[eastorski]

this was a suggestion from Milen in his task requirements. #12308 (option 2)

[shohamc1]

That issue has three options - I think option 3 is the cleanest because you will only have one thread using signaturesCache. With this solution you may have a race condition between the store and sync goroutines.

Option 1 from the issue description is fine by me too since it doesn't introduce this into the store, however I think Option 3 is best since we would have just one user of signaturesCache.

[eastorski]

I agree with Milen that it is better to keep the chain builder as slim as it is possible. signatureCache is a thread safe structure in fact, it has lock/unlock mechanism inside to protect its data from parallel access, that s why there is no risk of race conditions.

So, what is our solution here? Because I am still leaning to not move it to the chainBuilder code because of the arguments in the task description.

[shohamc1]

it has lock/unlock mechanism inside to protect its data from parallel access

What is the performance impact of this?

[antonis19]

I don't recommend simply firing this goroutine without proper context handling, since this could lead to a goroutine leak if insertBlocks is cancelled for example.

I would suggest something like this :

go func(ctx context.Context, blocks []*types.Block) {
		for i := range blocks {
			select {
			case <-ctx.Done():
				return
			default:
				wiggle, err := s.wiggleCalculator.CalculateWiggle(ctx, blocks[i].Header())
				if err != nil {
					s.logger.Error(
						syncLogPrefix("failed to update wiggle metrics"),
						"err", err,
					)
					continue
				}

				UpdateWiggleDuration(wiggle)
			}
		}
	}(ctx, blocks)

Also, think about if you want to parallelize the wiggle computation over the list of blocks (could be overkill, or could be useful depending on how heavy the wiggle computation is and how many blocks we are inserting at a time).