Add Scala concurrent plugin

[milanvdm]

What does this PR do?

This PR is a draft to add specific Scala support to the ElasticAPM agent.
The goal is to add basic support for Scala Futures.

Known issues

• Test multiple Scala version (2.12.x and 2.13.x) -> cross-compiling multiple Scala versions doesn't seem to work/be possible with the Maven plugin
• LinkageError when the agent has a specific Scala dependency
• Not all edge-cases are caught in the tests, some of them were only found when running a small test-setup

Checklist

• My code follows the style guidelines of this project
• I have rebased my changes on top of the latest master branch
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have updated CHANGELOG.asciidoc
• I have updated supported-technologies.asciidoc
• Added an API method or config option? Document in which version this will be introduced
• Added an instrumentation plugin? How did you make sure that old, non-supported versions are not instrumented by accident?

Related issues

Part of #818

[cla-checker-service]

💚 CLA has been signed

[milanvdm]

@SylvainJuge As I just started exploring the APM, I wanted to start with a test that just checks if it is matching one of the methods I wrote a MethodMatcher for.
Any tips/example on how to write a test for that?

[milanvdm]

The tests I created are passing now. But it is hard for me to judge to what extent these tests are a good representation of reality and testing edge-cases.

The project I created to test a lot of tooling together would also be a good test run: https://github.com/milanvdm/scala-elastic-apm
For this Ill need to release the java-agent locally, not sure how to do this though.

[felixbarny]

For this Ill need to release the java-agent locally, not sure how to do this though.

Here are some instructions around that: https://github.com/elastic/apm-agent-java#build-form-source

[milanvdm]

Thanks! I managed to release it locally and use it in my test project as well.

Without the specific scala-concurrent instrumentation, it was not able to trace all the different parts of the program. From some talks with the maintainer of Kamon, this seems to be related to how the scala execution-context handles an internal queue for scheduling.

With the newly added scala-concurrent instrumentation, it was able to trace all 20 http-calls and 20 database requests, all done in an async way: https://github.com/milanvdm/scala-elastic-apm/blob/master/src/main/scala/me/milan/MainF.scala

Not sure what the next steps would be to get this PR merged :)

[felixbarny]

To summarize my current thinking: Ideally, we'd capture context when it's scheduled for execution and restore the context on execution. It seems like with the current instrumentation that works well most of the time. Unless there's something that breaks context propagation like something related to AsyncHttpClient.

In an ideal world, we should fix the loss of context. Pragmatically, it's probably not a bad idea to just combine the two approaches. I expect nothing bad will happen if both the Promise instrumentation (which captures the context when the promise is created and restores when it executes) and the executor instrumentation capture and restore the same context.

I'm willing to give this a shot. In the Future (see what I did there? ;) we might be able to remove this instrumentation when we have more extensive coverage of reactive frameworks. Also, when we see that this instrumentation does interfere with the existing context propagation we can re-evaluate. But given that our Scala support is not really mature anyway (we don't have Akka HTTP support yet so I suspect we don't have many Scala users) I think it's well worth the risk of even having this on by default.

[milanvdm]

@felixbarny Thanks for the write up :)

I fully agree with your arguments. Once this is merged, I'll continue with adding some extra libraries for Scala.

Some leftover comments (which may need to be solved in a different PR):

• I will test again if this works for Scala 2.12 or not. But besides that, I think we should not support deprecated 2.11. This will need to be checked somehow.
• I think using this Maven setup won't be Scalable (back at ya ;). So it may be something to consider to somehow combine an sbt project with Maven to be able to do cross-compilation and testing for the Scala plugins.

I can create some issues for that if you want to track it.

[milanvdm]

Shall I also create an issue pointing to my test-branch that shows an example on where the AsyncHttpClient seems to lose context?

[codecov-commenter]

Codecov Report

Merging #1048 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1048   +/-   ##
=========================================
  Coverage     59.31%   59.31%           
  Complexity       90       90           
=========================================
  Files           354      354           
  Lines         16249    16249           
  Branches       2268     2268           
=========================================
  Hits           9638     9638           
  Misses         5958     5958           
  Partials        653      653           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85f3edd...737bb0e. Read the comment docs.

[felixbarny]

Shall I also create an issue pointing to my test-branch that shows an example on where the AsyncHttpClient seems to lose context?

Yes, that'd be great

[felixbarny]

As a last attempt, could you try out the artifact from #1230? I've added some fixes around ForkJoinPools that might have an impact.

[milanvdm]

@felixbarny Will have a look :) Do you build artifacts for branches or do I need to build it locally?

[felixbarny]

Yes, there’s a link to the artifacts in the comment added by the CI

[milanvdm]

@felixbarny Same issue when using the artifact from that branch

[felixbarny]

Tests are failing with NPEs on CI. Could you have a look?

[milanvdm]

@felixbarny For some reason, tracer.startRootTransaction(null) now returns a null which was not the case before.
Did something change regarding this?

[felixbarny]

Ah, yes. You need to start the agent after building it and before starting transactions