scala instrumentation by realark · Pull Request #255 · DataDog/dd-trace-java

realark · 2018-03-06T20:17:55Z

(merging to feature branch)

Propagate traces alongscala.concurrent.* classes (Future, Promise, Map) by instrumenting Java executors.

ExecutorInstrumentation: Provides async context propagation by wrapping calls to submit and execute.
FutureInstrumentation: Cancels jobs which are removed from executors. This is an optimization to allow the trace to report without waiting for GC to collect the continuation reference.

As a precaution, the instrumented executors/futures are limited to a whitelist of tested implementation classes. In the future we may want to remove this whitelist and instrument all executors.

tylerbenson

Lots of questions and comments. If possible I'd really like to see the tests moved out of ittests. I also don't like the new dependency for dd-trace-api.

(Sorry if I got carried away.)

tylerbenson · 2018-03-06T22:26:22Z

Can you move these tests to the java-concurrent instrumentation, since isn't that what it depends on?

Since the tests use @Trace you can add a testCompile dependency on the trace annotation instrumentation as well.

I originally did that, but ran into a problem. Bootstrap instrumentation is incompatible with our current test api.

My plan is to fix that as part of the akka tests, then move these tests over. Any objections to leaving them in the integration tests for a little while?

I guess that's fine.

tylerbenson · 2018-03-06T22:28:42Z

Eew... are these sleeps necessary?
I would much rather use something like a Phaser than have a bunch of sleeps...

tylerbenson · 2018-03-06T22:31:53Z

This is a java class in the groovy folder. Can you move it over or make it a groovy class?

tylerbenson · 2018-03-07T00:27:32Z

Try using a Phaser instead.

tylerbenson · 2018-03-07T00:33:27Z

Do you care about the ordering or the hierarchy of the trace? Might be good to assert the parental relationship, not just the count.

tylerbenson · 2018-03-07T01:24:16Z

I'm curious... what's the reason to use a HashSet here? If the type matters, maybe you should inspect the type of Collection to see if it's a List or a Set and choose the one that matches.

No particular reason for picking HashSet. I just need a collection to pass to the executor. A list would work fine also. I don't need to preserve the type of tasks.

tylerbenson · 2018-03-07T01:25:40Z

There are several cases above where you don't wrap the job. Maybe do a type check before casting?

Everything from @Advice.Enter should be a DatadogWrapper at this point (assuming wrappedJobs is non-null). Did I miss something?

tylerbenson · 2018-03-07T01:31:22Z

Maybe consider adding the close() method for canceling?

tylerbenson · 2018-03-07T01:36:05Z

Can this be moved to the module instead of being in ittests, or does the bootstrap class issue prevent that?

tylerbenson · 2018-03-07T01:36:55Z

tylerbenson · 2018-03-07T01:43:18Z

Do you have a feel for what versions of scala this would support?

realark · 2018-03-07T18:56:50Z

I've tested against Scala 2.11 and 2.12.

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

I don't think this is used anymore...

tylerbenson · 2018-03-07T23:04:24Z

Any time I see us instrumenting an anonymous class like this, I expect it to be very fragile across versions.

realark · 2018-03-09T20:57:39Z

@tylerbenson Changes pushed. Notably, I removed the opentracing dependency from dd-trace-api.

Please review new changes.

tylerbenson · 2018-03-12T02:13:16Z

+    testWriter.size() == 1
+    trace.size() == 2
+    trace.get(0).operationName == "parent"
+    trace.get(1).operationName == "asyncChild"


Minor, but maybe assert that child's parentId is parent's spanId?

tylerbenson · 2018-03-12T02:19:59Z

+package datadog.trace.context;
+
+/** An object when can propagate a datadog trace across multiple threads. */
+public interface ContextPropagator {


Thoughts on just renaming this to Scope or TraceScope?

tylerbenson

Only question is if we want to change the name of ContextPropagator.

realark · 2018-03-12T17:43:21Z

Renamed ContextPropagator to TraceScope.

realark added the tag: do not merge Do not merge changes label Mar 6, 2018

realark force-pushed the ark/scala_instrumentation branch 6 times, most recently from 20aef1c to 3f6bf62 Compare March 6, 2018 23:29

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

tylerbenson previously requested changes Mar 7, 2018

View reviewed changes

realark force-pushed the ark/scala_instrumentation branch from 3f6bf62 to dfe267e Compare March 7, 2018 21:29

realark removed the tag: do not merge Do not merge changes label Mar 7, 2018

tylerbenson reviewed Mar 7, 2018

View reviewed changes

tylerbenson force-pushed the configurable-scope-manager branch from 13c354a to 4fffb61 Compare March 9, 2018 04:32

realark mentioned this pull request Mar 9, 2018

Introduce ContextualScopeManager #233

Merged

Andrew Kent added 5 commits March 9, 2018 11:50

Executor Instrumentation for Scala

9a83413

Whitelist Executor Instrumentation

4a11fc6

Disable async instrumentation by default.

81b70fb

Test propagation across non-tracing contexts

4bebce2

Remove opentracing from dd-trace-api

2118053

realark force-pushed the ark/scala_instrumentation branch 2 times, most recently from 90111f5 to 7f57924 Compare March 9, 2018 20:54

Assert on executor span relationships

45aff57

realark force-pushed the ark/scala_instrumentation branch from 7f57924 to 45aff57 Compare March 9, 2018 20:59

tylerbenson reviewed Mar 12, 2018

View reviewed changes

tylerbenson approved these changes Mar 12, 2018

View reviewed changes

Rename ContextPropagator to TraceScope

428e304

realark force-pushed the ark/scala_instrumentation branch from c8d961a to 428e304 Compare March 12, 2018 17:29

realark merged commit 34a8c0e into configurable-scope-manager Mar 12, 2018

realark deleted the ark/scala_instrumentation branch March 12, 2018 17:43

realark added this to the 0.5.0 milestone Mar 13, 2018

Conversation

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

Uh oh!

Uh oh!

tylerbenson left a comment • 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!

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

tylerbenson commented Mar 7, 2018

Uh oh!

realark commented Mar 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

realark commented Mar 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylerbenson left a comment

Choose a reason for hiding this comment

Uh oh!

realark commented Mar 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

realark commented Mar 6, 2018 •

edited

Loading

tylerbenson left a comment •

edited

Loading

realark Mar 7, 2018 •

edited

Loading