diff --git a/framework/src/BBT.Aether.AspNetCore/BBT/Aether/AspNetCore/Telemetry/DaprDiagnosticRequest.cs b/framework/src/BBT.Aether.AspNetCore/BBT/Aether/AspNetCore/Telemetry/DaprDiagnosticRequest.cs new file mode 100644 index 0000000..92aed1a --- /dev/null +++ b/framework/src/BBT.Aether.AspNetCore/BBT/Aether/AspNetCore/Telemetry/DaprDiagnosticRequest.cs @@ -0,0 +1,49 @@ +using System; + +namespace BBT.Aether.AspNetCore.Telemetry; + +/// +/// The single definition of "a Dapr call that is diagnostic noise rather than business work" — +/// state store, lock, secret and configuration operations, in both their gRPC and HTTP API spellings. +/// +/// +/// +/// It lives in its own type because two very different places must agree on it, and a copy in either +/// would be a silent trap. The tracing filter uses it to decide that a span is not worth exporting; +/// uses it to decide that the same span must not +/// become the parent a remote service sees. If those two lists ever disagreed, a request would be +/// filtered but still propagated — which is exactly the orphan this pairing exists to prevent. +/// +/// +internal static class DaprDiagnosticRequest +{ + internal static bool Matches(Uri? uri) + { + var path = uri?.AbsolutePath; + if (string.IsNullOrEmpty(path)) + { + return false; + } + + if (path.StartsWith("/dapr.proto.runtime.v1.Dapr/", StringComparison.OrdinalIgnoreCase)) + { + return path.EndsWith("/GetState", StringComparison.OrdinalIgnoreCase) + || path.EndsWith("/GetBulkState", StringComparison.OrdinalIgnoreCase) + || path.EndsWith("/SaveState", StringComparison.OrdinalIgnoreCase) + || path.EndsWith("/DeleteState", StringComparison.OrdinalIgnoreCase) + || path.EndsWith("/ExecuteStateTransaction", StringComparison.OrdinalIgnoreCase) + || path.EndsWith("/GetSecret", StringComparison.OrdinalIgnoreCase) + || path.EndsWith("/GetBulkSecret", StringComparison.OrdinalIgnoreCase) + || path.EndsWith("/GetConfiguration", StringComparison.OrdinalIgnoreCase) + || path.EndsWith("/SubscribeConfiguration", StringComparison.OrdinalIgnoreCase) + || path.EndsWith("/TryLockAlpha1", StringComparison.OrdinalIgnoreCase) + || path.EndsWith("/UnlockAlpha1", StringComparison.OrdinalIgnoreCase); + } + + return path.StartsWith("/v1.0/state/", StringComparison.OrdinalIgnoreCase) + || path.StartsWith("/v1.0-alpha1/state/", StringComparison.OrdinalIgnoreCase) + || path.StartsWith("/v1.0-alpha1/lock/", StringComparison.OrdinalIgnoreCase) + || path.StartsWith("/v1.0/secrets/", StringComparison.OrdinalIgnoreCase) + || path.StartsWith("/v1.0/configuration/", StringComparison.OrdinalIgnoreCase); + } +} diff --git a/framework/src/BBT.Aether.AspNetCore/BBT/Aether/AspNetCore/Telemetry/FilteredSpanParentPropagator.cs b/framework/src/BBT.Aether.AspNetCore/BBT/Aether/AspNetCore/Telemetry/FilteredSpanParentPropagator.cs new file mode 100644 index 0000000..c22f728 --- /dev/null +++ b/framework/src/BBT.Aether.AspNetCore/BBT/Aether/AspNetCore/Telemetry/FilteredSpanParentPropagator.cs @@ -0,0 +1,52 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; + +namespace BBT.Aether.AspNetCore.Telemetry; + +/// +/// Wraps the ambient so a span the tracing filter is +/// about to drop never becomes the parent a remote service sees. +/// +/// +/// +/// The decision itself lives in , which also carries the +/// measurements: why the obvious "is it recorded?" test cannot work, and which three narrower fixes +/// were tried and refuted first. +/// +/// +/// This layer is .NET's own injection, which DiagnosticsHandler performs before it raises the +/// DiagnosticSource start event. In a host where OpenTelemetry's HttpClient instrumentation is also +/// active, the second injection overwrites this one — so +/// exists as well and the two must agree. This one +/// is what protects a host that has HttpClient instrumentation turned off. +/// +/// +/// The propagator to delegate the actual header work to. +public sealed class FilteredSpanParentPropagator(DistributedContextPropagator inner) : DistributedContextPropagator +{ + private readonly DistributedContextPropagator _inner = + inner ?? throw new ArgumentNullException(nameof(inner)); + + /// + public override IReadOnlyCollection Fields => _inner.Fields; + + /// + public override void Inject(Activity? activity, object? carrier, PropagatorSetterCallback? setter) => + _inner.Inject(FilteredSpanRedirect.Target(activity, carrier), carrier, setter); + + /// + public override void ExtractTraceIdAndState( + object? carrier, + PropagatorGetterCallback? getter, + out string? traceId, + out string? traceState) => + _inner.ExtractTraceIdAndState(carrier, getter, out traceId, out traceState); + + /// + public override IEnumerable>? ExtractBaggage( + object? carrier, + PropagatorGetterCallback? getter) => + _inner.ExtractBaggage(carrier, getter); + +} diff --git a/framework/src/BBT.Aether.AspNetCore/BBT/Aether/AspNetCore/Telemetry/FilteredSpanParentTextMapPropagator.cs b/framework/src/BBT.Aether.AspNetCore/BBT/Aether/AspNetCore/Telemetry/FilteredSpanParentTextMapPropagator.cs new file mode 100644 index 0000000..fece144 --- /dev/null +++ b/framework/src/BBT.Aether.AspNetCore/BBT/Aether/AspNetCore/Telemetry/FilteredSpanParentTextMapPropagator.cs @@ -0,0 +1,69 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using OpenTelemetry; +using OpenTelemetry.Context.Propagation; + +namespace BBT.Aether.AspNetCore.Telemetry; + +/// +/// The OpenTelemetry counterpart of : stops a filtered span +/// from becoming the parent a remote service sees. +/// +/// +/// +/// Both wrappers are needed, and which one lands on the wire is not obvious. .NET's +/// DiagnosticsHandler injects through and *then* +/// raises the DiagnosticSource start event; OpenTelemetry's HttpClient instrumentation handles that +/// event and injects again, with the just-created System.Net.Http.HttpRequestOut context — +/// overwriting whatever was already there. Measured on 2026-09-13: with only the +/// wrapper installed, the header written was verifiably +/// the recorded gRPC span's id, and the Dapr sidecar still reported a parent that matched no +/// exported span. The ids it used were the filtered activity's, i.e. the second injection's. +/// +/// +/// So the rule has to be enforced at both layers. This one applies it where OpenTelemetry injects: +/// when the context handed to it is not , walk +/// up to the nearest ancestor that is, and propagate that instead. +/// +/// +/// Baggage travels unchanged. Only the parent id and its trace flags are redirected — the trace id +/// is the same on every ancestor, so a redirect can never move the call into a different trace. +/// +/// +/// The propagator that does the actual header work, normally the W3C one. +public sealed class FilteredSpanParentTextMapPropagator(TextMapPropagator inner) : TextMapPropagator +{ + private readonly TextMapPropagator _inner = inner ?? throw new ArgumentNullException(nameof(inner)); + + /// + public override ISet Fields => _inner.Fields!; + + /// + public override PropagationContext Extract( + PropagationContext context, + T carrier, + Func?> getter) => + _inner.Extract(context, carrier, getter); + + /// + public override void Inject( + PropagationContext context, + T carrier, + Action setter) + { + var target = FilteredSpanRedirect.Target(Activity.Current, carrier); + + // Only redirect when the current activity really is the one being injected; otherwise the + // context came from somewhere we cannot reason about and must travel untouched. + var redirected = target is not null + && Activity.Current is not null + && Activity.Current.Context.SpanId == context.ActivityContext.SpanId + && target != Activity.Current + ? new PropagationContext(target.Context, context.Baggage) + : context; + + _inner.Inject(redirected, carrier, setter); + } + +} diff --git a/framework/src/BBT.Aether.AspNetCore/BBT/Aether/AspNetCore/Telemetry/FilteredSpanRedirect.cs b/framework/src/BBT.Aether.AspNetCore/BBT/Aether/AspNetCore/Telemetry/FilteredSpanRedirect.cs new file mode 100644 index 0000000..5786e03 --- /dev/null +++ b/framework/src/BBT.Aether.AspNetCore/BBT/Aether/AspNetCore/Telemetry/FilteredSpanRedirect.cs @@ -0,0 +1,59 @@ +using System.Diagnostics; +using System.Net.Http; + +namespace BBT.Aether.AspNetCore.Telemetry; + +/// +/// The one decision both propagator wrappers make: when the request about to go out is a Dapr +/// diagnostic call — a span the tracing filter will drop — the parent written to the wire must be +/// the enclosing activity, not the one being dropped. +/// +/// +/// +/// The defect. Filtering a span does not remove its . The activity is +/// created, its id is written into the outgoing traceparent, and only afterwards is it marked +/// unrecorded. The receiver cannot know the id names a span nobody will write: a Dapr sidecar with +/// samplingRate: "1" samples on its own terms and records a span whose parent document never +/// arrives. Elastic APM resolves nesting strictly through parent.id and re-roots such a span +/// to the trace root; OpenObserve groups by trace id and hides it, which is why the same deployment +/// looks healthy on one backend and littered on the other. +/// +/// +/// Why the obvious test does not work. The natural rule — "if this context is not recorded, +/// propagate its nearest recorded ancestor" — is unusable, because the flag has not been cleared +/// yet. Measured on 2026-09-13 by logging every injection: the context handed to the propagator for +/// a filtered Dapr state call arrives as Recorded, with +/// Activity.Current = System.Net.Http.HttpRequestOut. OpenTelemetry's HttpClient +/// instrumentation injects the headers first and applies FilterHttpRequestMessage afterwards. +/// At injection time nothing about the activity distinguishes it from one that will be exported. +/// +/// +/// What does work. The carrier is the itself, so the same +/// predicate the filter will apply can be applied here, before the header is written — and +/// is that predicate, shared rather than copied precisely so the +/// two decisions cannot drift apart. The parent then becomes the enclosing activity, which for a +/// Dapr state or lock call is the dapr.proto.runtime.v1.Dapr/GetState gRPC client span the +/// framework already exports. +/// +/// +internal static class FilteredSpanRedirect +{ + /// + /// The activity whose id should go on the wire for , or + /// when nothing needs redirecting. + /// + /// + /// Both guards are load-bearing. A carrier that is not an is + /// some other transport we know nothing about; and a redirect is only safe while + /// really is the activity for this request, which is what makes its + /// the enclosing span rather than an unrelated one. A parentless + /// activity is left alone: propagating nothing would start a fresh trace at the receiver and + /// break the correlation this exists to protect. + /// + internal static Activity? Target(Activity? current, object? carrier) => + carrier is HttpRequestMessage request + && DaprDiagnosticRequest.Matches(request.RequestUri) + && current is { Parent: not null } + ? current.Parent + : current; +} diff --git a/framework/src/BBT.Aether.AspNetCore/Microsoft/Extensions/DependencyInjection/AetherTelemetryServiceCollectionExtensions.cs b/framework/src/BBT.Aether.AspNetCore/Microsoft/Extensions/DependencyInjection/AetherTelemetryServiceCollectionExtensions.cs index 1d9d970..33c001d 100644 --- a/framework/src/BBT.Aether.AspNetCore/Microsoft/Extensions/DependencyInjection/AetherTelemetryServiceCollectionExtensions.cs +++ b/framework/src/BBT.Aether.AspNetCore/Microsoft/Extensions/DependencyInjection/AetherTelemetryServiceCollectionExtensions.cs @@ -13,6 +13,8 @@ using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using OpenTelemetry; +using OpenTelemetry.Context.Propagation; using OpenTelemetry.Logs; using OpenTelemetry.Metrics; using OpenTelemetry.Resources; @@ -100,6 +102,13 @@ public static IServiceCollection AddAetherTelemetry( { if (!opts.TracingEnabled) return; + // A filtered span must not become the parent a remote service sees. Instrumentation + // filters run after the Activity exists and only clear the Recorded flag, so the id + // that goes on the wire names a span nobody will export — and a receiver that + // samples on its own terms (a Dapr sidecar does) then records an orphan. See + // FilteredSpanParentPropagator for the measurements and the rejected alternatives. + InstallFilteredSpanParentPropagator(); + var excludedPatterns = CompileRegex(opts.Logging.ExcludedPaths .Concat(opts.Tracing.ExcludedPaths)); @@ -293,44 +302,36 @@ private static bool IsExcluded(string? value, List patterns) return false; } - private static bool ShouldTraceHttpRequest(HttpRequestMessage request, List excludedPatterns) + /// + /// Wraps both injection layers once per process. Idempotent on purpose: AddAetherTelemetry can + /// be called more than once in tests and in hosts that compose several modules, and stacking + /// wrappers would re-run the same decision for no gain. + /// + private static void InstallFilteredSpanParentPropagator() { - if (IsExcluded(request.RequestUri?.ToString(), excludedPatterns)) + if (DistributedContextPropagator.Current is not FilteredSpanParentPropagator) { - return false; + DistributedContextPropagator.Current = + new FilteredSpanParentPropagator(DistributedContextPropagator.Current); } - return AetherTracingRuntime.IsVerbose || !IsDaprDiagnosticRequest(request.RequestUri); + // Both layers inject and OpenTelemetry's runs last, overwriting what .NET wrote — so + // wrapping only the .NET propagator leaves the defect in place. Measured, not assumed. + if (Propagators.DefaultTextMapPropagator is not FilteredSpanParentTextMapPropagator) + { + Sdk.SetDefaultTextMapPropagator( + new FilteredSpanParentTextMapPropagator(Propagators.DefaultTextMapPropagator)); + } } - private static bool IsDaprDiagnosticRequest(Uri? uri) + private static bool ShouldTraceHttpRequest(HttpRequestMessage request, List excludedPatterns) { - var path = uri?.AbsolutePath; - if (string.IsNullOrEmpty(path)) + if (IsExcluded(request.RequestUri?.ToString(), excludedPatterns)) { return false; } - if (path.StartsWith("/dapr.proto.runtime.v1.Dapr/", StringComparison.OrdinalIgnoreCase)) - { - return path.EndsWith("/GetState", StringComparison.OrdinalIgnoreCase) - || path.EndsWith("/GetBulkState", StringComparison.OrdinalIgnoreCase) - || path.EndsWith("/SaveState", StringComparison.OrdinalIgnoreCase) - || path.EndsWith("/DeleteState", StringComparison.OrdinalIgnoreCase) - || path.EndsWith("/ExecuteStateTransaction", StringComparison.OrdinalIgnoreCase) - || path.EndsWith("/GetSecret", StringComparison.OrdinalIgnoreCase) - || path.EndsWith("/GetBulkSecret", StringComparison.OrdinalIgnoreCase) - || path.EndsWith("/GetConfiguration", StringComparison.OrdinalIgnoreCase) - || path.EndsWith("/SubscribeConfiguration", StringComparison.OrdinalIgnoreCase) - || path.EndsWith("/TryLockAlpha1", StringComparison.OrdinalIgnoreCase) - || path.EndsWith("/UnlockAlpha1", StringComparison.OrdinalIgnoreCase); - } - - return path.StartsWith("/v1.0/state/", StringComparison.OrdinalIgnoreCase) - || path.StartsWith("/v1.0-alpha1/state/", StringComparison.OrdinalIgnoreCase) - || path.StartsWith("/v1.0-alpha1/lock/", StringComparison.OrdinalIgnoreCase) - || path.StartsWith("/v1.0/secrets/", StringComparison.OrdinalIgnoreCase) - || path.StartsWith("/v1.0/configuration/", StringComparison.OrdinalIgnoreCase); + return AetherTracingRuntime.IsVerbose || !DaprDiagnosticRequest.Matches(request.RequestUri); } private static void EnrichHttpClientActivity(Activity activity, HttpRequestMessage request) diff --git a/framework/test/BBT.Aether.AspNetCore.Tests/BBT/Aether/AspNetCore/Telemetry/FilteredSpanParentPropagatorTests.cs b/framework/test/BBT.Aether.AspNetCore.Tests/BBT/Aether/AspNetCore/Telemetry/FilteredSpanParentPropagatorTests.cs new file mode 100644 index 0000000..c41b49b --- /dev/null +++ b/framework/test/BBT.Aether.AspNetCore.Tests/BBT/Aether/AspNetCore/Telemetry/FilteredSpanParentPropagatorTests.cs @@ -0,0 +1,102 @@ +using System.Collections.Generic; +using System.Diagnostics; +using System.Net.Http; +using BBT.Aether.AspNetCore.Telemetry; +using Shouldly; +using Xunit; + +namespace BBT.Aether.AspNetCore.Tests.BBT.Aether.AspNetCore.Telemetry; + +/// +/// The contract is one sentence: a span the tracing filter is about to drop must never be the +/// parent a remote service sees. +/// +/// The tests drive the public propagator with a real rather than +/// calling the internal decision directly, because the request URI is the whole input — a test that +/// bypassed the carrier would pass while the propagator read the wrong thing off it. +/// +/// +public class FilteredSpanParentPropagatorTests +{ + // A const, never ActivitySource.Name: AddActivityListener invokes every registered predicate + // while it constructs a source, so a predicate that reads the field re-enters the static + // initializer that is still running and poisons the type for the rest of the process. + private const string SourceName = "FilteredSpanParentPropagatorTests"; + + private static readonly ActivitySource Source = new(SourceName); + + private const string DaprStateCall = "http://localhost:42111/dapr.proto.runtime.v1.Dapr/GetState"; + private const string DaprLockCall = "http://localhost:42111/dapr.proto.runtime.v1.Dapr/TryLockAlpha1"; + private const string OrdinaryCall = "http://partner-domain/api/v1/partner/workflows/x/instances/y"; + + [Theory] + [InlineData(DaprStateCall)] + [InlineData(DaprLockCall)] + public void Inject_ForADaprDiagnosticCall_PropagatesTheEnclosingSpan(string url) + { + using var listener = Listen(); + + using var enclosing = Source.StartActivity("dapr.proto.runtime.v1.Dapr/GetState")!; + using var transport = Source.StartActivity("System.Net.Http.HttpRequestOut")!; + + ParentIdWrittenFor(transport, url).ShouldBe(enclosing.SpanId.ToHexString(), + "the wire must name the gRPC client span, which is exported, and not the transport span, " + + "which the filter is about to drop"); + } + + [Fact] + public void Inject_ForAnOrdinaryRequest_PropagatesTheRequestsOwnSpan() + { + using var listener = Listen(); + + using var enclosing = Source.StartActivity("caller")!; + using var transport = Source.StartActivity("System.Net.Http.HttpRequestOut")!; + + ParentIdWrittenFor(transport, OrdinaryCall).ShouldBe(transport.SpanId.ToHexString(), + "nothing about this request is filtered, so the propagator must not alter the trace"); + } + + [Fact] + public void Inject_WhenTheRequestSpanHasNoParent_ChangesNothing() + { + using var listener = Listen(); + + using var orphanRoot = Source.StartActivity("System.Net.Http.HttpRequestOut")!; + + // Propagating nothing would start a fresh trace at the receiver and break the correlation + // this propagator exists to protect, so a parentless span travels as itself. + ParentIdWrittenFor(orphanRoot, DaprStateCall).ShouldBe(orphanRoot.SpanId.ToHexString()); + } + + /// Runs Inject through the real W3C propagator and returns the parent-id it wrote. + private static string? ParentIdWrittenFor(Activity activity, string url) + { + using var request = new HttpRequestMessage(HttpMethod.Post, url); + var propagator = new FilteredSpanParentPropagator(DistributedContextPropagator.CreateDefaultPropagator()); + var headers = new Dictionary(); + + propagator.Inject(activity, request, (_, key, value) => headers[key] = value); + + // traceparent is `00---`; the third field is the claim under test. + return headers.TryGetValue("traceparent", out var traceparent) + ? traceparent.Split('-')[2] + : null; + } + + /// + /// Without a listener that samples, StartActivity returns null and every test here would + /// silently pass on a null-forgiving dereference. + /// + private static ActivityListener Listen() + { + var listener = new ActivityListener + { + ShouldListenTo = source => source.Name == SourceName, + Sample = (ref ActivityCreationOptions _) => + ActivitySamplingResult.AllDataAndRecorded + }; + + ActivitySource.AddActivityListener(listener); + return listener; + } +}