stats: expose client authority in OutHeader - #9296
Conversation
|
Can you please sign the CLA and move the PR out of draft for us to start reviewing. |
|
Thanks for the guidance. The CLA requires the contributor to review and accept the legal terms personally, so I cannot make that legal attestation on their behalf. I am leaving the PR in draft until that authorization is completed. Once the repository reports the CLA as signed, I can continue with the ready-for-review step. |
|
Thanks for the heads-up. I’m leaving this PR in draft while the required CLA step remains outstanding; I won’t claim that legal agreement is signed from here. Once that requirement is handled on GitHub, the PR can be reviewed. |
|
There is nothing that we as contributors need to do to approve the legal terms. You just need to click the button/link and agree to the terms. |
|
Thanks for the clarification. EasyCLA now reports the commits as authorized under the signed CLA, and the PR is marked ready for review. The remaining check failures are separate from the CLA status and will need to be handled independently. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9296 +/- ##
==========================================
+ Coverage 83.02% 83.22% +0.20%
==========================================
Files 423 423
Lines 35215 35140 -75
==========================================
+ Hits 29236 29246 +10
+ Misses 4453 4400 -53
+ Partials 1526 1494 -32
🚀 New features to boost your workflow:
|
|
Thanks for the report. Coverage for the changed lines is complete at head |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the Authority field to the OutHeader struct in the stats package to track the :authority pseudo-header for client RPCs, and updates the HTTP/2 client to populate it. It also adds corresponding test coverage, including a new test case for authority overrides. The reviewer suggested a valuable improvement to validate st.Authority against the default authority (the server address) when no override is specified, ensuring broader test coverage for this new field.
| if e.authority != "" && st.Authority != e.authority { | ||
| t.Fatalf("st.Authority = %s, want %s", st.Authority, e.authority) | ||
| } |
There was a problem hiding this comment.
Currently, st.Authority is only validated when an explicit authority override is set in the test configuration (e.authority != ""). For all other tests, the Authority field in OutHeader goes completely unchecked.
We should validate st.Authority against the default authority (which is e.serverAddr) when no override is specified. This ensures that the default authority is also correctly populated and verified across all existing test cases.
wantAuthority := e.authority
if wantAuthority == "" {
wantAuthority = e.serverAddr
}
if st.Authority != wantAuthority {
t.Fatalf("st.Authority = %s, want %s", st.Authority, wantAuthority)
}There was a problem hiding this comment.
Implemented in efbf744. Client OutHeader now validates the explicit authority override when present and the default server address otherwise; the full stats package test suite passes.
|
The Gemini review suggestion is implemented in efbf744: the existing client stats assertion now checks the default server authority as well as explicit overrides. go test ./stats -count=1 passes. |
|
Thanks for the review. I applied the requested default-authority coverage in efbf744 and verified the full stats package with go test ./stats -count=1. |
| h.mu.Lock() | ||
| checkConnStats(t, h.gotConn) | ||
| h.mu.Unlock() | ||
| expect.authority = cc.authority |
There was a problem hiding this comment.
Nit: Can this field be set as part of the literal struct initialization for expectedData above?
There was a problem hiding this comment.
Done in fef007f. Initialized authority directly inside the expectedData struct literal; go test ./stats passes.
Fixes #9235
Add the effective client authority to stats.OutHeader and populate it from callHdr.Host, as requested in the issue discussion. The change includes unary client stats regression coverage.
Tests:
mise exec go@1.26.5 -- gofmt -d stats/stats.go stats/stats_test.go internal/transport/http2_client.go
mise exec go@1.26.5 -- go test ./stats ./internal/transport
RELEASE NOTES: