feat(otel): Emit OpenTelemetry HTTP semantic conventions on HTTP server spans (ASP.NET) - #9027
Open
zacharycmontoya wants to merge 20 commits into
Open
zacharycmontoya wants to merge 20 commits into
zacharycmontoya wants to merge 20 commits into
Conversation
zacharycmontoya
changed the title
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9027) and master. ✅ No regressions detected |
BenchmarksBenchmark execution time: 2026-09-09 23:47:55 Comparing candidate commit ff36979 in PR branch Found 2 performance improvements and 1 performance regressions! Performance is the same for 69 metrics, 0 unstable metrics, 65 known flaky benchmarks, 61 flaky benchmarks without significant changes.
|
zacharycmontoya
force-pushed
the
otel-aspnet-rebased
branch
from
August 12, 2026 16:33
25aa9de to
932ec01
Compare
zacharycmontoya
force-pushed
the
otel-aspnet-rebased
branch
from
August 14, 2026 22:56
932ec01 to
9d283d3
Compare
Contributor
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. 1 occurrences of : - http.request.method: GET,
[...]
- server.address: 127.0.0.1,
[...]
- url.path: /proxy,
- url.scheme: http,
[...]
- server.port: 00000,
1 occurrences of : - http.request.method: GET,
[...]
- server.address: 127.0.0.1,
[...]
- url.path: /,
- url.scheme: http,
1 occurrences of : - server.port: 00000,
|
zacharycmontoya
force-pushed
the
otel-aspnet-rebased
branch
13 times, most recently
from
August 19, 2026 23:48
725c379 to
d53e484
Compare
zacharycmontoya
force-pushed
the
otel-aspnet-rebased
branch
from
August 20, 2026 00:01
d53e484 to
8ea90dc
Compare
zacharycmontoya
force-pushed
the
otel-aspnet-rebased
branch
from
August 20, 2026 02:43
8ea90dc to
10e58a6
Compare
zacharycmontoya
force-pushed
the
otel-aspnet-rebased
branch
from
September 1, 2026 20:54
10e58a6 to
fc9799f
Compare
…erTestBase, so we can re-use it for testing the Owin.WebApi2 app
zacharycmontoya
force-pushed
the
otel-aspnet-rebased
branch
from
September 1, 2026 21:04
fc9799f to
0cf83e2
Compare
…re fixture startup behavior
…rsion tag for OWIN Web API requests
…method in the resource name of OWIN spans (removes HttpSemanticConventions.GetServerResourceNameFromRawMethod in favor of HttpSemanticConventions.GetServerResourceName)
…ectly calculate the resource name with the "{method} {http.route}" pattern
… before the inferred proxy span logic runs so we don't have a new inferred proxy scope.
andrewlock
approved these changes
Sep 4, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
LGTM in general, though I confess I found it somewhat tricky to follow all the different pieces. Only vaguely looked at the snapshots so taking you're word they look good to you
| /// <summary> | ||
| /// Gets a value indicating whether this request started the scope, and is therefore the | ||
| /// one that names and finishes it. <c>false</c> for a request produced by | ||
| /// <see cref="HttpServerUtility.TransferRequest(string)"/> under OpenTelemetry semantics, |
Member
There was a problem hiding this comment.
It's quite tricky to follow the logic here, so just to clarify, if the request was not created using otel semantics, what do we see? One span for the initial request, and one for the Transferred request? 🤔
Comment on lines
+63
to
+69
| if (AspNetWebApi2Integration.UsesExistingServerSpan(tracer)) | ||
| { | ||
| // With OpenTelemetry semantics a request has a single HTTP server span, so enrich the | ||
| // ASP.NET one instead of nesting an aspnet-webapi.request span inside it. The controller | ||
| // context is carried through so the route can be refreshed once the action has run. | ||
| AspNetWebApi2Integration.UpdateExistingServerSpan(tracer, boxedControllerContext); | ||
| return new CallTargetState(scope: null, state: boxedControllerContext); |
Comment on lines
+216
to
+219
| if (!StringUtil.IsNullOrEmpty(route) && StringUtil.IsNullOrEmpty(span.Tags.GetTag(Trace.Tags.HttpRoute))) | ||
| { | ||
| span.Tags.SetTag(Trace.Tags.HttpRoute, route); | ||
| } |
Member
There was a problem hiding this comment.
Should we have special cases for this, e.g. check if it's an AspNetTags etc? 🤔
All of which makes me wonder - given our custom tag bags already use the property when this method is called, is this actually more or less expensive than doing the if type check first? 😅 I don't know if we've benchmarked it, and it's possible we've been making incorrect assumptions there :oops:
| /// the ddapm test-agent session the application under test exports OTLP to, where the fixture | ||
| /// should write its diagnostics, and a once-per-test-class initialization point. | ||
| /// </summary> | ||
| public interface IAspNetFixture |
Member
There was a problem hiding this comment.
This only applies to Otlp tests right, maybe we should rename this to IOtlpAspNetFixture to be clear?
EDIT: hmm, no, it looks like this is on the generalised IisFixture 🤔 In that case, should OtlpSession be nullable? Because it doesn't make any sense to have an OtlpTestAgentSession if we're doing "normal" aspnet tests right?
Contributor
Author
There was a problem hiding this comment.
Good point, I'll make OtlpTestAgentSession nullable
Comment on lines
+48
to
+49
| /// minutes -- and which tolerates up to 16ms of skew when it does anchor. The two clocks | ||
| /// therefore disagree by a few milliseconds, so without a margin a span created just after the |
Member
There was a problem hiding this comment.
Doesn't the first statement say they could differ by up to 16ms? Do we not need that same tolerance to avoid flake here?
link04
approved these changes
Sep 4, 2026
Comment on lines
+115
to
+120
| if (iastInstance.Settings.Enabled && iastInstance.OverheadController.AcquireRequest()) | ||
| { | ||
| var traceContext = scope.Span?.Context?.TraceContext; | ||
| traceContext?.EnableIastInRequest(); | ||
| traceContext?.IastRequestContext?.AddRequestData(httpRequest); | ||
| } |
Contributor
There was a problem hiding this comment.
Transferred requests reuse the original server scope, but line 219 calls ReportToSecurityAndIast again. That executes this AcquireRequest() a second time for the same TraceContext, while TraceContext.CloseWebSpan releases the overhead-controller slot only once when the trace's single IastRequestContext is closed. Every transferred request can therefore leak one available IAST request slot and eventually disable IAST processing. Could we skip acquisition when this trace already has an IastRequestContext (while still adding the transferred request data), or otherwise balance every successful acquisition?
Comment on lines
+63
to
+69
| if (AspNetWebApi2Integration.UsesExistingServerSpan(tracer)) | ||
| { | ||
| // With OpenTelemetry semantics a request has a single HTTP server span, so enrich the | ||
| // ASP.NET one instead of nesting an aspnet-webapi.request span inside it. The controller | ||
| // context is carried through so the route can be refreshed once the action has run. | ||
| AspNetWebApi2Integration.UpdateExistingServerSpan(tracer, boxedControllerContext); | ||
| return new CallTargetState(scope: null, state: boxedControllerContext); |
Contributor
There was a problem hiding this comment.
Seems like yes, this is still present at the current head. The checked-in OTLP snapshots for TransferRequest/401, TransferRequest/500, and TransferRequest/503 each contain two exception.type events on the one reused server span. The transfer/proxy fixes do not change either exception-recording path. I think the coalesced-span path still needs to ensure that only one handler records the exception.
anna-git
approved these changes
Sep 7, 2026
anna-git
left a comment
anna-git
left a comment
Contributor
There was a problem hiding this comment.
lgtm for asm parts, left a few comments
| if (responseObject is not null) | ||
| { | ||
| var scope = SharedItems.TryPeekScope(HttpContext.Current, AspNetMvcIntegration.HttpContextKey); | ||
| var scope = SharedItems.TryPeekScopeOrServerScope(HttpContext.Current, AspNetMvcIntegration.HttpContextKey); |
Contributor
There was a problem hiding this comment.
not sure but do we need to call TryPeekScopeOrServerScope too in method begin?
Related I'm getting a claude comment:
Code Origin for Spans lookups not updated for the new shared-span model — three call sites (AsyncControllerActionInvoker_BeginInvokeActionMethod_Integration.cs:69 (tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AspNet/AsyncControllerActionInvoker_BeginInvokeActionMethod_Integration.cs#L69), ControllerActionInvoker_InvokeAction_Integration.cs:83 (tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AspNet/ControllerActionInvoker_InvokeAction_Integration.cs#L83), ReflectedHttpActionDescriptor_ExecuteAsync_Integration.cs:83 (tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AspNet/ReflectedHttpActionDescriptor_ExecuteAsync_Integration.cs#L83)) still call TryPeekScope instead of TryPeekScopeOrServerScope, while sibling AppSec lookups a few lines later in the same files were correctly converted. Under OTel semantics, Code Origin for Spans silently stops working for MVC/WebApi actions.
| { | ||
| // No span of our own: the route information belongs on the ASP.NET server span, and | ||
| // this is the first point at which the executed route is guaranteed to be resolved. | ||
| AspNetWebApi2Integration.UpdateExistingServerSpan(Tracer.Instance, existingSpanContext); |
Contributor
There was a problem hiding this comment.
maybe we should set the exception on the existing server span as scope can now be null under otel semantics?
- Make the WebTags argument non-nullable in HttpSemanticConventions.SetHttpServerRequestValues - Remove changes to the "newResourceNamesEnabled" calculation in the AspNet integrations (they're already handled in TracerSettings) - Rename new AspNetWebApi2Integration helper methods - Update AspNetWebApi2Integration.GetCurrentRequestProtocol so we let duck cast exceptions bubble up (to potentially disable the integration) - Refactor and document AspNetWebApi2Integration helper methods to clarify which methods update the existing server span and which update the integration's web api span - Add logging to GetRouteTemplate exception handling, so we can better understand what errors (if any) are emitted there
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Updates the server spans produced by the ASP.NET, ASP.NET MVC 5, and ASP.NET Web API 2 (both IIS-hosted and OWIN self-hosted) integrations so that their span name and request attributes match the OpenTelemetry HTTP server span specification when
DD_TRACE_OTEL_SEMANTICS_ENABLED=true. This is the ASP.NET follow-up to the ASP.NET Core work in #8995, and reuses theWebTags/HttpSemanticConventionsfoundation introduced there. Also extends the OTLP test harness from #8995 to .NET Framework so the new attribute set can be snapshot-tested end-to-end against IIS Express and OWIN-hosted samples.Reason for change
When
DD_TRACE_OTEL_SEMANTICS_ENABLED=true, the SDK should produce spans that align with the OpenTelemetry semantic conventions across every span kind. Today the ASP.NET server integrations emit spans with the Datadog attribute set (http.method,http.url,http.request.headers.host) and a Datadog resource name built from various algorithms. After these changes, a user who has opted into OpenTelemetry semantics will get a server span whose name and attributes match what the OpenTelemetry ASP.NET instrumentation itself would produce, so they can switch freely between the two without needing to update dashboards, monitors, or downstream tooling built for OpenTelemetry HTTP server spans.Implementation details
In general, when OpenTelemetry semantics is enabled, the ASP.NET integrations calculate the span resource name and the set of span attributes as prescribed by the OpenTelemetry specification. This is done by creating a branch in each integration to call either
HttpSemanticConventions.SetHttpServerRequestValueswhen using OpenTelemetry semantics orSpanExtensions.DecorateWebServerSpanwhen using Datadog semantics, but not both.Also, ASP.NET required some additional logic due to the following factors:
aspnet.requestspan from theTracingHttpModuleand anaspnet-webapi.requestspan from the Web API integration nested inside it. The conventions describe exactly one HTTP server span per request, so this PR unifies the spans into one.TracingHttpModulestarts its span atBeginRequest, long before MVC or Web API has matched a route, but the span name must be{method} {http.route}when route information is available, so this PR updates the span when instrumentation has access to more detailed route information.TracingHttpModule(ASP.NET)Aside from generating a span that aligns with OpenTelemetry semantic conventions, the only update to this instrumentation is specifically handling the
Server.TransferRequest()scenario that re-runs the pipeline with a freshHttpContextbut the sameExecutionContext. In this scenario,BeginRequestwill avoid creating a new span, report the active span to Security, and mark the scope to not be disposed whenEndRequestruns.AspNetWebApi2Integration(Web API) andAspNetMvcIntegration(MVC)This either generates a new span that aligns with OpenTelemetry semantic conventions when no active server span is present or it updates the active server span with an updated
http.routespan attribute and an updated resource name.When there is an active server span, no new child span is generated to report to AppSec, so the active scope is found using
SharedItems.TryPeekScopeOrServerScope(context, key)and reported against.Note: The tags
aspnet.controller,aspnet.action,aspnet.routewere only carried on MVC / Web API child spans, so these keys may no longer be reported under OpenTelemetry semantics. Instead, their information is carried in thehttp.routespan attribute.HttpSemanticConventionsSetHttpServerRequestValues— a newUri-based overload is introduced sinceSystem.Webexposes the request as an absolute URI rather than as separate scheme/host/path/query components.SetServerAddressAndPort— Theserver.address/server.portspan attributes are derived from theHostheader as the spec requires, falling back to the request URI when there is noHostheader.WebTags.NetworkProtocolVersion/network.protocol.versionis now populated on ASP.NET server spans, which uses the newGetNetworkProtocolVersionmethod to parse the protocol string.Test coverage
Unit tests
HttpSemanticConventionsTests— provides coverage for the newHttpSemanticConventionsmethodsIntegration tests — OTLP harness extended to .NET Framework
OtlpServerTestBase— provides a shared harness for either IIS-hosted or self-hosted test applications. This mirrorsOtlpAspNetCoreTestBase.OtlpAspNetTestBase— provides the IIS Express (IisFixture) harness for running ASP.NET applicationsOwinFixture— moved out ofOwinWebApi2Testsso the OTLP suite can share the self-hosted OWIN server.OtlpAspNetMvc5Tests/OtlpAspNetWebApi2Tests/OtlpOwinWebApi2Tests— provides OTLP snapshot testing that exercises for their corresponding test applicationsFor the new end-to-end integration tests, each suite runs only under two scenarios:
Understanding the snapshots
{method}(and toHTTP) with no route, and howhttp.request.method_originalis reportedOther details
📝 This PR is Stacked on top of #8995, which introduces several additional changes like updates to the
WebTagsimplementation and extension methods to standardize the access to the HttpMethod/HttpClientIp/NetworkClientIp tags.JIRA: APMAPI-2053
Follow-up work
network.protocol.nameis still unset on server spans