fix(api): incorrect field mappings in GRPC GetStatsInfo and GetNodeInfo - #6930
fix(api): incorrect field mappings in GRPC GetStatsInfo and GetNodeInfo#6930xxo1shine wants to merge 1 commit into
Conversation
Node and network API responses populated two fields from the wrong source values because of copy-and-paste mapping errors. Map needSyncFromPeer from the corresponding peer state and assign UDP inbound traffic to the udpInTraffic protobuf field.
|
Since both issues are caused by errors in manual field mappings, could we also audit similar POJO-to-protobuf conversions for duplicated, swapped, or omitted fields? I noticed two related cases: NodeInfoService maps syncFlag from peerConnection.isDisconnect(), although syncFlag indicates whether the peer is synchronizing, and NetMetricManager#getNetProtoInfo() does not map LatencyDetailInfo.witness to the protobuf builder. Could we verify and fix these cases and add corresponding regression tests? |
|
|
||
| @Test | ||
| public void testProcessMessage() { | ||
| MetricsUtil.getMeter(MetricsKey.NET_TCP_IN_TRAFFIC).mark(1000); |
There was a problem hiding this comment.
[NIT] Test isolation backlog
- Make the traffic sentinels independent of global meter history.
MetricsUtilkeeps a staticMetricRegistry, whileBaseMethodTestresets the Spring context andArgsbut not that registry. Adding fixed amounts therefore does not guarantee that the four final cumulative counts remain distinct; prior activity in the same test JVM can make one of theassertNotEqualschecks fail even when the mapping is correct.
Suggestion: derive marks from captured baseline counts so the resulting values are deterministic and distinct, or swap in and restore a test-scoped registry instead of relying on cumulative global totals.
There was a problem hiding this comment.
I checked how these meters are used in the test suite. Currently, no other tests write non-zero values to these four traffic meters. This test sets four distinct counts immediately after starting the application and reads the results before any actual network traffic can be generated. Therefore, although the MetricRegistry is static, there is no historical data in the current test execution path that could cause these counts to collide, so the fixed sentinel values remain deterministic and distinct. Based on this, I do not think the test needs any additional changes at this time.
I have completed a comprehensive audit of similar manual POJO-to-protobuf field mappings across the codebase. So far, the only additional issues I found are the two you identified: the incorrect source for syncFlag and the missing mapping for LatencyDetailInfo.witness. I will fix both issues in this PR and add the corresponding regression tests. Thank you for the thorough review. |
waynercheung
left a comment
There was a problem hiding this comment.
[MUST] I verified the two original mapping fixes and their targeted tests locally on 49cc8a2 with JDK 17. Reverting either production line makes its corresponding test fail, so the tests detect the two specific regressions originally reported in #6926. The current head, however, still contains only those two fixes and their regression tests.
In the PR discussion, and again in #6926, you confirmed that this PR would also cover:
NodeInfoServicepopulatingsyncFlagfromisDisconnect();- the missing
LatencyDetailInfo.witnessprotobuf mapping; - the corresponding regression tests and the HTTP / gRPC / release-note scope.
Before merging, please either:
- push the promised changes, update the description accordingly, and request re-review; or
- open explicitly linked follow-up issues/PRs and update the description to make the split explicit. This is entirely reasonable; it just needs to be stated.
For option 2, Fixes #6926 may remain if #6926 is considered complete once its two originally reported defects are fixed. If #6926 is intended to remain the umbrella tracker for all four defects, please downgrade it to Refs #6926 so the remaining two do not lose their tracking when the issue is closed.
For reference on the two outstanding items:
-
witness: the mapping belongs in the latency-detail loop ofgetNetProtoInfo()(NetMetricManager.java, beforedetailInfo.setTop99(...)at line 202 in the current head):detailInfo.setWitness(detail.getWitness());
Please cover it with a non-empty witness assertion so an omitted mapping cannotpass.
- syncFlag: when PeerManager.getPeers() builds its result, it filters out peers for which isDisconnect() is true. Consequently, isDisconnect() is normally false again when NodeInfoService reads it, except for a possible disconnect race; either way, it represents connection lifecycle state rather than synchronization state. This affects both HTTP and gRPC.
This defect is in the PeerConnection -> PeerInfo layer, so it should be covered in the existing NodeInfoServiceTest, not only in NodeInfoTest.
Any new commit needs another review, since the existing approval and CI results cover only 49cc8a2.
waynercheung
left a comment
There was a problem hiding this comment.
Approving. Scoping this PR to the two originally reported mappings and keeping Fixes #6926 both look right, which resolves the concern from my earlier review.
Verified on 49cc8a2 (JDK 17): reverting either production line makes its test fail, so both mappings are fixed and effectively guarded.
Two follow-up observations for the record, with no action requested here:
-
witness- the latency-detail loop ingetNetProtoInfo()(NetMetricManager.java:199-209) never copies it, although the POJO sets it at line 241 and it is the only identifier in that message. Every returnednet.latency.detail[]entry therefore carries the protobuf default empty string and cannot be attributed to an individual witness. Filling it needs no schema change and only replaces a default value. -
syncFlag-NodeInfoService:162sources it frompeerConnection.isDisconnect()while iteratingPeerManager.getPeers(), which already filters on!peer.isDisconnect(), so it readsfalsefor every peer and reflects connection lifecycle rather than synchronization state. It has behaved this way sinceecd451402a(2018-11-12), which replacedpeerConnection.getSyncFlag()withisDisconnect(), so re-sourcing it would itself be a behaviour change for HTTP and gRPC alike. Correcting or deprecating are both worth evaluating, but not in this PR.
Non-blocking: the description has no behaviour-change section. After upgrading, tcpOutTraffic reports the actual TCP outbound value instead of UDP inbound, udpInTraffic is no longer unset, and needSyncFromPeer no longer mirrors syncFlag. Worth a release-note line as you mentioned in #6926 on 2026-08-28, noting that no protobuf schema change and no consensus change is involved.
Happy to open follow-up tracking for these if useful; leaving that call to you.
@lvs0075 FYI on the outcome of the audit you raised.
What does this PR do?
Node and network API responses populated two fields from the wrong source values because of copy-and-paste mapping errors.
Map
needSyncFromPeerfrom the corresponding peer state and assign UDP inbound traffic to theudpInTrafficprotobuf field.Fixes #6926
Why are these changes required?
This PR has been tested by:
Follow up
Extra details