Copilot commented on code in PR #3315:
URL: https://github.com/apache/dubbo-go/pull/3315#discussion_r3161173605
##########
protocol/triple/triple_invoker.go:
##########
@@ -161,6 +162,11 @@ func mergeAttachmentToOutgoing(ctx context.Context, inv
base.Invocation) (contex
if timeout, ok := inv.GetAttachment(constant.TimeoutKey); ok {
ctx = context.WithValue(ctx, tri.TimeoutKey{}, timeout)
}
+ // Reset outgoing headers with a fresh empty header before appending.
+ // Without this, AppendToOutgoingContext mutates a shared http.Header
pointer
+ // from the parent context, causing traceparent accumulation when the
same
+ // parent ctx is reused across serial RPCs (e.g. Service A → B then A →
C).
+ ctx = tri.NewOutgoingContext(ctx, http.Header{})
Review Comment:
mergeAttachmentToOutgoing unconditionally resets the outgoing metadata to an
empty header. This prevents traceparent accumulation, but it also drops any
outgoing headers already present on ctx (e.g., set earlier via
tri.NewOutgoingContext/tri.AppendToOutgoingContext by user code or other
filters). Consider cloning existing outgoing headers into a new map (to avoid
shared mutations) and optionally deleting only trace propagation keys before
appending invocation attachments, so non-trace metadata is preserved while
still isolating per-RPC headers.
```suggestion
// Rebuild outgoing headers from a cloned copy so appends for this RPC
do not
// mutate headers stored on a reused parent context. Preserve existing
// non-trace metadata, but drop trace propagation headers to avoid
// traceparent/tracestate accumulation across serial RPCs.
outgoingHeader := http.Header{}
if header, ok := tri.FromOutgoingContext(ctx); ok && header != nil {
outgoingHeader = header.Clone()
outgoingHeader.Del("traceparent")
outgoingHeader.Del("tracestate")
outgoingHeader.Del("Traceparent")
outgoingHeader.Del("Tracestate")
}
ctx = tri.NewOutgoingContext(ctx, outgoingHeader)
```
##########
protocol/triple/triple_invoker_test.go:
##########
@@ -503,3 +503,84 @@ func Test_mergeAttachmentToOutgoing(t *testing.T) {
})
}
}
+
+// TestParseAttachments_TraceparentNotOverwritten verifies that
parseAttachments does NOT
+// copy traceparent/tracestate from ctx attachments into the invocation,
preserving the
+// values injected by the OTEL filter. (Regression test for issue #3240, Root
Cause 1)
+func TestParseAttachments_TraceparentNotOverwritten(t *testing.T) {
+ // Simulate: OTEL filter has already injected the correct traceparent
into invocation
+ otelTraceparent :=
"00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01"
+ otelTracestate := "vendor=opaque"
+
+ inv := invocation.NewRPCInvocationWithOptions(
+ invocation.WithAttachment("traceparent", otelTraceparent),
+ invocation.WithAttachment("tracestate", otelTracestate),
+ )
+
+ // Simulate: upstream request ctx carries a DIFFERENT (stale)
traceparent
+ upstreamTraceparent :=
"00-cccccccccccccccccccccccccccccccc-dddddddddddddddd-01"
+ upstreamTracestate := "vendor=stale"
+ userAtta := map[string]any{
+ "traceparent": upstreamTraceparent,
+ "tracestate": upstreamTracestate,
+ "user-key": "user-val", // normal user key should still be
copied
+ }
+ ctx := context.WithValue(context.Background(), constant.AttachmentKey,
userAtta)
+
+ url := common.NewURLWithOptions()
+ parseAttachments(ctx, url, inv)
+
+ // traceparent and tracestate must remain as the OTEL filter set them
+ tp, _ := inv.GetAttachment("traceparent")
+ assert.Equal(t, otelTraceparent, tp, "traceparent must not be
overwritten by ctx attachment")
+
+ ts, _ := inv.GetAttachment("tracestate")
+ assert.Equal(t, otelTracestate, ts, "tracestate must not be overwritten
by ctx attachment")
+
+ // normal user key must still be copied
+ uk, _ := inv.GetAttachment("user-key")
+ assert.Equal(t, "user-val", uk, "non-trace user attachment must still
be copied")
+}
+
+// TestMergeAttachmentToOutgoing_HeaderIsolation verifies that serial calls to
+// mergeAttachmentToOutgoing from the same parent context produce independent
outgoing
+// headers. (Regression test for issue #3240, Root Cause 2)
+func TestMergeAttachmentToOutgoing_HeaderIsolation(t *testing.T) {
+ parentCtx := context.Background()
Review Comment:
TestMergeAttachmentToOutgoing_HeaderIsolation seeds parentCtx with
context.Background(), which does not reproduce the original accumulation bug:
the old implementation only accumulates when the parent ctx already contains
Triple metadata (extraDataKey/headerOutgoingKey) so subsequent calls mutate the
same http.Header. To make this a real regression test, initialize parentCtx
with an existing outgoing header (e.g.,
tri.NewOutgoingContext(context.Background(), http.Header{})) before calling
mergeAttachmentToOutgoing twice.
```suggestion
parentCtx := tri.NewOutgoingContext(context.Background(), http.Header{})
```
##########
protocol/triple/triple_invoker.go:
##########
@@ -202,13 +208,26 @@ func parseInvocation(ctx context.Context, url
*common.URL, invocation base.Invoc
return callType, inRaw, method, nil
}
+// tracePropagationKeys lists header keys managed by trace propagators (e.g.
W3C TraceContext).
+// These must not be copied from user-supplied ctx attachments into the
invocation because the
+// OTEL filter has already injected the correct values for the current span.
Allowing the copy
+// would silently overwrite them with stale upstream values.
+var tracePropagationKeys = map[string]struct{}{
+ "traceparent": {},
+ "tracestate": {},
+}
+
// parseAttachments retrieves attachments from users passed-in and URL, then
injects them into ctx
func parseAttachments(ctx context.Context, url *common.URL, invocation
base.Invocation) {
// retrieve users passed-in attachment
attaRaw := ctx.Value(constant.AttachmentKey)
if attaRaw != nil {
if userAtta, ok := attaRaw.(map[string]any); ok {
for key, val := range userAtta {
+ // Skip trace-propagation keys to preserve
values injected by the OTEL filter.
+ if _, skip := tracePropagationKeys[key]; skip {
+ continue
+ }
Review Comment:
parseAttachments now skips traceparent/tracestate from ctx attachments
unconditionally. That preserves OTEL-injected values, but it also prevents
propagating trace context at all when the OTEL client filter is disabled (i.e.,
when invocation doesn't already contain these keys). A safer approach is to
skip only when the invocation already has a value for these keys (so ctx can't
overwrite), and to match keys case-insensitively (ctx attachments may not
always be lowercase).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
