Repository navigation
Support host-aware HTTP resource name normalization - #13048
RahulPrasanna-code wants to merge 1 commit into
Conversation
- Support host-prefixed mapping patterns in AntPatternHttpPathNormalizer (e.g., auth.example.com/oauth2/token:oauth)
- Support exact URL path mappings without requiring wildcard characters
- Support {host} placeholder interpolation in mapping replacement values
- Extend HttpResourceNames and HttpResourceDecorator with overloaded methods accepting CharSequence host
- Pass host from HttpClientDecorator and UrlInstrumentation to HTTP_RESOURCE_DECORATOR
- Maintain 100% backward compatibility for existing path-only mappings
- Add unit tests for AntPatternHttpPathNormalizer, HttpResourceDecorator, and HttpClientDecorator
Fixes DataDog#6886
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: 82f55ddede
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| public final void withServerPath( | ||
| AgentSpan span, CharSequence method, CharSequence path, boolean encoded) { | ||
| withServerPath(span, method, null, path, encoded); |
There was a problem hiding this comment.
When a host-prefixed DD_TRACE_HTTP_SERVER_PATH_RESOURCE_NAME_MAPPING is configured, every normal server span still enters this overload and gets host = null: the only production call found is HttpServerDecorator.onRequest at line 365, even though URIDataAdapter.host() is already available there. Consequently the new server matcher can never evaluate host + path, so server host rules always fall back to path normalization; pass the effective request host to the five-argument overload.
Useful? React with 👍 / 👎.
| if (pattern.startsWith("http://")) { | ||
| pattern = pattern.substring(7); | ||
| } else if (pattern.startsWith("https://")) { | ||
| pattern = pattern.substring(8); |
There was a problem hiding this comment.
When a user configures the newly supported form https://secure.com/api/*:secure_api, it never reaches this normalization: both path-resource-name mappings are loaded by ConfigProvider.getOrderedMap, and ConfigConverter.loadMap treats the first : as the key/value separator, producing key https and value //secure.com/api/*:secure_api. Thus the scheme-stripping branch is unreachable for real configuration (the added test constructs the Map directly), and the rule never matches; the configuration parser needs an escaping or scheme-aware representation.
Useful? React with 👍 / 👎.
|
|
||
| public static Pair<CharSequence, Byte> computeForClient( | ||
| CharSequence method, @Nonnull CharSequence path, boolean encoded) { | ||
| return computeForClient(method, null, path, encoded); |
There was a problem hiding this comment.
When a manual/API or OTel client span derives its resource from http.url/url.full, TagInterceptor.setResourceFromUrl parses the complete URI but passes only its path to computeForClient (TagInterceptor.java:303-317), and this newly added delegation then forces the host to null. A host-prefixed client rule therefore works for HttpClientDecorator spans but silently fails on the existing tag-interceptor normalization path; that caller should extract the parsed URI host and invoke the host-aware overload.
Useful? React with 👍 / 👎.
|
|
||
| boolean hasHost = host != null && !host.isEmpty(); | ||
| if (hasHost) { | ||
| String hostAndPath = path.startsWith("/") ? (host + path) : (host + "/" + path); |
There was a problem hiding this comment.
When an outbound URI contains a mixed- or upper-case host such as https://AUTH.Example.COM/oauth2/token while the mapping uses the normal lower-case DNS spelling, URI.getHost() preserves the original case and this concatenates it directly into the case-sensitive AntPathMatcher. Although hostnames are case-insensitive, exact and wildcard rules such as auth.example.com/... will not match and the span falls back to the generic path, so normalize the configured and runtime host portions to a common case while preserving path case.
Useful? React with 👍 / 👎.
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Host-aware server mappings never receive the request host, so host-prefixed rules cannot match and host placeholders expand to an empty string. Existing path mappings beginning with double slashes also stop matching.
🤖 Bits Code Review · Commit 82f55dd
|
|
||
| public final void withServerPath( | ||
| AgentSpan span, CharSequence method, CharSequence path, boolean encoded) { | ||
| withServerPath(span, method, null, path, encoded); |
There was a problem hiding this comment.
HttpServerDecorator.onRequest still calls this overload, which supplies a null host despite having url.host() available. Consequently, host-prefixed server mappings never match, and server replacements containing {host} receive an empty string. Update the server request call site to pass the host into the new overload.
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 (pattern.startsWith("http://")) { | ||
| pattern = pattern.substring(7); | ||
| } else if (pattern.startsWith("https://")) { | ||
| pattern = pattern.substring(8); |
There was a problem hiding this comment.
Configuring https://secure.com/api/*:secure_api does not reach this scheme-handling branch: ConfigConverter.parseOrderedMap splits at the first colon, producing the key https and value //secure.com/api/*:secure_api. Requests therefore silently use fallback normalization. The new test constructs the map directly and bypasses configuration parsing; scheme support also requires updating the configuration parsing path.
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 if (pattern.startsWith("https://")) { | ||
| pattern = pattern.substring(8); | ||
| } else if (pattern.startsWith("//")) { | ||
| pattern = pattern.substring(2); |
There was a problem hiding this comment.
An existing path-only mapping such as //foo/:mapped previously matched the request path //foo/123. Stripping its leading slashes changes the pattern to foo/, which AntPathMatcher rejects against an absolute path. Preserve the original pattern alongside the host-pattern interpretation so existing resource mappings continue working.
| pattern = pattern.substring(2); | |
| resourceNameMatchers.put(pattern, entry.getValue()); | |
| pattern = pattern.substring(2); |
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
| String hostAndPath = path.startsWith("/") ? (host + path) : (host + "/" + path); | ||
| for (Map.Entry<String, String> entry : resourceNameMatchers.entrySet()) { | ||
| String pattern = entry.getKey(); | ||
| if (!pattern.startsWith("/") && matcher.match(pattern, hostAndPath)) { |
There was a problem hiding this comment.
For https://AUTH.EXAMPLE.COM/oauth2/token, URI.getHost() preserves uppercase characters, but AntPathMatcher compares them case-sensitively. The rule auth.example.com/oauth2/*:oauth therefore misses the same HTTP host solely because its casing differs, causing fallback resource names. Normalize the hostname portions of both configured patterns and request inputs while preserving path case.
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
What Does This Do
Extends the HTTP resource name normalization pipeline to support host-aware pattern matching and placeholder interpolation in custom path resource name mappings (
dd.trace.http.client.path-resource-name-mappinganddd.trace.http.server.path-resource-name-mapping):auth.provider-a.com/oauth2/token:host_a_oauthand wildcard hosts like*.provider-b.com/oauth2/*:provider_b_oauthto match againsthost + path, allowing disambiguation of identical paths across different remote providers./oauth2/token:oauthorhost-a.com/oauth2/token:host_a_oauth) without requiring wildcard characters (*or?), which were previously filtered out during normalizer initialization.{host}Placeholder Interpolation: Supports{host}placeholder interpolation in replacement values (e.g.,/oauth2/token:{host}_oauthproducesauth.provider-a.com_oauth).HttpClientDecorator,HttpResourceDecorator,HttpResourceNames, andUrlInstrumentationto passhostto the normalizer chain while preserving 100% backward compatibility for all existing 3-argument methods and path-only configurations.Motivation
Solves #6886
When a service makes HTTP client calls to multiple third-party endpoints sharing identical URI paths (such as
https://auth.provider-a.com/oauth2/tokenandhttps://login.provider-b.com/oauth2/token), the tracer previously stripped the host and normalized both spans to the exact sameresource.name(POST /oauth2/token). This prevented distinguishing metrics per external provider in Datadog APM by resource name.Additional Notes
withClientPath(span, method, path),setForClient(span, method, path, encoded), etc.) are preserved and delegate cleanly withhost = null. Path-only rules continue matching as before.AntPatternHttpPathNormalizermapsPair<String, String>(host, path) to ensure fast normalization without repeated pattern evaluations.AntPatternHttpPathNormalizerTest: exact path matching, host-prefixed rules, wildcard hosts, schemes normalization, and{host}placeholder interpolation.HttpResourceDecoratorTest: host disambiguation, wildcard host matching,{host}interpolation, and fallback compatibility with null host.HttpClientDecoratorTest: end-to-end span decoration with host-aware client mapping.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issue (usedSolves #6886)