-
Notifications
You must be signed in to change notification settings - Fork 137
fix(atenet): propagate trace context through router and namespace span/metric attrs #429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,9 +15,13 @@ | |
| package router | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" | ||
| extproc "github.com/envoyproxy/go-control-plane/envoy/service/ext_proc/v3" | ||
| envoy_type "github.com/envoyproxy/go-control-plane/envoy/type/v3" | ||
| "go.opentelemetry.io/otel" | ||
| "go.opentelemetry.io/otel/propagation" | ||
| ) | ||
|
|
||
| // reqError carries an HTTP-mappable status code and a client-safe message. | ||
|
|
@@ -43,6 +47,23 @@ func addAuthorityMutation(auth string, mut *extproc.HeaderMutation) { | |
| ) | ||
| } | ||
|
|
||
| // injectTraceContext injects the trace context from ctx into the header mutation, | ||
| // so that Envoy forwards traceparent/tracestate to the upstream worker and spans | ||
| // are connected across the full request path. | ||
| func injectTraceContext(ctx context.Context, mutation *extproc.HeaderMutation) { | ||
| headers := make(map[string]string) | ||
| otel.GetTextMapPropagator().Inject(ctx, propagation.MapCarrier(headers)) | ||
| for k, v := range headers { | ||
| mutation.SetHeaders = append(mutation.SetHeaders, &corev3.HeaderValueOption{ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're not setting
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| Header: &corev3.HeaderValue{ | ||
| Key: k, | ||
| Value: v, | ||
| }, | ||
| AppendAction: corev3.HeaderValueOption_OVERWRITE_IF_EXISTS_OR_ADD, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func immediateResponse(statusCode envoy_type.StatusCode, message string) *extproc.ProcessingResponse { | ||
| return &extproc.ProcessingResponse{ | ||
| Response: &extproc.ProcessingResponse_ImmediateResponse{ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,13 +21,17 @@ import ( | |
| "errors" | ||
| "log/slog" | ||
| "strings" | ||
| "sync" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/agent-substrate/substrate/pkg/proto/ateapipb" | ||
| corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" | ||
| extprocv3 "github.com/envoyproxy/go-control-plane/envoy/service/ext_proc/v3" | ||
| envoy_type "github.com/envoyproxy/go-control-plane/envoy/type/v3" | ||
| "go.opentelemetry.io/otel" | ||
| "go.opentelemetry.io/otel/propagation" | ||
| sdktrace "go.opentelemetry.io/otel/sdk/trace" | ||
| "google.golang.org/grpc" | ||
| "google.golang.org/grpc/codes" | ||
| "google.golang.org/grpc/status" | ||
|
|
@@ -230,17 +234,19 @@ func TestExtProcHeadersEvaluation(t *testing.T) { | |
| } | ||
|
|
||
| mutation := res.Response.GetHeaderMutation() | ||
| if len(mutation.GetSetHeaders()) != 1 { | ||
| t.Fatalf("expected exactly one Header option set, found: %v", mutation.GetSetHeaders()) | ||
| } | ||
|
|
||
| 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() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| var found bool | ||
| for _, h := range headers { | ||
| if strings.ToLower(h.Header.Key) == ":authority" { | ||
| found = true | ||
| if string(h.Header.RawValue) != tc.expectedTarget { | ||
| t.Errorf("invalid destination mapping found: %s, expected: %s", h.Header.RawValue, tc.expectedTarget) | ||
| } | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if string(headerOption.Header.RawValue) != tc.expectedTarget { | ||
| t.Errorf("invalid destination mapping found: %s, expected: %s", headerOption.Header.RawValue, tc.expectedTarget) | ||
| if !found { | ||
| t.Errorf(":authority header not found in mutation, got: %v", headers) | ||
| } | ||
|
|
||
| // Confirm that query logs recorded metric trace details | ||
|
|
@@ -252,3 +258,100 @@ func TestExtProcHeadersEvaluation(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| // inMemoryExporter stores ended spans for test inspection. | ||
| type inMemoryExporter struct { | ||
| mu sync.Mutex | ||
| spans []sdktrace.ReadOnlySpan | ||
| } | ||
|
|
||
| func (e *inMemoryExporter) ExportSpans(_ context.Context, spans []sdktrace.ReadOnlySpan) error { | ||
| e.mu.Lock() | ||
| defer e.mu.Unlock() | ||
| e.spans = append(e.spans, spans...) | ||
| return nil | ||
| } | ||
|
|
||
| func (e *inMemoryExporter) Shutdown(context.Context) error { | ||
| e.mu.Lock() | ||
| defer e.mu.Unlock() | ||
| e.spans = nil | ||
| return nil | ||
| } | ||
|
|
||
| func (e *inMemoryExporter) Ended() []sdktrace.ReadOnlySpan { | ||
| e.mu.Lock() | ||
| defer e.mu.Unlock() | ||
| ret := make([]sdktrace.ReadOnlySpan, len(e.spans)) | ||
| copy(ret, e.spans) | ||
| return ret | ||
| } | ||
|
|
||
| func TestInjectTraceContext_AddsTraceparentMatchingParentSpan(t *testing.T) { | ||
| prevProp := otel.GetTextMapPropagator() | ||
| otel.SetTextMapPropagator(propagation.TraceContext{}) | ||
| t.Cleanup(func() { otel.SetTextMapPropagator(prevProp) }) | ||
|
|
||
| exporter := &inMemoryExporter{} | ||
| tp := sdktrace.NewTracerProvider( | ||
| sdktrace.WithSampler(sdktrace.AlwaysSample()), | ||
| sdktrace.WithSpanProcessor(sdktrace.NewSimpleSpanProcessor(exporter)), | ||
| ) | ||
|
|
||
| ctx, span := tp.Tracer("test").Start(context.Background(), "parent") | ||
| parentTraceID := span.SpanContext().TraceID() | ||
|
|
||
| mutation := &extprocv3.HeaderMutation{} | ||
| injectTraceContext(ctx, mutation) | ||
| span.End() | ||
|
|
||
| var traceparent string | ||
| for _, h := range mutation.GetSetHeaders() { | ||
| if strings.ToLower(h.Header.Key) == "traceparent" { | ||
| traceparent = h.Header.Value | ||
| if h.GetAppendAction() != corev3.HeaderValueOption_OVERWRITE_IF_EXISTS_OR_ADD { | ||
| t.Errorf("AppendAction = %v, want OVERWRITE_IF_EXISTS_OR_ADD", h.GetAppendAction()) | ||
| } | ||
| break | ||
| } | ||
| } | ||
| if traceparent == "" { | ||
| t.Fatal("traceparent not found in header mutation") | ||
| } | ||
|
|
||
| parts := strings.Split(traceparent, "-") | ||
| if len(parts) < 2 { | ||
| t.Fatalf("invalid traceparent format: %s", traceparent) | ||
| } | ||
| if parts[1] != parentTraceID.String() { | ||
| t.Errorf("trace ID in traceparent = %s, want parent trace ID = %s", parts[1], parentTraceID.String()) | ||
| } | ||
| } | ||
|
|
||
| func TestInjectTraceContext_AppendActionDefaultsToOverwrite(t *testing.T) { | ||
| prevProp := otel.GetTextMapPropagator() | ||
| otel.SetTextMapPropagator(propagation.TraceContext{}) | ||
| t.Cleanup(func() { otel.SetTextMapPropagator(prevProp) }) | ||
|
|
||
| exporter := &inMemoryExporter{} | ||
| tp := sdktrace.NewTracerProvider( | ||
| sdktrace.WithSampler(sdktrace.AlwaysSample()), | ||
| sdktrace.WithSpanProcessor(sdktrace.NewSimpleSpanProcessor(exporter)), | ||
| ) | ||
|
|
||
| ctx, span := tp.Tracer("test").Start(context.Background(), "parent") | ||
| defer span.End() | ||
|
|
||
| mutation := &extprocv3.HeaderMutation{} | ||
| injectTraceContext(ctx, mutation) | ||
|
|
||
| for _, h := range mutation.GetSetHeaders() { | ||
| if strings.ToLower(h.Header.Key) == "traceparent" { | ||
| if h.GetAppendAction() != corev3.HeaderValueOption_OVERWRITE_IF_EXISTS_OR_ADD { | ||
| t.Errorf("AppendAction = %v, want OVERWRITE_IF_EXISTS_OR_ADD", h.GetAppendAction()) | ||
| } | ||
| return | ||
| } | ||
| } | ||
| t.Error("traceparent not found in mutation") | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,8 +46,8 @@ func NewActorResumer(apiClient ateapipb.ControlClient) *ActorResumer { | |
| func (r *ActorResumer) ResumeActor(ctx context.Context, atespace, actorName string) (*ateapipb.Actor, error) { | ||
| ctx, span := otel.Tracer(routerServiceName).Start(ctx, "ResumeActor", | ||
| trace.WithAttributes( | ||
| attribute.String("atespace", atespace), | ||
| attribute.String("actor", actorName), | ||
| attribute.String("ate.atespace", atespace), | ||
| attribute.String("ate.actor.name", actorName), | ||
|
Comment on lines
+49
to
+50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| )) | ||
| defer span.End() | ||
|
|
||
|
|
@@ -57,6 +57,9 @@ func (r *ActorResumer) ResumeActor(ctx context.Context, atespace, actorName stri | |
| // resume operation continues running for Caller 2 and Caller 3 without failing. | ||
| bgCtx, bgCancel := context.WithTimeout(context.Background(), 15*time.Second) | ||
| 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)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks good, but can you please add test asserting the resume child span shares the parent traceid?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| backoff := wait.Backoff{ | ||
| Steps: 7, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.