stats/opentelemetry: add e2e test for baggage propagation#9160
stats/opentelemetry: add e2e test for baggage propagation#9160ulascansenturk wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9160 +/- ##
==========================================
- Coverage 83.20% 83.17% -0.03%
==========================================
Files 418 419 +1
Lines 33709 33824 +115
==========================================
+ Hits 28048 28134 +86
- Misses 4245 4264 +19
- Partials 1416 1426 +10 🚀 New features to boost your workflow:
|
OpenTelemetry baggage already propagates through the gRPC pipeline when the W3C Baggage propagator is configured, since the tracing carriers are generic TextMapCarriers used for Inject/Extract. There was, however, no test verifying this behavior. Add TestRPC_BaggagePropagation which sets baggage on the client context and asserts the server handler observes it for both unary and streaming RPCs. Fixes grpc#8520
b496c54 to
d3232f3
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new end-to-end test, TestRPC_BaggagePropagation, to verify that OpenTelemetry baggage is correctly propagated from the client to the server across both unary and streaming RPCs. Feedback on the changes points out a critical issue in the streaming RPC handler (FullDuplexCallF), where a non-EOF error returned by stream.Recv() would result in an infinite loop and high CPU usage. It is recommended to update the loop to handle and return any non-nil errors.
| for { | ||
| if _, err := stream.Recv(); err == io.EOF { | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
If stream.Recv() returns an error other than io.EOF (such as a canceled context or a transport error), the loop will continue infinitely because there is no exit condition for other errors. This can cause a tight loop spinning at 100% CPU. We should return the error if it is not nil.
for {
if _, err := stream.Recv(); err != nil {
if err == io.EOF {
return nil
}
return err
}
}There was a problem hiding this comment.
Good catch. Fixed: the loop now returns the error when it isn't io.EOF, so a canceled context or transport error no longer spins at 100% CPU.
In TestRPC_BaggagePropagation, the FullDuplexCall server handler looped on stream.Recv() and only exited on io.EOF, so any other error (canceled context, transport error) would spin the loop at 100% CPU. Return the error when it is not io.EOF.
|
Thanks @ulascansenturk for this contribution. Assigning to @easwars for second set of eyes. |
Summary
Fixes #8520.
OpenTelemetry baggage already propagates correctly through the gRPC pipeline when the W3C Baggage propagator is configured — the tracing carriers (
IncomingCarrier/OutgoingCarrier) are genericTextMapCarriers used by the configured propagator'sInject/Extract, so baggage rides in thebaggagemetadata header exactly like trace context. No production code change is needed; the gap was the lack of a test proving it.This PR adds
TestRPC_BaggagePropagation, which:key1=value1,key2=value2) on the client context.propagation.Baggage{}in the trace options.Verification
-race.propagation.Baggage{}makes the test fail (server sees empty baggage), so it exercises propagation rather than passing vacuously.go vetandgofmtclean.RELEASE NOTES: none