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

Release scala-3 versions of kamon-akka, kamon-pekko and kamon-pekko-http #1295

Closed
wants to merge 10 commits into from
Closed

Conversation

TjarkoG
Copy link
Contributor

@TjarkoG TjarkoG commented Sep 4, 2023

release scala3 releases of:

  • kamon-akka
  • kamon-pekko
  • kamon-pekko-http

since Akka.actor >=2.6.17 was released with scala3 we can release them as scala3 packages as well
Pekko was released for scala3 from v1.0.0

@TjarkoG
Copy link
Contributor Author

TjarkoG commented Sep 5, 2023

The release of the Pekko packages with Scala 3.x would probably need
#1291 to be merged as well

@mkemaldurmus
Copy link

@ivantopo Do you have any plan to pekko integration?

@hughsimpson
Copy link
Contributor

Hi 👋 I'll be helping out for a bit to get some of these outstanding prs merged. There are a few issues with this pr - as it stands the akka + pekko modules don't actually compile with scala 3. I had a crack at fixing that locally, but ran into some test failures. Will try to get this moving ASAP however

@TjarkoG
Copy link
Contributor Author

TjarkoG commented Oct 4, 2023

Hi @hughsimpson Thanks for the Help.
When running locally with java 8 and
SBT_OPTS="-Xmx4G" sbt +compile i don't see any errors. Am i doing something wrong?
Could you tell me how to reproduce? then i can try to fix it

@TjarkoG
Copy link
Contributor Author

TjarkoG commented Oct 4, 2023

ah i've just nor realized thet the Merge of the main removed the scala3-release for pekko
as is with java8 SBT_OPTS="-Xmx4G" sbt +publishLocal released a scala 3 package for kamon-akka.
With pekko i have found that the release of pekko(-http) was done with scala 3.3.0 (latest lts)
so a Scala-3 release of the pekko-package would require kamon to be released with scala3.3 as well...
this could be considdered a breaking change since scala-3 are not backwards-compatible due to tasty.
i am uncertain how Kamon has handled the "breaking" minor-version of scala3 up until now.

@hughsimpson
Copy link
Contributor

Think bumping to scala 3.3 is fine TBH - it's the LTS version after all. Anyway, the issues I ran into for compilation were (other than crossScalaVersions not containing scala_3_version - build.sbt used += before := so it was being removed) mostly missing the return type of various implicits. For testing with scala 3, best way I know of is sbt ++3.3 kamon-pekko/test

@TjarkoG
Copy link
Contributor Author

TjarkoG commented Oct 4, 2023

i have Fixed the Compile-errors and was able to locally publish
kamon-pekko_3
kamon-pekko-http_3 &
kamon-akka_3

bus as mentioned above i needed to upgrade scala from 3.2.0 to 3.3.0 to do it.
edit:
looks like not all errors are fixed.
back on it

@@ -0,0 +1,323 @@
package kamon.instrumentation.pekko.http
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this file can/should be deduplicated between scala versions now - it's essentially the same, modulo formatting, for 2.12/2.13/3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the scala-3 version is explicitly handing the Tuple in line 290.
the implicit way didn't compile

Copy link
Contributor

@hughsimpson hughsimpson Oct 4, 2023

Choose a reason for hiding this comment

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

Don't think it's worth having three copies just to avoid that in the other two copies 😄 Shouldn't cause an error with them (specifying tprovide[T](values) should also work to avoid passing the implicit explicitly)

@TjarkoG
Copy link
Contributor Author

TjarkoG commented Oct 4, 2023

okay... so i've managed to fix the compiler-errors, but all test-suites for akka, pekko and pekko-http fail with a logging entry like

[pool-1-thread-1] ERROR 2023-10-04 14:53:33  Logger : Error => org.apache.pekko.actor.ActorSystemImpl with message Cannot assign final java.lang.String name to class org.apache.pekko.actor.ActorSystem. Class loader: sun.misc.Launcher$AppClassLoader@18b4aac2: java.lang.IllegalStateException: Cannot assign final java.lang.String name to class org.apache.pekko.actor.ActorSystem
	at kanela.agent.libs.net.bytebuddy.asm.Advice$OffsetMapping$ForArgument.resolve(Advice.java:1581)
	at kanela.agent.libs.net.bytebuddy.asm.Advice$OffsetMapping$ForArgument$Unresolved.resolve(Advice.java:1689)
	at kanela.agent.libs.net.bytebuddy.asm.Advice$Dispatcher$Inlining$Resolved$ForMethodExit.doApply(Advice.java:8960)
	at kanela.agent.libs.net.bytebuddy.asm.Advice$Dispatcher$Inlining$Resolved$ForMethodExit.apply(Advice.java:8918)
	at kanela.agent.libs.net.bytebuddy.asm.Advice$Dispatcher$Inlining$Resolved$AdviceMethodInliner.visitMethod(Advice.java:8332)
	at kanela.agent.libs.net.bytebuddy.jar.asm.ClassReader.readMethod(ClassReader.java:1353)
	at kanela.agent.libs.net.bytebuddy.jar.asm.ClassReader.accept(ClassReader.java:744)
	at kanela.agent.libs.net.bytebuddy.jar.asm.ClassReader.accept(ClassReader.java:424)
	at kanela.agent.libs.net.bytebuddy.asm.Advice$Dispatcher$Inlining$Resolved$AdviceMethodInliner.apply(Advice.java:8325)
	at kanela.agent.libs.net.bytebuddy.asm.Advice$AdviceVisitor$WithExitAdvice.onUserEnd(Advice.java:10835)
	at kanela.agent.libs.net.bytebuddy.asm.Advice$AdviceVisitor.visitMaxs(Advice.java:10614)
	at kanela.agent.libs.net.bytebuddy.jar.asm.ClassReader.readCode(ClassReader.java:2665)
	at kanela.agent.libs.net.bytebuddy.jar.asm.ClassReader.readMethod(ClassReader.java:1514)
	at kanela.agent.libs.net.bytebuddy.jar.asm.ClassReader.accept(ClassReader.java:744)
	at kanela.agent.libs.net.bytebuddy.jar.asm.ClassReader.accept(ClassReader.java:424)
	at kanela.agent.libs.net.bytebuddy.dynamic.scaffold.TypeWriter$Default$ForInlining.create(TypeWriter.java:4014)
	at kanela.agent.libs.net.bytebuddy.dynamic.scaffold.TypeWriter$Default.make(TypeWriter.java:2224)
	at kanela.agent.libs.net.bytebuddy.dynamic.DynamicType$Builder$AbstractBase$UsingTypeWriter.make(DynamicType.java:4057)
	at kanela.agent.libs.net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer.doTransform(AgentBuilder.java:11903)
	at kanela.agent.libs.net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer.transform(AgentBuilder.java:11838)
	at kanela.agent.libs.net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer.access$1700(AgentBuilder.java:11555)
	at kanela.agent.libs.net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer$LegacyVmDispatcher.run(AgentBuilder.java:12238)
	at kanela.agent.libs.net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer$LegacyVmDispatcher.run(AgentBuilder.java:12178)
	at java.security.AccessController.doPrivileged(Native Method)
	at kanela.agent.libs.net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer.doPrivileged(AgentBuilder.java)
	at kanela.agent.libs.net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer.transform(AgentBuilder.java:11747)
	at sun.instrument.TransformerManager.transform(TransformerManager.java:188)
	at sun.instrument.InstrumentationImpl.transform(InstrumentationImpl.java:428)
	at java.lang.ClassLoader.defineClass1(Native Method)
	at java.lang.ClassLoader.defineClass(ClassLoader.java:757)
	at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
	at java.net.URLClassLoader.defineClass(URLClassLoader.java:473)
	at java.net.URLClassLoader.access$100(URLClassLoader.java:74)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:369)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:363)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.net.URLClassLoader.findClass(URLClassLoader.java:362)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:419)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:352)
	at kamon.instrumentation.pekko.RouterMetricsSpec.<init>(RouterMetricsSpec.scala:33)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at java.lang.Class.newInstance(Class.java:442)
	at org.scalatest.tools.Framework$ScalaTestTask.execute(Framework.scala:450)
	at sbt.ForkMain$Run.lambda$runTest$1(ForkMain.java:414)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:750)

and a nullpointer:

[info] kamon.instrumentation.pekko.RouterMetricsSpec *** ABORTED ***
[info]   java.lang.NullPointerException:
[info]   at kamon.instrumentation.pekko.instrumentations.InstrumentNewExecutorServiceOnPekko$.around(DispatcherInstrumentation.scala:104)
[info]   at org.apache.pekko.dispatch.ForkJoinExecutorConfigurator$ForkJoinExecutorServiceFactory.createExecutorService(ForkJoinExecutorConfigurator.scala)
[info]   at org.apache.pekko.dispatch.Dispatcher$LazyExecutorServiceDelegate.executor$lzyINIT1(Dispatcher.scala:54)
[info]   at org.apache.pekko.dispatch.Dispatcher$LazyExecutorServiceDelegate.executor(Dispatcher.scala:54)
[info]   at org.apache.pekko.dispatch.ExecutorServiceDelegate.execute(ThreadPoolBuilder.scala:228)
[info]   at org.apache.pekko.dispatch.ExecutorServiceDelegate.execute$(ThreadPoolBuilder.scala:224)
[info]   at org.apache.pekko.dispatch.Dispatcher$LazyExecutorServiceDelegate.execute(Dispatcher.scala:53)
[info]   at org.apache.pekko.dispatch.Dispatcher.registerForExecution(Dispatcher.scala:137)
[info]   at org.apache.pekko.dispatch.MessageDispatcher.attach(AbstractDispatcher.scala:163)
[info]   at org.apache.pekko.actor.dungeon.Dispatch.start(Dispatch.scala:130)
[info]   ...

I think i have reached the limit of my scala-knowlage.
to my untrained eye it looks like an error in the Kanela agent.

@hughsimpson
Copy link
Contributor

So that's where.I got to too 😅 . These errors can be fixed, I believe, by judicious application of the @static annotation to the advice methods - I'm working my way through this at the moment. Assuming you've no objections I'll push to your branch once I've got it working, and merge this

@TjarkoG
Copy link
Contributor Author

TjarkoG commented Oct 4, 2023

no objections at all.

@TjarkoG
Copy link
Contributor Author

TjarkoG commented Oct 5, 2023

i'm rather new to the Github-collaboration thing.
do i need to grant you access to the fork-repo or do you have automatic access because of this PR?

@hughsimpson
Copy link
Contributor

hughsimpson commented Oct 5, 2023

Good question... since I was able to merge master back into your branch I guess I already have access -- I think you implicitly grant it (branch access) to repo maintainers when you open the pr. Anyway, I've got the pekko-http tests passing, but context still isn't being propagated over actor calls, so I've got a lot of failing tests still. Might be a while on this I'm afraid 😅

@TjarkoG
Copy link
Contributor Author

TjarkoG commented Oct 25, 2023

Hey @hughsimpson
Is it harder to do than you thought or is it just allot of busywork?
If it's easy to do but a lot maybe i could help. (If you can explain what to do)

@hughsimpson
Copy link
Contributor

hughsimpson commented Oct 26, 2023

Hey! Sorry, I've not looked at this for a little while. I've actually got the [akka/pekko]-http modules passing all tests, but context propagation over actor messages wasn't working when I last left it. I'll try to take another look at this tomorrow and see if I can get anywhere.

Edit: sorry, didn't get to that today. Personal reasons. But this is near the top of my priority list now. Think we probably need a new version cut first anyway, but I will want this soon myself so it will be addressed.

@hughsimpson
Copy link
Contributor

If you want to know what I was doing in the meantime, it was mostly adding the @static annotation to all the methods that 'couldn't be found' (and, eventually, probably a bunch that didn't need it, because I resorted to a regex). Also had to change a lof of

object Foo {
  // stuff
}

to

object Foo
class Foo {
  // stuff
}

in the classes where I introduced the static notations. But that turns out not not be the right thing everywhere... I'll push something to a branch to show you at some point, although I confess I'll have another go at actually fixing before sharing where I've got to

@hughsimpson
Copy link
Contributor

I've got the kamon-pekko module tests passing now here

@TjarkoG
Copy link
Contributor Author

TjarkoG commented Nov 7, 2023

Thanks will have a look at it

@hughsimpson
Copy link
Contributor

As you suggested, probably a good idea to rebase this. Are you still up for having a crack at applying the same fixes to the akka modules?

@TjarkoG
Copy link
Contributor Author

TjarkoG commented Nov 7, 2023

Thought i had rebased already but seem like i messed it up ...
i can try to understand what exaclty the fix was and apply it to akka as well... will propably take a bit longer but i'll look into it

@TjarkoG
Copy link
Contributor Author

TjarkoG commented Nov 8, 2023

I think i've fixed the rebaseing and will have a look at the failing akka tests later today

@hughsimpson
Copy link
Contributor

Awesome! Thanks for the help 🙏 it really makes a difference!

@@ -28,6 +28,9 @@ import kanela.agent.libs.net.bytebuddy.asm.Advice.{Argument, OnMethodEnter, OnMe

class ActorInstrumentation extends InstrumentationBuilder {

onType("akka.actor.dungeon.Dispatch")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I've caused a regression with Akka 2.5 with this one. It looks like this advice will need to be split between Akka versions

@@ -222,7 +222,7 @@ class TracingTestActor extends Actor {
}

class ActorWithMaterializer extends Actor {
implicit val mat = ActorMaterializer()
implicit val mat: Materializer = Materializer(context)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs reverting because of Akka 2.5...

@hughsimpson
Copy link
Contributor

I've created a new branch on https://github.com/kamon-io/Kamon/tree/akka_pekko_scala_3 and will push some additional test fixes to it today

@hughsimpson
Copy link
Contributor

Closing this in favour of #1311

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.

3 participants