Skip to content
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

Jump to bottom

Add span for jax-rs representing controller execution #435

Merged
merged 2 commits into from
Aug 15, 2018
Merged

Add span for jax-rs representing controller execution #435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5cc84e
Add span for jax-rs representing controller execution
tylerbenson Aug 10, 2018
984bc75
Add span.origin.type for JSP too.
tylerbenson Aug 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 40 additions & 3 deletions ...ns/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package datadog.trace.instrumentation.jaxrs;

import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType;
import static io.opentracing.log.Fields.ERROR_OBJECT;
import static net.bytebuddy.matcher.ElementMatchers.declaresMethod;
import static net.bytebuddy.matcher.ElementMatchers.isAnnotatedWith;
import static net.bytebuddy.matcher.ElementMatchers.named;
Expand All @@ -9,10 +10,12 @@
import datadog.trace.agent.tooling.Instrumenter;
import datadog.trace.api.DDTags;
import io.opentracing.Scope;
import io.opentracing.Span;
import io.opentracing.tag.Tags;
import io.opentracing.util.GlobalTracer;
import java.lang.annotation.Annotation;
import java.lang.reflect.Method;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.Map;
Expand Down Expand Up @@ -54,11 +57,11 @@ public Map<ElementMatcher, String> transformers() {
public static class JaxRsAnnotationsAdvice {

@Advice.OnMethodEnter(suppress = Throwable.class)
public static void nameSpan(@Advice.This final Object obj, @Advice.Origin final Method method) {
// TODO: do we need caching for this?
public static Scope nameSpan(@Advice.Origin final Method method) {

// TODO: do we need caching for this?
final LinkedList<Path> classPaths = new LinkedList<>();
Class<?> target = obj.getClass();
Class<?> target = method.getDeclaringClass();
while (target != Object.class) {
final Path annotation = target.getAnnotation(Path.class);
if (annotation != null) {
Expand Down Expand Up @@ -92,6 +95,40 @@ public static void nameSpan(@Advice.This final Object obj, @Advice.Origin final
scope.span().setTag(DDTags.RESOURCE_NAME, resourceName);
Tags.COMPONENT.set(scope.span(), "jax-rs");
}

// Now create a span representing the method execution.

final Class<?> clazz = method.getDeclaringClass();
Copy link
Contributor

Choose a reason for hiding this comment

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

If this expected to be a 'declaring class' - is seems like it would be possible for clients to have some sort of a 'parent' class where methods are defined and multiple child classes that actually implement things. Having parent class in spans may be confusing. Would it make sense to use actual object's class name here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same behavior that @Trace has. Perhaps that should be changed as well? Maybe we need better tests around this? (I'd suggest that be a separate task/pr...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. I would appreciate if you could create a task for that, otherwise PR looks good.
Thanks!

final String methodName = method.getName();

String className = clazz.getSimpleName();
if (className.isEmpty()) {
className = clazz.getName();
if (clazz.getPackage() != null) {
final String pkgName = clazz.getPackage().getName();
if (!pkgName.isEmpty()) {
className = clazz.getName().replace(pkgName, "").substring(1);
}
}
}

final String operationName = className + "." + methodName;

return GlobalTracer.get()
.buildSpan(operationName)
.withTag(Tags.COMPONENT.getKey(), "jax-rs-controller")
.startActive(true);
}

@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void stopSpan(
@Advice.Enter final Scope scope, @Advice.Thrown final Throwable throwable) {
if (throwable != null) {
final Span span = scope.span();
Tags.ERROR.set(span, true);
span.log(Collections.singletonMap(ERROR_OBJECT, throwable));
}
scope.close();
}
}
}
109 changes: 82 additions & 27 deletions ...rumentation/jax-rs-annotations/src/test/groovy/JaxRsAnnotationsInstrumentationTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import datadog.opentracing.DDSpanContext
import datadog.trace.agent.test.AgentTestRunner
import io.opentracing.util.GlobalTracer
import io.opentracing.tag.Tags

import javax.ws.rs.DELETE
import javax.ws.rs.GET
Expand All @@ -10,26 +9,43 @@ import javax.ws.rs.POST
import javax.ws.rs.PUT
import javax.ws.rs.Path

import static datadog.trace.agent.test.ListWriterAssert.assertTraces
import static datadog.trace.agent.test.TestUtils.runUnderTrace

class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {

def "span named '#resourceName' from annotations on class"() {
def "span named '#name' from annotations on class"() {
setup:
def scope = GlobalTracer.get().buildSpan("test").startActive(false)
DDSpanContext spanContext = scope.span().context()
obj.call()
runUnderTrace("test") {
obj.call()
}

expect:
spanContext.resourceName == resourceName

cleanup:
scope.close()
assertTraces(TEST_WRITER, 1) {
trace(0, 2) {
span(0) {
operationName "test"
resourceName name
parent()
tags {
"$Tags.COMPONENT.key" "jax-rs"
defaultTags()
}
}
span(1) {
operationName "${className}.call"
resourceName "${className}.call"
childOf span(0)
tags {
"$Tags.COMPONENT.key" "jax-rs-controller"
defaultTags()
}
}
}
}

where:
resourceName | obj
"test" | new Jax() {
// invalid because no annotations
void call() {}
}
name | obj
"/a" | new Jax() {
@Path("/a")
void call() {}
Expand All @@ -39,10 +55,6 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
@Path("/b")
void call() {}
}
"test" | new InterfaceWithPath() {
// invalid because no annotations
void call() {}
}
"POST /c" | new InterfaceWithPath() {
@POST
@Path("/c")
Expand All @@ -52,10 +64,6 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
@HEAD
void call() {}
}
"test" | new AbstractClassWithPath() {
// invalid because no annotations
void call() {}
}
"POST /abstract/d" | new AbstractClassWithPath() {
@POST
@Path("/d")
Expand All @@ -65,10 +73,6 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
@PUT
void call() {}
}
"test" | new ChildClassWithPath() {
// invalid because no annotations
void call() {}
}
"OPTIONS /abstract/child/e" | new ChildClassWithPath() {
@OPTIONS
@Path("/e")
Expand All @@ -79,6 +83,43 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
void call() {}
}
"POST /abstract/child" | new ChildClassWithPath()

className = getName(obj.class)
}

def "no annotations has no effect"() {
setup:
runUnderTrace("test") {
obj.call()
}

expect:
assertTraces(TEST_WRITER, 1) {
trace(0, 1) {
span(0) {
operationName "test"
resourceName "test"
tags {
defaultTags()
}
}
}
}

where:
obj | _
new Jax() {
void call() {}
} | _
new InterfaceWithPath() {
void call() {}
} | _
new AbstractClassWithPath() {
void call() {}
} | _
new ChildClassWithPath() {
void call() {}
} | _
}

interface Jax {
Expand All @@ -102,4 +143,18 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
@POST
void call() {}
}

def getName(Class clazz) {
String className = clazz.getSimpleName()
if (className.isEmpty()) {
className = clazz.getName()
if (clazz.getPackage() != null) {
final String pkgName = clazz.getPackage().getName()
if (!pkgName.isEmpty()) {
className = clazz.getName().replace(pkgName, "").substring(1)
}
}
}
return className
}
}
4 changes: 3 additions & 1 deletion ...mentation/jsp-2.3/src/main/java/datadog/trace/instrumentation/jsp/JSPInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,14 @@ public Map<ElementMatcher, String> transformers() {
public static class HttpJspPageAdvice {

@Advice.OnMethodEnter(suppress = Throwable.class)
public static Scope startSpan(@Advice.Argument(0) final HttpServletRequest req) {
public static Scope startSpan(
@Advice.This final Object obj, @Advice.Argument(0) final HttpServletRequest req) {
final Scope scope =
GlobalTracer.get()
.buildSpan("jsp.render")
.withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_SERVER)
.withTag(DDTags.SPAN_TYPE, DDSpanTypes.WEB_SERVLET)
.withTag("span.origin.type", obj.getClass().getSimpleName())
.withTag("servlet.context", req.getContextPath())
.startActive(true);

Expand Down
15 changes: 15 additions & 0 deletions dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "java-web-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" "org.apache.catalina.core.ApplicationFilterChain"
"servlet.context" "/$jspWebappContext"
"http.status_code" 200
defaultTags()
Expand All @@ -114,6 +115,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "jsp-http-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" jspClassName
"servlet.context" "/$jspWebappContext"
"jsp.requestURL" reqUrl
defaultTags()
Expand Down Expand Up @@ -175,6 +177,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "java-web-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" "org.apache.catalina.core.ApplicationFilterChain"
"servlet.context" "/$jspWebappContext"
"http.status_code" 200
defaultTags()
Expand All @@ -191,6 +194,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "jsp-http-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" "getQuery_jsp"
"servlet.context" "/$jspWebappContext"
"jsp.requestURL" reqUrl
defaultTags()
Expand Down Expand Up @@ -249,6 +253,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "java-web-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" "org.apache.catalina.core.ApplicationFilterChain"
"servlet.context" "/$jspWebappContext"
"http.status_code" 200
defaultTags()
Expand All @@ -265,6 +270,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "jsp-http-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" "post_jsp"
"servlet.context" "/$jspWebappContext"
"jsp.requestURL" reqUrl
defaultTags()
Expand Down Expand Up @@ -320,6 +326,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "java-web-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" "org.apache.catalina.core.ApplicationFilterChain"
"servlet.context" "/$jspWebappContext"
"http.status_code" 500
errorTags(JasperException, String)
Expand All @@ -337,6 +344,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "jsp-http-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" jspClassName
"servlet.context" "/$jspWebappContext"
"jsp.requestURL" reqUrl
errorTags(exceptionClass, errorMessage)
Expand Down Expand Up @@ -398,6 +406,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "java-web-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" "org.apache.catalina.core.ApplicationFilterChain"
"servlet.context" "/$jspWebappContext"
"http.status_code" 200
defaultTags()
Expand All @@ -414,6 +423,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "jsp-http-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" "includeHtml_jsp"
"servlet.context" "/$jspWebappContext"
"jsp.requestURL" reqUrl
defaultTags()
Expand Down Expand Up @@ -468,6 +478,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "java-web-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" "org.apache.catalina.core.ApplicationFilterChain"
"servlet.context" "/$jspWebappContext"
"http.status_code" 200
defaultTags()
Expand All @@ -484,6 +495,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "jsp-http-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" "includeMulti_jsp"
"servlet.context" "/$jspWebappContext"
"jsp.requestURL" reqUrl
defaultTags()
Expand All @@ -500,6 +512,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "jsp-http-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" "javaLoopH2_jsp"
"servlet.context" "/$jspWebappContext"
"jsp.requestURL" reqUrl
defaultTags()
Expand Down Expand Up @@ -533,6 +546,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "jsp-http-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" "javaLoopH2_jsp"
"servlet.context" "/$jspWebappContext"
"jsp.requestURL" reqUrl
defaultTags()
Expand Down Expand Up @@ -604,6 +618,7 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
"span.kind" "server"
"component" "java-web-servlet"
"span.type" DDSpanTypes.WEB_SERVLET
"span.origin.type" "org.apache.catalina.core.ApplicationFilterChain"
"servlet.context" "/$jspWebappContext"
"http.status_code" 500
errorTags(JasperException, String)
Expand Down