Play 2.4-2.6 Instrumentation by realark · Pull Request #277 · DataDog/dd-trace-java

realark · 2018-03-30T18:14:16Z

First phase of play instrumentation tested against play 2.4 - 2.6.

Instrument play Actions for sync and async cases.
Extract Play headers for propagation

Remaining work

Integrate with scala/akka instrumentation to start/stop the trace
Investigate other versions of play (only tested against 2.6, but earlier versions should work as the api instrumented is not often updated).
Performance tests
Enable instrumentation by default

I can either continue pushing work to this branch, or if you want to review and merge into master (behind a feature flag) that works for me too. Whatever is easier.

tylerbenson · 2018-04-02T11:58:09Z

Play 2.4 is the first version to require Java 8. Before that required Java 6: https://www.playframework.com/documentation/2.6.x/Highlights24#Java-8-support

Play 2.5 was the one that discontinued Scala 2.10: https://www.playframework.com/documentation/2.5.x/Migration25#Scala-2.10-support-discontinued

Though the biggest change between 2.5 and 2.6 is the change from using Netty to Akka HTTP:
https://www.playframework.com/documentation/2.6.x/Migration26#Akka-HTTP-as-the-default-server-engine

If we want to support 2.5, we'd certainly want independent tests for both 2.5 and 2.6. Though if we want 2.4, we'd need to specify the supported Scala version too.

tylerbenson

Looks good. I don't think that we can merge this until we solve the Java 7 compile issue. What do you think?

tylerbenson · 2018-04-02T12:02:13Z

+
+  @Override
+  protected boolean defaultEnabled() {
+    return false;


Do we want to somehow chain this to if the scala/akka instrumentation is enabled? Might be funny if this is enabled, but the others aren't.

I think it's okay to leave the options separate. The play instrumentation will still apply and trace controller actions without the concurrent instrumentation, but it won't capture async work created by the controllers.

tylerbenson · 2018-04-02T12:06:48Z

+        @Advice.Thrown final Throwable throwable,
+        @Advice.Argument(0) final Request req,
+        @Advice.Return(readOnly = false) Future<Result> responseFuture) {
+      // more about routes here: https://github.com/playframework/playframework/blob/master/documentation/manual/releases/release26/migration26/Migration26.md


This won't work if we want to support lower versions.

tylerbenson · 2018-04-02T12:10:06Z

+        @Advice.Return(readOnly = false) Future<Result> responseFuture) {
+      // more about routes here: https://github.com/playframework/playframework/blob/master/documentation/manual/releases/release26/migration26/Migration26.md
+      final Option handlerOption = req.attrs().get(Router.Attrs.HANDLER_DEF.underlying());
+      if (!handlerOption.isEmpty()) {


This also has the Java 7 incompatibility problem...

tylerbenson · 2018-04-02T12:12:05Z

+      }
+      scope.span().setTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_SERVER);
+      scope.span().setTag(Tags.HTTP_METHOD.getKey(), req.method());
+      scope.span().setTag(DDTags.SPAN_TYPE, DDSpanTypes.WEB_SERVLET);


Awkward.... guess that could have been named better ;-)

tylerbenson · 2018-04-02T12:15:45Z

+      final Map<String, Object> errorLogs = new HashMap<>(4);
+      errorLogs.put("event", Tags.ERROR.getKey());
+      errorLogs.put("error.kind", throwable.getClass().getName());
+      errorLogs.put("error.object", throwable);


This is the only element required to be logged... the rest are already implicitly done in DDSpan.setErrorMeta().

I usually just call:

span.log(Collections.singletonMap("error.object", throwable));

tylerbenson · 2018-04-02T12:20:55Z

+      try {
+        Tags.HTTP_STATUS.set(span, result.header().status());
+      } catch (Throwable t) {
+        LoggerFactory.getLogger(RequestCallback.class).debug("error in play instrumentation", t);


Since this isn't advice, I think you could also add the @Slf4j annotation to the class.

tylerbenson · 2018-04-02T12:25:00Z

+  TestServer testServer
+
+  def setupSpec() {
+    testServer = Helpers.testServer(9080, PlayTestUtils.buildTestApp())


You can do something like this to get a random open port each time you run the test.

👍Cool. I added randomOpenPort to TestUtils.

tylerbenson · 2018-04-02T12:29:04Z

+object PlayTestUtils {
+  def buildTestApp(): play.Application = {
+    // build play.api.Application with desired setting and pass into play.Application for testing
+    val apiApp :play.api.Application = new play.api.inject.guice.GuiceApplicationBuilder()


realark · 2018-04-03T00:54:09Z

Addressed some of the feedback.

To work with Java 7, I'll try compiling against an earlier version of play, and move 2.6 into a separate test-only subproject.

Simplify error collecting. Use slf4j to log. randomOpenPort util.

Please review new changes

realark · 2018-04-05T21:56:39Z

@tylerbenson Pushed up a few changes for play 2.4 compatibility.

Use deprecated play tags to extract routes for backwards compatibility
Added tests for play 2.4 and 2.6. I tried 2.5, but hit a netty bug which I was unable to resolve for setting up the test.

Testing is not compatible with Java 7 because the play test dependencies require java 8. However, the instrumentation is compiled to Java 7, so it is safe to ship this instrumentation on a java 7 jvm. It won't apply, but it won't cause any issues.

Additionally, there are still two TODOs in this PR. This is a reference to the scala async issue we discussed earlier. This will be followed up in another PR.

tylerbenson · 2018-04-06T03:13:50Z

@@ -0,0 +1,26 @@
+apply from: "${rootDir}/gradle/java.gradle"
+apply from: "${rootDir}/gradle/test-with-scala.gradle"


Do you want a version scan block here?

Sure, I'll add one.

I'm running into some problems setting that up.

I'm trying to use this configuration:

versionScan { group = "com.typesafe.play" module = "play_2.11" versions = "[2.4.0,)" verifyPresent = [ 'akka.japi.JavaPartialFunction' : null, 'play.api.mvc.Action' : null, 'play.api.mvc.Result' : null, 'play.api.mvc.Request' : null, 'scala.Option' : null, 'scala.Tuple2' : null, 'scala.concurrent.Future' : null ] }

I'm getting an error that I don't understand.

keyPresent: 1841->88 keyMissing: 1687->85 :dd-java-agent:instrumentation:play-2.4:verifyVersionScanError for com.typesafe.play:play_2.11 - not a 'keyPresent' identifier: akka.japi.JavaPartialFunction Error for com.typesafe.play:play_2.11 - not a 'keyPresent' identifier: play.api.mvc.Action Error for com.typesafe.play:play_2.11 - not a 'keyPresent' identifier: play.api.mvc.Result Error for com.typesafe.play:play_2.11 - not a 'keyPresent' identifier: play.api.mvc.Request Error for com.typesafe.play:play_2.11 - not a 'keyPresent' identifier: scala.Option Error for com.typesafe.play:play_2.11 - not a 'keyPresent' identifier: scala.Tuple2 Error for com.typesafe.play:play_2.11 - not a 'keyPresent' identifier: scala.concurrent.Future

Also, it seems like the versionScan plugin only expects one module at a time to test. Because play bundles many different modules it's useful to check for transitive deps (scala, akka). Is there a way to check for those?

I believe the transitive dependencies are included in the scan.

The error for something not being a key identifier means it can't be used to uniquely fingerprint that version range.

This isn't intended to be a list of classes we use in our instrumentation. This is strictly for trying to figure out what version of a library is currently being loaded.

tylerbenson · 2018-04-06T03:17:19Z

+
+test {
+  if (JavaVersion.current().isJava8Compatible()) {
+    exclude '*Play*Test*'


so you're excluding the test for Java 8+?

tylerbenson · 2018-04-06T03:21:26Z

@@ -0,0 +1,27 @@
+apply from: "${rootDir}/gradle/java.gradle"
+apply from: "${rootDir}/gradle/test-with-scala.gradle"


Instead of specifying a separate project, you might be able to just use a separate configuration. Try using this plugin: https://github.com/unbroken-dome/gradle-testsets-plugin. You can see an example of it being used in dd-trace-ot to specify integration tests. The main downside is the tests wouldn't run automatically with gradle test unless you do something else.

Neat. I'll check it out.

tylerbenson · 2018-04-06T03:27:51Z

 if (JavaVersion.current().isJava8Compatible()) {
+  // java 8 only instrumentation
+  include ':dd-java-agent:instrumentation:play-2.4'
+  include ':dd-java-agent:instrumentation:play-2.4:play-2.6-testing'


I don't like the idea of putting core agent stuff in here...

I want to skip these modules when built by a 1.7 VM because the play dependencies are 1.8 and won't pass the compilation task. Is there a better approach to that?

tylerbenson · 2018-04-06T03:28:52Z

+                    "scala.Option",
+                    "scala.Tuple2",
+                    "scala.concurrent.Future")
+                .and(classLoaderHasClassWithMethod("play.api.mvc.Request", "tags")))


Is this important?

All these classes/methods are used by the instrumentation. The tags method in particular is likely to go away in future play versions, so I want to be sure it's there before I apply the instrumentation.

tylerbenson · 2018-04-06T03:30:08Z

+        @Advice.Argument(0) final Request req,
+        @Advice.Return(readOnly = false) Future<Result> responseFuture) {
+      // more about routes here: https://github.com/playframework/playframework/blob/master/documentation/manual/releases/release26/migration26/Migration26.md
+      final Option pathOption = req.tags().get("ROUTE_PATTERN");


Does this work across 2.4 - 2.6? I thought they had a different API in 2.6.

2.6 has a new api, but they still support the old tags api. Likely this will go away in 2.7.

This reverts commit 2a42f7d.

realark · 2018-04-08T17:32:29Z

@tylerbenson I can't get the testset plugin to work with Java9. play-2.4:compileTestScala fails in CI on Java9. It passes locally, and it passes when run manually over ssh.

I've reverted those changes for now. Any ideas?

tylerbenson · 2018-04-09T15:36:14Z

That's odd... I'm not sure what might be the problem in CI. I'm fine leaving it as is. Just thought that would be a nice way to clean things up.

tylerbenson

This isn't really following the original intent for classLoaderHasClasses and the versionScan plugin, but it'll do.

Andrew Kent added 2 commits March 26, 2018 10:40

Gradle script to use scala in spock

2fce4de

Play Instrumentation

772872b

realark added the inst: others All other instrumentations label Mar 30, 2018

realark requested a review from tylerbenson March 30, 2018 18:14

tylerbenson previously requested changes Apr 2, 2018

View reviewed changes

Play instrumentation cleanup

5f8a834

Simplify error collecting. Use slf4j to log. randomOpenPort util.

realark force-pushed the ark/play branch 4 times, most recently from 41e1553 to 589aed4 Compare April 5, 2018 20:32

realark changed the title ~~Play 2.6 Instrumentation~~ Play 2.4-2.6 Instrumentation Apr 5, 2018

realark force-pushed the ark/play branch from 589aed4 to 72f0370 Compare April 5, 2018 20:57

Play 2.4 and 2.6 testing

2449810

realark force-pushed the ark/play branch from 72f0370 to 2449810 Compare April 5, 2018 21:59

tylerbenson reviewed Apr 6, 2018

View reviewed changes

Andrew Kent added 2 commits April 6, 2018 12:28

Enable play testing for java8+

cb864b1

Move play2.6 tests out of subproject and into sourceSet

2a42f7d

realark force-pushed the ark/play branch from 2018644 to 2a42f7d Compare April 6, 2018 18:03

Revert "Move play2.6 tests out of subproject and into sourceSet"

fc27e6b

This reverts commit 2a42f7d.

tylerbenson approved these changes Apr 9, 2018

View reviewed changes

realark merged commit 63ae144 into master Apr 9, 2018

realark deleted the ark/play branch April 9, 2018 17:39

tylerbenson added this to the 0.7.0 milestone Apr 26, 2018

		@@ -0,0 +1,26 @@
		apply from: "${rootDir}/gradle/java.gradle"
		apply from: "${rootDir}/gradle/test-with-scala.gradle"

		@@ -0,0 +1,27 @@
		apply from: "${rootDir}/gradle/java.gradle"
		apply from: "${rootDir}/gradle/test-with-scala.gradle"

Conversation

realark commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tylerbenson commented Apr 2, 2018

Uh oh!

tylerbenson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

realark commented Apr 3, 2018

Uh oh!

realark commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

realark commented Apr 8, 2018

Uh oh!

tylerbenson commented Apr 9, 2018

Uh oh!

tylerbenson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

realark commented Mar 30, 2018 •

edited

Loading

realark commented Apr 5, 2018 •

edited

Loading