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 👍 / 👎.
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 👍 / 👎.
| 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.
|
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
1 participant
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.