Spark Java Framework Instrumentation by JorgenG · Pull Request #254 · DataDog/dd-trace-java

JorgenG · 2018-03-06T09:30:30Z

This is a WIP of looking into sparkjava support.

tylerbenson · 2018-03-07T05:24:28Z

This looks wrong...

tylerbenson · 2018-03-07T05:54:25Z

You're also going to want to add instrumentation for spark specifically to set the resource name for a request. This is similar to what I'm doing in the spring web instrumentation. So for example, on this line, the resource name should be GET /param/:param, regardless of what the parameter is.

Unfortunately Spark doesn't seem to be a great spot for this. Maybe getTarget()? When it is called, get the matchUri value? That doesn't have the HTTP method though.

Other ideas?

tylerbenson · 2018-03-07T05:55:55Z

This doesn't need to be copied in both places. I'd suggest removing this class.

tylerbenson · 2018-03-07T05:56:52Z

Instead say, sparkjava uses servlet filters under the covers, so we want to include that instrumentation here for verification.

tylerbenson · 2018-03-07T05:58:17Z

Remove these lines and replace with a dependency on sparkjava (the earliest version the instrumentation supports), then rename the project to match sparkjava-<version>.

tylerbenson · 2018-03-07T05:59:10Z

Rename class to match.

tylerbenson · 2018-03-07T06:22:06Z

While I don't love it, it does seem this is a best way to generically cover what you need. The other option might be to instrument Jetty's Handler.handle interface. Which do you think is better?

Looking at the Jetty handler interface, I like that a lot more. I think that will fit better and have less edge cases like the Filter has.

Should that go in a seperate package "jetty"?

tylerbenson · 2018-03-07T06:25:38Z

Since this span could come from a potentially large number of classes, I'd like to have something to trace it back to the origin. Please add a tag with the class name like so:

.withTag("span.origin.type", statement.getClass().getName());

Would you mind also adding that to the other servlet-{2,3} instrumentation as well?
Thanks!

What is statement here? Cannot find it, and nothing logically matches.

Sorry that was copy paste from the statement instrumentation. For jetty instrumentation it would be this.getClass().

tylerbenson · 2018-03-07T06:28:38Z

once you have the instrumentation working, I'd like to see this test suite include deeper verification and some edge cases. Use this for inspiration.

tylerbenson · 2018-03-07T06:38:04Z

Also, it appears spark dropped support for Java 7 in 2.0, so if we don't plan on supporting anything before then, you can exclude the tests from running on java 7 by doing this, except change the check to JavaVersion.current().isJava8Compatible().

JorgenG · 2018-03-07T06:54:53Z

Thanks for the good feedback. I do see that I have messed up a bit, so I want to clear some things up.

I had the impression that blindly adding instrumentation for Filter3 would not be a good solution. So my original intention was to not have Filter3Instrumentation under servlet-3 (Although I managed to commit it).

Does my view match your understanding? Or would having the Filter3Instrumentation be correct at that location, and then add spark specific instrumentation on spark-specific classes?

Right now there are two "identical" files.

EDIT: saw your specific comment about this, will remove!

JorgenG · 2018-03-07T07:32:20Z

What are all these opentracing things used for?

JorgenG · 2018-03-07T07:48:24Z

I have made some changes now.

Removed Filter3Instrumentation
Added HandlerInstrumentation for Jetty
Added RoutesInstrumentation for enriching with match url from SparkJava

Once the overall structure is ok, I'll take another look at the tests etc.

JorgenG · 2018-03-07T08:42:09Z

I have attempted to use google java format in the project, but the verification step still fails. Any tips?

tylerbenson · 2018-03-07T10:42:50Z

There’s a gradle task you can run to format everything. I think it’s googleFormatJava or something like that.

tylerbenson

I am ok with the approach. Needs some polish and more testing on both the jetty and spark side. Let me know if you get stuck. Thanks for working through this. I hope it’s interesting for you.

tylerbenson · 2018-03-07T10:54:53Z

This part is a bit confusing and poorly documented. We use this to make sure we are only instrumenting classes we expect it to work on. Please update the group and module for jetty. You can remove the legacy part.

I think this jetty hook requires the Async things from servlet?

tylerbenson · 2018-03-07T10:59:46Z

We might still want to check if there is an active trace and return early to avoid duplicate traces if the existing servlet instrumentation applies.

tylerbenson · 2018-03-07T11:01:23Z

Change to isJava8Compatible()

tylerbenson · 2018-03-07T11:06:50Z

The main reason for adding this line is to limit the versions of the sparkjava library we try to instrument. This list should match the verifyPresent block in a versionScan in the gradle file.

Added version scan for sparkjava as well.

tylerbenson · 2018-03-07T11:11:07Z

Standard practice. Since this is new instrumentation, can you override the default enabled method to return false? We will change to enabled by default in a future release. Same applies to the spark instrumentation.

tylerbenson · 2018-03-07T11:15:15Z

Any reason this instrumentation wouldn’t work with older versions of jetty? We usually name the instrumentation with the earliest version it can support.

Added support for oldest 8.0.0 where signature is the same for Handler.

JorgenG · 2018-03-07T11:53:51Z

I will work on some tests later this week.

tylerbenson · 2018-03-08T02:39:39Z

This version range should match the version for the instrumentation, so in this case [ 8.0.0.v20110901,).

tylerbenson · 2018-03-08T02:40:15Z

This version range should match the version for the instrumentation, so in this case [ 2.5,).

tylerbenson · 2018-03-08T02:41:16Z

These classes are probably not what you need, unless 8.0 was the first version to support Servlet 3.

Looks like that is in fact the case: https://wiki.eclipse.org/Jetty/Starting/Jetty_Version_Comparison_Table. Carry on.

tylerbenson · 2018-03-08T02:46:08Z

Was this class newly added in 2.5 and available in all following versions?

tylerbenson · 2018-03-09T04:53:07Z

I think I'd like to see this as a setupSpec() method with a corresponding cleanupSpec() method to shut it down. (Is it possible to shut down programmatically?)

tylerbenson · 2018-03-09T04:57:01Z

I think this would look nicer if span.context() was declared as a variable and maybe even the tags.

Partially fixed by extracting to a variable.

tylerbenson · 2018-03-09T04:58:54Z

After a clean build, I think this is mostly ready. I'm going to pass over to @realark for secondary review.

JorgenG · 2018-03-09T11:30:08Z

My problem right now is that the basic test does not work. I think this is because my understanding of how to activate the instrumentation is wrong.

Since the tests are failing, we can't verify if it works. Since defaultEnabled() returns false, how will it be an active instrumentation in test setting?

JorgenG · 2018-03-09T11:50:23Z

In the latest commit, we can observe that Spark is started in the tests, however no traces are registered, thus our tests don't work. How can we activate tracing for the default disabled instrumentation?

realark · 2018-03-09T21:24:43Z

Looks like this instrumentation is only intended to apply to Java 8 and above, correct? If so, I think we have a problem. Attempting to load a 1.8 class on a 1.7 jvm, will cause an exception to be thrown when we initialize the instrumentation.

@JorgenG There's nothing for you to do about this (unless you can change source and target compatibility to 1.7?). @tylerbenson and I will have to figure out a solution. I'm sure we'll want to do this often, especially as more frameworks are compiled against newer jvms.

Maybe we could configure the tests to be Java 8 but leave the instrumentation at Java 7?

realark · 2018-03-09T21:28:45Z

@JorgenG You can enable the instrumentation in the test using a static initializer to set the system property.

Kafka example: https://github.com/DataDog/dd-trace-java/blob/master/dd-java-agent/instrumentation/kafka-clients-0.11/src/test/groovy/KafkaClientTest.groovy#L23-L25

I think you'll want something like this:

  static {
    System.setProperty("dd.integration.sparkjava.enabled", "true")
  }

realark

I think the PR looks good once the tests are enabled.

I'm good to merge after we figure out the java 1.7 issue.

JorgenG · 2018-03-13T09:51:09Z

Attempted to add the enabling of instrumentation in tests, not able to get it working. I am leaving for two weeks vacation on thursday, and will be back at beginning of April. Will not be able to do any contributions during this time, so if you are able to provide support on getting the tests running that would be nice.

realark · 2018-04-06T17:46:18Z

@JorgenG Thanks for your patience while we work on unblocking you. We're still working on a solution for Java 8 instrumentation, but we'll let you know as soon as we can get this merged.

…. add sparkjava routematch instrumentation

…being loaded properly

and rebase off master.

realark · 2018-04-26T19:28:51Z

 import java.lang.reflect.Field

-@Timeout(1)
+@Timeout(10)


realark · 2018-04-26T19:33:16Z

Looks good!

tylerbenson · 2018-04-26T21:36:31Z

@JorgenG Thanks for your contribution. Glad we could finally get this merged. Sorry for the delay.

JorgenG · 2018-04-27T06:50:31Z

Thanks for all the help, and happy to see it getting in. 👍

tylerbenson reviewed Mar 7, 2018

View reviewed changes

JorgenG commented Mar 7, 2018

View reviewed changes

tylerbenson reviewed Mar 7, 2018

View reviewed changes

tylerbenson reviewed Mar 8, 2018

View reviewed changes

tylerbenson changed the title ~~WIP: fix: first attempt at sparkjava trace instrumentation~~ Spark Java Framework Instrumentation Mar 8, 2018

tylerbenson reviewed Mar 8, 2018

View reviewed changes

tylerbenson reviewed Mar 9, 2018

View reviewed changes

tylerbenson requested a review from realark March 9, 2018 04:59

realark reviewed Mar 9, 2018

View reviewed changes

realark suggested changes Mar 9, 2018

View reviewed changes

realark mentioned this pull request Mar 30, 2018

Ratpack instrumentation support #276

Merged

Jørgen Grøndal added 4 commits April 26, 2018 14:53

fix: first attempt at sparkjava trace instrumentation

91aa3c8

fix: support java 7 in test

6c4b909

fix: add jetty handler instrumentaiton. remove filter3instrumentation…

c49d4f6

…. add sparkjava routematch instrumentation

fix: use correct jetty version

79f1226

Jørgen Grøndal added 8 commits April 26, 2018 14:53

fix: use google java format

593a9b7

fix: formatting and add version to spark java instrumentation

8bfeae4

fix: support jetty 8+, sparkjava 2.5+

d0822c7

fix: add versionScan for sparkjava

8b289c7

feat: find oldest supported spark, update version, make tests run

7e233ac

feat: get tests running, although failing due to instrumentation not …

ede4620

…being loaded properly

fix: create a test to verify that spark is indeed responding

bb5c413

fix: attempt to enable instrumentations

de350c7

tylerbenson added this to the 0.7.0 milestone Apr 26, 2018

Fix spark instrumentation

3db2d65

and rebase off master.

tylerbenson added inst: others All other instrumentations tag: community Community contribution labels Apr 26, 2018

realark reviewed Apr 26, 2018

View reviewed changes

realark approved these changes Apr 26, 2018

View reviewed changes

tylerbenson merged commit fc41529 into DataDog:master Apr 26, 2018

JorgenG deleted the feat/add-sparkjava-instrumentation branch April 27, 2018 06:50

Conversation

JorgenG commented Mar 6, 2018

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!

tylerbenson commented Mar 7, 2018

Uh oh!

JorgenG commented Mar 7, 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!

JorgenG commented Mar 7, 2018

Uh oh!

JorgenG commented Mar 7, 2018

Uh oh!

tylerbenson commented Mar 7, 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!

JorgenG commented Mar 7, 2018

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

JorgenG commented Mar 7, 2018 •

edited

Loading