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.

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

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

pjfanning
Copy link
Contributor

Akka version of #12359 (#11755)

@pjfanning pjfanning requested a review from a team as a code owner October 1, 2024 13:09
@pjfanning
Copy link
Contributor Author

This isn't quite ready. I'll look into the issues tonight.

@pjfanning pjfanning marked this pull request as draft October 1, 2024 13:46
Update AkkaScheduleInstrumentation.java

Update AkkaSchedulerTest.scala
@pjfanning pjfanning marked this pull request as ready for review October 1, 2024 15:05
@pjfanning
Copy link
Contributor Author

Build passing now.

@Advice.OnMethodEnter(suppress = Throwable.class)
public static void enterSchedule(
@Advice.Argument(value = 2, readOnly = false) Runnable runnable) {
Context context = Java8BytecodeBridge.currentContext();
Copy link
Contributor

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?

Copy link
Contributor Author

@pjfanning pjfanning Oct 1, 2024

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?

Copy link
Contributor Author

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
says to use Java8BytecodeBridge.currentContext() inside Advice classes.

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.

Copy link
Member

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:

* 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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Member

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

@laurit laurit merged commit 05fc637 into open-telemetry:main Oct 2, 2024
56 checks passed
@pjfanning pjfanning deleted the akka-schedule branch October 2, 2024 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants