Introduce ContextualScopeManager by tylerbenson · Pull Request #233 · DataDog/dd-trace-java

tylerbenson · 2018-02-16T12:14:05Z

First pass design on having multiple kinds of ScopeManagers. Focus on naming and API design. See test for API usage... Still requires casting to some degree.
(Continuing from #223.)

Opening now so you have a chance to review today/tomorrow.

realark · 2018-02-16T21:36:59Z

+
+public class ContextualScopeManager implements ScopeManager {
+  final ThreadLocal<Scope> tlsScope = new ThreadLocal<>();
+  final Set<ScopeContext> scopeContexts = new CopyOnWriteArraySet<>();


Because we're iterating a list, ScopeContexts which were added earlier will have a chance to activate before later added ones. Do you think this is the correct behavior or should we consider using a stack based approach (i.e. give the most recently added scope managers a chance to activate first)?

I thought about that... you're suggesting essentially reverse the order of iteration?

Yes. I don't have a strong feeling about it, but that was my initial expectation.

Can you think of a use case where that difference would be important? I'm open to the idea... this was just an area of the design I hadn't fully explored the implications of.

My 2c, a predictable iteration order would be preferable when debugging something like this. In the Ratpack case, we might be integrating an external library such as rxJava and want to be providing a different scope in this case. Order of addition would be as good as any to determine the order. Stack ordering is most natural to me.

With the approach as here each ScopeContext might need to be aware of other ScopeContexts to deliver the correct scope (I can't think of a concrete case so I may be wrong).

I've replaced it with a Deque.

realark

Looks good. I just have one comment.

tylerbenson · 2018-02-16T23:28:37Z

@jonmort would you also mind taking a look? Think this would cover your use-case?

jonmort

This looks to meet my use case for instrumenting Ratpack. I've not tried it (and I won't get to for at least a week as I'm off on vacation) but the approach looks flexible enough to accommodate all of the async cases I can think of. Nice work.

jonmort · 2018-02-18T20:27:10Z

+
+public class ContextualScopeManager implements ScopeManager {
+  final ThreadLocal<Scope> tlsScope = new ThreadLocal<>();
+  final Set<ScopeContext> scopeContexts = new CopyOnWriteArraySet<>();


My 2c, a predictable iteration order would be preferable when debugging something like this. In the Ratpack case, we might be integrating an external library such as rxJava and want to be providing a different scope in this case. Order of addition would be as good as any to determine the order. Stack ordering is most natural to me.

With the approach as here each ScopeContext might need to be aware of other ScopeContexts to deliver the correct scope (I can't think of a concrete case so I may be wrong).

jonmort · 2018-02-18T20:31:22Z

+      }
+    }
+    if (span instanceof DDSpan && ((DDSpan) span).context().useRefCounting) {
+      return new RefCountingScope(this, new AtomicInteger(1), span);


The behaviour here appears to be different for the RefCountingScope with respect to finishOnClose. From what I see calling close() on RefCountingScope will always finish the scope.

I've combined the two into a single Scope type. Now the span won't finish unless both ref counter is 0 and the boolean is true.

tylerbenson · 2018-02-21T06:08:07Z

@realark @jonmort I resolved both of your concerns by replacing the CopyOnWriteArraySet with ConcurrentLinkedDeque. Priority is now given to the last added.

Also added additional tests.

Rather than it just be a list of spans… This allows for tracking of if the trace was reported or not, and in the future to do so independently of finishing the span.

Wait until all spans are finished or garbage collected before reporting trace.

If all the current spans on a trace complete before a continuation can be activated, previously the trace could have been reported, not allowing additional spans to be added. This PR changes it so any created continuations must be dereferenced (GC’d) before allowing the trace to be reported.

to allow a completed trace to be reported more timely. Continuation is closed when the returned scope is closed, but can also be closed directly.

Still doesn’t work with Spring Boot because the way they structure their Jars.

To prevent errors on injected classes.

There've been quite a bit of changes since last review.

tylerbenson · 2018-03-09T04:47:25Z

@palazzem @realark do we think this is ready to merge into master?

realark · 2018-03-09T18:09:04Z

@tylerbenson Not yet. I'd like to get #255 merged in first.

We expect customers to be using these also, so we can’t change them.

scala instrumentation

Implement the TraceInterceptor API

tylerbenson added type: enhancement Enhancements and improvements tag: do not merge Do not merge changes type: feature request tag: community Community contribution labels Feb 16, 2018

tylerbenson requested review from palazzem and realark February 16, 2018 12:14

tylerbenson force-pushed the configurable-scope-manager branch from 7d67333 to cc31bbe Compare February 16, 2018 12:21

realark reviewed Feb 16, 2018

View reviewed changes

realark previously approved these changes Feb 16, 2018

View reviewed changes

jonmort reviewed Feb 18, 2018

View reviewed changes

tylerbenson mentioned this pull request Feb 20, 2018

Resteasy support #234

Closed

tylerbenson force-pushed the configurable-scope-manager branch 3 times, most recently from ca8f949 to fd98cc1 Compare February 27, 2018 06:35

tylerbenson added this to the 0.5.0 milestone Feb 28, 2018

tylerbenson force-pushed the configurable-scope-manager branch from eebc935 to b52a939 Compare March 2, 2018 02:31

jonmort and others added 12 commits March 9, 2018 14:31

Closes #222: Introduce a ScopeManager property

f78c290

Introduce ScopeContext and ReferenceCountingScopes

27c1d71

Combine Threadlocal and RefCounting into single Scope

9b1d454

Also added additional tests.

Improve threadsafety and organization

8da3a04

Extract a trace out to a specific class

5d1297f

Rather than it just be a list of spans… This allows for tracking of if the trace was reported or not, and in the future to do so independently of finishing the span.

Remove deprecated usage.

f31632b

Move SpanFactory to correct package.

5cbc114

Decouple trace flush from collection

c99f10c

Wait until all spans are finished or garbage collected before reporting trace.

Rename to PendingTrace

5c3e4c9

Fix tests

087b2e7

Allow closing of continuation explicitly

f57faba

to allow a completed trace to be reported more timely. Continuation is closed when the returned scope is closed, but can also be closed directly.

Enable bootstrap instrumentation and helper injection.

4fffb61

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

tylerbenson added 3 commits March 9, 2018 14:32

Initial pass at TraceInterceptor API

d073cd8

Add instrumentation to catch additional classloaders.

286e9d2

Still doesn’t work with Spring Boot because the way they structure their Jars.

Wrap super/parent type matchers in failSafe

fb74c7b

To prevent errors on injected classes.

Andrew Kent and others added 10 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

Assert on executor span relationships

45aff57

Remove API jar relocation

58c0dfa

We expect customers to be using these also, so we can’t change them.

Rename ContextPropagator to TraceScope

428e304

Merge pull request #255 from DataDog/ark/scala_instrumentation

34a8c0e

scala instrumentation

Merge pull request #253 from DataDog/tyler/traceinterceptor

0fbec69

Implement the TraceInterceptor API

realark self-requested a review March 12, 2018 22:36

realark approved these changes Mar 12, 2018

View reviewed changes

realark merged commit 054ef0f into master Mar 12, 2018

realark deleted the configurable-scope-manager branch March 12, 2018 22:38

realark removed this from the 0.5.0 milestone Mar 13, 2018

Conversation

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

realark left a comment

Choose a reason for hiding this comment

Uh oh!

tylerbenson commented Feb 16, 2018

Uh oh!

jonmort 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!

tylerbenson commented Feb 21, 2018

Uh oh!

tylerbenson commented Mar 9, 2018

Uh oh!

realark commented Mar 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tylerbenson commented Feb 16, 2018 •

edited

Loading