Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
import datadog.trace.bootstrap.instrumentation.api.InternalSpanTypes;
import datadog.trace.bootstrap.instrumentation.api.OtelHttpSemantics;
import datadog.trace.bootstrap.instrumentation.api.Tags;
import datadog.trace.bootstrap.instrumentation.api.URIUtils;
import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString;
Expand All @@ -42,6 +43,7 @@ public abstract class HttpClientDecorator<REQUEST, RESPONSE> extends UriBasedCli
private static final String DD_CLIENT_LIBRARY_LANGUAGE_HEADER_NAME = "DD-Client-Library-Language";

private static final BitSet CLIENT_ERROR_STATUSES = Config.get().getHttpClientErrorStatuses();
private static final boolean OTEL_SEMANTICS = Config.get().isTraceOtelSemanticsEnabled();

private static final UTF8BytesString DEFAULT_RESOURCE_NAME = UTF8BytesString.create("/");

Expand Down Expand Up @@ -118,7 +120,14 @@ protected void doOnRequest(final AgentSpan span, final REQUEST request) {
DSM_TRANSACTION_SOURCE_READER);

String method = method(request);
span.setTag(Tags.HTTP_METHOD, method);
if (OTEL_SEMANTICS) {
OtelHttpSemantics.setRequestMethod(span, method);
if (shouldSetResourceName()) {
OtelHttpSemantics.setMethodResourceName(span, method);
}
} else {
span.setTag(Tags.HTTP_METHOD, method);
}

if (CLIENT_TAG_HEADERS) {
for (Map.Entry<String, String> headerTag :
Expand All @@ -135,19 +144,28 @@ protected void doOnRequest(final AgentSpan span, final REQUEST request) {
final URI url = url(request);
if (url != null) {
onURI(span, url);
span.setTag(
Tags.HTTP_URL,
URIUtils.lazyValidURL(url.getScheme(), url.getHost(), url.getPort(), url.getPath()));
if (Config.get().isHttpClientTagQueryString()) {
span.setTag(DDTags.HTTP_QUERY, url.getQuery());
span.setTag(DDTags.HTTP_FRAGMENT, url.getFragment());
if (OTEL_SEMANTICS) {
OtelHttpSemantics.setClientUrl(span, url);
// url.full may trigger URLAsResourceNameRule; restore the OTel span name afterwards.
if (shouldSetResourceName()) {
OtelHttpSemantics.setMethodResourceName(span, method);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Prevent path mappings from overriding method-only client names

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

}
} else {
span.setTag(
Tags.HTTP_URL,
URIUtils.lazyValidURL(
url.getScheme(), url.getHost(), url.getPort(), url.getPath()));
if (Config.get().isHttpClientTagQueryString()) {
span.setTag(DDTags.HTTP_QUERY, url.getQuery());
span.setTag(DDTags.HTTP_FRAGMENT, url.getFragment());
}
}
if (shouldSetResourceName()) {
if (!OTEL_SEMANTICS && shouldSetResourceName()) {
HTTP_RESOURCE_DECORATOR.withClientPath(span, method, url.getPath());
}
// SSRF exploit prevention check
onHttpClientRequest(span, url.toString());
} else if (shouldSetResourceName()) {
} else if (!OTEL_SEMANTICS && shouldSetResourceName()) {
span.setResourceName(DEFAULT_RESOURCE_NAME);
}
} catch (final BlockingException e) {
Expand Down Expand Up @@ -177,6 +195,9 @@ protected void doOnResponse(final AgentSpan span, final RESPONSE response) {
span.setHttpStatusCode(status);
if (CLIENT_ERROR_STATUSES.get(status)) {
span.setError(true);
if (OTEL_SEMANTICS) {
OtelHttpSemantics.setErrorType(span, status);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import datadog.context.propagation.Propagators;
import datadog.trace.api.Config;
import datadog.trace.api.DDTags;
import datadog.trace.api.KnownTags;
import datadog.trace.api.datastreams.DataStreamsTransactionExtractor;
import datadog.trace.api.datastreams.DataStreamsTransactionTracker;
import datadog.trace.api.function.TriConsumer;
Expand All @@ -34,6 +35,7 @@
import datadog.trace.bootstrap.instrumentation.api.ClientIpAddressData;
import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities;
import datadog.trace.bootstrap.instrumentation.api.InternalSpanTypes;
import datadog.trace.bootstrap.instrumentation.api.OtelHttpSemantics;
import datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities;
import datadog.trace.bootstrap.instrumentation.api.TagContext;
import datadog.trace.bootstrap.instrumentation.api.Tags;
Expand Down Expand Up @@ -68,13 +70,15 @@ public abstract class HttpServerDecorator<REQUEST, CONNECTION, RESPONSE, REQUEST
public static final String DD_IGNORE_COMMIT_ATTRIBUTE = "datadog.commit.ignore";

private static final UTF8BytesString DEFAULT_RESOURCE_NAME = UTF8BytesString.create("/");
private static final boolean OTEL_SEMANTICS = Config.get().isTraceOtelSemanticsEnabled();
protected static final UTF8BytesString NOT_FOUND_RESOURCE_NAME = UTF8BytesString.create("404");
protected static final boolean SHOULD_SET_404_RESOURCE_NAME =
Config.get().isRuleEnabled("URLAsResourceNameRule")
!OTEL_SEMANTICS
&& Config.get().isRuleEnabled("URLAsResourceNameRule")
&& Config.get().isRuleEnabled("Status404Rule")
&& Config.get().isRuleEnabled("Status404Decorator");
private static final boolean SHOULD_SET_URL_RESOURCE_NAME =
Config.get().isRuleEnabled("URLAsResourceNameRule");
!OTEL_SEMANTICS && Config.get().isRuleEnabled("URLAsResourceNameRule");

private static final BitSet SERVER_ERROR_STATUSES = Config.get().getHttpServerErrorStatuses();
private static final String DEFAULT_INSTRUMENTATION_NAME = "http-server";
Expand Down Expand Up @@ -287,7 +291,9 @@ protected void doOnRequest(
// - peer.ipv4
// - peer.ipv6
final boolean shouldTagIps =
config.isClientIpEnabled() || traceClientIpResolverEnabled && APPSEC_ACTIVE;
OTEL_SEMANTICS

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, will remove OTEL_SEMANTICS || to restore the opt-in behavior for IP tag collection

|| config.isClientIpEnabled()
|| traceClientIpResolverEnabled && APPSEC_ACTIVE;
// Whether to stash IP data for later tagging or not.
// AI Guard requires client IP tags on the local root span when an ai_guard span is created.
// Resolve the IPs eagerly but do not tag the span yet; stash them on the request context so
Expand Down Expand Up @@ -329,7 +335,12 @@ protected void doOnRequest(

if (request != null) {
String method = method(request);
span.setTag(Tags.HTTP_METHOD, method);
if (OTEL_SEMANTICS) {
OtelHttpSemantics.setRequestMethod(span, method);
OtelHttpSemantics.setMethodResourceName(span, method);
} else {
span.setTag(Tags.HTTP_METHOD, method);
}

// Copy of HttpClientDecorator url handling
try {
Expand All @@ -339,23 +350,59 @@ protected void doOnRequest(
boolean encoded = supportsRaw && config.isHttpServerRawResource();
boolean valid = url.isValid();
String path = encoded ? url.rawPath() : url.path();
if (valid) {
span.setTag(
Tags.HTTP_URL, URIUtils.lazyValidURL(url.scheme(), url.host(), url.port(), path));
} else if (supportsRaw) {
span.setTag(Tags.HTTP_URL, URIUtils.lazyInvalidUrl(url.raw()));
}
if (extracted != null && extracted.getXForwardedHost() != null) {
span.setTag(Tags.HTTP_HOSTNAME, extracted.getXForwardedHost());
} else if (url.host() != null) {
span.setTag(Tags.HTTP_HOSTNAME, url.host());
if (OTEL_SEMANTICS) {
String scheme =
OtelHttpSemantics.forwardedValue(
extracted == null ? null : extracted.getXForwardedProto());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Enable forwarded-header extraction for OpenTelemetry semantics

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

if (scheme == null) {
scheme = url.scheme();
}
if (valid) {
if (scheme != null) {
span.setTag(KnownTags.URL_SCHEME_NAME, scheme);
}
// Ensure that path components have a leading '/'
if (path == null || path.isEmpty()) {
path = "/";
} else if (path.charAt(0) != '/') {
path = "/" + path;
}
span.setTag(KnownTags.URL_PATH_NAME, path);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve API Security URL fallback for OTel server spans

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

} else if (supportsRaw) {
span.setTag(KnownTags.URL_PATH_NAME, url.raw());
}
OtelHttpSemantics.setServerAddressAndPort(
span,
extracted == null ? null : extracted.getXForwardedHost(),
extracted == null ? null : extracted.getXForwardedPort(),
scheme,
url.host(),
url.port());
} else {
if (valid) {
span.setTag(
Tags.HTTP_URL, URIUtils.lazyValidURL(url.scheme(), url.host(), url.port(), path));
} else if (supportsRaw) {
span.setTag(Tags.HTTP_URL, URIUtils.lazyInvalidUrl(url.raw()));
}
if (extracted != null && extracted.getXForwardedHost() != null) {
span.setTag(Tags.HTTP_HOSTNAME, extracted.getXForwardedHost());
} else if (url.host() != null) {
span.setTag(Tags.HTTP_HOSTNAME, url.host());
}
}

if (valid && config.isHttpServerTagQueryString()) {
if (valid && (OTEL_SEMANTICS || config.isHttpServerTagQueryString())) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

String query =
supportsRaw && config.isHttpServerRawQueryString() ? url.rawQuery() : url.query();
span.setTag(DDTags.HTTP_QUERY, query);
span.setTag(DDTags.HTTP_FRAGMENT, url.fragment());
if (OTEL_SEMANTICS) {
if (query != null && !query.isEmpty()) {
span.setTag(KnownTags.URL_QUERY_NAME, query);
}
} else {
span.setTag(DDTags.HTTP_QUERY, query);
span.setTag(DDTags.HTTP_FRAGMENT, url.fragment());
}
}
Flow<Void> flow = callIGCallbackURI(span, url, method);
if (flow.getAction() instanceof RequestBlockingAction) {
Expand Down Expand Up @@ -422,6 +469,9 @@ protected void doOnRequest(
}
if (shouldTagIps) {
span.setTag(Tags.NETWORK_CLIENT_IP, peerIp);
if (OTEL_SEMANTICS && span.getTag(KnownTags.HTTP_CLIENT_IP_OTEL_NAME) == null) {
span.setTag(KnownTags.HTTP_CLIENT_IP_OTEL_NAME, peerIp);
}
}
}
if (shouldStashIps && (peerIp != null || inferredAddressStr != null)) {
Expand Down Expand Up @@ -484,6 +534,9 @@ protected void doOnResponseStatus(final AgentSpan span, final int status) {
if (!BlockingException.class.getName().equals(span.getTag("error.type"))) {
span.setError(SERVER_ERROR_STATUSES.get(status), ErrorPriorities.HTTP_SERVER_DECORATOR);
}
if (OTEL_SEMANTICS && SERVER_ERROR_STATUSES.get(status)) {
OtelHttpSemantics.setErrorType(span, status);
}
}

if (SHOULD_SET_404_RESOURCE_NAME && status == 404) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,36 @@
import datadog.context.Context;
import datadog.context.propagation.CarrierSetter;
import datadog.trace.api.Config;
import datadog.trace.api.KnownTags;
import datadog.trace.api.datastreams.DataStreamsContext;
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.OtelHttpSemantics;
import datadog.trace.bootstrap.instrumentation.api.Tags;
import java.net.URI;
import javax.annotation.Nonnull;

public abstract class UriBasedClientDecorator extends ClientDecorator {
private static final boolean OTEL_SEMANTICS = Config.get().isTraceOtelSemanticsEnabled();

public void onURI(@Nonnull final AgentSpan span, @Nonnull final URI uri) {
String host = uri.getHost();
int port = uri.getPort();
if (null != host && !host.isEmpty()) {
span.setTag(Tags.PEER_HOSTNAME, host);
if (OTEL_SEMANTICS) {
span.setTag(KnownTags.SERVER_ADDRESS_NAME, host);
int serverPort = OtelHttpSemantics.serverPort(uri);
if (serverPort > 0) {
span.setTag(KnownTags.SERVER_PORT_NAME, serverPort);
}
} else {
span.setTag(Tags.PEER_HOSTNAME, host);
int port = uri.getPort();
if (port > 0) {
setPeerPort(span, port);
}
}
if (Config.get().isHttpClientSplitByDomain() && host.charAt(0) >= 'A') {
span.setServiceName(host, component());
}
if (port > 0) {
setPeerPort(span, port);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package datadog.trace.bootstrap.instrumentation.decorator;

import datadog.trace.api.Config;
import datadog.trace.api.cache.DDCache;
import datadog.trace.api.cache.DDCaches;
import datadog.trace.api.naming.SpanNaming;
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.InternalSpanTypes;
import datadog.trace.bootstrap.instrumentation.api.OtelHttpSemantics;
import datadog.trace.bootstrap.instrumentation.api.Tags;
import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString;
import java.net.URI;
Expand All @@ -16,6 +18,7 @@
import org.slf4j.LoggerFactory;

public class UrlConnectionDecorator extends UriBasedClientDecorator {
private static final boolean OTEL_SEMANTICS = Config.get().isTraceOtelSemanticsEnabled();
private static final DDCache<String, CharSequence> CACHE = DDCaches.newFixedSizeCache(16);

private static final Function<String, CharSequence> ADDER =
Expand Down Expand Up @@ -51,7 +54,11 @@ protected String service() {
@Override
public void onURI(@Nonnull AgentSpan span, @Nonnull URI uri) {
super.onURI(span, uri);
span.setTag(Tags.HTTP_URL, uri.toString());
if (OTEL_SEMANTICS) {
OtelHttpSemantics.setClientUrl(span, uri);
} else {
span.setTag(Tags.HTTP_URL, uri.toString());
}
}

public void onURL(@Nonnull final AgentSpan span, @Nonnull final URL url) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import datadog.trace.api.Config;
import datadog.trace.api.normalize.HttpResourceNames;
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.OtelHttpSemantics;
import datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities;
import datadog.trace.bootstrap.instrumentation.api.Tags;
import datadog.trace.bootstrap.instrumentation.api.URIUtils;
Expand All @@ -12,18 +13,27 @@ public class HttpResourceDecorator {
public static final HttpResourceDecorator HTTP_RESOURCE_DECORATOR = new HttpResourceDecorator();

private static final UTF8BytesString DEFAULT_RESOURCE_NAME = UTF8BytesString.create("/");
private static final boolean OTEL_SEMANTICS = Config.get().isTraceOtelSemanticsEnabled();

private final boolean shouldSetUrlResourceName =
Config.get().isRuleEnabled("URLAsResourceNameRule");

private HttpResourceDecorator() {}

public final void withClientPath(AgentSpan span, CharSequence method, CharSequence path) {
HttpResourceNames.setForClient(span, method, path, false);
if (OTEL_SEMANTICS) {
OtelHttpSemantics.setMethodResourceName(span, method);
Comment on lines +24 to +25

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Limit method-only naming to HTTP spans

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

} else {
HttpResourceNames.setForClient(span, method, path, false);
}
}

public final void withServerPath(
AgentSpan span, CharSequence method, CharSequence path, boolean encoded) {
if (OTEL_SEMANTICS) {
OtelHttpSemantics.setMethodResourceName(span, method);
return;
}
if (!shouldSetUrlResourceName) {
span.setResourceName(DEFAULT_RESOURCE_NAME);
return;
Expand All @@ -44,7 +54,11 @@ public final void withRoute(
routeTag = URIUtils.decode(route.toString());
}
span.setTag(Tags.HTTP_ROUTE, routeTag);
if (Config.get().isHttpServerRouteBasedNaming()) {
if (OTEL_SEMANTICS) {
final CharSequence resourceName =
HttpResourceNames.join(OtelHttpSemantics.spanNameMethod(method), routeTag);
span.setResourceName(resourceName, ResourceNamePriorities.HTTP_FRAMEWORK_ROUTE);
} else if (Config.get().isHttpServerRouteBasedNaming()) {
final CharSequence resourceName = HttpResourceNames.join(method, route);
span.setResourceName(resourceName, ResourceNamePriorities.HTTP_FRAMEWORK_ROUTE);
}
Expand Down
Loading
Loading