Skip to content

Fix the Play Framework's span resource name priority so that the client JAX-RS 404 cannot override it #8591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 16, 2025
Merged

Fix the Play Framework's span resource name priority so that the client JAX-RS 404 cannot override it #8591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c9f92b7
Experimental change to prevent JAX-RS instrumentation from overriding…
ygree Mar 19, 2025
b2eac2f
Add debug for setResourceName
ygree Mar 20, 2025
cd6af7c
Merge branch 'master' into ygree/fix-play-jaxrs-resource-name-priority
ygree May 22, 2025
8fa65e0
Clean up. Fix 404 handling.
ygree Jun 9, 2025
a4d72e4
Merge branch 'master' into ygree/fix-play-jaxrs-resource-name-priority
ygree Jun 10, 2025
25257d0
Merge branch 'master' into ygree/fix-play-jaxrs-resource-name-priority
ygree Jun 10, 2025
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
2 changes: 1 addition & 1 deletion .../src/main/java/datadog/trace/bootstrap/instrumentation/decorator/HttpServerDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public abstract class HttpServerDecorator<REQUEST, CONNECTION, RESPONSE, REQUEST

private static final UTF8BytesString DEFAULT_RESOURCE_NAME = UTF8BytesString.create("/");
protected static final UTF8BytesString NOT_FOUND_RESOURCE_NAME = UTF8BytesString.create("404");
private static final boolean SHOULD_SET_404_RESOURCE_NAME =
protected static final boolean SHOULD_SET_404_RESOURCE_NAME =
Config.get().isRuleEnabled("URLAsResourceNameRule")
&& Config.get().isRuleEnabled("Status404Rule")
&& Config.get().isRuleEnabled("Status404Decorator");
Expand Down
4 changes: 2 additions & 2 deletions dd-java-agent/instrumentation/play-2.6/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ repositories {
}
}

addTestSuiteForDir('baseTest', 'baseTest')
addTestSuiteForDir('latestDepTest', 'latestDepTest')
addTestSuite('baseTest')
addTestSuite('latestDepTest')

sourceSets {
main_play27 {
Expand Down
2 changes: 1 addition & 1 deletion ...strumentation/play-2.6/src/main/java/datadog/trace/instrumentation/play26/PlayAdvice.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public static void stopTraceOnResponse(
// set the resource name on the upstream akka/netty span if there is one
if (rootSpan != null && playControllerSpan.getResourceName() != null) {
rootSpan.setResourceName(
playControllerSpan.getResourceName(), ResourceNamePriorities.HTTP_PATH_NORMALIZER);
playControllerSpan.getResourceName(), ResourceNamePriorities.HTTP_FRAMEWORK_ROUTE);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use a higher priority to prevent the parent span from being accidentally changed by the JAX-RS instrumentation.

}
}
}
7 changes: 7 additions & 0 deletions .../play-2.6/src/main/java/datadog/trace/instrumentation/play26/PlayHttpServerDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import datadog.trace.bootstrap.instrumentation.api.AgentPropagation;
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.AgentSpanContext;
import datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities;
import datadog.trace.bootstrap.instrumentation.api.URIDataAdapter;
import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString;
import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator;
Expand Down Expand Up @@ -186,4 +187,10 @@ public AgentSpan onError(final AgentSpan span, Throwable throwable) {
}
return super.onError(span, throwable);
}

public void updateOn404Only(final AgentSpan span, final Result result) {
if (SHOULD_SET_404_RESOURCE_NAME && status(result) == 404) {
span.setResourceName(NOT_FOUND_RESOURCE_NAME, ResourceNamePriorities.HTTP_404);
}
}
}
5 changes: 4 additions & 1 deletion .../play-2.6/src/main/java/datadog/trace/instrumentation/play26/RequestCompleteCallback.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,11 @@ public Object apply(final Try<Result> result) {
if (result.isFailure()) {
DECORATE.onError(span, result.failed().get());
} else {
Result response = result.get();
if (REPORT_HTTP_STATUS) {
DECORATE.onResponse(span, result.get());
DECORATE.onResponse(span, response);
} else {
DECORATE.updateOn404Only(span, response);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set the name of the non-found resource name to be used at https://github.com/DataDog/dd-trace-java/pull/8591/files#diff-293ad0cf2c613fd3386ed5b93cd54cbe26b6cd352071e86dc849ccae48f0fda0R87.
This is needed since the Akka HTTP-level response handler uses the standard HTTP_404 priority, which is lower than the HTTP_FRAMEWORK_ROUTE priority that we have set.

}
}
DECORATE.beforeFinish(span);
Expand Down
10 changes: 10 additions & 0 deletions dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,16 @@ public byte getResourceNamePriority() {
}

public void setResourceName(final CharSequence resourceName, byte priority) {
if (log.isDebugEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: that's generally not required since the log debug checks it and does not perform toString or concat (because of {})

log.debug(
"setResourceName `{}`->`{}` with priority {}->{} for traceId={} spanId={}",
this.resourceName,
resourceName,
resourceNamePriority,
priority,
traceId,
spanId);
}
if (null == resourceName) {
return;
}
Expand Down
4 changes: 4 additions & 0 deletions gradle/java_no_deps.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,16 +307,20 @@ def isJavaVersionAllowedForProperty(JavaVersion version, String propertyPrefix =
def definedMin = project.hasProperty(minProp)
def definedMax = project.hasProperty(maxProp)
if (definedMin && project.getProperty(minProp).compareTo(version) > 0) {
logger.info("isJavaVersionAllowedForProperty is false b/o minProp=${minProp} is defined and greater than version=${version}")
Copy link
Contributor

Choose a reason for hiding this comment

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

are those logs intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They aren't related to the change, but I added them to figure out why Gradle didn't run the tests. They still require the --info parameter, but it's better than nothing in my opinion. There are a few conditions, and the build is silent about not running certain tasks, which might be puzzling unless you remember those conditions and check the current Java version and the module requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with dropping them, but since they were useful when I worked on the change, they might be useful to someone else or myself in the future. That's the main reason I want to keep them.

return false
}
//default to the general min if defined and specific one is was not defined
if (!propertyPrefix.isEmpty() && !definedMin && project.hasProperty('minJavaVersionForTests') && project.getProperty('minJavaVersionForTests').compareTo(version) > 0) {
logger.info("isJavaVersionAllowedForProperty is false b/o minJavaVersionForTests=${project.getProperty('minJavaVersionForTests')} is defined and greater than version=${version}")
return false
}
if (definedMax && project.getProperty(maxProp).compareTo(version) < 0) {
logger.info("isJavaVersionAllowedForProperty is false b/o maxProp=${project.getProperty(maxProp)} is defined and lower than version=${version}")
return false
}
if (!propertyPrefix.isEmpty() && !definedMax && project.hasProperty('maxJavaVersionForTests') && project.getProperty('maxJavaVersionForTests').compareTo(version) < 0) {
logger.info("isJavaVersionAllowedForProperty is false b/o maxJavaVersionForTests=${project.getProperty('maxJavaVersionForTests')} is defined and lower than version=${version}")
return false
}
return true
Expand Down