Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import com.google.errorprone.bugpatterns.PrivateConstructorForUtilityClass;
import com.google.errorprone.matchers.Description;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.Tree;

@AutoService(BugChecker.class)
@BugPattern(
Expand All @@ -31,7 +32,7 @@ public class OtelPrivateConstructorForUtilityClass extends BugChecker

@Override
public Description matchClass(ClassTree tree, VisitorState state) {
if (tree.getSimpleName().toString().endsWith("Advice")) {
if (isAdviceOrAdviceNestedClass(tree, state)) {
return NO_MATCH;
}
Description description = delegate.matchClass(tree, state);
Expand All @@ -40,4 +41,17 @@ public Description matchClass(ClassTree tree, VisitorState state) {
}
return describeMatch(tree);
}

private static boolean isAdviceOrAdviceNestedClass(ClassTree tree, VisitorState state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say this is probably not worth it. Instead of creating the nested VirtualFields class in advice you are better off encapsulating the virtual field access in a util class that internally uses the virtual field. For example in case of ArmeriaServerCallInstrumentation I should have really added an util class in https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/grpc-1.6/library/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/internal and done the virtual field access there. It is easier to search from ide where the util class is used instead of searching for something like VirtualField.find(ServerCall.class, String.class). If creating a new util class for the virtual fields feels like an overkill we could add the logic to the Singletons class. I think the separate VirtualFields class is fineish in the akka instrumentation for the task context propagation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, fixed in 0f1460d

if (tree.getSimpleName().toString().endsWith("Advice")) {
return true;
}
for (Tree parent : state.getPath()) {
if (parent instanceof ClassTree
&& ((ClassTree) parent).getSimpleName().toString().endsWith("Advice")) {
return true;
}
}
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@
import akka.dispatch.Envelope;
import akka.dispatch.sysmsg.SystemMessage;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.util.VirtualField;
import io.opentelemetry.javaagent.bootstrap.executors.PropagatedContext;
import io.opentelemetry.javaagent.bootstrap.executors.TaskAdviceHelper;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
Expand Down Expand Up @@ -42,9 +40,8 @@ public static class InvokeAdvice {

@Advice.OnMethodEnter(suppress = Throwable.class)
public static Scope enter(@Advice.Argument(0) Envelope envelope) {
VirtualField<Envelope, PropagatedContext> virtualField =
VirtualField.find(Envelope.class, PropagatedContext.class);
return TaskAdviceHelper.makePropagatedContextCurrent(virtualField, envelope);
return TaskAdviceHelper.makePropagatedContextCurrent(
VirtualFields.ENVELOPE_PROPAGATED_CONTEXT, envelope);
}

@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
Expand All @@ -60,9 +57,8 @@ public static class SystemInvokeAdvice {

@Advice.OnMethodEnter(suppress = Throwable.class)
public static Scope enter(@Advice.Argument(0) SystemMessage systemMessage) {
VirtualField<SystemMessage, PropagatedContext> virtualField =
VirtualField.find(SystemMessage.class, PropagatedContext.class);
return TaskAdviceHelper.makePropagatedContextCurrent(virtualField, systemMessage);
return TaskAdviceHelper.makePropagatedContextCurrent(
VirtualFields.SYSTEM_MESSAGE_PROPAGATED_CONTEXT, systemMessage);
}

@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@

import akka.dispatch.sysmsg.SystemMessage;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.bootstrap.executors.ExecutorAdviceHelper;
import io.opentelemetry.javaagent.bootstrap.executors.PropagatedContext;
Expand Down Expand Up @@ -49,9 +48,8 @@ public static class DispatchSystemAdvice {
public static PropagatedContext enter(@Advice.Argument(1) SystemMessage systemMessage) {
Context context = Java8BytecodeBridge.currentContext();
if (ExecutorAdviceHelper.shouldPropagateContext(context, systemMessage)) {
VirtualField<SystemMessage, PropagatedContext> virtualField =
VirtualField.find(SystemMessage.class, PropagatedContext.class);
return ExecutorAdviceHelper.attachContextToTask(context, virtualField, systemMessage);
return ExecutorAdviceHelper.attachContextToTask(
context, VirtualFields.SYSTEM_MESSAGE_PROPAGATED_CONTEXT, systemMessage);
}
return null;
}
Expand All @@ -61,10 +59,11 @@ public static void exit(
@Advice.Argument(1) SystemMessage systemMessage,
@Advice.Enter PropagatedContext propagatedContext,
@Advice.Thrown Throwable throwable) {
VirtualField<SystemMessage, PropagatedContext> virtualField =
VirtualField.find(SystemMessage.class, PropagatedContext.class);
ExecutorAdviceHelper.cleanUpAfterSubmit(
propagatedContext, throwable, virtualField, systemMessage);
propagatedContext,
throwable,
VirtualFields.SYSTEM_MESSAGE_PROPAGATED_CONTEXT,
systemMessage);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

import akka.dispatch.Envelope;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.bootstrap.executors.ExecutorAdviceHelper;
import io.opentelemetry.javaagent.bootstrap.executors.PropagatedContext;
Expand Down Expand Up @@ -43,9 +42,8 @@ public static class DispatchEnvelopeAdvice {
public static PropagatedContext enterDispatch(@Advice.Argument(1) Envelope envelope) {
Context context = Java8BytecodeBridge.currentContext();
if (ExecutorAdviceHelper.shouldPropagateContext(context, envelope.message())) {
VirtualField<Envelope, PropagatedContext> virtualField =
VirtualField.find(Envelope.class, PropagatedContext.class);
return ExecutorAdviceHelper.attachContextToTask(context, virtualField, envelope);
return ExecutorAdviceHelper.attachContextToTask(
context, VirtualFields.ENVELOPE_PROPAGATED_CONTEXT, envelope);
}
return null;
}
Expand All @@ -55,9 +53,8 @@ public static void exitDispatch(
@Advice.Argument(1) Envelope envelope,
@Advice.Enter PropagatedContext propagatedContext,
@Advice.Thrown Throwable throwable) {
VirtualField<Envelope, PropagatedContext> virtualField =
VirtualField.find(Envelope.class, PropagatedContext.class);
ExecutorAdviceHelper.cleanUpAfterSubmit(propagatedContext, throwable, virtualField, envelope);
ExecutorAdviceHelper.cleanUpAfterSubmit(
propagatedContext, throwable, VirtualFields.ENVELOPE_PROPAGATED_CONTEXT, envelope);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.instrumentation.akkaactor;

import akka.dispatch.Envelope;
import akka.dispatch.sysmsg.SystemMessage;
import io.opentelemetry.instrumentation.api.util.VirtualField;
import io.opentelemetry.javaagent.bootstrap.executors.PropagatedContext;

public class VirtualFields {

private VirtualFields() {}

public static final VirtualField<Envelope, PropagatedContext> ENVELOPE_PROPAGATED_CONTEXT =
VirtualField.find(Envelope.class, PropagatedContext.class);
public static final VirtualField<SystemMessage, PropagatedContext>
SYSTEM_MESSAGE_PROPAGATED_CONTEXT =
VirtualField.find(SystemMessage.class, PropagatedContext.class);
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

import akka.dispatch.forkjoin.ForkJoinTask;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.bootstrap.executors.ExecutorAdviceHelper;
import io.opentelemetry.javaagent.bootstrap.executors.PropagatedContext;
Expand Down Expand Up @@ -51,9 +50,8 @@ public static class SetAkkaForkJoinStateAdvice {
public static PropagatedContext enterJobSubmit(@Advice.Argument(0) ForkJoinTask<?> task) {
Context context = Java8BytecodeBridge.currentContext();
if (ExecutorAdviceHelper.shouldPropagateContext(context, task)) {
VirtualField<ForkJoinTask<?>, PropagatedContext> virtualField =
VirtualField.find(ForkJoinTask.class, PropagatedContext.class);
return ExecutorAdviceHelper.attachContextToTask(context, virtualField, task);
return ExecutorAdviceHelper.attachContextToTask(
context, VirtualFields.FORK_JOIN_TASK_PROPAGATED_CONTEXT, task);
}
return null;
}
Expand All @@ -63,9 +61,8 @@ public static void exitJobSubmit(
@Advice.Argument(0) ForkJoinTask<?> task,
@Advice.Enter PropagatedContext propagatedContext,
@Advice.Thrown Throwable throwable) {
VirtualField<ForkJoinTask<?>, PropagatedContext> virtualField =
VirtualField.find(ForkJoinTask.class, PropagatedContext.class);
ExecutorAdviceHelper.cleanUpAfterSubmit(propagatedContext, throwable, virtualField, task);
ExecutorAdviceHelper.cleanUpAfterSubmit(
propagatedContext, throwable, VirtualFields.FORK_JOIN_TASK_PROPAGATED_CONTEXT, task);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
import akka.dispatch.forkjoin.ForkJoinPool;
import akka.dispatch.forkjoin.ForkJoinTask;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.util.VirtualField;
import io.opentelemetry.javaagent.bootstrap.executors.PropagatedContext;
import io.opentelemetry.javaagent.bootstrap.executors.TaskAdviceHelper;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
Expand Down Expand Up @@ -62,14 +60,13 @@ public static class ForkJoinTaskAdvice {
*/
@Advice.OnMethodEnter(suppress = Throwable.class)
public static Scope enter(@Advice.This ForkJoinTask<?> thiz) {
VirtualField<ForkJoinTask<?>, PropagatedContext> virtualField =
VirtualField.find(ForkJoinTask.class, PropagatedContext.class);
Scope scope = TaskAdviceHelper.makePropagatedContextCurrent(virtualField, thiz);
Scope scope =
TaskAdviceHelper.makePropagatedContextCurrent(
VirtualFields.FORK_JOIN_TASK_PROPAGATED_CONTEXT, thiz);
if (thiz instanceof Runnable) {
VirtualField<Runnable, PropagatedContext> runnableVirtualField =
VirtualField.find(Runnable.class, PropagatedContext.class);
Scope newScope =
TaskAdviceHelper.makePropagatedContextCurrent(runnableVirtualField, (Runnable) thiz);
TaskAdviceHelper.makePropagatedContextCurrent(
VirtualFields.RUNNABLE_PROPAGATED_CONTEXT, (Runnable) thiz);
if (null != newScope) {
if (null != scope) {
newScope.close();
Expand All @@ -79,10 +76,9 @@ public static Scope enter(@Advice.This ForkJoinTask<?> thiz) {
}
}
if (thiz instanceof Callable) {
VirtualField<Callable<?>, PropagatedContext> callableVirtualField =
VirtualField.find(Callable.class, PropagatedContext.class);
Scope newScope =
TaskAdviceHelper.makePropagatedContextCurrent(callableVirtualField, (Callable<?>) thiz);
TaskAdviceHelper.makePropagatedContextCurrent(
VirtualFields.CALLABLE_PROPAGATED_CONTEXT, (Callable<?>) thiz);
if (null != newScope) {
if (null != scope) {
newScope.close();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.instrumentation.akkaactor;

import akka.dispatch.forkjoin.ForkJoinTask;
import io.opentelemetry.instrumentation.api.util.VirtualField;
import io.opentelemetry.javaagent.bootstrap.executors.PropagatedContext;
import java.util.concurrent.Callable;

public class VirtualFields {

private VirtualFields() {}

public static final VirtualField<ForkJoinTask<?>, PropagatedContext>
FORK_JOIN_TASK_PROPAGATED_CONTEXT =
VirtualField.find(ForkJoinTask.class, PropagatedContext.class);
public static final VirtualField<Runnable, PropagatedContext> RUNNABLE_PROPAGATED_CONTEXT =
VirtualField.find(Runnable.class, PropagatedContext.class);
public static final VirtualField<Callable, PropagatedContext> CALLABLE_PROPAGATED_CONTEXT =
VirtualField.find(Callable.class, PropagatedContext.class);
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

import akka.http.scaladsl.model.Uri;
import akka.http.scaladsl.server.PathMatcher;
import io.opentelemetry.instrumentation.api.util.VirtualField;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import net.bytebuddy.asm.Advice;
Expand Down Expand Up @@ -41,7 +40,7 @@ public static void onEnter(
@Advice.Argument(0) Uri.Path prefix, @Advice.Return PathMatcher<?> result) {
// store the path being matched inside a VirtualField on the given matcher, so it can be used
// for constructing the route
VirtualField.find(PathMatcher.class, String.class).set(result, prefix.toString());
VirtualFields.PATH_MATCHER_ROUTE.set(result, prefix.toString());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import akka.http.scaladsl.model.Uri;
import akka.http.scaladsl.server.PathMatcher;
import akka.http.scaladsl.server.PathMatchers;
import io.opentelemetry.instrumentation.api.util.VirtualField;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import net.bytebuddy.asm.Advice;
Expand Down Expand Up @@ -48,7 +47,7 @@ public static void onExit(
}
// if present use the matched path that was remembered in PathMatcherInstrumentation,
// otherwise just use a *
String prefix = VirtualField.find(PathMatcher.class, String.class).get(pathMatcher);
String prefix = VirtualFields.PATH_MATCHER_ROUTE.get(pathMatcher);
if (prefix == null) {
if (PathMatchers.Slash$.class == pathMatcher.getClass()) {
prefix = "/";
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.instrumentation.akkahttp.server.route;

import akka.http.scaladsl.server.PathMatcher;
import io.opentelemetry.instrumentation.api.util.VirtualField;

public class VirtualFields {

private VirtualFields() {}

public static final VirtualField<PathMatcher<?>, String> PATH_MATCHER_ROUTE =
VirtualField.find(PathMatcher.class, String.class);
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ public void transform(TypeTransformer transformer) {
@SuppressWarnings("unused")
public static class ConstructorAdvice {

public static class VirtualFields {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Until we drop support for inlined advice, both the class and the constant need to be public.
They are accessed directly from the instrumented class, which lives in a different package.

When only indy is support, we can make it a private constant of the advice instead.

public static final VirtualField<ServerCall<?, ?>, String> SERVER_CALL_AUTHORITY =
VirtualField.find(ServerCall.class, String.class);
}

@Advice.OnMethodExit(suppress = Throwable.class)
public static void onExit(
@Advice.This ServerCall<?, ?> serverCall,
Expand All @@ -43,7 +48,7 @@ public static void onExit(
// ArmeriaServerCall does not implement getAuthority. We will store the value for authority
// header as virtual field, this field is read in grpc instrumentation in
// TracingServerInterceptor
VirtualField.find(ServerCall.class, String.class).set(serverCall, authority);
VirtualFields.SERVER_CALL_AUTHORITY.set(serverCall, authority);
}
}
}
Expand Down
Loading