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

Add code attributes to play mvc controller spans #13140

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add code attributes to play mvc controller spans #13140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 23 additions & 0 deletions ...java/io/opentelemetry/javaagent/instrumentation/play/v2_4/ActionCodeAttributesGetter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.instrumentation.play.v2_4;

import io.opentelemetry.instrumentation.api.incubator.semconv.code.CodeAttributesGetter;
import javax.annotation.Nullable;

public class ActionCodeAttributesGetter implements CodeAttributesGetter<ActionData> {
@Nullable
@Override
public Class<?> getCodeClass(ActionData actionData) {
return actionData.codeClass();
}

@Nullable
@Override
public String getMethodName(ActionData actionData) {
return actionData.methodName();
}
}
26 changes: 26 additions & 0 deletions ...aagent/src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/ActionData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.instrumentation.play.v2_4;

import java.lang.reflect.Method;

public class ActionData {
private final Class<?> target;
private final Method method;

public ActionData(Class<?> target, Method method) {
this.target = target;
this.method = method;
}

public Class<?> codeClass() {
return target;
}

public String methodName() {
return method.getName();
}
}
33 changes: 15 additions & 18 deletions ...main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/ActionInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,15 @@
import static io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge.currentContext;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface;
import static io.opentelemetry.javaagent.instrumentation.play.v2_4.Play24Singletons.instrumenter;
import static io.opentelemetry.javaagent.instrumentation.play.v2_4.Play24Singletons.updateSpan;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.returns;
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;

import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import java.lang.reflect.Method;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;
Expand Down Expand Up @@ -50,17 +49,14 @@ public void transform(TypeTransformer transformer) {
public static class ApplyAdvice {

@Advice.OnMethodEnter(suppress = Throwable.class)
public static void onEnter(
@Advice.Argument(0) Request<?> req,
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope) {
public static ActionScope onEnter(
@Advice.This Object target,
@Advice.Origin Method method,
@Advice.Argument(0) Request<?> req) {
Context parentContext = currentContext();
if (!instrumenter().shouldStart(parentContext, null)) {
return;
}

context = instrumenter().start(parentContext, null);
scope = context.makeCurrent();
ActionData actionData = new ActionData(target.getClass(), method);
return ActionScope.start(parentContext, actionData);
}

@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
Expand All @@ -69,20 +65,21 @@ public static void stopTraceOnResponse(
@Advice.Thrown Throwable throwable,
@Advice.Argument(0) Request<?> req,
@Advice.Return(readOnly = false) Future<Result> responseFuture,
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope) {
if (scope == null) {
@Advice.Enter ActionScope actionScope) {
if (actionScope == null || actionScope.getScope() == null) {
return;
}
scope.close();
actionScope.closeScope();

updateSpan(actionScope.getContext(), req);

updateSpan(context, req);
// span finished in RequestCompleteCallback
if (throwable == null) {
responseFuture.onComplete(
new RequestCompleteCallback(context), ((Action<?>) thisAction).executionContext());
new RequestCompleteCallback(actionScope.getContext()),
((Action<?>) thisAction).executionContext());
} else {
instrumenter().end(context, null, null, throwable);
actionScope.end(throwable);
}
}
}
Expand Down
53 changes: 53 additions & 0 deletions ...agent/src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/ActionScope.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.instrumentation.play.v2_4;

import static io.opentelemetry.javaagent.instrumentation.play.v2_4.Play24Singletons.instrumenter;

import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;

/** Container used to carry state between enter and exit advices */
public final class ActionScope {

private final ActionData actionData;
private final Context context;
private final Scope scope;

public ActionScope(Context context, Scope scope, ActionData actionData) {
this.actionData = actionData;
this.context = context;
this.scope = scope;
}

public Context getContext() {
return context;
}

public Scope getScope() {
return scope;
}

public static ActionScope start(Context parentContext, ActionData actionData) {
if (!instrumenter().shouldStart(parentContext, actionData)) {
return null;
}

Context context = instrumenter().start(parentContext, actionData);
return new ActionScope(context, context.makeCurrent(), actionData);
}

public void closeScope() {
if (scope != null) {
scope.close();
}
}

public void end(Throwable throwable) {
closeScope();
instrumenter().end(context, actionData, null, throwable);
}
}
20 changes: 13 additions & 7 deletions .../src/main/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/Play24Singletons.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import io.opentelemetry.api.GlobalOpenTelemetry;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.incubator.semconv.code.CodeAttributesExtractor;
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import io.opentelemetry.instrumentation.api.semconv.http.HttpServerRoute;
import io.opentelemetry.instrumentation.api.semconv.http.HttpServerRouteSource;
Expand All @@ -16,15 +17,20 @@
import scala.Option;

public final class Play24Singletons {

private static final String SPAN_NAME = "play.request";
private static final Instrumenter<Void, Void> INSTRUMENTER =
Instrumenter.<Void, Void>builder(
GlobalOpenTelemetry.get(), "io.opentelemetry.play-mvc-2.4", s -> SPAN_NAME)
.setEnabled(ExperimentalConfig.get().controllerTelemetryEnabled())
.buildInstrumenter();
private static final Instrumenter<ActionData, Void> INSTRUMENTER;

static {
INSTRUMENTER =
Instrumenter.<ActionData, Void>builder(
GlobalOpenTelemetry.get(), "io.opentelemetry.play-mvc-2.4", s -> SPAN_NAME)
.addAttributesExtractor(
CodeAttributesExtractor.create(new ActionCodeAttributesGetter()))
.setEnabled(ExperimentalConfig.get().controllerTelemetryEnabled())
.buildInstrumenter();
}

public static Instrumenter<Void, Void> instrumenter() {
public static Instrumenter<ActionData, Void> instrumenter() {
return INSTRUMENTER;
}

Expand Down
11 changes: 10 additions & 1 deletion ...test/java/io/opentelemetry/javaagent/instrumentation/play/v2_4/server/PlayServerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@
import static io.opentelemetry.instrumentation.testing.junit.http.ServerEndpoint.QUERY_PARAM;
import static io.opentelemetry.instrumentation.testing.junit.http.ServerEndpoint.REDIRECT;
import static io.opentelemetry.instrumentation.testing.junit.http.ServerEndpoint.SUCCESS;
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo;
import static io.opentelemetry.semconv.HttpAttributes.HTTP_ROUTE;
import static io.opentelemetry.semconv.incubating.CodeIncubatingAttributes.CODE_FUNCTION;
import static io.opentelemetry.semconv.incubating.CodeIncubatingAttributes.CODE_NAMESPACE;

import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
Expand Down Expand Up @@ -112,9 +115,15 @@ protected void configure(HttpServerTestOptions options) {
}

@Override
@SuppressWarnings("deprecation") // uses deprecated semconv
public SpanDataAssert assertHandlerSpan(
SpanDataAssert span, String method, ServerEndpoint endpoint) {
span.hasName("play.request").hasKind(INTERNAL);
span.hasName("play.request")
.hasKind(INTERNAL)
.hasAttributesSatisfyingExactly(
equalTo(CODE_NAMESPACE, "play.api.mvc.ActionBuilder$$anon$2"),
equalTo(CODE_FUNCTION, "apply"));
Comment on lines +124 to +125
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion this is not useful. It points to framework code and is the same for all endpoints.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree with Lauri here, if the value is the same for all spans then it does not provide much value for the end-user as with it they can't guess which part of the application code is related to the span.

We will very likely have exactly the same kind of issues with Webflux or other reactive frameworks where the application code is provided through nested lambdas and can't be described with a dedicated class/method. For those cases, the "span identity" should be provided through other attributes for example HTTP verb, path or span name which allows the end-user to understand the link with application code.

If we capture this technical and implementation-specific information, do we have a way to make any good use of it ?

One example I could think of is if we attempt to correlate those attributes with another source of information like inferred spans or profiling, however for those the correlation would be implemented through trace and span ID rather than on those code-level attributes.

I think this example is really interesting from the perspective of semconv attributes clarification as there are at least two distinct use-cases for those attributes:

  • provide more information about a span for the end-user: we may recommend to capture information that is related to the application code rather than framework/library details
  • capture "technical information" in a structured way that could not be described otherwise: profiling, inferred spans, ...


if (endpoint == EXCEPTION) {
span.hasStatus(StatusData.error());
span.hasException(new IllegalArgumentException(EXCEPTION.getBody()));
Expand Down