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 support for scala 2.13 async operations #109

Merged
merged 1 commit into from
Dec 9, 2020

Conversation

junder31
Copy link
Contributor

Overview

This adds support for async operations in scala 2.13. In scala 2.12 the mechanism that was instrumented was CallbackRunnable. In 2.13 this class no longer exists. Transformation now preforms a similar role. The instrumentation on Transformation is fairly similar to the CallbackRunnable instrumentation from previous version of the scala instrumentation.

Testing

This passes the same tests that the older instrumentation provided. I have also deployed it with one of my scala2.13 applications and verified that the transaction traces contain all the asynchronous work I expected to be included.

Checks

Are your contributions backwards compatible with relevant frameworks and APIs? N/A
Does your code contain any breaking changes? N/A
Does your code introduce any new dependencies? Just scala2.13 which is being instrumented.

@XiXiaPdx
Copy link
Contributor

Hi @junder31 , nice work!! For the app you emailed me, I now see scala.concurrent.impl.Promise$Transformation.run() and scala.concurrent.impl.Promise$Transformation.submitWithValue() reported in the Transacton segments. Just confirming this app is now running with your latest Scala2.13 async changes?

@junder31
Copy link
Contributor Author

@XiXiaPdx yes the app I sent you includes all the changes I have been working on. That includes the akka-http change I have a PR open for, this PR, and one more that fixes an issue with the scalapb-grpc clients that I have not submitted a PR for. These three changes have all the scala/play applications at my company instrumented quite well. We do have some other scala teams that use different frameworks and libraries that I will be working on in the coming weeks/months that may result in some more PRs.

@NewField
private AgentBridge.TokenAndRefCount tokenAndRefCount;

private Transformation_Instrumentation(final Function1 _fun, final ExecutionContext _ec, final Try<T> _arg, final int _xform) {
Copy link
Contributor

@tspring tspring Nov 16, 2020

Choose a reason for hiding this comment

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

Hi @junder31, I think you could remove this weave method. We instrumented the constructor in 2.12 due to an issue with Akka-http linking. It would also allow removing the token newfield and @Trace on submitWithValue, the latter of which is causing the ProducerConsumerWithFutures.testProducerConsumer test to fail with extra segments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tspring I made the changes you suggested. I'll give it a shot on one of my apps and see if instrumentation is still working. The test case at least passing now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tspring looks like I'm still getting the transactions segments I was expecting on my app as well with these changes.

@tspring
Copy link
Contributor

tspring commented Nov 17, 2020

Thanks @junder31 , we appreciate your work on this. Last thing, I fixed the github actions issue preventing the tests from passing but it looks like you'll need merge/rebase main.

* Override the submitWithValue so we can replace it with our wrapped version.
* It plays similar role to the 2.12 setter for "value" on the old CallbackRunnable
*/
public final Promise.Transformation submitWithValue(final Try<T> resolved) {
Copy link

Choose a reason for hiding this comment

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

I might be wrong, but shouldn't we have this annotation here?

@Trace(async = true, excludeFromTransactionTrace = true)

This way the segments are reported and it allows to align the ProducerConsumerWithFutures.testProducerConsumer test with how it looks for 2.12.
Currently it has zero segments reported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, the annotation with async=true should only be necessary when calling Token::link. For the 2.12 instrumentation we had to create a second token in the CallbackRunnable constructor to fix an issue Akka-Http. It's not clear to me the Java/scala.concurrent.impl.CallbackRunnable/value_$eq segments are valuable and ideally they wouldn't be included in traces either, but I'd be interested to hear if you find them useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe they are useful. Until I worked on the scala instrumentation I had no idea that method even existed. I think most scala users would actually find those segments confusing because they are completely unaware that futures work like that under the hood.

Copy link

Choose a reason for hiding this comment

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

I don't have a strong opinion here. If for practical purposes it's better to leave these segments out - I'm totally fine with it. Just pointed out the difference in behavior here.

@tspring tspring merged commit 3866b9f into newrelic:main Dec 9, 2020
@kford-newrelic kford-newrelic added this to the Scala 2.13 Support milestone Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants