Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds metrics tracking to the SQLiteStore's event processing pipeline using the go-ethereum metrics package. The metrics track operation counts (creates, updates, deletes, extends, owner changes), batch processing success/failure rates, and operation timing.
Changes:
- Added seven global metrics variables for tracking operations and timing using go-ethereum's metrics package
- Introduced a
blockStatsstruct to track operation counts per block - Refactored from simple local counter variables to a map-based tracking system that preserves per-block statistics
- Metrics are updated after successful transaction commits to ensure accuracy
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if s, ok := stats[block.Number]; ok { | ||
| totalCreates += s.creates | ||
| totalUpdates += s.updates | ||
| totalDeletes += s.deletes | ||
| totalExtends += s.extends | ||
| totalOwnerChanges += s.ownerChanges | ||
|
|
||
| // Update metrics specifically per block | ||
| if s.creates > 0 { | ||
| metricCreates.Inc(int64(s.creates)) | ||
| } | ||
| if s.updates > 0 { | ||
| metricUpdates.Inc(int64(s.updates)) | ||
| } | ||
| if s.deletes > 0 { | ||
| metricDeletes.Inc(int64(s.deletes)) | ||
| } | ||
| if s.extends > 0 { | ||
| metricExtends.Inc(int64(s.extends)) | ||
| } | ||
| if s.ownerChanges > 0 { | ||
| metricOwnerChanges.Inc(int64(s.ownerChanges)) |
There was a problem hiding this comment.
The variable name 's' shadows the receiver variable 's' (the SQLiteStore instance). While this works, it reduces code clarity and could lead to confusion. Consider using a more descriptive name like 'blockStat' or 'stat' instead of 's' to avoid shadowing and improve readability.
| if s, ok := stats[block.Number]; ok { | |
| totalCreates += s.creates | |
| totalUpdates += s.updates | |
| totalDeletes += s.deletes | |
| totalExtends += s.extends | |
| totalOwnerChanges += s.ownerChanges | |
| // Update metrics specifically per block | |
| if s.creates > 0 { | |
| metricCreates.Inc(int64(s.creates)) | |
| } | |
| if s.updates > 0 { | |
| metricUpdates.Inc(int64(s.updates)) | |
| } | |
| if s.deletes > 0 { | |
| metricDeletes.Inc(int64(s.deletes)) | |
| } | |
| if s.extends > 0 { | |
| metricExtends.Inc(int64(s.extends)) | |
| } | |
| if s.ownerChanges > 0 { | |
| metricOwnerChanges.Inc(int64(s.ownerChanges)) | |
| if stat, ok := stats[block.Number]; ok { | |
| totalCreates += stat.creates | |
| totalUpdates += stat.updates | |
| totalDeletes += stat.deletes | |
| totalExtends += stat.extends | |
| totalOwnerChanges += stat.ownerChanges | |
| // Update metrics specifically per block | |
| if stat.creates > 0 { | |
| metricCreates.Inc(int64(stat.creates)) | |
| } | |
| if stat.updates > 0 { | |
| metricUpdates.Inc(int64(stat.updates)) | |
| } | |
| if stat.deletes > 0 { | |
| metricDeletes.Inc(int64(stat.deletes)) | |
| } | |
| if stat.extends > 0 { | |
| metricExtends.Inc(int64(stat.extends)) | |
| } | |
| if stat.ownerChanges > 0 { | |
| metricOwnerChanges.Inc(int64(stat.ownerChanges)) |
| } | ||
| case operation.Update != nil: | ||
| updates++ | ||
| blockStat.updates++ |
There was a problem hiding this comment.
The blockStat.updates counter is incremented before checking if the update operation should be skipped (line 253-255). When an update is not the last update for a key, it continues to the next operation without processing, but the counter has already been incremented. This leads to overcounting of update operations in metrics, as they count all update operations including those that were skipped. Consider moving the increment to after the skip check at line 255, so only processed updates are counted.
|
|
||
| startTime := time.Now() | ||
|
|
||
| metricOperationStarted.Inc(1) |
There was a problem hiding this comment.
The metricOperationStarted counter is incremented before checking if any blocks will actually be processed. If all blocks in the batch are skipped (line 165-167), the batch will still show as both started and successful (line 528), even though no database modifications occurred. While this accurately tracks batch processing, it may not reflect actual data processing work. Consider whether the metric should track batches processed or blocks processed, and adjust accordingly for clearer observability.
| metricDeletes = metrics.NewRegisteredCounter("arkiv_store/deletes", nil) | ||
| metricExtends = metrics.NewRegisteredCounter("arkiv_store/extends", nil) | ||
| metricOwnerChanges = metrics.NewRegisteredCounter("arkiv_store/owner_changes", nil) | ||
| // more responsive |
There was a problem hiding this comment.
The comment "more responsive" is unclear and provides no context. Consider replacing with a more descriptive comment explaining why these specific parameters (sample size of 100 and alpha of 0.4) were chosen for the exponential decay sample, or what "more responsive" means in this context (e.g., "Uses exponential decay sample for more responsive metrics that weight recent measurements higher").
| // more responsive | |
| // Tracks operation duration (ms) using an exponential decay sample so the histogram | |
| // is more responsive to recent performance by weighting newer measurements higher | |
| // (sample size 100, alpha 0.4). |
| if s.creates > 0 { | ||
| metricCreates.Inc(int64(s.creates)) | ||
| } | ||
| if s.updates > 0 { | ||
| metricUpdates.Inc(int64(s.updates)) | ||
| } | ||
| if s.deletes > 0 { | ||
| metricDeletes.Inc(int64(s.deletes)) | ||
| } | ||
| if s.extends > 0 { | ||
| metricExtends.Inc(int64(s.extends)) | ||
| } | ||
| if s.ownerChanges > 0 { | ||
| metricOwnerChanges.Inc(int64(s.ownerChanges)) | ||
| } |
There was a problem hiding this comment.
The zero checks (e.g., if s.creates > 0) before incrementing metrics are unnecessary. Counter.Inc() can safely be called with 0, and these conditional checks add complexity without providing any benefit. Consider removing these checks and unconditionally incrementing the metrics with the count values to simplify the code.
| if s.creates > 0 { | |
| metricCreates.Inc(int64(s.creates)) | |
| } | |
| if s.updates > 0 { | |
| metricUpdates.Inc(int64(s.updates)) | |
| } | |
| if s.deletes > 0 { | |
| metricDeletes.Inc(int64(s.deletes)) | |
| } | |
| if s.extends > 0 { | |
| metricExtends.Inc(int64(s.extends)) | |
| } | |
| if s.ownerChanges > 0 { | |
| metricOwnerChanges.Inc(int64(s.ownerChanges)) | |
| } | |
| metricCreates.Inc(int64(s.creates)) | |
| metricUpdates.Inc(int64(s.updates)) | |
| metricDeletes.Inc(int64(s.deletes)) | |
| metricExtends.Inc(int64(s.extends)) | |
| metricOwnerChanges.Inc(int64(s.ownerChanges)) |
1 participant
No description provided.