Add remoteServiceTagsIdx field to Connection proto#441
Add remoteServiceTagsIdx field to Connection proto#441
Conversation
BarFinsdd
force-pushed
the
bar.fins/add-remote-service-tags-idx
branch
from
225ae79 to
25c5d46
Compare
Add field 57 to Connection message for tracking tags of the remote/destination service on same-host connections, used with CollectorConnections#encodedTags. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
BarFinsdd
force-pushed
the
bar.fins/add-remote-service-tags-idx
branch
from
25c5d46 to
8706863
Compare
Resolve conflicts from master which added TCP congestion signal fields (57-63). Renumber remoteServiceTagsIdx from 57 to 64. Generated pb.go files taken from master — need regeneration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71ebde74ad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
process/connections.pb.go
Outdated
| TcpEcnNegotiated bool `protobuf:"varint,63,opt,name=tcpEcnNegotiated,proto3" json:"tcpEcnNegotiated,omitempty"` | ||
| // remoteServiceTagsIdx is the index of the tags associated with the remote/destination service for same-host connections. | ||
| // Meant to be used with CollectorConnections#encodedTags. | ||
| RemoteServiceTagsIdx int32 `protobuf:"varint,57,opt,name=remoteServiceTagsIdx,proto3" json:"remoteServiceTagsIdx,omitempty"` |
There was a problem hiding this comment.
connections.proto keeps lastTcpRtoCount on field 57 and adds remoteServiceTagsIdx as field 64, but the generated Go type and codec bind remoteServiceTagsIdx to field 57 here. That makes old payloads with lastTcpRtoCount decode into the wrong field and causes new Go payloads to omit field 64 entirely for other consumers, which is a wire-compatibility break.
Useful? React with 👍 / 👎.
| } | ||
| m.TcpEcnNegotiated = bool(v != 0) | ||
| default: | ||
| iNdEx = preIndex |
There was a problem hiding this comment.
connections.proto still defines lastTcpRecoveryCount through tcpEcnNegotiated (fields 58-63), but this generated decoder now falls through to default immediately after case 57, so those fields are treated as unknown and dropped; corresponding struct/marshal support was also removed. This silently removes TCP congestion/ECN telemetry in any path using this Go payload.
Useful? React with 👍 / 👎.
5 participants
What does this PR do?
Add field 57 to Connection message for tracking tags of the remote/destination service on same-host connections, used with CollectorConnections#encodedTags.
Motivation
Additional Notes
https://datadoghq.atlassian.net/browse/USMON-1523
Possible Drawbacks / Trade-offs
Describe how to test/QA your changes
Reviewer's Checklist
Reviewers: please see the review guidelines.