Repository navigation
Emit OpenTelemetry semantic conventions on HTTP spans - #12765
zacharycmontoya wants to merge 6 commits into
Conversation
4930519 to
b4d1988
Compare
f6f083f to
1f7dfff
Compare
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
4891a63 to
8cfde98
Compare
06fde25 to
4788421
Compare
984c8d3 to
f5b4d45
Compare
zacharycmontoya
changed the title
6906acf to
ed69f54
Compare
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: c8d62ca2f1
ℹ️ 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".
| if (OTEL_SEMANTICS) { | ||
| OtelHttpSemantics.setMethodResourceName(span, method); |
There was a problem hiding this comment.
When DD_TRACE_OTEL_SEMANTICS_ENABLED=true, this shared helper is also called by ElasticsearchRestClientDecorator.onRequest and OpensearchRestClientDecorator.onRequest, even though those decorators create ELASTICSEARCH and OPENSEARCH database spans. Consequently, resources such as GET /index/_search collapse to just GET, destroying endpoint-level grouping for every Elasticsearch/OpenSearch request. Keep the OTel naming behavior in the HTTP decorators or exempt these non-HTTP callers.
Useful? React with 👍 / 👎.
| - { dd-name: peer.hostname, type: string, required: recommended } | ||
| - { dd-name: peer.port, type: int } |
There was a problem hiding this comment.
With OTel semantics enabled, OTLP serialization projects known tags through this registry, and db.client also includes the outbound_peer mixin. Removing these mappings therefore makes existing database client spans that emit peer.hostname and peer.port serialize those legacy keys instead of server.address and server.port; only the HTTP decorators were migrated to emit the new canonical tags. Preserve the mappings for non-HTTP outbound spans or migrate all affected client decorators before removing them.
Useful? React with 👍 / 👎.
| if (domainName != null && !domainName.isEmpty()) { | ||
| span.setTag(KnownTags.URL_SCHEME_NAME, "https"); | ||
| span.setTag(KnownTags.SERVER_ADDRESS_NAME, domainName); |
There was a problem hiding this comment.
When OTel semantics are enabled and x-dd-proxy-domain-name is present, inferred AWS API Gateway and HTTP API spans record url.scheme=https and server.address but never record server.port. The inferred URL is explicitly HTTPS, so omitting the known default port makes these spans inconsistent with the ordinary HTTP paths and causes queries or aggregations on server.port=443 to miss inferred proxy traffic. Set server.port to 443 alongside the address.
Useful? React with 👍 / 👎.
| String queryTag = otelSemanticsEnabled ? KnownTags.URL_QUERY_NAME : DDTags.HTTP_QUERY; | ||
| Object query = unsafeTags.getObject(queryTag); |
There was a problem hiding this comment.
When OTel semantics are enabled, selecting only url.query bypasses LambdaAppSecHandler.applyHttpTags, which still writes the raw query to DDTags.HTTP_QUERY and explicitly relies on QueryObfuscator to scrub and reappend it. For HTTP-triggered Lambda requests with AppSec active and server query tagging enabled, sensitive values such as token=secret therefore remain in the emitted http.query.string tag. Process both query tags or migrate the Lambda tag construction to the OTel representation.
Useful? React with 👍 / 👎.
| } else if (path.charAt(0) != '/') { | ||
| path = "/" + path; | ||
| } | ||
| span.setTag(KnownTags.URL_PATH_NAME, path); |
There was a problem hiding this comment.
For OTel server spans without an http.route, this replacement leaves only url.path. GatewayBridge.maybeSampleForApiSecurity explicitly reads http.route and then http.url to infer an endpoint, so it receives neither value and cannot perform URL-based API Security sampling or endpoint discovery for route-less requests such as unmatched and low-level server traffic. Teach that consumer to fall back to url.path or retain a compatible URL value.
Useful? React with 👍 / 👎.
![datadog-prod-us1-5[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
With OpenTelemetry semantics enabled, percent-encoded sensitive parameter names can bypass redaction and expose secrets in exported URLs. Forwarded-header extraction, non-HTTP destination attributes, and configured client resource names also have confirmed correctness gaps.
🤖 Bits Code Review · Commit c8d62ca
| span.setTag(KnownTags.HTTP_URL_OTEL_NAME, withoutQuery(redactedUrl(url))); | ||
| // QueryObfuscator scrubs this before rebuilding url.full and removes it from client spans. | ||
| final String query = url.getRawQuery(); | ||
| if (query != null) { |
There was a problem hiding this comment.
With OpenTelemetry semantics enabled, a URL such as https://example.com/?t%6fken=secret passes the raw encoded parameter name to QueryObfuscator. Its default pattern misses that name, so url.full exports the secret. The previous shared client path used getQuery(), which decoded the name to token for redaction. Normalize encoded names for matching while preserving valid URL encoding during reconstruction.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| if (OTEL_SEMANTICS) { | ||
| String scheme = | ||
| OtelHttpSemantics.forwardedValue( | ||
| extracted == null ? null : extracted.getXForwardedProto()); |
There was a problem hiding this comment.
With AppSec and AI Guard disabled and the default DD_TRACE_CLIENT_IP_ENABLED=false, ContextInterpreter still discards forwarded headers. Behind a reverse proxy, these new attributes consequently report the backend scheme, host, and port, while client.address falls back to the proxy IP. Enable collection during context extraction for OpenTelemetry semantics too; the added decorator tests bypass extraction by constructing HttpHeaders directly.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| tags: | ||
| - { dd-name: peer.hostname, type: string, required: recommended, otel-name: server.address } | ||
| - { dd-name: peer.port, type: int, otel-name: server.port } | ||
| - { dd-name: peer.hostname, type: string, required: recommended } |
There was a problem hiding this comment.
Removing these shared mappings also affects non-HTTP clients. LettuceClientDecorator still writes peer.port, and DatabaseClientDecorator writes peer.hostname; neither emits the new canonical destination tags. With OpenTelemetry semantics enabled, their OTLP output therefore loses server.address and server.port. Migrate those producers or preserve their conversion through registry/exporter handling; simply restoring the aliases conflicts with the newly registered canonical names.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| OtelHttpSemantics.setClientUrl(span, url); | ||
| // url.full may trigger URLAsResourceNameRule; restore the OTel span name afterwards. | ||
| if (shouldSetResourceName()) { | ||
| OtelHttpSemantics.setMethodResourceName(span, method); |
There was a problem hiding this comment.
When a request matches DD_TRACE_HTTP_CLIENT_PATH_RESOURCE_NAME_MAPPING, setting url.full invokes TagInterceptor and installs a path-based resource name at priority 4. This restoration uses priority 1, so DDSpanContext rejects it and retains a name such as GET /orders/{id} instead of GET. Prevent legacy URL resource naming in OpenTelemetry mode across the relevant URL-setting paths.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| // - peer.ipv6 | ||
| final boolean shouldTagIps = | ||
| config.isClientIpEnabled() || traceClientIpResolverEnabled && APPSEC_ACTIVE; | ||
| OTEL_SEMANTICS |
There was a problem hiding this comment.
OTEL_SEMANTICS || here turns on client-IP collection whenever OTel semantics are enabled. That covers http.client_ip, network.client.ip, the raw X-Forwarded-For / Forwarded header tags, and the new client.address fallback below. On master these are deliberately opt-in (DD_TRACE_CLIENT_IP_ENABLED or AppSec). Collecting client IPs by default, especially when they can be geolocated, violates privacy rules in some jurisdictions, so we can't enable it through a semantics flag.
The OTel HTTP server conventions mark client.address and network.peer.address as Recommended, not Required, so leaving them out still conforms. Opt-in also needs no OTel-specific code: with DD_TRACE_CLIENT_IP_ENABLED=true, the registry already exports http.client_ip as client.address and network.client.ip as network.peer.address.
Could you drop OTEL_SEMANTICS || here, and change HttpDecoratorsOtelSemanticsForkedTest (L104–105) to assert the IP attributes are absent with only the semantics flag, plus a second case asserting them present with DD_TRACE_CLIENT_IP_ENABLED=true? It's also worth checking that system-tests#7919 doesn't expect them without the opt-in.
Written by Claude, reviewed by @dougqh
There was a problem hiding this comment.
There's a perf side to this too. With shouldTagIps forced on, every server request in OTel mode runs ClientIpAddressResolver.resolve (header parsing plus InetAddress handling) and the private-IP checks. It also adds up to five http.forwarded* tags plus http.client_ip / network.client.ip, which makes every exported span bigger. Dropping OTEL_SEMANTICS || takes care of this as well.
Written by Claude, reviewed by @dougqh
| - { dd-name: http.status_code, type: int, required: conditional, otel-name: http.response.status_code } | ||
| - { dd-name: network.protocol.version, type: string, required: recommended } # passes through: dd-name already is the OTel name | ||
| - { dd-name: http.url, type: string, required: required, otel-name: url.full } # shared by http.server and http.client => one otel-name. url.full is the client-correct rename; server's spec mapping (url.path + url.scheme + url.query) is a one-to-many split reserved for the derivation layer (needs span.kind). TODO(otel): server split. | ||
| - { dd-name: server.address, type: string, required: conditional } # OTel-only; set only while OTel semantics are enabled |
There was a problem hiding this comment.
Declaring server.address / server.port as dd-names on the http parent, and removing the otel-names from http.hostname, peer.hostname and outbound peer.port, causes a few problems:
- OTLP regressions. Every non-HTTP outbound span that uses
outbound_peer(db clients, Lettuce, …) stops exportingserver.address/server.port, as the bots noted. So do HTTP spans for anyone usingOTEL_TRACES_EXPORTER=otlpwithout OTel semantics, because the decorators only writeserver.addresswhile the flag is on. - The direction-scoped export already exists. The
OtlpTraceJsonCollectorTestrows commented out with "restore when an OTel name can be qualified by a direction" are the master test for exactly that:http.hostname → server.addressinbound, andpeer.hostname → server.address/peer.port → server.portoutbound, from Scope OpenTelemetry tag names by span direction in the tag registry (otlp - tag registry - phase 2) #12713. - Two stored forms for one concept. In OTel mode, client spans have no
peer.hostnameand server spans have nohttp.hostname, so in-process readers keyed on the Datadog tags (stats peer tags, sampling rules, peer service) silently miss them. Resolve direction-dependent tag names by the span's kind (otlp - tag registry - phase 3) #12731 goes the other way:server.addressis a direction-scoped alias that resolves to the single Datadog tag.
Suggestion: keep writing peer.hostname / peer.port (adding the 80/443 default in OTel mode) and http.hostname, and let the registry rename them on export. The only genuinely new tag is inbound server.port, which can be declared on http.server alone without colliding with outbound peer.port → server.port, since uniqueness is checked per direction. That also removes the conflict.
Written by Claude, reviewed by @dougqh
| # Inbound only: the client's socket address. Server spans also carry that address in peer.ipv4 / | ||
| # peer.ipv6, so do not map those inbound too, or the attribute is exported twice. | ||
| - { dd-name: network.client.ip, type: string, required: recommended, otel-name: network.peer.address } | ||
| - { dd-name: network.client.ip, type: string, required: recommended, otel-name: network.peer.address, span-kind-neutral: true } # Temporary direction-free alias until name resolution is span-kind-aware |
There was a problem hiding this comment.
#12713 scoped this name to inbound spans on purpose. With span-kind-neutral: true, network.peer.address enters the direction-free lookup, so writing network.peer.address on an outbound span (OTel API or DD API) gets stored as network.client.ip. That's the wrong concept: on a client span it's the server's address.
As far as I can tell, this is only needed for two test reads: span.getTag("network.peer.address") in the forked decorator test, and the span with no kind in collectTagsAttributeKeys. Could those read by the Datadog name and give the span a kind, so this can stay inbound-only? The existing @TableTest already covers the inbound export.
Written by Claude, reviewed by @dougqh
|
More general concern: this adds seven copies of Most of it could be one projection step at export, matching .NET's model where the writer doesn't know the output convention:
What really needs write-site changes is small: the config defaults, inbound Written by Claude, reviewed by @dougqh |
| return -1; | ||
| } | ||
| try { | ||
| final int port = Integer.parseInt(value); |
There was a problem hiding this comment.
parsePort runs on X-Forwarded-Port and the forwarded Host port, which clients control. A malformed value throws and catches a NumberFormatException on every such request; the exception and its stack-trace capture are the expensive part. Ports are at most 5 digits, so a digit loop is simple and allocation-free:
int port = 0;
int len = value.length();
if (len == 0 || len > 5) return -1;
for (int i = 0; i < len; i++) {
char c = value.charAt(i);
if (c < '0' || c > '9') return -1;
port = port * 10 + (c - '0');
}
return port > 0 && port <= 65535 ? port : -1;(Same pattern as #12791 for the Lambda forwarded-port path.)
Written by Claude, reviewed by @dougqh
| } | ||
|
|
||
| public static void setClientUrl(final AgentSpan span, final URI url) { | ||
| span.setTag(KnownTags.HTTP_URL_OTEL_NAME, withoutQuery(redactedUrl(url))); |
There was a problem hiding this comment.
On master the client URL is stored as URIUtils.lazyValidURL(...), which builds nothing until serialization. Here url.full is built eagerly on the application thread: withoutQuery / redactedUrl copy the URL whenever there's a query, a fragment or userinfo. QueryObfuscator then rebuilds it in post-processing with the query re-appended. So a client URL with a query is now copied twice on every outbound call, where before it was built once, lazily.
Could this store a lazy CharSequence that does the redact and strip at serialization, following the lazyValidURL precedent? It also fits the "project at export" direction from my earlier comment: if the projection happens at export, the copying moves off the request thread entirely.
Written by Claude, reviewed by @dougqh
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
It looks like this is adding some conditional code that can be avoided by taking advantage of tag-conventions.yaml a bit more.
In particular, there are cases where just specifying a dd-name and otel-name on the same tag should be sufficient now. The tag registry will even handle disambiguating automatically based on inbound vs outbound spans. Admittedly, I'm still refining some of the mechanics (see #12731).
I'd also like to avoid working around tag interceptors (e.g. resetting the resource name after url.full triggers URLAsResourceNameRule), and/or move some of the translation into the OTLP serializer instead of placing it in instrumentations; there's more detail in my earlier comment. We can take those case by case.
I do have longer-term ideas for making all of that better, but they are longer term. I appreciate that you've already given me some time to bring our efforts together, so we won't hold this on every capability I want to add to the core.
| } | ||
|
|
||
| if (valid && config.isHttpServerTagQueryString()) { | ||
| if (valid && (OTEL_SEMANTICS || config.isHttpServerTagQueryString())) { |
There was a problem hiding this comment.
Similar to the client-IP gate: OTEL_SEMANTICS || here overrides a user's DD_HTTP_SERVER_TAG_QUERY_STRING=false opt-out. OtelHttpSemantics.setClientUrl does the same on the client side, ignoring isHttpClientTagQueryString(). Query strings often carry tokens or PII.
The OTel conventions do mark url.query as Conditionally Required when a query is present. Even so, a user's explicit opt-out should win over conformance, and the conventions themselves say sensitive content in url.query should be scrubbed. Could the semantics flag respect these opt-outs rather than override them?
Written by Claude, reviewed by @dougqh
|
Following up on my earlier comment with a concrete shape for the URL piece, using There are two axes, and both can be decided statically:
// HttpServerDecorator
private static final TagExtractor<HttpUrl> URL_TAGS =
Config.get().isTraceOtelSemanticsEnabled() ? OtelServerUrl.INSTANCE : DatadogServerUrl.INSTANCE;
...
span.setTagsFrom(url, URL_TAGS);
What this gets:
Two suggestions so it plays well with the rest of the tracer:
This also leaves room for the export-time projection: if the extractors depend only on the URL record and the span, moving them to serialization later is mechanical. Written by Claude, reviewed by @dougqh |
This branch has not been deployed
2 participants
What Does This Do
This PR updates HTTP client and server spans produced by the shared automatic-instrumentation decorators so that their span names, attributes, and error handling follow the OpenTelemetry HTTP semantic conventions when
DD_TRACE_OTEL_SEMANTICS_ENABLED=true.Behavior remains unchanged when OpenTelemetry semantics are disabled.
Implementation Details
Configuration
Enabling
DD_TRACE_OTEL_SEMANTICS_ENABLEDoverrides the effective values of the following settings:OTEL_TRACES_EXPORTER=otlpDD_WRITER_TYPE=OtlpWriterDD_TRACE_SPAN_ATTRIBUTE_SCHEMA=v0DD_TRACE_PEER_SERVICE_DEFAULTS_ENABLED=falseDD_HTTP_CLIENT_ERROR_STATUSES=400-599(in line with the OpenTelemetry specification)HTTP client changes
When
DD_TRACE_OTEL_SEMANTICS_ENABLED=true:{method}instead of{method} {path}.HTTPas the span name, emit_OTHERashttp.request.method, and preserve the original method inhttp.request.method_original.url.fullinstead ofhttp.url, with credentials redacted and query parameters obfuscated.server.addressandserver.port, including default ports 80 and 443.error.typeto the status code unless a more specific error type already exists.HTTP server changes
When
DD_TRACE_OTEL_SEMANTICS_ENABLED=true:{method} {http.route}when a route is available, or{method}otherwise. Request paths and404are no longer used as naming fallbacks.HTTPas the span name, emit_OTHERashttp.request.method, and preserve the original method inhttp.request.method_original.url.scheme,url.path, and the obfuscatedurl.query, rather than a singularhttp.url.server.addressandserver.port, honoring forwarded protocol, host, and port headers.client.addressandnetwork.peer.address.error.typeto the status code unless a more specific error type already exists.url.pathwhen the legacyhttp.urlattribute is absentTest coverage
Unit and component tests
OtelHttpSemanticsTestcovers method normalization, URL credential redaction, query removal, fragment preservation, default ports, and error-type behavior.HttpDecoratorsOtelSemanticsForkedTestcovers client and server decorator output, route naming, status handling, forwarded origins, explicit forwarded ports, and IPv6 hosts.ConfigTestverifies the forced OpenTelemetry settings, calculated configuration values, and client error-status defaults.QueryObfuscatorTestverifies sanitized clienturl.fullreconstruction and server/client handling ofurl.query.HttpEndpointPostProcessorTestverifies endpoint discovery fromurl.path.KnownTagsTestandOtlpTraceJsonCollectorTestverify tag registration and the OpenTelemetry names emitted over OTLP.Instrumentation tests
The shared HTTP test suites run in a forked JVM with OpenTelemetry semantics enabled for:
HTTP clients:
HTTP servers:
System tests
This is being tested by this PR: DataDog/system-tests#7919 . Currently the only test skips across all web servers are for Akka and Play servers, where the
http.routeneeds to be set on the root span. I added this below as follow-up work so the majority of frameworks could be served here without being blocked.Motivation
When
DD_TRACE_OTEL_SEMANTICS_ENABLED=true, the Java tracer should produce HTTP spans that align with the OpenTelemetry semantic conventions instead of emitting a mixture of Datadog and OpenTelemetry naming and attributes. This lets users switch between OpenTelemetry and Datadog automatic instrumentation with reduced updates to dashboards, monitors, or downstream processing.Additional Notes
This PR is easiest to review commit-by-commit: configuration, tag registration, client behavior, server behavior, client integration coverage, and server integration coverage are separated into focused commits.
Related .NET implementations: HTTP client spans and HTTP server spans.
Follow-up work requires the following:
http.routeContributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket(s): [https://datadoghq.atlassian.net/browse/APMAPI-2038], [https://datadoghq.atlassian.net/browse/APMAPI-2039]