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

Log a warning whenever GlobalOpenTelemetry.set() is called #5264

Merged
merged 1 commit into from
Jan 31, 2022
Merged

Log a warning whenever GlobalOpenTelemetry.set() is called #5264

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
34 changes: 31 additions & 3 deletions ...pentelemetry/javaagent/instrumentation/opentelemetryapi/OpenTelemetryInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.isStatic;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.returns;
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;

import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;
import org.slf4j.LoggerFactory;

// Our convention for accessing agent package
@SuppressWarnings("UnnecessarilyFullyQualified")
Expand All @@ -28,12 +31,23 @@ public ElementMatcher<TypeDescription> typeMatcher() {
@Override
public void transform(TypeTransformer transformer) {
transformer.applyAdviceToMethod(
isMethod().and(isStatic()).and(named("get")).and(takesArguments(0)),
OpenTelemetryInstrumentation.class.getName() + "$GetGlobalOpenTelemetryAdvice");
isMethod()
.and(isStatic())
.and(named("get"))
.and(takesArguments(0))
.and(returns(named("application.io.opentelemetry.api.OpenTelemetry"))),
OpenTelemetryInstrumentation.class.getName() + "$GetAdvice");
transformer.applyAdviceToMethod(
isMethod()
.and(isStatic())
.and(named("set"))
.and(takesArguments(1))
.and(takesArgument(0, named("application.io.opentelemetry.api.OpenTelemetry"))),
OpenTelemetryInstrumentation.class.getName() + "$SetAdvice");
}

@SuppressWarnings("unused")
public static class GetGlobalOpenTelemetryAdvice {
public static class GetAdvice {

@Advice.OnMethodEnter(skipOn = Advice.OnDefaultValue.class)
public static Object onEnter() {
Expand All @@ -47,4 +61,18 @@ public static void methodExit(
openTelemetry = ApplicationOpenTelemetry.INSTANCE;
}
}

@SuppressWarnings("unused")
public static class SetAdvice {

@Advice.OnMethodEnter(suppress = Throwable.class)
public static void onEnter() {
LoggerFactory.getLogger(application.io.opentelemetry.api.GlobalOpenTelemetry.class)
.warn(
Copy link
Member

Choose a reason for hiding this comment

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

maybe info? (which is the javaagent's default log level, so will still get logged)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that warn makes slightly more sense in this case (since the log warns you about incorrect API usage), but I don't really have a strong opinion on this. I'll change that if we decide so.

Copy link
Member

Choose a reason for hiding this comment

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

my only concern is about users adding the javaagent later to an existing app that was previously manually instrumented, and then they're stuck with this warning message which is not really actionable

let's keep at warn and we can always revisit later

Copy link
Contributor

Choose a reason for hiding this comment

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

my only concern is about users adding the javaagent later to an existing app that was previously manually instrumented, and then they're stuck with this warning message which is not really actionable

let's keep at warn and we can always revisit later

Why not actionable? They can remove their manual usages of GlobalOpenTelemetry.set?

Copy link
Member

Choose a reason for hiding this comment

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

sorry, I should have been more specific, I was thinking about ops users

"You are currently using the OpenTelemetry Instrumentation Java Agent;"
+ " all GlobalOpenTelemetry.set calls are ignored - the agent provides"
+ " the global OpenTelemetry object used by your application.",
new Throwable());
}
}
}