fix(metrics): add missing ResettingTimer case in GetAll() #33749 #2038
Conversation
Follow-up to ethereum#33748 Same issue - ResettingTimer can be registered via loadOrRegister() but GetAll() silently drops it during JSON export. The prometheus exporter handles it fine (collector.go:70), so this is just an oversight in the JSON path. Note: ResettingTimer.Snapshot() resets the timer by design, which is consistent with how the prometheus exporter uses it.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
There was a problem hiding this comment.
Pull request overview
Adds support for exporting ResettingTimer metrics via StandardRegistry.GetAll(), enabling these timers to appear in registry JSON output and other GetAll() consumers.
Changes:
- Extend
StandardRegistry.GetAll()to serialize*ResettingTimersnapshots (count/min/max/mean + percentiles).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case *ResettingTimer: | ||
| t := metric.Snapshot() | ||
| ps := t.Percentiles([]float64{0.5, 0.75, 0.95, 0.99, 0.999}) | ||
| values["count"] = t.Count() |
There was a problem hiding this comment.
In this new ResettingTimer branch, values["count"] is set to t.Count() (an int), whereas other metric types in GetAll use int64 counts (e.g., Counter/Meter/Timer). This creates inconsistent types in the returned map and can break callers that expect int64. Consider normalizing to int64 here (e.g., cast the count) for consistency with the rest of GetAll’s output.
| values["count"] = t.Count() | |
| values["count"] = int64(t.Count()) |
| case *ResettingTimer: | ||
| t := metric.Snapshot() | ||
| ps := t.Percentiles([]float64{0.5, 0.75, 0.95, 0.99, 0.999}) | ||
| values["count"] = t.Count() |
There was a problem hiding this comment.
ResettingTimer.Snapshot() resets the underlying timer; calling it from GetAll means any JSON marshalling of the registry (StandardRegistry.MarshalJSON uses GetAll) will also flush/reset these metrics. This can unintentionally drain samples if a debug/metrics endpoint is polled, and may interfere with other reporters that also call Snapshot on intervals. If the intent is only to expose current values, consider adding a non-resetting read method on ResettingTimer (or a dedicated "flush" path for reporters) so GetAll doesn’t have side effects.
| case *ResettingTimer: | ||
| t := metric.Snapshot() | ||
| ps := t.Percentiles([]float64{0.5, 0.75, 0.95, 0.99, 0.999}) | ||
| values["count"] = t.Count() | ||
| values["min"] = t.Min() | ||
| values["max"] = t.Max() | ||
| values["mean"] = t.Mean() | ||
| values["median"] = ps[0] | ||
| values["75%"] = ps[1] | ||
| values["95%"] = ps[2] | ||
| values["99%"] = ps[3] | ||
| values["99.9%"] = ps[4] |
There was a problem hiding this comment.
GetAll/MarshalJSON currently has test coverage for counters, but this new ResettingTimer serialization path isn’t covered. Adding a unit test that registers a ResettingTimer, updates it, and asserts the JSON/GetAll output includes the expected fields (and behavior on subsequent calls, given Snapshot resets) would help prevent regressions.
2 participants
Proposed changes
Ref: ethereum#33749
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which part of the codebase this PR will touch base on,
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that