fix(atenet): propagate trace context through router and namespace span/metric attrs#429
Conversation
89b4d22 to
4c6bd93
Compare
|
Max Smythe (@maxsmythe) can you take a look? |
Krisztian F (krisztianfekete)
left a comment
There was a problem hiding this comment.
Added some comments!
| headers := make(map[string]string) | ||
| otel.GetTextMapPropagator().Inject(ctx, propagation.MapCarrier(headers)) | ||
| for k, v := range headers { | ||
| mutation.SetHeaders = append(mutation.SetHeaders, &corev3.HeaderValueOption{ |
There was a problem hiding this comment.
You're not setting AppendAction, so this defaults to APPEND_IF_EXISTS_OR_ADD. The request already has a traceparent, so Envoy appends a second value and not replace it. The w3c propagator receives a multi value traceparent, which is invalid per spec, so the worker starts a fresh root span.
There was a problem hiding this comment.
Great catch! I''ve set AppendAction to OVERWRITE_IF_EXISTS_OR_ADD on all injected trace headers so Envoy replaces the existing traceparent instead of appending a second (invalid) value.
| headerOption := mutation.GetSetHeaders()[0] | ||
| if strings.ToLower(headerOption.Header.Key) != ":authority" { | ||
| t.Errorf("invalid resulting dynamic parameter key: %s", headerOption.Header.Key) | ||
| headers := mutation.GetSetHeaders() |
There was a problem hiding this comment.
Could you add a small test with a local TracerProvider + in-memory exporter, start a parent span, and assert the mutation has a traceparent matching it? Also same idea for the resumer reparenting.
There was a problem hiding this comment.
Done! Added TestInjectTraceContext_AddsTraceparentMatchingParentSpan - sets up a local TracerProvider with in-memory exporter, starts a parent span, calls injectTraceContext, and asserts the injected traceparent matches the parent span''s trace ID. Also added TestInjectTraceContext_AppendActionDefaultsToOverwrite as a focused AppendAction check.
| defer bgCancel() | ||
| // Propagate the caller's span context so the gRPC spans are children | ||
| // of ResumeActor rather than appearing as a separate trace. | ||
| bgCtx = trace.ContextWithSpanContext(bgCtx, trace.SpanContextFromContext(ctx)) |
There was a problem hiding this comment.
This looks good, but can you please add test asserting the resume child span shares the parent traceid?
There was a problem hiding this comment.
Done! Added TestActorResumer_ResumeChildSharesParentTraceID - sets up a local TracerProvider + in-memory exporter, starts a parent span, calls ResumeActor, and asserts the ResumeActor span shares the parent''s trace ID and parent span ID.
|
|
||
| // Route by rewriting the :authority header. | ||
| span.SetAttributes( | ||
| attribute.String("ate.target_addr", targetAddr), |
There was a problem hiding this comment.
This is exactly what the stable OTel server.address + server.port semconv attrs are for. Can you please use those?
There was a problem hiding this comment.
Good idea - much cleaner than a custom attribute. Replaced attribute.String(''ate.target_addr'', targetAddr) with semconv.ServerAddress(workerIP) + semconv.ServerPort(80) so we use the stable OTel semconv conventions.
| attribute.String("ate.atespace", atespace), | ||
| attribute.String("ate.actor.name", actorName), |
There was a problem hiding this comment.
Good call! Once #412 merges, I''ll rebase this branch and replace the inline ate.* attribute strings with the ateattr helpers. Will track that as a follow-up so it doesn''t block the trace context fix here.
| attribute.String("ate.actor.template.namespace", tmplNs), | ||
| attribute.String("ate.actor.template.name", tmplName), | ||
| attribute.String("ate.outcome", outcome), |
There was a problem hiding this comment.
Renaming these metric labels changes the time series identity, I left these out of #412 for this reason. Totally fine to do it, but make sure to call it out explicitly in the release note as it's a "breaking change".
There was a problem hiding this comment.
Good point - completely agreed. I''ve updated the release note in the PR description to explicitly call out the route_duration label rename as BREAKING, noting that dashboards and alert rules need updating.
900bcca to
76a2de8
Compare
…n/metric attrs Two bugs broke the trace chain through the router, creating two separate traces instead of one connected trace for each ResumeActor request. 1. Singleflight detaches trace context (primary): resumer.go used context.Background() inside singleflight.DoChan(). Fixed by propagating the caller's span context into the background context via trace.ContextWithSpanContext. 2. No traceparent injected into upstream request (secondary): extproc.go only rewrote the :authority header. Fixed by calling injectTraceContext(ctx, mutation) which uses otel.GetTextMapPropagator() to write traceparent/tracestate with OVERWRITE_IF_EXISTS_OR_ADD AppendAction. 3. Non-namespaced attribute keys: Renamed span and metric attributes to ate.* convention (agent-substrate#412 style). Replaced custom target_addr attribute with stable OTel semconv server.address + server.port.
76a2de8 to
d9b367e
Compare
|
Krisztian F (@krisztianfekete) Thanks for the thorough review! I've addressed all the feedback and pushed the changes. Mind taking another look when you get a chance? |
Fixes #427
Root cause
Two bugs broke the trace chain through the router, creating two separate traces instead of one connected trace for each ResumeActor request:
1. Singleflight detaches trace context (primary)
resumer.go:58 uses context.Background() inside singleflight.DoChan(). The otelgrpc client handler reads the trace context from the context passed to the gRPC call, so the ateapi handler spans ended up in a separate trace from the router's ResumeActor span.
Fix: Propagate the caller's span context into the background context via trace.ContextWithSpanContext. The caller's lifecycle detachment is preserved (background context parent), but the trace chain is maintained.
2. No traceparent injected into upstream request (secondary)
extproc.go:179-187 only rewrote the :authority header in the HeadersResponse mutation. When Envoy tracing is not configured (no OTLP collector), Envoy does not inject traceparent into upstream requests, so the worker pod receives no trace context.
Fix: After building the header mutation, call injectTraceContext(ctx, mutation) which uses otel.GetTextMapPropagator().Inject() to write traceparent/tracestate into the mutation set headers.
3. Non-namespaced attribute keys
Span and metric attributes used bare keys (atespace, actor, actor_template_namespace, etc.) instead of the ate.* namespaced convention established in #412.
Fix: Renamed attributes across extproc.go and resumer.go to match the dot-hierarchy convention from #412:
Also added missing ate.atespace and ate.actor.name attributes to the ExtProc.RequestHeaders span (previously had zero custom attributes).
Testing