-
Notifications
You must be signed in to change notification settings - Fork 901
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
Instrument Pekko (by copying from akka) #9527
Conversation
Apache Pekko (https://pekko.apache.org/) is a fork of Akka 2.6 with an Apache 2.0 license. As such, its instrumentation should be fairly simple. My process here was: 1. Copy the akka instrumentation modules 2. Refactor everything to point to pekko classes 3. Rename everything from "akka" to "pekko" 4. In `ExecutorMatchers` and `FutureInstrumentation` copy references to akka to also refer to pekko The only change I noticed is that there is no equivalent to the `akka.dispatch.forkjoin.ForkJoinPool` which the `:instrumentations:akka-actor-fork-join-2.5` module was instrumenting. As such I didn't copy that module over.
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.javaagent.instrumentation.pekkoactor; |
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.
our current convention is to also add the version number to package name io.opentelemetry.javaagent.instrumentation.pekkoactor.v1_0
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.
I've now done that for pekkoactor and pekkohttp, for both the src/main
and src/test
packages.
...a/io/opentelemetry/javaagent/instrumentation/pekkoactor/PekkoActorInstrumentationModule.java
Outdated
Show resolved
Hide resolved
...r-1.0/javaagent/src/test/scala/io/opentelemetry/instrumentation/pekkoactor/PekkoActors.scala
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public void transform(TypeTransformer transformer) { | ||
// This is mainly for compatibility with 10.0 |
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.
Perhaps this isn't needed for pekko as it is was for an older akka http version?
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.
Looking at Pekko, it seems only the singleRequest
method exists. I'll remove the advice for singleRequestImpl
instead, as it doesn't exist in HttpExt
.
...o/opentelemetry/javaagent/instrumentation/pekkohttp/client/HttpExtClientInstrumentation.java
Outdated
Show resolved
Hide resolved
...main/java/io/opentelemetry/javaagent/instrumentation/pekkohttp/client/OnCompleteHandler.java
Outdated
Show resolved
Hide resolved
I see the markdown-link-check is unhappy with a link unrelated to this PR. I think we need to fix this line to use something like this URL: Would you like me to include that in this PR? Or can we proceed without that check passing? |
A separate PR would be preferable. Often such issues resolve on their own, but we are not going to wait too long for it. |
@samwright link check seems to pass again |
Apache Pekko (https://pekko.apache.org/) is a fork of Akka 2.6 with an Apache 2.0 license. As such,
its instrumentation should be fairly simple. My process here was:
ExecutorMatchers
andFutureInstrumentation
copy references to akka to also refer to pekkoThe only change I noticed is that there is no equivalent to the
akka.dispatch.forkjoin.ForkJoinPool
which the:instrumentations:akka-actor-fork-join-2.5
module was instrumenting.As such I didn't copy that module over.