metadata: Add ValueFromOutgoingContext - #9282
Conversation
|
|
46b882e to
8bb5012
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9282 +/- ##
==========================================
+ Coverage 83.05% 83.21% +0.16%
==========================================
Files 422 423 +1
Lines 35040 35348 +308
==========================================
+ Hits 29101 29415 +314
+ Misses 4427 4425 -2
+ Partials 1512 1508 -4
🚀 New features to boost your workflow:
|
8bb5012 to
65c3f6b
Compare
|
/easycla |
f5bd38d to
004d63f
Compare
|
PR Validation CI -- Don't have permission to apply a type label and will need help from owners. |
4333acc to
0f15882
Compare
|
@mbissa gentle ping for a review! ty! |
Fixes: grpc#8860 There are use cases where only a single value needs to be read from outgoing gRPC metadata. Today the only way to do that is metadata.FromOutgoingContext, which merges and copies every header already staged into a brand-new map, even though the caller only wants one of them. The cost of that copy grows with however many headers have already accumulated in outgoing context — exactly the complaint raised in grpc#8860. Adds `metadata.ValueFromOutgoingContext(ctx, key) []string`, symmetric to the existing `ValueFromIncomingContext`, for reading a single header from outgoing gRPC metadata without merging and copying every other header already staged via `AppendToOutgoingContext`. This is the API @easwars suggested in grpc#8860 (comment): > Would adding a `ValueFromOutgoingContext` that is similar to `ValueFromIncomingContext` work for you? Please note that `ValueFromOutgoingContext` will not be as fast as `ValueFromIncomingContext` as the implementation would have check entries from the `map` **and** the `added` entries. per grpc#8860 (comment). That tradeoff is exactly what this implementation does: it checks rawMD.md (case-insensitively, matching ValueFromIncomingContext's semantics) and then walks rawMD.added, accumulating matches in the same order FromOutgoingContext would, without allocating a new map or copying unrelated keys/values. rawMD.added entries are matched case-insensitively via strings.EqualFold rather than assuming AppendToOutgoingContext already lowercased them, making no assumption about how rawMD.added was populated -- the same guarantee FromOutgoingContext already makes for both rawMD.md and rawMD.added. Benchmark (n = number of unrelated headers already staged via one NewOutgoingContext + one AppendToOutgoingContext call, before reading the target key): FromOutgoingContext/n=1 153.9 ns/op 432 B/op 4 allocs/op ValueFromOutgoingContext/n=1 59.8 ns/op 16 B/op 1 allocs/op FromOutgoingContext/n=10 423.2 ns/op 968 B/op 15 allocs/op ValueFromOutgoingContext/n=10 113.4 ns/op 16 B/op 1 allocs/op FromOutgoingContext/n=50 1641.0 ns/op 3592 B/op 55 allocs/op ValueFromOutgoingContext/n=50 333.3 ns/op 16 B/op 1 allocs/op At n=50, roughly 5x faster with 55x fewer allocations. Still O(n) in the number of already-staged headers in the worst case, since rawMD.added isn't indexed by key, but it avoids the wasted work of copying and lowercasing every header the caller doesn't want. Added TestValueFromOutgoingContext_PanicsOnOddPairs, TestValueFromOutgoingContext_AddedCaseInsensitive (constructs rawMD.added directly with a mixed-case key to verify the case-insensitive match), and a case covering a value present in rawMD.md accumulated with later AppendToOutgoingContext calls, bringing ValueFromOutgoingContext to 100% statement coverage. RELEASE NOTES: * metadata: Add ValueFromOutgoingContext, which reads a single metadata value from outgoing context without copying the entire outgoing metadata into a new map. Co-Authored-By: Claude <noreply@anthropic.com>
Per @easwars's review: - The raw.md if/else stays, but now has a comment explaining why: unlike ValueFromIncomingContext, this function can't return as soon as raw.md is checked, since raw.added still needs to be walked and accumulated regardless. The else prevents that from overwriting an exact match with an unrelated case-insensitive one. Considered extracting this into a shared valueFromMD(md, key) helper called by both ValueFromIncomingContext and ValueFromOutgoingContext, which would remove the if/else entirely. Verified with go1.26/gc (-gcflags="-m -m") that this does NOT get inlined at either call site (cost 116 exceeds the 80 budget) -- a real, non-eliminated function call on every invocation. Benchmarking the two real commits directly (git worktree, 3 runs each) showed no statistically distinguishable regression on this machine, but decided against carrying that risk on a low-level, per-RPC-call metadata primitive: a synthetic benchmark on one machine isn't strong enough evidence that stack-frame/call overhead is negligible everywhere this gets called from, so keeping the logic inline (duplicated with ValueFromIncomingContext's, but zero-cost by construction) was the more conservative choice. - Folded the raw.md+raw.added accumulation case into the main table (a "k1" key set in both, via the same shared ctx) instead of a separate post-loop block with its own mergeCtx. - Removed the now-unnecessary tCtx/ctx split now that everything lives in one shared ctx; matches ValueFromIncomingContext's simpler pattern. - Moved the "no outgoing metadata at all" case into its own TestValueFromOutgoingContext_NoMetadata, since it needs a bare context and no longer has a reason to share a function with the table. - Switched reflect.DeepEqual to cmp.Diff in the tests this PR adds, matching the (-want +got) convention already used elsewhere in the repo (e.g. credentials/jwt/token_file_call_creds_test.go). Pre-existing reflect.DeepEqual usage elsewhere in the file is untouched. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
0c61617 to
8d6f141
Compare
Addresses review feedback to use testing.B.Loop() instead of the classic for i := 0; i < b.N; i++ pattern. b.Loop also prevents the compiler from optimizing away the call, so the manual anti-DCE b.Fatal checks are no longer needed. Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
|
Moving to @mbissa for second set of eyes |
There was a problem hiding this comment.
Code Review
This pull request introduces the 'ValueFromOutgoingContext' function to retrieve a specific metadata value from the outgoing context without copying the entire metadata map, accompanied by comprehensive unit tests and benchmarks. The feedback suggests an optimization to avoid double slice allocation and redundant copying when matches are found in both 'raw.md' and 'raw.added' by deferring the slice allocation until an append is actually needed.
…e copy Addresses a gemini-code-assist review suggestion: when the key matches in both rawMD.md and rawMD.added, the previous code eagerly copied rawMD.md's value and then grew it via append, paying for two allocations. This defers allocating vals until an actual match is found in rawMD.added, roughly halving allocations for that case. Adds a benchmark case (key-found-in-md-and-added) exercising this path. Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ValueFromOutgoingContext function to retrieve metadata values from the outgoing context case-insensitively without copying the entire metadata structure, along with comprehensive unit tests and benchmarks. The feedback suggests a performance optimization in the hot path of ValueFromOutgoingContext by performing a direct string comparison before falling back to strings.EqualFold, as keys are typically already lowercased.
Addresses a second gemini-code-assist review suggestion: raw.added entries are already lowercased by AppendToOutgoingContext in practice, so a cheap == check before the case-insensitive fallback speeds up the common found case. Also switches BenchmarkValueFromOutgoingContext from "k1"/"k2"/"k3" to realistic-length gRPC header keys, since short keys understated strings.EqualFold's relative cost and were masking the effect of this change. Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ValueFromOutgoingContext function to retrieve specific metadata values from the outgoing context in a case-insensitive manner without copying the entire metadata map, along with corresponding tests and benchmarks. The review feedback highlights a compatibility issue where the new benchmarks use b.Loop() (introduced in Go 1.24), which will break compilation on older supported Go versions. It is recommended to revert to the traditional for i := 0; i < b.N; i++ loop structure to maintain backward compatibility.
|
@easwars sry I updated some code according to the gemini review, left the comments unresolved, in case you'd like to take another look 🙏 ty! |
|
@mbissa any chance you are around for review? thank you!!! |
Partially Fixes: #8860
Why
There are use cases where only a single value needs to be read from outgoing gRPC metadata. Today the only way to do that is
metadata.FromOutgoingContext, which merges and copies every header already staged into a brand-new map, even though the caller only wants one of them. The cost of that copy grows with however many headers have already accumulated in outgoing context — exactly the complaint raised in #8860.What
This PR adds
metadata.ValueFromOutgoingContext(ctx, key) []string, symmetric to the existingValueFromIncomingContext, for reading a single header from outgoing gRPC metadata without merging and copying every other header already staged viaAppendToOutgoingContext.This is the API @easwars suggested in #8860 (comment):
per #8860 (comment).
That tradeoff is exactly what this implementation does: it checks
rawMD.md(case-insensitively, matchingValueFromIncomingContext's semantics) and then walksrawMD.added, accumulating matches in the same orderFromOutgoingContextwould, without allocating a new map or copying unrelated keys/values.Matching
rawMD.addedentries case-insensitively viastrings.EqualFold, rather than assuming they're already lowercased byAppendToOutgoingContext, makes no assumption about howrawMD.addedwas populated — the same guaranteeFromOutgoingContextalready makes for bothrawMD.mdandrawMD.added.The allocation of
valsis deferred until a match is actually found inrawMD.added(pergemini-code-assist's review suggestion), rather than always callingcopyOfas soon asrawMD.mdmatches. When the key is found in bothrawMD.mdandrawMD.added, this avoids paying for acopyOfallocation that would otherwise immediately be discarded by the firstappend's growth — roughly halving allocations for that case (seekey-found-in-md-and-addedin the benchmark below).The
rawMD.addedloop also tries a direct==before falling back tostrings.EqualFold(a secondgemini-code-assistsuggestion), sinceAppendToOutgoingContextalready lowercases keys in practice. Benchmarked with realistic-length keys (e.g."grpc-timeout", not"k1"), this is a modest win when the key is found (~2-7% faster) at the cost of a small regression when it isn't (~6% slower — one extra comparison with no payoff) — a reasonable trade since looking up a header you expect to be present is the common case.Benchmark
nis the number of unrelated headers already staged (oneNewOutgoingContextcall plus oneAppendToOutgoingContextcall) before reading the target key:(measured with
b.Loop, per review feedback, which also removes the need for the manual anti-optimizationb.Fatalchecks the previous numbers were measured with)At n=50, roughly 5x faster with 55x fewer allocations. This is still O(n) in the number of already-staged headers in the worst case, since
rawMD.addedisn't indexed by key, but it avoids the wasted work of copying and lowercasing every header the caller doesn't want.Separately,
BenchmarkValueFromOutgoingContext(now using realistic-length keys like"grpc-timeout"/"content-type"instead of"k1"/"k3", which understatedstrings.EqualFold's cost) shows bothrawMD.addedoptimizations above:Testing
TestValueFromOutgoingContextcovers exact match, case-insensitive match, a value present inrawMD.mdaccumulated with two laterAppendToOutgoingContextcalls (must matchFromOutgoingContext's order, perTestAppendToOutgoingContext), values split solely across multipleAppendToOutgoingContextcalls, not-found, and no-outgoing-metadata-at-all.TestValueFromOutgoingContext_AddedCaseInsensitiveconstructsrawMD.addeddirectly with a mixed-case key (bypassingAppendToOutgoingContext's lowercasing) to verify the match is still found.TestValueFromOutgoingContext_PanicsOnOddPairscovers the defensive panic on a malformedrawMD.addedentry, mirroring the identical guard already present inFromOutgoingContext.BenchmarkValueFromOutgoingContextmirrors the existingBenchmarkValueFromIncomingContextshape (key-found / key-not-found), plus akey-found-in-md-and-addedcase covering the deferred-allocation path above.BenchmarkValueFromOutgoingContextVsFromOutgoingContextproduced the comparison table above.ValueFromOutgoingContextitself is at 100% statement coverage (go test -cover).go test ./metadata/...,go vet ./metadata/..., andgofmtare all clean. This branch is rebased on currentmaster.This is additive only — no existing exported behavior changes.
RELEASE NOTES: