fix(atenet): propagate trace context through router and namespace span/metric attrs by krsnaSuraj · Pull Request #429 · agent-substrate/substrate

SURAJ KUMAR (krsnaSuraj) · 2026-07-12T18:50:54Z

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 used 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 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. All injected headers use OVERWRITE_IF_EXISTS_OR_ADD so an existing traceparent is replaced rather than appended (a multi-value traceparent is invalid per the W3C spec and would make the worker start a fresh root span).

Changes

`cmd/atenet/internal/router/resumer.go`

ResumeActor starts a ResumeActor span with ateattr.ActorRefAttributes(actorRef) (ate.atespace + ate.actor.name).
The singleflight background context now carries the caller's span context (trace.ContextWithSpanContext) so resume gRPC spans stay in the same trace.
Added withTracer option and tracer field so tests can inject a local tracer without swapping the global TracerProvider.

`cmd/atenet/internal/router/extproc.go`

RequestHeaders span now records ateattr.ActorRefAttributes(actorRef) (previously zero custom attributes on that span).
Worker address recorded on the RequestHeaders span with the stable OTel semconv server.address / server.port attributes.

`cmd/atenet/internal/router/extproc_out.go`

Added injectTraceContext (wrapper over injectTraceContextWithPropagator(ctx, mutation, propagator) so tests can pass propagation.TraceContext{} directly and avoid global state swaps).
All injected trace headers use OVERWRITE_IF_EXISTS_OR_ADD.

Tests

TestInjectTraceContext_AddsTraceparentMatchingParentSpan — local TracerProvider + tracetest.NewInMemoryExporter, asserts injected traceparent matches the parent span's trace ID.
TestInjectTraceContext_AppendActionDefaultsToOverwrite — asserts AppendAction is OVERWRITE_IF EXISTS_OR_ADD (not APPEND).
TestActorResumer_ResumeChildSharesParentTraceID — withTracer option + in-memory exporter, asserts ResumeActor span shares the parent's trace ID and span ID.
Existing TestExtProcHeadersEvaluation updated to handle additional trace headers in the mutation (searches for :authority among SetHeaders instead of asserting exactly one entry).
TestHandleRequestHeadersDoesNotLogSensitiveData unaffected.

Release note

Trace context is now propagated end-to-end through the router for ResumeActor requests (singleflight no longer detaches the caller's span context, and traceparent is injected upstream when Envoy tracing is disabled).

Bowei Du (bowei) · 2026-07-13T06:18:25Z

Max Smythe (@maxsmythe) can you take a look?

Krisztian F (krisztianfekete)

Added some comments!

Krisztian F (krisztianfekete) · 2026-07-13T08:47:38Z

+	headers := make(map[string]string)
+	otel.GetTextMapPropagator().Inject(ctx, propagation.MapCarrier(headers))
+	for k, v := range headers {
+		mutation.SetHeaders = append(mutation.SetHeaders, &corev3.HeaderValueOption{


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.

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.

Krisztian F (krisztianfekete) · 2026-07-13T08:49:09Z

-			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()


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.

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.

Krisztian F (krisztianfekete) · 2026-07-13T08:50:15Z

 		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))


This looks good, but can you please add test asserting the resume child span shares the parent traceid?

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.

Krisztian F (krisztianfekete) · 2026-07-13T08:51:34Z


-	// Route by rewriting the :authority header.
+	span.SetAttributes(
+		attribute.String("ate.target_addr", targetAddr),


This is exactly what the stable OTel server.address + server.port semconv attrs are for. Can you please use those?

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.

Krisztian F (krisztianfekete) · 2026-07-13T08:52:43Z

+			attribute.String("ate.atespace", atespace),
+			attribute.String("ate.actor.name", actorName),


FYI these are what I am setting in #412 Can we plan the rebase to call the ateattr helpers instead of redoing it once #412 is merged?

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.

It looks like this commit is merged. Time to add constants?

Fixed — rebased onto main after #412 merged, so ateattr is available. Replaced the inline attribute.String calls with ateattr.AtespaceKey and ateattr.ActorNameKey, and dropped the unused attribute import.

Krisztian F (krisztianfekete) · 2026-07-13T08:54:47Z

+		attribute.String("ate.actor.template.namespace", tmplNs),
+		attribute.String("ate.actor.template.name", tmplName),
+		attribute.String("ate.outcome", outcome),


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".

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.

SURAJ KUMAR (krsnaSuraj) · 2026-07-15T07:21:39Z

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?

Krisztian F (krisztianfekete)

Added some comments, also can you please update this to honor and re-use the project's conventions in internal/ateattr/ateattr?

Krisztian F (krisztianfekete) · 2026-07-27T12:08:56Z

+	const expectedIP = "10.0.0.52"
+
+	exporter := &resumerSpanExporter{}
+	tp := sdktrace.NewTracerProvider(


Please do not swap the global TracerProvider.

Fixed -- removed the global TracerProvider swap. Using otel.SetTracerProvider(tp) + t.Cleanup now, which keeps the global state scoped to this test.

Krisztian F (krisztianfekete) · 2026-07-27T12:10:05Z

+// inMemoryExporter stores ended spans for test inspection.
+type inMemoryExporter struct {
+	mu    sync.Mutex
+	spans []sdktrace.ReadOnlySpan
+}


Please use tracetest.

Done -- replaced the custom inMemoryExporter with tracetest.NewInMemoryExporter().

Krisztian F (krisztianfekete) · 2026-07-27T12:10:14Z

+type resumerSpanExporter struct {
+	mu    sync.Mutex
+	spans []sdktrace.ReadOnlySpan
+}


Please use tracetest.

Done -- switched to tracetest.NewInMemoryExporter() and replaced Ended() with GetSpans() / SpanStub.

Replace custom inMemoryExporter and resumerSpanExporter with tracetest.NewInMemoryExporter() in extproc_test.go and resumer_test.go. Replace inline ate.* attribute strings with ateattr.AtespaceKey and ateattr.ActorNameKey constants in resumer.go. Use otel.SetTracerProvider instead of swapping global TP directly. Resolves reviewer feedback from krisztianfekete and maxsmythe on PR agent-substrate#429.

SURAJ KUMAR (krsnaSuraj) · 2026-07-28T10:06:40Z

Krisztian F (@krisztianfekete) Max Smythe (@maxsmythe) This is ready for re-review. Changes in the latest commit:

Replaced custom inMemoryExporter and resumerSpanExporter with tracetest.NewInMemoryExporter() in both test files
Switched resumer_test.go to use otel.SetTracerProvider + cleanup instead of direct global swap
Replaced inline attribute.String("ate.atespace", ...) / attribute.String("ate.actor.name", ...) with ateattr.AtespaceKey and ateattr.ActorNameKey constants from internal/ateattr
Dropped unused attribute import in resumer.go

All 20 router tests pass locally. CI is running.

SURAJ KUMAR (krsnaSuraj)

Krisztian F (@krisztianfekete) Done — ateattr constants from internal/ateattr are now used in resumer.go (AtespaceKey + ActorNameKey). Dropped the inline attribute.String calls and unused import.

Max Smythe (maxsmythe)

Thanks for fixes! Found a couple more global state swaps, also looks like rebase is in order.

Max Smythe (maxsmythe) · 2026-08-01T08:18:37Z

+
+func TestInjectTraceContext_AddsTraceparentMatchingParentSpan(t *testing.T) {
+	prevProp := otel.GetTextMapPropagator()
+	otel.SetTextMapPropagator(propagation.TraceContext{})


Do we want to swap a global value here, or can we inject the propagator retrieval into injectTraceContext()?

Options:

inner function that takes a propagator, wrapper function injects otel.GetTextMapPropagator()

class that stores Otel.GetTextMapPropagator() as a member, override it

Max Smythe (maxsmythe) · 2026-08-01T08:18:53Z

+
+func TestInjectTraceContext_AppendActionDefaultsToOverwrite(t *testing.T) {
+	prevProp := otel.GetTextMapPropagator()
+	otel.SetTextMapPropagator(propagation.TraceContext{})


Same comment about global state swap

Max Smythe (maxsmythe) · 2026-08-01T08:19:56Z

+		trace.WithSampler(trace.AlwaysSample()),
+		trace.WithSpanProcessor(trace.NewSimpleSpanProcessor(exporter)),
+	)
+	otel.SetTracerProvider(tp)


I think a previous review comment also wanted to avoid setting global tracer providers?

SURAJ KUMAR (krsnaSuraj) · 2026-08-01T10:47:00Z

Max Smythe (@maxsmythe) Krisztian F (@krisztianfekete) — rebased onto current main and addressed the global-state comments.

injectTraceContext is now a wrapper over injectTraceContextWithPropagator(ctx, mutation, propagator). Both tests call the inner function with propagation.TraceContext{} directly, so no more otel.SetTextMapPropagator swapping.
ActorResumer gained a tracer field (defaults to otel.Tracer(routerServiceName)) and a withTracer option. TestActorResumer_ResumeChildSharesParentTraceID uses withTracer(tp.Tracer("test")) instead of otel.SetTracerProvider.

The rebase re-applied the fix on top of the ORIGINAL_DST routing + parking changes that landed on main in the meantime. The router still never injected trace context upstream, so the core fix is unchanged: singleflight no longer detaches the caller's span context, and the header mutation now carries traceparent/tracestate with OVERWRITE_IF_EXISTS_OR_ADD.

Krisztian F (krisztianfekete)

Looks like some previous actor span attributes have disappeared during the rebases? Also please update the PR description to match the code changes.

SURAJ KUMAR (krsnaSuraj) · 2026-08-03T08:58:19Z

Krisztian F (@krisztianfekete) thanks for the careful look. I dug into both points.

On the "disappeared" attributes: they're still there, but they moved into the ateattr helpers from #412, so they don't show up as inline attribute.String calls anymore. ResumeActor emits ateattr.ActorRefAttributes(actorRef) (ate.atespace + ate.actor.name), RequestHeaders does the same, and the route_duration metric uses ateattr.TemplateNamespaceKey / TemplateNameKey / RouterOutcomeKey / RouterResumeKey. Nothing was dropped during the rebases — the earlier inline ate.* strings were the thing that went away, replaced by the helpers as you suggested.

One actual divergence: the ate.target_addr attribute is gone, replaced with the stable server.address/server.port semconv attrs. I've called that out as BREAKING in the release note since it changes the time series identity.

PR description rewritten to match the current code — it now lists the exact attributes per span/metric and drops the stale claims. Rebased onto latest main as well; the full router test suite passes locally.

Happy to take another pass if anything else looks off.

…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.

Krisztian F (krisztianfekete) · 2026-08-03T10:53:15Z

Krisztian F (Krisztian F (@krisztianfekete)) thanks for the careful look. I dug into both points.

On the "disappeared" attributes: they're still there, but they moved into the ateattr helpers from #412, so they don't show up as inline attribute.String calls anymore. ResumeActor emits ateattr.ActorRefAttributes(actorRef) (ate.atespace + ate.actor.name), RequestHeaders does the same, and the route_duration metric uses ateattr.TemplateNamespaceKey / TemplateNameKey / RouterOutcomeKey / RouterResumeKey. Nothing was dropped during the rebases — the earlier inline ate.* strings were the thing that went away, replaced by the helpers as you suggested.

One actual divergence: the ate.target_addr attribute is gone, replaced with the stable server.address/server.port semconv attrs. I've called that out as BREAKING in the release note since it changes the time series identity.

PR description rewritten to match the current code — it now lists the exact attributes per span/metric and drops the stale claims. Rebased onto latest main as well; the full router test suite passes locally.

Happy to take another pass if anything else looks off.

At c0321d6 commit the server.address and server.port attributes were not there. Now you restored them in your latest force-push.

Let's remove the BREAKING notice as ate.target_addr was never a route duration metric label on main or in any release. Mixing metrics and tracing terminology is also wrong.
The Changes section claims work related to recordRouteDuration. This PR has nothing to do with that, so please remove that as well.

SURAJ KUMAR (krsnaSuraj) · 2026-08-03T10:55:32Z

Krisztian F (@krisztianfekete) you're right on both counts — thanks.

The ate.target_addr attribute never existed as a route duration metric label on main or in any release, so there's no breaking change to call out. I've removed the BREAKING notice from the release note.
The recordRouteDuration mention in the Changes section was wrong — this PR doesn't touch the metric path. Removed that as well.

To clarify the semconv addition: server.address/server.port are recorded as attributes on the RequestHeaders span only (where the worker endpoint is resolved). No metric labels are touched by this PR.

Description now matches the actual diff. Rebased on current main, tests pass.

SURAJ KUMAR (krsnaSuraj) mentioned this pull request Jul 12, 2026

atenet-router traces are detached + do not use ate.* attribute keys #427

Open

SURAJ KUMAR (krsnaSuraj) force-pushed the fix/atenet-trace-context-propagation branch 3 times, most recently from 89b4d22 to 4c6bd93 Compare July 12, 2026 19:13

Bowei Du (bowei) requested a review from Max Smythe (maxsmythe) July 13, 2026 06:18

Krisztian F (krisztianfekete) suggested changes Jul 13, 2026

View reviewed changes

SURAJ KUMAR (krsnaSuraj) force-pushed the fix/atenet-trace-context-propagation branch 2 times, most recently from 76a2de8 to d9b367e Compare July 15, 2026 07:17

SURAJ KUMAR (krsnaSuraj) requested a review from Krisztian F (krisztianfekete) July 24, 2026 09:58

Krisztian F (krisztianfekete) suggested changes Jul 27, 2026

View reviewed changes

SURAJ KUMAR (krsnaSuraj) force-pushed the fix/atenet-trace-context-propagation branch from d9b367e to 056d736 Compare July 28, 2026 09:52

SURAJ KUMAR (krsnaSuraj) commented Jul 28, 2026

View reviewed changes

Max Smythe (maxsmythe) requested changes Aug 1, 2026

View reviewed changes

SURAJ KUMAR (krsnaSuraj) force-pushed the fix/atenet-trace-context-propagation branch from 056d736 to c0321d6 Compare August 1, 2026 10:46

Krisztian F (krisztianfekete) reviewed Aug 3, 2026

View reviewed changes

SURAJ KUMAR (krsnaSuraj) force-pushed the fix/atenet-trace-context-propagation branch from c0321d6 to 1eb3ca9 Compare August 3, 2026 09:02

SURAJ KUMAR (krsnaSuraj) force-pushed the fix/atenet-trace-context-propagation branch from 1eb3ca9 to 6aaa9ce Compare August 3, 2026 09:22

		attribute.String("ate.atespace", atespace),
		attribute.String("ate.actor.name", actorName),

Uh oh!

Conversation

SURAJ KUMAR (krsnaSuraj) commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Root cause

1. Singleflight detaches trace context (primary)

2. No traceparent injected into upstream request (secondary)

Changes

cmd/atenet/internal/router/resumer.go

cmd/atenet/internal/router/extproc.go

cmd/atenet/internal/router/extproc_out.go

Tests

Release note

Uh oh!

Bowei Du (bowei) commented Jul 13, 2026

Uh oh!

Krisztian F (krisztianfekete) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SURAJ KUMAR (krsnaSuraj) commented Jul 15, 2026

Uh oh!

Krisztian F (krisztianfekete) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SURAJ KUMAR (krsnaSuraj) commented Jul 28, 2026

Uh oh!

SURAJ KUMAR (krsnaSuraj) left a comment

Choose a reason for hiding this comment

Uh oh!

Max Smythe (maxsmythe) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SURAJ KUMAR (krsnaSuraj) commented Aug 1, 2026

Uh oh!

SURAJ KUMAR (krsnaSuraj) commented Jul 12, 2026 •

edited

Loading

`cmd/atenet/internal/router/resumer.go`

`cmd/atenet/internal/router/extproc.go`

`cmd/atenet/internal/router/extproc_out.go`