Rename resource name to span name (#327)

This commit is contained in:
Trask Stalnaker 2020-04-16 12:13:42 -07:00 committed by GitHub
parent 1f0cd54fd7
commit 7f76929cd5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
24 changed files with 104 additions and 106 deletions

View File

@ -51,7 +51,7 @@ public class AwsSdkClientDecorator extends HttpClientDecorator<Request, Response
@Override
public Span onRequest(final Span span, final Request request) {
// Call super first because we override the resource name below.
// Call super first because we override the span name below.
super.onRequest(span, request);
final String awsServiceName = request.getServiceName();

View File

@ -48,7 +48,7 @@ class CouchbaseSpanUtil {
"local.address" { it == null || String }
// Not all couchbase operations have operation id. Notably, 'ViewQuery's do not
// We assign a resourceName of 'Bucket.query' and this is shared with n1ql queries
// We assign a spanName of 'Bucket.query' and this is shared with n1ql queries
// that do have operation ids
"couchbase.operation_id" { it == null || String }

View File

@ -29,9 +29,9 @@ public class HystrixDecorator extends BaseDecorator {
final String groupName = command.getCommandGroup().name();
final boolean circuitOpen = command.isCircuitBreakerOpen();
final String resourceName = groupName + "." + commandName + "." + methodName;
final String spanName = groupName + "." + commandName + "." + methodName;
span.updateName(resourceName);
span.updateName(spanName);
span.setAttribute("hystrix.command", commandName);
span.setAttribute("hystrix.group", groupName);
span.setAttribute("hystrix.circuit-open", circuitOpen);

View File

@ -33,51 +33,51 @@ import javax.ws.rs.Path;
public class JaxRsAnnotationsDecorator extends BaseDecorator {
public static final JaxRsAnnotationsDecorator DECORATE = new JaxRsAnnotationsDecorator();
private final WeakMap<Class<?>, Map<Method, String>> resourceNames = newWeakMap();
private final WeakMap<Class<?>, Map<Method, String>> spanNames = newWeakMap();
public static final Tracer TRACER =
OpenTelemetry.getTracerProvider().get("io.opentelemetry.auto.jaxrs-1.0");
public void onControllerStart(
final Span span, final Span parent, final Class target, final Method method) {
final String resourceName = getPathResourceName(target, method);
updateParent(parent, resourceName);
final String spanName = getPathSpanName(target, method);
updateParent(parent, spanName);
// When jax-rs is the root, we want to name using the path, otherwise use the class/method.
final boolean isRootScope = !parent.getContext().isValid();
if (isRootScope && !resourceName.isEmpty()) {
span.updateName(resourceName);
if (isRootScope && !spanName.isEmpty()) {
span.updateName(spanName);
} else {
span.updateName(DECORATE.spanNameForClass(target) + "." + method.getName());
}
}
private void updateParent(final Span span, final String resourceName) {
private void updateParent(final Span span, final String spanName) {
if (span == null) {
return;
}
if (!resourceName.isEmpty()) {
span.updateName(resourceName);
if (!spanName.isEmpty()) {
span.updateName(spanName);
}
}
/**
* Returns the resource name given a JaxRS annotated method. Results are cached so this method can
* be called multiple times without significantly impacting performance.
* Returns the span name given a JaxRS annotated method. Results are cached so this method can be
* called multiple times without significantly impacting performance.
*
* @return The result can be an empty string but will never be {@code null}.
*/
private String getPathResourceName(final Class<?> target, final Method method) {
Map<Method, String> classMap = resourceNames.get(target);
private String getPathSpanName(final Class<?> target, final Method method) {
Map<Method, String> classMap = spanNames.get(target);
if (classMap == null) {
resourceNames.putIfAbsent(target, new ConcurrentHashMap<Method, String>());
classMap = resourceNames.get(target);
spanNames.putIfAbsent(target, new ConcurrentHashMap<Method, String>());
classMap = spanNames.get(target);
// classMap should not be null at this point because we have a
// strong reference to target and don't manually clear the map.
}
String resourceName = classMap.get(method);
if (resourceName == null) {
String spanName = classMap.get(method);
if (spanName == null) {
String httpMethod = null;
Path methodPath = null;
final Path classPath = findClassPath(target);
@ -102,11 +102,11 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
}
}
}
resourceName = buildResourceName(httpMethod, classPath, methodPath);
classMap.put(method, resourceName);
spanName = buildSpanName(httpMethod, classPath, methodPath);
classMap.put(method, spanName);
}
return resourceName;
return spanName;
}
private String locateHttpMethod(final Method method) {
@ -161,20 +161,20 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
return null;
}
private String buildResourceName(
private String buildSpanName(
final String httpMethod, final Path classPath, final Path methodPath) {
final String resourceName;
final StringBuilder resourceNameBuilder = new StringBuilder();
final String spanName;
final StringBuilder spanNameBuilder = new StringBuilder();
if (httpMethod != null) {
resourceNameBuilder.append(httpMethod);
resourceNameBuilder.append(" ");
spanNameBuilder.append(httpMethod);
spanNameBuilder.append(" ");
}
boolean skipSlash = false;
if (classPath != null) {
if (!classPath.value().startsWith("/")) {
resourceNameBuilder.append("/");
spanNameBuilder.append("/");
}
resourceNameBuilder.append(classPath.value());
spanNameBuilder.append(classPath.value());
skipSlash = classPath.value().endsWith("/");
}
@ -185,12 +185,12 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
path = path.length() == 1 ? "" : path.substring(1);
}
} else if (!path.startsWith("/")) {
resourceNameBuilder.append("/");
spanNameBuilder.append("/");
}
resourceNameBuilder.append(path);
spanNameBuilder.append(path);
}
resourceName = resourceNameBuilder.toString().trim();
return resourceName;
spanName = spanNameBuilder.toString().trim();
return spanName;
}
}

View File

@ -53,7 +53,7 @@ class JaxRsAnnotations1InstrumentationTest extends AgentTestRunner {
def "span named '#name' from annotations on class when is not root span"() {
setup:
def startingCacheSize = resourceNames.size()
def startingCacheSize = spanNames.size()
runUnderTrace("test") {
obj.call()
}
@ -75,8 +75,8 @@ class JaxRsAnnotations1InstrumentationTest extends AgentTestRunner {
}
}
}
resourceNames.size() == startingCacheSize + 1
resourceNames.get(obj.class).size() == 1
spanNames.size() == startingCacheSize + 1
spanNames.get(obj.class).size() == 1
when: "multiple calls to the same method"
runUnderTrace("test") {
@ -85,8 +85,8 @@ class JaxRsAnnotations1InstrumentationTest extends AgentTestRunner {
}
}
then: "doesn't increase the cache size"
resourceNames.size() == startingCacheSize + 1
resourceNames.get(obj.class).size() == 1
spanNames.size() == startingCacheSize + 1
spanNames.get(obj.class).size() == 1
where:
name | obj
@ -143,7 +143,7 @@ class JaxRsAnnotations1InstrumentationTest extends AgentTestRunner {
// JavaInterfaces classes are loaded on a different classloader, so we need to find the right cache instance.
decorator = obj.class.classLoader.loadClass(JaxRsAnnotationsDecorator.name).getField("DECORATE").get(null)
resourceNames = (WeakMap<Class, Map<Method, String>>) decorator.resourceNames
spanNames = (WeakMap<Class, Map<Method, String>>) decorator.spanNames
}
def "no annotations has no effect"() {

View File

@ -43,7 +43,7 @@ class JerseyTest extends AgentTestRunner {
assertTraces(1) {
trace(0, 2) {
span(0) {
operationName expectedResourceName
operationName expectedSpanName
tags {
}
}
@ -58,7 +58,7 @@ class JerseyTest extends AgentTestRunner {
}
where:
resource | expectedResourceName | controllerName | expectedResponse
resource | expectedSpanName | controllerName | expectedResponse
"/test/hello/bob" | "POST /test/hello/{name}" | "Test1.hello" | "Test1 bob!"
"/test2/hello/bob" | "POST /test2/hello/{name}" | "Test2.hello" | "Test2 bob!"
"/test3/hi/bob" | "POST /test3/hi/{name}" | "Test3.hello" | "Test3 bob!"

View File

@ -43,52 +43,51 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
public static final Tracer TRACER =
OpenTelemetry.getTracerProvider().get("io.opentelemetry.auto.jaxrs-2.0");
private final WeakMap<Class<?>, Map<Method, String>> resourceNames =
WeakMap.Provider.newWeakMap();
private final WeakMap<Class<?>, Map<Method, String>> spanNames = WeakMap.Provider.newWeakMap();
public void onJaxRsSpan(
final Span span, final Span parent, final Class target, final Method method) {
final String resourceName = getPathResourceName(target, method);
updateParent(parent, resourceName);
final String spanName = getPathSpanName(target, method);
updateParent(parent, spanName);
// When jax-rs is the root, we want to name using the path, otherwise use the class/method.
final boolean isRootScope = !parent.getContext().isValid();
if (isRootScope && !resourceName.isEmpty()) {
span.updateName(resourceName);
if (isRootScope && !spanName.isEmpty()) {
span.updateName(spanName);
} else {
span.updateName(
DECORATE.spanNameForClass(target) + (method == null ? "" : "." + method.getName()));
}
}
private void updateParent(final Span span, final String resourceName) {
private void updateParent(final Span span, final String spanName) {
if (span == null) {
return;
}
if (!resourceName.isEmpty()) {
span.updateName(resourceName);
if (!spanName.isEmpty()) {
span.updateName(spanName);
}
}
/**
* Returns the resource name given a JaxRS annotated method. Results are cached so this method can
* be called multiple times without significantly impacting performance.
* Returns the span name given a JaxRS annotated method. Results are cached so this method can be
* called multiple times without significantly impacting performance.
*
* @return The result can be an empty string but will never be {@code null}.
*/
private String getPathResourceName(final Class<?> target, final Method method) {
Map<Method, String> classMap = resourceNames.get(target);
private String getPathSpanName(final Class<?> target, final Method method) {
Map<Method, String> classMap = spanNames.get(target);
if (classMap == null) {
resourceNames.putIfAbsent(target, new ConcurrentHashMap<Method, String>());
classMap = resourceNames.get(target);
spanNames.putIfAbsent(target, new ConcurrentHashMap<Method, String>());
classMap = spanNames.get(target);
// classMap should not be null at this point because we have a
// strong reference to target and don't manually clear the map.
}
String resourceName = classMap.get(method);
if (resourceName == null) {
String spanName = classMap.get(method);
if (spanName == null) {
String httpMethod = null;
Path methodPath = null;
final Path classPath = findClassPath(target);
@ -113,11 +112,11 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
}
}
}
resourceName = buildResourceName(httpMethod, classPath, methodPath);
classMap.put(method, resourceName);
spanName = buildSpanName(httpMethod, classPath, methodPath);
classMap.put(method, spanName);
}
return resourceName;
return spanName;
}
private String locateHttpMethod(final Method method) {
@ -172,20 +171,20 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
return null;
}
private String buildResourceName(
private String buildSpanName(
final String httpMethod, final Path classPath, final Path methodPath) {
final String resourceName;
final StringBuilder resourceNameBuilder = new StringBuilder();
final String spanName;
final StringBuilder spanNameBuilder = new StringBuilder();
if (httpMethod != null) {
resourceNameBuilder.append(httpMethod);
resourceNameBuilder.append(" ");
spanNameBuilder.append(httpMethod);
spanNameBuilder.append(" ");
}
boolean skipSlash = false;
if (classPath != null) {
if (!classPath.value().startsWith("/")) {
resourceNameBuilder.append("/");
spanNameBuilder.append("/");
}
resourceNameBuilder.append(classPath.value());
spanNameBuilder.append(classPath.value());
skipSlash = classPath.value().endsWith("/");
}
@ -196,12 +195,12 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
path = path.length() == 1 ? "" : path.substring(1);
}
} else if (!path.startsWith("/")) {
resourceNameBuilder.append("/");
spanNameBuilder.append("/");
}
resourceNameBuilder.append(path);
spanNameBuilder.append(path);
}
resourceName = resourceNameBuilder.toString().trim();
return resourceName;
spanName = spanNameBuilder.toString().trim();
return spanName;
}
}

View File

@ -53,7 +53,7 @@ class JaxRsAnnotations2InstrumentationTest extends AgentTestRunner {
def "span named '#name' from annotations on class when is not root span"() {
setup:
def startingCacheSize = resourceNames.size()
def startingCacheSize = spanNames.size()
runUnderTrace("test") {
obj.call()
}
@ -75,8 +75,8 @@ class JaxRsAnnotations2InstrumentationTest extends AgentTestRunner {
}
}
}
resourceNames.size() == startingCacheSize + 1
resourceNames.get(obj.class).size() == 1
spanNames.size() == startingCacheSize + 1
spanNames.get(obj.class).size() == 1
when: "multiple calls to the same method"
runUnderTrace("test") {
@ -85,8 +85,8 @@ class JaxRsAnnotations2InstrumentationTest extends AgentTestRunner {
}
}
then: "doesn't increase the cache size"
resourceNames.size() == startingCacheSize + 1
resourceNames.get(obj.class).size() == 1
spanNames.size() == startingCacheSize + 1
spanNames.get(obj.class).size() == 1
where:
name | obj
@ -143,7 +143,7 @@ class JaxRsAnnotations2InstrumentationTest extends AgentTestRunner {
// JavaInterfaces classes are loaded on a different classloader, so we need to find the right cache instance.
decorator = obj.class.classLoader.loadClass(JaxRsAnnotationsDecorator.name).getField("DECORATE").get(null)
resourceNames = (WeakMap<Class, Map<Method, String>>) decorator.resourceNames
spanNames = (WeakMap<Class, Map<Method, String>>) decorator.spanNames
}
def "no annotations has no effect"() {

View File

@ -71,7 +71,7 @@ abstract class JaxRsFilterTest extends AgentTestRunner {
assertTraces(1) {
trace(0, 2) {
span(0) {
operationName parentResourceName != null ? parentResourceName : "test.span"
operationName parentSpanName != null ? parentSpanName : "test.span"
tags {
}
}
@ -85,7 +85,7 @@ abstract class JaxRsFilterTest extends AgentTestRunner {
}
where:
resource | abortNormal | abortPrematch | parentResourceName | controllerName | expectedResponse
resource | abortNormal | abortPrematch | parentSpanName | controllerName | expectedResponse
"/test/hello/bob" | false | false | "POST /test/hello/{name}" | "Test1.hello" | "Test1 bob!"
"/test2/hello/bob" | false | false | "POST /test2/hello/{name}" | "Test2.hello" | "Test2 bob!"
"/test3/hi/bob" | false | false | "POST /test3/hi/{name}" | "Test3.hello" | "Test3 bob!"

View File

@ -90,7 +90,7 @@ class SpringTemplateJMS2Test extends AgentTestRunner {
server.stop()
}
def "sending a message to #jmsResourceName generates spans"() {
def "sending a message to #expectedSpanName generates spans"() {
setup:
template.convertAndSend(destination, messageText)
TextMessage receivedMessage = template.receive(destination)

View File

@ -33,7 +33,7 @@ public class JMSDecorator extends ClientDecorator {
OpenTelemetry.getTracerProvider().get("io.opentelemetry.auto.jms-1.1");
public String spanNameForReceive(final Message message) {
return toResourceName(message, null);
return toSpanName(message, null);
}
public String spanNameForReceive(final Method method) {
@ -41,16 +41,16 @@ public class JMSDecorator extends ClientDecorator {
}
public String spanNameForConsumer(final Message message) {
return toResourceName(message, null);
return toSpanName(message, null);
}
public String spanNameForProducer(final Message message, final Destination destination) {
return toResourceName(message, destination);
return toSpanName(message, destination);
}
private static final String TIBCO_TMP_PREFIX = "$TMP$";
public static String toResourceName(final Message message, final Destination destination) {
public static String toSpanName(final Message message, final Destination destination) {
Destination jmsDestination = null;
try {
jmsDestination = message.getJMSDestination();

View File

@ -58,7 +58,7 @@ class SpringTemplateJMS1Test extends AgentTestRunner {
broker.stop()
}
def "sending a message to #jmsResourceName generates spans"() {
def "sending a message to #expectedSpanName generates spans"() {
setup:
template.convertAndSend(destination, messageText)
TextMessage receivedMessage = template.receive(destination)

View File

@ -59,11 +59,11 @@ public class JSPDecorator extends BaseDecorator {
public String spanNameOnRender(final HttpServletRequest req) {
// get the JSP file name being rendered in an include action
final Object includeServletPath = req.getAttribute(RequestDispatcher.INCLUDE_SERVLET_PATH);
String resourceName = req.getServletPath();
String spanName = req.getServletPath();
if (includeServletPath instanceof String) {
resourceName = includeServletPath.toString();
spanName = includeServletPath.toString();
}
return "Render " + resourceName;
return "Render " + spanName;
}
public void onRender(final Span span, final HttpServletRequest req) {

View File

@ -564,8 +564,8 @@ class JSPInstrumentationBasicTests extends AgentTestRunner {
// serviceName jspWebappContext
operationName expectedOperationName("GET")
spanKind SERVER
// FIXME: this is not a great resource name for serving static content.
// resourceName "GET /$jspWebappContext/$staticFile"
// FIXME: this is not a great span name for serving static content.
// spanName "GET /$jspWebappContext/$staticFile"
errored false
tags {
"$MoreTags.NET_PEER_IP" "127.0.0.1"

View File

@ -76,7 +76,7 @@ public class PlayAdvice {
playControllerScope.closeScope();
final Span rootSpan = TRACER.getCurrentSpan();
// set the resource name on the upstream akka/netty span
// set the span name on the upstream akka/netty span
DECORATE.onRequest(rootSpan, req);
}
}

View File

@ -77,10 +77,9 @@ class PlayWSClientTest extends HttpClientTest {
@Override
boolean testRemoteConnection() {
// On connection failures the operation and resource names end up different from expected.
// On connection failures the operation and span names end up different from expected.
// This would require a lot of changes to the base client test class to support
// span.operationName = "netty.connect"
// span.resourceName = "netty.connect"
// span.spanName = "netty.connect"
false
}
}

View File

@ -74,7 +74,7 @@ public class PlayAdvice {
playControllerScope.closeScope();
final Span rootSpan = TRACER.getCurrentSpan();
// set the resource name on the upstream akka/netty span
// set the span name on the upstream akka/netty span
DECORATE.onRequest(rootSpan, req);
}

View File

@ -74,7 +74,7 @@ public class PlayAdvice {
playControllerScope.closeScope();
final Span rootSpan = TRACER.getCurrentSpan();
// set the resource name on the upstream akka/netty span
// set the span name on the upstream akka/netty span
DECORATE.onRequest(rootSpan, req);
}
}

View File

@ -173,7 +173,7 @@ public class RabbitChannelInstrumentation extends Instrumenter.Default {
public static class ChannelPublishAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static void setResourceNameAddHeaders(
public static void setSpanNameAddHeaders(
@Advice.Argument(0) final String exchange,
@Advice.Argument(1) final String routingKey,
@Advice.Argument(value = 4, readOnly = false) AMQP.BasicProperties props,

View File

@ -75,7 +75,7 @@ public class RabbitCommandInstrumentation extends Instrumenter.Default {
public static class CommandConstructorAdvice {
@Advice.OnMethodExit
public static void setResourceNameAddHeaders(@Advice.This final Command command) {
public static void setSpanNameAddHeaders(@Advice.This final Command command) {
final Span span = CURRENT_RABBIT_SPAN.get();
if (span != null && command.getMethod() != null) {

View File

@ -59,7 +59,7 @@ public final class TracingHandler implements Handler {
response -> {
try (final Scope ignored = currentContextWith(ratpackSpan)) {
if (nettySpan != null) {
// Rename the netty span resource name with the ratpack route.
// Rename the netty span name with the ratpack route.
DECORATE.onContext(nettySpan, ctx);
}
DECORATE.onResponse(ratpackSpan, response);

View File

@ -61,7 +61,7 @@ class HttpServletTest extends AgentTestRunner {
}
}
span(2) {
operationName "${expectedResourceName}.doGet"
operationName "${expectedSpanName}.doGet"
childOf span(1)
tags {
}
@ -76,7 +76,7 @@ class HttpServletTest extends AgentTestRunner {
}
}]
expectedResourceName = servlet.class.anonymousClass ? servlet.class.name : servlet.class.simpleName
expectedSpanName = servlet.class.anonymousClass ? servlet.class.name : servlet.class.simpleName
}
def "test service exception"() {

View File

@ -40,7 +40,7 @@ public class DispatcherHandlerAdvice {
}
// Unfortunately Netty EventLoop is not instrumented well enough to attribute all work to the
// right things so we have to store span in request itself. We also store parent (netty's) span
// so we could update resource name.
// so we could update span name.
exchange.getAttributes().put(AdviceUtils.PARENT_SPAN_ATTRIBUTE, parentSpan);
final Span span = TRACER.spanBuilder("DispatcherHandler.handle").startSpan();

View File

@ -59,8 +59,8 @@ public class HandlerAdapterAdvice {
final PathPattern bestPattern =
exchange.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE);
if (parentSpan != null && bestPattern != null) {
final String resourceName = bestPattern.getPatternString();
parentSpan.updateName(resourceName);
final String spanName = bestPattern.getPatternString();
parentSpan.updateName(spanName);
}
return spanWithScope;