-
Notifications
You must be signed in to change notification settings - Fork 69
fix(metrics): add missing GaugeInfo case in GetAll() #33748 #2041
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev-upgrade
Are you sure you want to change the base?
Conversation
GetAll did not return GaugeInfo metrics, which affects "chain/info" and "geth/info".
|
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Fixes missing GaugeInfo handling in StandardRegistry.GetAll(), ensuring GaugeInfo-based metrics (e.g., chain/info, xdc/info) appear in the registry’s JSON output.
Changes:
- Add a
case *GaugeInfobranch inStandardRegistry.GetAll()to includeGaugeInfovalues in the returned metrics map.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case *GaugeInfo: | ||
| values["value"] = metric.Snapshot().Value() |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetAll() now includes GaugeInfo, but there’s no unit test asserting that GaugeInfo metrics are present/encoded correctly in the JSON registry output (StandardRegistry.MarshalJSON / WriteJSONOnce). Consider extending metrics/json_test.go (or adding a new test) to register a GaugeInfo, Update it with a sample GaugeInfoValue, and assert the resulting JSON includes the expected object under "value".
| case *GaugeFloat64: | ||
| values["value"] = metric.Snapshot().Value() | ||
| case *GaugeInfo: | ||
| values["value"] = metric.Snapshot().Value() |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning GaugeInfoSnapshot.Value() here exposes a map (GaugeInfoValue) that originates from GaugeInfo.value. Since GaugeInfo.Snapshot() currently doesn’t lock/copy the underlying map, JSON marshaling of GetAll() can race with GaugeInfo.Update() and potentially panic with concurrent map iteration/write, and callers could also mutate the returned map. Consider ensuring GaugeInfo.Snapshot() (or this GetAll case) takes the mutex and returns a deep-copied map before storing it in the result.
| values["value"] = metric.Snapshot().Value() | |
| // Avoid exposing the underlying GaugeInfo map directly. | |
| // Take the snapshot value and deep-copy it into a new map | |
| // before storing it in the result. | |
| if snapshotVal := metric.Snapshot().Value(); snapshotVal != nil { | |
| copied := make(map[string]interface{}, len(snapshotVal)) | |
| for k, v := range snapshotVal { | |
| copied[k] = v | |
| } | |
| values["value"] = copied | |
| } else { | |
| values["value"] = nil | |
| } |
Proposed changes
GetAll did not return GaugeInfo metrics, which affects "chain/info" and "geth/info".
Ref: ethereum#33748
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