-
Notifications
You must be signed in to change notification settings - Fork 877
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.
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 Akka Scheduler context propagation #12373
Conversation
...n/java/io/opentelemetry/javaagent/instrumentation/akkaactor/AkkaScheduleInstrumentation.java
Outdated
Show resolved
Hide resolved
...n/java/io/opentelemetry/javaagent/instrumentation/akkaactor/AkkaScheduleInstrumentation.java
Outdated
Show resolved
Hide resolved
919d755
to
496ce36
Compare
This isn't quite ready. I'll look into the issues tonight. |
Update AkkaScheduleInstrumentation.java Update AkkaSchedulerTest.scala
496ce36
to
aacbcee
Compare
Build passing now. |
@Advice.OnMethodEnter(suppress = Throwable.class) | ||
public static void enterSchedule( | ||
@Advice.Argument(value = 2, readOnly = false) Runnable runnable) { | ||
Context context = Java8BytecodeBridge.currentContext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just for my own curiosity: why getting the current context from Java8BytecodeBride?
why not 'Context.current()' directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what most of the code does so I did it too. Are you a regular contributor to this lib and if so, can you explain why I should change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is instrumented with the javaagent; for example, the `Java8BytecodeBridge` that should be used |
I assume this includes here but I am a first time contributor so I will change this if any of the regular contributors say so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's needed when the Advice is inlined into bytecode that targets a version of Java prior to Java 8
interestingly, I think it will no longer be needed after #11457 (since we will no longer be inlining our advice code)
the javadoc explains a bit more:
Lines 13 to 17 in b184587
* A helper for accessing methods that rely on new Java 8 bytecode features such as calling a static | |
* interface methods. In instrumentation, we may need to call these methods in code that is inlined | |
* into an instrumented class, however many times the instrumented class has been compiled to a | |
* previous version of bytecode and so we cannot inline calls to static interface methods, as those | |
* were not supported prior to Java 8 and will lead to a class verification error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what most of the code does so I did it too. Are you a regular contributor to this lib and if so, can you explain why I should change?
i was not asking for a change.. just curious to know the reason behind. @trask explained it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trask The existing opentelemetry-java-instrumentation code for Akka and Pekko uses Java8BytecodeBridge. Is it ok if I stick this 'style' and that all the code can be changed to stop using Java8BytecodeBridge in a future commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, definitely, it's totally fine to use Java8BytecodeBridge
Akka version of #12359 (#11755)