feat(otel): Emit OpenTelemetry HTTP semantic convention on HTTP client spans - #8975
Open
zacharycmontoya wants to merge 9 commits into
Open
feat(otel): Emit OpenTelemetry HTTP semantic convention on HTTP client spans#8975zacharycmontoya wants to merge 9 commits into
zacharycmontoya wants to merge 9 commits into
Conversation
- Add "http.response.status_code" and "url.full" as the OTel Name for the HttpTags first-class HttpMethod and Host properties - Introduce HttpRequestMethodOriginal and ServerPort properties on HttpTags to set OpenTelemetry span attributes "http.request.method_original" and "server.port" - Add one central `HttpSemanticConventions.SetHttpClientRequestValues `method that updtes a span and its HttpTags object with all of the logic for required and some recommended span attributes - Call the above `HttpSemanticConventions.SetHttpClientRequestValues` from `ScopeFactory.CreateInactiveOutboundHttpSpan` and the remoting client `HttpProcessAndSendIntegration`
…guration by using v0 schema
…th DD_TRACE_OTEL_SEMANTICS_ENABLED emitting DD MsgPack
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3da552364
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+40
to
+41
| private static readonly Dictionary<string, string> CanonicalRequestMethods = | ||
| new(StringComparer.OrdinalIgnoreCase) |
There was a problem hiding this comment.
When a user sets the standard OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS option—for example, to recognize PROPFIND—this hard-coded dictionary still classifies that method as _OTHER and names the span HTTP. The OpenTelemetry HTTP semantic conventions define this setting specifically so instrumentations can extend the recognized-method list; parse it through the repository's supported configuration system and include its values during normalization.
AGENTS.md reference: AGENTS.md:L284-L288
Useful? React with 👍 / 👎.
Member
There was a problem hiding this comment.
Seems like a reasonable ask, but could also consider it out of scope? Should probably be something mentioned on the RFC either way?
Comment on lines
+73
to
+76
| tags.HttpRequestMethodOriginal = | ||
| !StringUtil.IsNullOrEmpty(httpMethod) && !string.Equals(httpMethod, requestMethod, StringComparison.Ordinal) | ||
| ? httpMethod | ||
| : null; |
There was a problem hiding this comment.
For a recognized method supplied with noncanonical casing, such as get, this assigns http.request.method_original=get even though http.request.method is normalized to GET. Under the OpenTelemetry HTTP convention, http.request.method_original is conditionally required only when the normalized method is _OTHER; emitting it for recognized methods produces an attribute shape that differs from conforming OpenTelemetry instrumentation.
Useful? React with 👍 / 👎.
Member
There was a problem hiding this comment.
Easy to change the comparison if we need to, not sure which way we need to go
| if (requestUri is not null) | ||
| { | ||
| tags.HttpUrl = HttpRequestUtils.GetUrlFull(requestUri, queryStringManager); | ||
| tags.Host = HttpRequestUtils.GetNormalizedHost(requestUri.Host); |
There was a problem hiding this comment.
For an IPv6 request such as http://[::1]:8080/, Uri.Host returns [::1], so this emits the bracketed URI-host syntax as server.address. OpenTelemetry expects the address value itself (for example, ::1); retaining brackets makes IPv6 peers group under a noncanonical address and prevents parity with conforming instrumentation, while the brackets should remain only in url.full.
Useful? React with 👍 / 👎.
|
|
||
| // Uri.Port is the default port for the scheme when the URL doesn't specify one, | ||
| // which is what we want to report in "server.port" | ||
| tags.ServerPort = requestUri.Port; |
There was a problem hiding this comment.
For an absolute URI whose scheme has no registered default port, such as one handled by a custom HttpMessageHandler using http+unix://socket/path, Uri.Port returns -1. This assignment consequently serializes server.port=-1, which is not a valid server port; only populate the attribute when the URI exposes a valid port.
Useful? React with 👍 / 👎.
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8975) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8975) - mean (75ms) : 71, 79
master - mean (72ms) : 69, 75
section Bailout
This PR (8975) - mean (78ms) : 73, 83
master - mean (81ms) : 73, 89
section CallTarget+Inlining+NGEN
This PR (8975) - mean (1,090ms) : 1052, 1127
master - mean (1,096ms) : 1052, 1141
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8975) - mean (115ms) : 109, 121
master - mean (115ms) : 108, 121
section Bailout
This PR (8975) - mean (112ms) : 108, 115
master - mean (115ms) : 108, 122
section CallTarget+Inlining+NGEN
This PR (8975) - mean (787ms) : 763, 811
master - mean (785ms) : 763, 807
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8975) - mean (98ms) : 95, 100
master - mean (103ms) : 97, 109
section Bailout
This PR (8975) - mean (103ms) : 97, 109
master - mean (99ms) : 96, 103
section CallTarget+Inlining+NGEN
This PR (8975) - mean (942ms) : 903, 982
master - mean (950ms) : 901, 999
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8975) - mean (97ms) : 92, 103
master - mean (98ms) : 92, 105
section Bailout
This PR (8975) - mean (97ms) : 96, 99
master - mean (102ms) : 96, 108
section CallTarget+Inlining+NGEN
This PR (8975) - mean (824ms) : 784, 864
master - mean (822ms) : 782, 862
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8975) - mean (192ms) : 188, 196
master - mean (211ms) : 205, 217
section Bailout
This PR (8975) - mean (195ms) : 193, 197
master - mean (215ms) : 211, 219
section CallTarget+Inlining+NGEN
This PR (8975) - mean (1,177ms) : 1130, 1223
master - mean (1,268ms) : 1214, 1321
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8975) - mean (274ms) : 269, 279
master - mean (303ms) : 294, 312
section Bailout
This PR (8975) - mean (274ms) : 271, 276
master - mean (305ms) : 295, 315
section CallTarget+Inlining+NGEN
This PR (8975) - mean (939ms) : 915, 962
master - mean (1,009ms) : 984, 1035
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8975) - mean (268ms) : 264, 271
master - mean (298ms) : 291, 305
section Bailout
This PR (8975) - mean (268ms) : 265, 270
master - mean (297ms) : 291, 303
section CallTarget+Inlining+NGEN
This PR (8975) - mean (1,143ms) : 1084, 1202
master - mean (1,200ms) : 1160, 1239
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8975) - mean (266ms) : 262, 269
master - mean (300ms) : 294, 306
section Bailout
This PR (8975) - mean (266ms) : 263, 269
master - mean (301ms) : 293, 308
section CallTarget+Inlining+NGEN
This PR (8975) - mean (1,029ms) : 984, 1073
master - mean (1,094ms) : 989, 1199
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BenchmarksBenchmark execution time: 2026-07-31 23:34:07 Comparing candidate commit b3da552 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 71 metrics, 0 unstable metrics, 65 known flaky benchmarks, 61 flaky benchmarks without significant changes.
|
andrewlock
reviewed
Aug 3, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
LGTM in general, main points:
- I only took a cursory look at the snapshots, they LGTM, but didn't comb through them extensively
- The resource names kinda suck, is that the best we can do? 😢
- We can probably do a small optimization to the
userInfoscenario for the Url normalization
| // HTTP methods are case-sensitive, but the libraries we instrument are not always, | ||
| // so treat a case-insensitive match as the canonical method. The original value is | ||
| // reported separately in "http.request.method_original". | ||
| return CanonicalRequestMethods.TryGetValue(httpMethod, out var canonicalMethod) |
Member
There was a problem hiding this comment.
nit: potentially faster to inline the string.Equals checks here too tbh instead of using a dictionary? Doesn't make sense if we're going to pull that data from settings though, only if we hardcode
| /// Gets the span name for a request with the provided "http.request.method" value, when no | ||
| /// low-cardinality target is available. | ||
| /// </summary> | ||
| internal static string GetSpanName(string requestMethod) |
Member
There was a problem hiding this comment.
nit: this is the resource name in our model, so probably should call it that here too?
Suggested change
| internal static string GetSpanName(string requestMethod) | |
| internal static string GetResourceName(string requestMethod) |
| /// </summary> | ||
| internal static string GetSpanName(string requestMethod) | ||
| => string.Equals(requestMethod, OtherRequestMethod, StringComparison.Ordinal) | ||
| ? UnknownMethodSpanName |
Member
There was a problem hiding this comment.
I totally understand why we need to just use HTTP, but also, that makes for pretty crap resource names 🙁 Is there nothing else we could/should include in here? e.g. even if it's just the host or something?
How does the Otel auto-instrumentation handle it today?
Comment on lines
+78
to
+79
| // Do not update this when OpenTelemetry semantics are enabled | ||
| // since OpenTelemetry semantics supercedes V1Tags |
Member
There was a problem hiding this comment.
Does this comment need to be here? 🤔 It's potentially "wrong" if you explicitly enable peer service tags, right?
| /// <param name="queryStringManager">Used to truncate and obfuscate the query string</param> | ||
| internal static string GetUrlFull(Uri uri, QueryStringManager? queryStringManager = null) | ||
| { | ||
| var userInfo = uri.UserInfo; |
Member
There was a problem hiding this comment.
Does this trigger any allocation of the underlying values? The UriComponents approach in the previous methods are designed to avoid that, so this might invalidate that path, and push is into the "slow" path Anyway.
Given we end up having to do a bunch of string manipulation afterwards anyway, which would double the allocations, I feel like it probably makes more sense to just re-implement the basic case either way?
e.g.
var userInfo = uri.UserInfo;
if (StringUtil.IsNullOrEmpty(userInfo))
{
return GetUrl(uri, queryStringManager);;
}
var redacted = userInfo.IndexOf(':') >= 0 ? RedactedUserNameAndPassword : RedactedUserName;
var queryString = queryStringManager?.TruncateAndObfuscate(uri.Query) ?? string.Empty;
return uri.IsDefaultPort
? $"{uri.Scheme}://{redacted}{uri.Host}{uri.AbsolutePath}{queryString}"
: FormattableString.Invariant($"{uri.Scheme}://{redacted}{uri.Host}:{uri.Port}{uri.AbsolutePath}{queryString}");| private void NormalizeWebRequestSpans(JToken tracesRequests, OtlpFieldNames names) | ||
| { | ||
| // The sample's HttpListener binds a random port each run. url.full is covered by | ||
| // VerifyHelper's localhost:<port> scrubber, but server.port carries the bare number. |
Member
There was a problem hiding this comment.
This can probably be added to the standard Verify scrubbers do you think?
Comment on lines
+191
to
+194
| // .NET 9.0 changed the behaviour when AllowWriteStreamBuffering=false | ||
| // The net result is that we end up creating a "WebRequest" span instead | ||
| // of an "HttpClient" span in one of the cases. Rather than creating a whole | ||
| // separate set of snapshots for .NET 9+, just "fixing" that one span instead. |
| [InlineData("v1")] | ||
| [InlineData("v0")] | ||
| [InlineData(null)] | ||
| public async Task CreateOutboundHttpScope_WithOpenTelemetrySemantics_NeverUsesV1SchemaTags(string requestedSchemaVersion) |
Member
There was a problem hiding this comment.
"never", except if you explicitly enable peer tags, right?
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
2 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 HTTP client spans produced by our automatic instrumentation so that their span name and request attributes match the OpenTelemetry HTTP client span specification when
DD_TRACE_OTEL_SEMANTICS_ENABLED=true. Also forces the metadata schema version tov0whenever OpenTelemetry semantics are enabled, so thatv1-only Datadog attributes can no longer be layered on top of OpenTelemetry attributes.Reason for change
When
DD_TRACE_OTEL_SEMANTICS_ENABLED=true, the SDK should produce spans that align with the OpenTelemetry semantic conventions. Today an HTTP client span emits the Datadog attribute set (http.method,http.url,out.host) and a Datadog resource name (GET localhost:8080/api/users). After the changes, a user who has opted into OpenTelemetry semantics (and OTLP traces) will be able to switch freely between OpenTelemetry and Datadog instrumentations without needing to modify OpenTelemetry-based dashboards, monitors, or downstream tooling for their HTTP client spans.Implementation details
Configuration
This is enabled by the recently introduced
DD_TRACE_OTEL_SEMANTICS_ENABLED=trueconfiguration.This also now overrides
TracerSettings.MetadataSchemaVersionby setting hard-coding it toV0, logging a warning and recording the calculated value in telemetry withConfigurationOrigins.Calculated. This decision may be updated through additional peer service discussions in the Datadog RFC.One narrow case is intentionally left alone (possibly to be addressed in a follow-up): explicitly setting
DD_TRACE_PEER_SERVICE_DEFAULTS_ENABLED=truestill selectsHttpV1Tags, since that is an independent knob, sopeer.servicecan still appear alongside OpenTelemetry attributes in that opt-in configuration.ITags changes
HttpTagsnow carries first-class properties to carry OpenTelemetry concepts, in addition to Datadog concepts. When a property is represented in both Datadog and OpenTelemetry semantics, the corresponding tag name is emitted based onSpan.OpenTelemetrySemanticsEnabled:HttpMethodhttp.methodhttp.request.methodHttpUrlhttp.urlurl.fullHostout.hostserver.addressHttpStatusCodehttp.status_codehttp.response.status_codeHttpRequestMethodOriginalhttp.request.method_originalServerPortserver.portHttpRequestMethodOriginalandServerPortare OpenTelemetry-only concepts, so they have no Datadog name and are only ever populated when OpenTelemetry semantics are enabled.HttpStatusCodeandServerPortare backed by anint?and serialized as integer attributes over OTLP.HttpSemanticConventionsAdds a single
HttpSemanticConventions.SetHttpClientRequestValues(span, tags, httpMethod, requestUri, queryStringManager)entry point that owns all of the logic for producing an HTTP client span that aligns with the OpenTelemetry semantic conventions, so that individual integrations only need to supply the corresponding data from their request object. The logic can be summarized in the following way:http.request.methodis normalized to its canonical (uppercase) form. The known set is the RFC 9110 methods plusPATCHandQUERY. Any unrecognized method is reported as_OTHER.http.request.method_originalis only set when the value supplied by the instrumented library differs (case-sensitively) fromhttp.request.method. It is always assigned, including tonull, so that the last update wins because some integrations call into this method more than once for the same span.HTTPwhen the method is_OTHER). The spec's{method} {target}form needs a low-cardinality target, and we do not have one for client spans until we supporturl.template— notably, the spec forbids falling back to the URI path, so the DatadogGET localhost:8080/api/usersresource name is not reused here.server.portcomes fromUri.Port, which is the scheme's default port when the URL does not specify one. The spec requires this span attribute to be reported.url.fullcredential redactionurl.fullis the OpenTelemetry equivalent ofhttp.url, which means it reports an absolute URL with redaction/obfuscation.This PR adds
HttpRequestUtils.GetUrlFull()to produce the same value asGetUrl(), except that it also handles credentials supplied in the URL by redacting them rather than dropping them, as the spec requires:http://user:pass@localhost/pathbecomeshttp://REDACTED:REDACTED@localhost/path, and a username with no password becomeshttp://REDACTED@localhost/pathrather than inventing a password. Query-string truncation and obfuscation still run throughQueryStringManageras before.Two deviations from the spec are known, and are pinned by
[InlineData]rows inHttpRequestUtilsTestsso that they are recorded rather than mistaken for compliance:DD_TRACE_OBFUSCATION_QUERY_STRING_REGEXP, which replaces the whole match with<redacted>instead of preserving the key (sig=REDACTED), and whose default pattern does not matchsigorX-Amz-Credentialat all. That part of the spec is Development stability and explicitly allows the default key list to be overridden, which is effectively what our regex does.Integration call sites
SetHttpClientRequestValuesis called from:ScopeFactory.CreateInactiveOutboundHttpSpan, which covers every outbound HTTP integration that goes throughScopeFactory(HttpMessageHandler,WebRequest,WebClient). The existing Datadog path is preserved in theelsebranch ofif (span.OpenTelemetrySemanticsEnabled).HttpProcessAndSendIntegration, which builds its tags separately fromScopeFactory.Test coverage
Unit tests
HttpSemanticConventionsTests— Exercises every known HTTP method in canonical and lower-case form and unrecognized methods, ensuring the the reported HTTP method and span name are calculated.ScopeFactoryTests— Asserts the full serialized tag set for an outbound HTTP span with either Datadog semantics or OpenTelemetry semantics, including method normalization, default-port behaviour, and URL credential redaction.TagsListTests— New tests assert that each aliased pair enumerates exactly one name per concept, that either name can be used withGetTag/SetTag, and thatserver.portis omitted when unset (though our integrations must always set it).HttpRequestUtilsTests— AssertsGetUrlFullredaction, query obfuscation interaction, and the two spec deviations noted above.TracerSettingsTests— Tests the overriding of the metadata schema version tov0when OpenTelemetry semantics are enabled, and unchanged when not using OpenTelemetry semantics.Integration tests
SpanMetadataOTelRules- Adds span assertions for OpenTelemetry HTTP client spans inIsHttpClientRequestOTelWebRequestTests.SubmitsTracesV0WithOpenTelemetrySemantics/SubmitsTracesV1WithOpenTelemetrySemantics— the existing msgpack snapshot test parameterized on the new flag. Both cases share the newWebRequestTests_otel.verified.txtsnapshot, because OpenTelemetry semantics forcev0regardless of the requested schema. Span validation runs through the newSpanMetadataOTelRules.IsHttpClientRequestOTelrule set, which encodes the spec's required / conditionally-required / recommended attributes for a client span.WebRequestTests.SubmitsOtlpTraces— a new[SkippableTheory]that performs snapshot testing of the application, testing OTLP JSON / OTLP protobuf with OTel semantics off/on. This uses the ddapm test-agent much likeOpenTelemetrySdkTests. Two snapshot files are created: one per semantics setting.TestAgentOtlpCollection— a shared xUnit collection withDisableParallelization = true, applied to bothWebRequestTestsandOpenTelemetrySdkTests.POST /test/session/clearwipes the test-agent session globally, so two classes reading OTLP from it must not run concurrently.OpenTelemetrySdkTestswas previously safe only because all of its OTLP tests happened to share one implicit per-class collection; adding OTLP tests to a second class breaks that.WebRequestTests' own single-classCollectionDefinitionis replaced by the shared one, which also disables parallelization.Other details
Follow-up work
error.typeis not set for HTTP client transport failures (DNS, connection refused, TLS, timeout). Under OpenTelemetry semanticsSpan.SetExceptiononly records anexceptionspan event, and the spec wants the fully-qualified exception type in a span-levelerror.type.url.templateis not supported, which would provide the{target}placeholder in OpenTelemetry the spec's{method} {target}span name format.network.protocol.name,network.protocol.version,network.peer.address,network.peer.port, andhttp.request.resend_count.url.fulldeviations above (query-key preservation and the dropped fragment).HttpStatusCodealready carries anOtelNameonWebTags,InferredProxyTags, andAwsSdkTags, those span types currently emit a mixedhttp.response.status_code+http.method+http.urlattribute set until their own PRs land.