Ratpack instrumentation support by jonmort · Pull Request #276 · DataDog/dd-trace-java

jonmort · 2018-03-30T15:40:49Z

I've been working on this for quite a while and been round and round on it. This is finally ready for some feedback so please provide it.

There are several areas I'm not sure about

Naming of the instrumentation
Component Tag- There are two here, the main handler that implements the http response and the http client
Resource Tag - I've left this out because I couldn't see how to build it well. I also had no idea what it should be
Use of Java 8 Features. Ratpack only supports Java 8 so I used them
Bytebuddy - I've not worked with this before so I tried to copy the patterns I saw. I'm particularly unsure about how the classLoaderHasClasses() calls are supposed to be used
Version plugin in the gradle file - I declared all of the classes that have no compile dependency on them in the gradle file - those classes that are instrumented. This appeared to be what the other classes instrumentations did but I was not sure
Logging - I felt like I wanted to add some more, but there isn't much elsewhere so I didn't
Versions of Ratpack to support and test. I chose 1.4.6+ as the API is good for the integration. In Ratpack 1.6 there will be a much better way to instrument HttpClient, what I have works but it would be much cleaner when HttpClient interceptors are available
Appropriate level of comments - I added a bit more than I found elsewhere, but they aren't extensive

Also, is there a contributors agreement I need to sign?

realark · 2018-03-30T18:53:33Z

@jonmort Awesome work!

I'll get back to you about that contributors agreement and the rest of your questions early next week.

One blocker is this instrumentation is compiled against java 8. We have to support java 1.7, and attempting to load java 8 bytecode crashes the app. We'll put out a solution for this next week so we can unblock this PR and #254.

tylerbenson

Nice job. Certainly more thorough in some aspects than what we normally do, and something for us to aspire to.

I added a lot of questions and comments, mainly for discussion. The main things that need to be resolved are:

Resource Naming - This is rather datadog specific, so let us know if you need more explanation about this.
Proper JVM Handling - You can add the test specific handling as mentioned in the comments, but we either need to change the way the project builds, or change the instrumentation back to JVM 7 compatible bytecode. Do you have any ideas for how we could handle this?

One other question to consider... in the future, if/when we add Netty support, how would we need to change this instrumentation?

tylerbenson · 2018-04-02T03:43:00Z

I don’t think it is safe for advice to reference this method. It should be moved to a utility class that is injected into the class loader.

Oops, yes. Looks sketchy

tylerbenson · 2018-04-02T03:50:52Z

A lot of this is done automatically when setting the error object, so you don’t need to do it again here.

How much of it? I copied this from the AWS implementation

tylerbenson · 2018-04-02T04:04:52Z

We usually name the project after the earliest version supported by the instrumentation. So this should be ratpack-1.4.6. Is this the earliest version the instrumentation can work with?

Earliest tested with, I can try with 1.3 too

tylerbenson · 2018-04-02T04:08:23Z

Nice strategy for sharing the span between classes.

This was inspired from the ratpack tracing for zipkin

tylerbenson · 2018-04-02T04:10:17Z

Since this isn’t the actual span, can you rename consistently to spanRef (like you did in other places) to avoid confusion?

tylerbenson · 2018-04-02T04:46:50Z

No, only 5XX status should be errors.

Can you expand on why that is? 4xx responses are, by definition, also errors...

@jdbevan 4xx errors are generally client side errors and thus not something you'd usually want to page someone for. In certain cases where it is something you consider more critical, you can add a TraceInterceptor to change it.

tylerbenson · 2018-04-02T04:49:36Z

This is not needed. The super class has a tracer you can reference directly that already uses ListWriter.

tylerbenson · 2018-04-02T05:27:26Z

Please add a test that uses path variables.
You should also set the resource name in the instrumentation and assert on it here.

Resource names should be based on the PathBinding which should be available on the context.

I hadn't worked out how to do this when i first looked at it, now I see we can do this beforeSend and pull the path bindings then. I wasn't quite sure what the resource name was in ratpack, but the path binding description appears to be perfectly good

tylerbenson · 2018-04-02T06:56:34Z

If you feel it complicates the tests too much, you can remove this so okhttp doesn’t get instrumented.

I'll take a look. Again I think I copied from servlet

tylerbenson · 2018-04-02T08:25:48Z

This is how we’ve been restricting the tests from running specific JVM versions for Ratpack.
https://github.com/DataDog/dd-trace-java/blob/master/dd-java-agent/instrumentation/okhttp-3/okhttp-3.gradle

This won’t help the need to compile the instrumentation with Java 8 though.
As Andrew said. Let us figure out a strategy for that.

tylerbenson · 2018-04-02T10:58:07Z

Looks like this is something we could potentially do with custom sourceSets: https://stackoverflow.com/a/18191391, though that doesn't mean it will work with the java 7 compiler.

Maybe instead, we could save the artifacts from the java 8 compile, then use them in the java 7 test run. Changing the JVM might invalidate the gradle Up-to-date detection though.

jonmort · 2018-04-04T14:46:32Z

Earliest tested with, I can try with 1.3 too

jonmort · 2018-04-04T14:49:06Z

I need datadog.opentracing.scopemanager.ContextualScopeManager to be able to call addScopeContext

jonmort · 2018-04-04T14:50:45Z

I'll take a look. Again I think I copied from servlet

jonmort · 2018-04-04T14:51:11Z

Oops, yes. Looks sketchy

jonmort · 2018-04-04T14:51:35Z

How much of it? I copied this from the AWS implementation

jonmort · 2018-04-04T14:53:49Z

This was inspired from the ratpack tracing for zipkin

jonmort · 2018-04-04T14:55:35Z

Actually it won't. It will add to anything else already there so when calling execution.getAll it will return all RatpackScopes. I'll modify to clear it out first

jonmort · 2018-04-04T15:43:39Z

I hadn't worked out how to do this when i first looked at it, now I see we can do this beforeSend and pull the path bindings then. I wasn't quite sure what the resource name was in ratpack, but the path binding description appears to be perfectly good

jonmort · 2018-04-04T15:58:25Z

@tylerbenson is this span name ok?

jonmort · 2018-04-04T16:42:54Z

I have addressed all the comments. I'm waiting on a solution for Java 7. I realised there was a Java 8 method reference in the instrumentation which i have replaced with a new class to ensure Java 7 byte code compatibility for the instrumentation class. The other classes have significant use of Java 8 language features.

Regarding Netty, I think it might be possible to integrate this instrumentation, but mostly when using Ratpack you don't want Netty details as the abstractions Ratpack provides are more functional

tylerbenson

Please see new comments below.

As for the Java 7 issue, we are looking into how to fix that. Thanks for your patience.

tylerbenson · 2018-04-05T01:35:32Z

Sorry I had you move this to a utility class... This whole method can be replaced with a simple call to:
span.log(Collections.singletonMap(ERROR_OBJECT, throwable));

We do all the rest inside the tracer.

tylerbenson · 2018-04-05T01:37:55Z

I think ratpack.request would be better.

tylerbenson · 2018-04-05T05:49:55Z

@jdbevan 4xx errors are generally client side errors and thus not something you'd usually want to page someone for. In certain cases where it is something you consider more critical, you can add a TraceInterceptor to change it.

tylerbenson · 2018-04-05T05:56:50Z

The reason is spans are added to the front of the list as they complete: https://github.com/DataDog/dd-trace-java/blob/master/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java#L87
This means the first span is usually the root span.

tylerbenson · 2018-04-05T05:57:44Z

Any idea in what cases this would happen?

This will happen when there is no handler bound to the path (I think the response will be 404 or 405).

tylerbenson · 2018-04-05T06:01:40Z

Sorry I didn't mention this before, but the resource name should ideally be prefixed with the HTTP verb, so in this case, the resource name would be GET /:foo/:bar?/baz. Is that possible?

Yes that makes sense. I'm also missing the leading slash. I'll put that in too

tylerbenson · 2018-04-05T06:06:22Z

These classes should mirror what is listed in the versionScan.verifyPresent block. This is how we ensure we don't try to instrument something in 1.3

tylerbenson · 2018-04-05T06:07:31Z

I'm ok restricting it to 1.4 as long as it doesn't try to inadvertently instrument any unsupported classes. We ensure this by including the below verifyPresent classes in the classLoaderHasClasses list in the instrumentation.

So this is where it gets complex! The reason 1.3 isn't supported is because the interfaces have different methods, they are the same classes but method signatures changed. Specifically it is the RequestSpec that has the changes. I could rely on a class that is only in 1.4 to force this, but one that isn't instrumented

Yeah, that is ok. The classes in that list aren't the ones we're explicitly instrumenting. They're intended as a "fingerprint" to identify the supported versions.

I've sorted out the fingerprint now. It is pretty much just that method on that class that identifies 1.4 from 1.3

tylerbenson · 2018-04-05T06:13:14Z

Please re-order alphabetically.

jonmort · 2018-04-05T14:17:00Z

This method (getDescription) is only available on Ratpack 1.4+. I don't instrument PathBinding but do the getDescription method on it. I couln't see a way to express that in the agent builder. Any suggestions?

The only classes in this list should be the ones that are unique to version 1.4+. In some cases, just class name isn't enough to uniquely identify. That's why we also have the optional method name in there.

jonmort · 2018-04-06T13:08:26Z

@tylerbenson I went out on a limb here and added a way to check for a class that has a particular method as I could not see a way to do it otherwise. I looked at using classLoaderHasClassWithField but it felt too fragile to use on a Ratpack internal class.

jonmort · 2018-04-06T14:12:59Z

@tylerbenson this is where I use the new ClassLoader Matcher, which gets reused for all the instrumentation

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

@jonmort 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.

jonmort · 2018-04-10T07:11:11Z

I notice that ClassLoaderMatcher just got updated to add the same functionality that I did, but with one key difference. The method matcher that was added uses Classes to pick out the method arg and mine uses Strings. I used Strings because if we are checking for the presence of a method with a signature as a fingerprint for versions the parameter classes may not be loaded when the check is done. In short I don't think it is safe to use Class in this instance

tylerbenson · 2018-04-10T15:41:53Z

Good point... @realark you may want to reconsider this^

realark · 2018-04-11T20:27:58Z

Thanks @jonmort That's a good point. I'm adding that fix to my next play pr.

jonmort · 2018-04-18T11:55:26Z

@tylerbenson I've isolated the Java 8 code now and we have green builds. Is there anything further that needs doing here?

tylerbenson

I think this looks great. You did a nice job and resolved all the comments I had. I'm ok merging this as-is. I left a few questions, but nothing blocking. Sorry for the delay in review. Let me know if you have anything else you want to do, otherwise I'll merge it.

tylerbenson · 2018-04-24T02:59:51Z

+Here we introduce a sourceSet for the java 8 code which needs to be compiled with a source and target of 1.8
+The instrumentation classes must be compiled with java 7 and do nothing when ratpack is not on the classpath. The
+java 8 classes are used lazily so there is no direct linking between the 1.7 and 1.8 bytecode.
+*/


Thanks for the nice comment.

tylerbenson · 2018-04-24T03:12:48Z

+
+    @Override
+    public void execute(RequestSpec requestSpec) throws Exception {
+      requestAction.execute(


Do we need any logic to prevent double-wrapping here?

The scenario in which requestSpec could have already been wrapped happens only if the client has some fancy logic to re-use requests, or chain the actions, this is quite unlikely but possible so the wise thing would be to guard against double wrapping

tylerbenson · 2018-04-24T03:32:56Z

+    /*
+    This test is somewhat convoluted and it raises some questions about how this is supposed to work
+     */
+    setup:


Do you think there are potential problems that still need to be resolved?

I should probably remove this comment. In a previous iteration there was some more setup code.

My concern about this test is the length of it. It has close to 100 lines of assertions but I think each assertion is necessary and I'd rather verbose tests than hiding them in a helper method

tylerbenson · 2018-04-24T13:26:46Z

Ready to merge now?

jonmort · 2018-04-24T14:03:04Z

Yes ready to merge

tylerbenson suggested changes Apr 2, 2018

View reviewed changes

jonmort commented Apr 4, 2018

View reviewed changes

jonmort force-pushed the ratpack-support branch from 84bce7e to 54d70b1 Compare April 4, 2018 16:21

tylerbenson suggested changes Apr 5, 2018

View reviewed changes

jonmort commented Apr 5, 2018

View reviewed changes

jonmort force-pushed the ratpack-support branch from 3170fb8 to fa1d057 Compare April 6, 2018 09:48

jonmort commented Apr 6, 2018

View reviewed changes

jonmort changed the title ~~Ratpack instrumentaiton support~~ Ratpack instrumentation support Apr 9, 2018

jonmort force-pushed the ratpack-support branch 2 times, most recently from c080a1a to 1d2f28b Compare April 18, 2018 10:36

jonmort added 7 commits April 18, 2018 11:42

Ratpack instrumentaiton support

6e92221

Apply review comments

6c1c830

Rename to Ratpack 1.4

582c0af

Resource naming and other review comments

1ba0b69

Correctly look for classes to determine minimum ratpack version

18da244

Only build ratpack when using a java 8 compiler

4875165

Introduce a java 8 source set for Ratpack integration

fa90af7

jonmort force-pushed the ratpack-support branch from 1d2f28b to fa90af7 Compare April 18, 2018 10:44

tylerbenson approved these changes Apr 24, 2018

View reviewed changes

jonmort added 2 commits April 24, 2018 08:07

Add a guard against wrapping the ratpack request action multiple times

b4510b7

Tidy up RatpackTest comments

35ad8c9

realark merged commit f295b73 into DataDog:master Apr 24, 2018

tylerbenson added this to the 0.7.0 milestone Apr 26, 2018

Conversation

jonmort commented Mar 30, 2018

Uh oh!

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

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

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 Apr 2, 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!

jonmort commented Apr 4, 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!

tylerbenson Apr 2, 2018 •

edited

Loading

tylerbenson commented Apr 2, 2018 •

edited

Loading