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 http.route to the server span when ServerSpanNaming is updated #5086

Merged
merged 4 commits into from
Jan 14, 2022
Merged

Add http.route to the server span when ServerSpanNaming is updated #5086

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cf13340
Add `http.route` to the server span when `ServerSpanNaming` is updated
Jan 12, 2022
ef505c7
fix camel tests
Jan 12, 2022
6e50c45
fix test compilation failure
Jan 12, 2022
81edcfc
assert route in camel instrumentation
Jan 13, 2022
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
34 changes: 26 additions & 8 deletions ...ation-api/src/main/java/io/opentelemetry/instrumentation/api/server/ServerSpanNaming.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@
import io.opentelemetry.context.Context;
import io.opentelemetry.context.ContextKey;
import io.opentelemetry.instrumentation.api.instrumenter.ContextCustomizer;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
import javax.annotation.Nullable;

// TODO: move to ...instrumenter.http and rename to HttpRouteHolder (?)
/** Helper container for tracking whether instrumentation should update server span name or not. */
public final class ServerSpanNaming {

private static final ContextKey<ServerSpanNaming> CONTEXT_KEY =
ContextKey.named("opentelemetry-servlet-span-naming-key");
ContextKey.named("opentelemetry-http-server-route-key");

public static <REQUEST> ContextCustomizer<REQUEST> get() {
return (context, request, startAttributes) -> {
Expand All @@ -27,9 +29,7 @@ public static <REQUEST> ContextCustomizer<REQUEST> get() {
}

private volatile Source updatedBySource;
// Length of the currently set name. This is used when setting name from a servlet filter
// to pick the most descriptive (longest) name.
private volatile int nameLength;
@Nullable private volatile String route;

private ServerSpanNaming(Source initialSource) {
this.updatedBySource = initialSource;
Expand Down Expand Up @@ -78,7 +78,7 @@ public static <T, U> void updateServerSpanName(
if (serverSpanNaming == null) {
String name = serverSpanName.get(context, arg1, arg2);
if (name != null && !name.isEmpty()) {
serverSpan.updateName(name);
updateSpanData(serverSpan, name);
}
return;
}
Expand All @@ -91,15 +91,33 @@ public static <T, U> void updateServerSpanName(
if (name != null
&& !name.isEmpty()
&& (!onlyIfBetterName || serverSpanNaming.isBetterName(name))) {
serverSpan.updateName(name);
updateSpanData(serverSpan, name);
serverSpanNaming.updatedBySource = source;
serverSpanNaming.nameLength = name.length();
serverSpanNaming.route = name;
}
}
}

// TODO: instead of calling setAttribute() consider storing the route in context end retrieving it
// in the AttributesExtractor
private static void updateSpanData(Span serverSpan, String route) {
serverSpan.updateName(route);
serverSpan.setAttribute(SemanticAttributes.HTTP_ROUTE, route);
}

// This is used when setting name from a servlet filter to pick the most descriptive (longest)
// route.
private boolean isBetterName(String name) {
return name.length() > nameLength;
String route = this.route;
int routeLength = route == null ? 0 : route.length();
return name.length() > routeLength;
}

// TODO: use that in HttpServerMetrics
@Nullable
public static String getRoute(Context context) {
ServerSpanNaming serverSpanNaming = context.get(CONTEXT_KEY);
return serverSpanNaming == null ? null : serverSpanNaming.route;
}

public enum Source {
Expand Down
9 changes: 9 additions & 0 deletions ...on/akka/akka-http-10.0/javaagent/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
* SPDX-License-Identifier: Apache-2.0
*/

import io.opentelemetry.api.common.AttributeKey
import io.opentelemetry.instrumentation.test.AgentTestTrait
import io.opentelemetry.instrumentation.test.base.HttpServerTest
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes

abstract class AkkaHttpServerInstrumentationTest extends HttpServerTest<Object> implements AgentTestTrait {

Expand All @@ -28,6 +30,13 @@ abstract class AkkaHttpServerInstrumentationTest extends HttpServerTest<Object>
boolean testCapturedHttpHeaders() {
false
}

@Override
Set<AttributeKey<?>> httpAttributes(ServerEndpoint endpoint) {
def attributes = super.httpAttributes(endpoint)
attributes.remove(SemanticAttributes.HTTP_ROUTE)
attributes
}
}

class AkkaHttpServerInstrumentationTestSync extends AkkaHttpServerInstrumentationTest {
Expand Down
4 changes: 2 additions & 2 deletions ...c/test/groovy/io/opentelemetry/javaagent/instrumentation/apachecamel/RestCamelTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

package io.opentelemetry.javaagent.instrumentation.apachecamel

import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP

import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification
import io.opentelemetry.instrumentation.test.RetryOnAddressAlreadyInUseTrait
import io.opentelemetry.instrumentation.test.utils.PortUtils
Expand All @@ -20,6 +18,7 @@ import spock.lang.Shared
import static io.opentelemetry.api.trace.SpanKind.CLIENT
import static io.opentelemetry.api.trace.SpanKind.INTERNAL
import static io.opentelemetry.api.trace.SpanKind.SERVER
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP

class RestCamelTest extends AgentInstrumentationSpecification implements RetryOnAddressAlreadyInUseTrait {

Expand Down Expand Up @@ -100,6 +99,7 @@ class RestCamelTest extends AgentInstrumentationSpecification implements RetryOn
"$SemanticAttributes.NET_PEER_PORT" Long
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_ROUTE" String
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we assert the value for route?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added. Turns out camel isn't using ServerSpanNaming to update the server span name and the route is incorrect -- another TODO added to my list.

}
}
it.span(3) {
Expand Down
4 changes: 2 additions & 2 deletions ...lemetry/javaagent/instrumentation/apachecamel/TwoServicesWithDirectClientCamelTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

package io.opentelemetry.javaagent.instrumentation.apachecamel

import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP

import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification
import io.opentelemetry.instrumentation.test.RetryOnAddressAlreadyInUseTrait
import io.opentelemetry.instrumentation.test.utils.PortUtils
Expand All @@ -22,6 +20,7 @@ import spock.lang.Shared
import static io.opentelemetry.api.trace.SpanKind.CLIENT
import static io.opentelemetry.api.trace.SpanKind.INTERNAL
import static io.opentelemetry.api.trace.SpanKind.SERVER
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP

class TwoServicesWithDirectClientCamelTest extends AgentInstrumentationSpecification implements RetryOnAddressAlreadyInUseTrait {

Expand Down Expand Up @@ -136,6 +135,7 @@ class TwoServicesWithDirectClientCamelTest extends AgentInstrumentationSpecifica
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH" Long
"$SemanticAttributes.HTTP_ROUTE" String
}
}
it.span(5) {
Expand Down
14 changes: 13 additions & 1 deletion ...groovy/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpServerTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEn
import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR
import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.INDEXED_CHILD
import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND
import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM
import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM
import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT
Expand All @@ -38,12 +39,23 @@ abstract class AbstractArmeriaHttpServerTest extends HttpServerTest<Server> {

abstract ServerBuilder configureServer(ServerBuilder serverBuilder)

@Override
String expectedHttpRoute(ServerEndpoint endpoint) {
switch (endpoint) {
case NOT_FOUND:
// TODO(anuraaga): Revisit this when applying instrumenters to more libraries, Armeria
// currently reports '/*' which is a fallback route.
return "/*"
default:
return super.expectedHttpRoute(endpoint)
}
}

@Override
List<AttributeKey<?>> extraAttributes() {
[
SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH,
SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH,
SemanticAttributes.HTTP_ROUTE,
SemanticAttributes.HTTP_SERVER_NAME,
SemanticAttributes.NET_PEER_NAME,
SemanticAttributes.NET_TRANSPORT
Expand Down
10 changes: 5 additions & 5 deletions instrumentation/dropwizard/dropwizard-testing/src/test/groovy/DropwizardTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,14 @@ class DropwizardTest extends HttpServerTest<DropwizardTestSupport> implements Ag
}

@Override
String expectedServerSpanName(ServerEndpoint endpoint) {
String expectedHttpRoute(ServerEndpoint endpoint) {
switch (endpoint) {
case PATH_PARAM:
return "/path/{id}/param"
case NOT_FOUND:
return "/*"
return getContextPath() + "/*"
case PATH_PARAM:
return getContextPath() + "/path/{id}/param"
default:
return endpoint.resolvePath(address).path
return super.expectedHttpRoute(endpoint)
}
}

Expand Down
2 changes: 1 addition & 1 deletion instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ class GrailsTest extends HttpServerTest<ConfigurableApplicationContext> implemen
}

@Override
String expectedServerSpanName(ServerEndpoint endpoint) {
String expectedHttpRoute(ServerEndpoint endpoint) {
if (endpoint == PATH_PARAM) {
return getContextPath() + "/test/path"
} else if (endpoint == QUERY_PARAM) {
Expand Down
21 changes: 15 additions & 6 deletions instrumentation/grizzly-2.0/javaagent/src/test/groovy/GrizzlyFilterchainServerTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
* SPDX-License-Identifier: Apache-2.0
*/

import io.opentelemetry.api.common.AttributeKey
import io.opentelemetry.instrumentation.test.AgentTestTrait
import io.opentelemetry.instrumentation.test.base.HttpServerTest
import java.nio.charset.StandardCharsets
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
import org.glassfish.grizzly.filterchain.BaseFilter
import org.glassfish.grizzly.filterchain.FilterChain
import org.glassfish.grizzly.filterchain.FilterChainBuilder
Expand All @@ -25,6 +26,7 @@ import org.glassfish.grizzly.nio.transport.TCPNIOTransportBuilder
import org.glassfish.grizzly.utils.DelayedExecutor
import org.glassfish.grizzly.utils.IdleTimeoutFilter

import java.nio.charset.StandardCharsets
import java.util.concurrent.Executors

import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.AUTH_REQUIRED
Expand Down Expand Up @@ -61,6 +63,18 @@ class GrizzlyFilterchainServerTest extends HttpServerTest<HttpServer> implements
transport.shutdownNow()
}

@Override
Set<AttributeKey<?>> httpAttributes(ServerEndpoint endpoint) {
def attributes = super.httpAttributes(endpoint)
attributes.remove(SemanticAttributes.HTTP_ROUTE)
attributes
}

@Override
String expectedServerSpanName(ServerEndpoint endpoint) {
return "HTTP GET"
}

@Override
boolean testException() {
// justification: grizzly async closes the channel which
Expand Down Expand Up @@ -230,9 +244,4 @@ class GrizzlyFilterchainServerTest extends HttpServerTest<HttpServer> implements
}
}
}

@Override
String expectedServerSpanName(ServerEndpoint endpoint) {
return "HTTP GET"
}
}
19 changes: 14 additions & 5 deletions instrumentation/grizzly-2.0/javaagent/src/test/groovy/GrizzlyTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
* SPDX-License-Identifier: Apache-2.0
*/

import io.opentelemetry.api.common.AttributeKey
import io.opentelemetry.instrumentation.test.AgentTestTrait
import io.opentelemetry.instrumentation.test.base.HttpServerTest
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
import org.glassfish.grizzly.http.server.HttpHandler
import org.glassfish.grizzly.http.server.HttpServer
import org.glassfish.grizzly.http.server.Request
Expand Down Expand Up @@ -42,6 +44,18 @@ class GrizzlyTest extends HttpServerTest<HttpServer> implements AgentTestTrait {
return server
}

@Override
Set<AttributeKey<?>> httpAttributes(ServerEndpoint endpoint) {
def attributes = super.httpAttributes(endpoint)
attributes.remove(SemanticAttributes.HTTP_ROUTE)
attributes
}

@Override
String expectedServerSpanName(ServerEndpoint endpoint) {
return "HTTP GET"
}

@Override
void stopServer(HttpServer server) {
server.stop()
Expand Down Expand Up @@ -108,11 +122,6 @@ class GrizzlyTest extends HttpServerTest<HttpServer> implements AgentTestTrait {
}
}

@Override
String expectedServerSpanName(ServerEndpoint endpoint) {
return "HTTP GET"
}

static class ExceptionHttpHandler extends HttpHandler {

@Override
Expand Down
1 change: 1 addition & 0 deletions ...ion/jaxrs/jaxrs-1.0/javaagent/src/test/groovy/JaxRsAnnotations1InstrumentationTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class JaxRsAnnotations1InstrumentationTest extends AgentInstrumentationSpecifica
kind SERVER
hasNoParent()
attributes {
"$SemanticAttributes.HTTP_ROUTE" paramName
}
}
span(1) {
Expand Down
12 changes: 7 additions & 5 deletions instrumentation/jaxrs/jaxrs-1.0/javaagent/src/test/groovy/JerseyTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,10 @@ class JerseyTest extends AgentInstrumentationSpecification {
assertTraces(1) {
trace(0, 2) {
span(0) {
name expectedSpanName
name expectedRoute
kind SERVER
attributes {
"$SemanticAttributes.HTTP_ROUTE" expectedRoute
}
}

Expand All @@ -56,7 +57,7 @@ class JerseyTest extends AgentInstrumentationSpecification {
}

where:
resource | expectedSpanName | controllerName | expectedResponse
resource | expectedRoute | controllerName | expectedResponse
"/test/hello/bob" | "/test/hello/{name}" | "Test1.hello" | "Test1 bob!"
"/test2/hello/bob" | "/test2/hello/{name}" | "Test2.hello" | "Test2 bob!"
"/test3/hi/bob" | "/test3/hi/{name}" | "Test3.hello" | "Test3 bob!"
Expand All @@ -76,9 +77,10 @@ class JerseyTest extends AgentInstrumentationSpecification {
assertTraces(1) {
trace(0, 2) {
span(0) {
name expectedSpanName
name expectedRoute
kind SERVER
attributes {
"$SemanticAttributes.HTTP_ROUTE" expectedRoute
}
}
span(1) {
Expand All @@ -94,7 +96,7 @@ class JerseyTest extends AgentInstrumentationSpecification {
}

where:
resource | expectedSpanName | controller1Name | expectedResponse
"/test3/nested" | "/test3/nested" | "Test3.nested" | "Test3 nested!"
resource | expectedRoute | controller1Name | expectedResponse
"/test3/nested" | "/test3/nested" | "Test3.nested" | "Test3 nested!"
}
}
1 change: 1 addition & 0 deletions ...mentation/jaxrs/jaxrs-2.0/jaxrs-2.0-common/testing/src/main/groovy/JaxRsFilterTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ abstract class JaxRsFilterTest extends AgentInstrumentationSpecification {
kind SERVER
if (!runsOnServer()) {
attributes {
"$SemanticAttributes.HTTP_ROUTE" parentResourceName
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions ...ation/jaxrs/jaxrs-2.0/jaxrs-2.0-common/testing/src/main/groovy/JaxRsHttpServerTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
* SPDX-License-Identifier: Apache-2.0
*/

import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP

import io.opentelemetry.api.common.AttributeKey
import io.opentelemetry.instrumentation.test.AgentTestTrait
import io.opentelemetry.instrumentation.test.asserts.TraceAssert
Expand All @@ -20,6 +18,7 @@ import static io.opentelemetry.api.trace.StatusCode.ERROR
import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM
import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS
import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP
import static java.util.concurrent.TimeUnit.SECONDS
import static org.junit.jupiter.api.Assumptions.assumeTrue

Expand Down Expand Up @@ -287,6 +286,7 @@ abstract class JaxRsHttpServerTest<S> extends HttpServerTest<S> implements Agent
"$SemanticAttributes.HTTP_SERVER_NAME" String
"$SemanticAttributes.NET_TRANSPORT" IP_TCP
"$SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH" { it == null || it instanceof Long } // Optional
"$SemanticAttributes.HTTP_ROUTE" path
if (fullUrl.getPath().endsWith(ServerEndpoint.CAPTURE_HEADERS.getPath())) {
"http.request.header.x_test_request" { it == ["test"] }
"http.response.header.x_test_response" { it == ["test"] }
Expand Down
1 change: 1 addition & 0 deletions ...s-2.0/jaxrs-2.0-common/testing/src/main/groovy/JaxrsAnnotationsInstrumentationTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ abstract class JaxrsAnnotationsInstrumentationTest extends AgentInstrumentationS
kind SERVER
hasNoParent()
attributes {
"$SemanticAttributes.HTTP_ROUTE" paramName
}
}
span(1) {
Expand Down
Loading