ScopeManager #113

pauldraper · 2018-08-19T15:58:28Z

Addresses #112

See https://github.com/opentracing/specification/blob/f7ca62c9/rfc/scope_manager.md

Continuation passing style

I first tried adhering to the draft RFC (somewhat working code), but it got clumsy, particularly for the Zone.js implementation. Just about every JS continuation library uses continuation passing style (Zone.js, Node.js domains, continuation-local-storage, etc.), and I believe it's the correct choice. Plus, the existing RFC IMO has a lot of ambiguity for the implementer. I added my findings to opentracing/specification#126.

Support

Unfortunately, there just isn't a great universal, out-of-the-box CLS solution.

There are two things that can be addressed by CLS: native async syntax (async/await), and common APIs (e.g. fs.readFile).

The are a surprising number of issues with current solutions. https://github.com/othiym23/node-continuation-local-storage is widely used, but can't be used with async/await which in 2018 is increasingly common. https://github.com/gms1/node-async-context can be, but (so far) doesn't have a lot of adoption and requires Node 8+. https://github.com/angular/zone.js/ has related TC39 proposal, but it's been stalled for two years.

In the end, the JS community has not figured out CLS (yet). I added a few implementations with very minimal dependencies that between them have fairly broad coverage. (See README.md for details.)

Implementations:

AsyncHookSpanManager - uses Node.js async_hooks
AsyncWrapSpanManager - uses async-hook-jl which uses Node.js AsyncWrap
ZoneSpanManager - uses Zone.js

In the future, I expect the situation to improve. But this seems a good start, these wrappers are rather simple (~50 lines each).

coveralls · 2018-08-19T16:18:04Z

Coverage increased (+1.5%) to 90.019% when pulling 71e124f on rivethealth:pauldraper/scope-manager into 0f14554 on opentracing:master.

See https://github.com/opentracing/specification/blob/f7ca62c9/rfc/scope_manager.md However, use continuation passing style which better fits JS and available APIs. Implementations: * AsyncHookSpanManager - uses Node.js async_hooks * AsyncWrapSpanManager - uses async-hook-jl which uses Node.js AsyncWrap * ZoneSpanManager - uses Zone.js

pauldraper · 2018-08-19T23:07:36Z

Update: looks like https://github.com/RisingStack/jaeger-node and https://github.com/RisingStack/opentracing-auto use async_hooks. I believe that would be the most common method of CLS, since if you use Node.js and a recent version of it, it has the fewest drawbacks.

rmfitzpatrick · 2018-10-09T17:44:55Z

src/tracer.ts

@@ -88,6 +144,11 @@ export class Tracer {
                options.references = [childOf];
            }
            delete(options.childOf);
+        } else if (!options.references) {


Java and Python have an "ignore active span" option to prevent this automatic reference. Any chance you'd want to follow suite?

Yes, this should

rochdev · 2018-10-29T16:42:09Z

README.md

+```
+
+JavaScript support for continuation local storage is a patchwork.
+This library includes simple wrappers around a few common APIs for continuation contexts.


This should be selected automatically for the user according to the environment. This will avoid the user needing extensive knowledge of all the possible ways to do context propagation in JavaScript.

I've mentioned how I think this should be selected at the bottom of #112 (comment)

rochdev · 2018-10-29T16:42:50Z

README.md

+        tracer.activeSpan() // span
+    })();
+});
+tracer.activeSpan() // null


Should these be on tracer.scopeManager() instead?

rochdev · 2018-10-29T16:44:33Z

README.md

+new AsyncHookSpanManager();
+```
+
+**AsyncWrapSpanManager**


Since AsyncWrap cannot have multiple hooks, and has many bugs, possible crashes and memory leaks in older Node versions, I would probably skip it entirely and add something based on a fixed fork of async-listener as described in #112 (comment)

FWIW, in my experience it's also slower than async-listener.

rochdev · 2018-10-29T17:00:48Z

src/tracer.ts

+     * @return {A} the return value of the executed function.
+     * @template A
+     */
+    async runSpanAsync<A>(name: string, options: SpanOptions, f: () => Promise<A>): Promise<A> {


Not sure I understand why this is necessary as await can be used on any promise which can be returned by runSpan as well.

It's necessary if you want to stop the span once the promise is resolved.

Why not simply detect if the returned value is a promise in runSpan and handle it there?

rochdev · 2018-10-29T17:01:47Z

src/tracer.ts

+     * @return {A} the return value of the executed function.
+     * @template A
+     */
+    runSpan<A>(name: string, options: SpanOptions, f: () => A): A {


This name sounds a bit weird to me, as this method doesn't run a span.

Other suggestion?

It starts and stops (runs?) the span.
It runs the function, with a new active span for its duration.

I think I would actually remove it completely for the first version, to keep the API surface as small as possible. Then more helpers can be added over time if necessary and discussed individually.

rochdev · 2018-11-28T21:48:02Z

src/tracer.ts

+     *
+     * @return {SpanManager} - the span manager, which may be a noop.
+     */
+    spanManager(): SpanManager {


I think it this could be called simply scope(). It makes sense to call tracer.scope().span() and tracer.scope().run() because these actions are indeed on the current scope.

rochdev · 2018-11-28T21:49:12Z

src/tracer.ts

+     *
+     * @return the active Span. This is a shorthand for tracer.spanManager().active().
+     */
+    activeSpan(): Span | null {


Similar to my comment below, I don't see the point of this since the alternative is not any more involved.

ghost · 2021-06-11T16:01:50Z

src/async_wrap_span_manager.ts

+});
+
+/**
+ * ScopeManger using Node.js {@link async_hooks|https://nodejs.org/api/async_hooks.html}


Suggested change

* ScopeManger using Node.js {@link async_hooks|https://nodejs.org/api/async_hooks.html}

* ScopeManager using Node.js {@link async_hooks|https://nodejs.org/api/async_hooks.html}

pauldraper mentioned this pull request Aug 19, 2018

Scope Manager should use continuation passing opentracing/specification#126

Open

pauldraper force-pushed the pauldraper/scope-manager branch from 897e88f to 0974d2f Compare August 19, 2018 16:13

pauldraper force-pushed the pauldraper/scope-manager branch from 0974d2f to 98c21ac Compare August 19, 2018 21:40

pauldraper force-pushed the pauldraper/scope-manager branch from 98c21ac to 71e124f Compare August 19, 2018 22:12

pauldraper mentioned this pull request Aug 20, 2018

Separate OT libraries DataDog/dd-trace-js#251

Closed

rmfitzpatrick reviewed Oct 9, 2018

View reviewed changes

rochdev mentioned this pull request Oct 29, 2018

ScopeManager for in-process context propagation #112

Open

rochdev reviewed Oct 29, 2018

View reviewed changes

rochdev reviewed Nov 28, 2018

View reviewed changes

mayurkale22 mentioned this pull request Jun 19, 2019

Add Context-* package and implementation open-telemetry/opentelemetry-js#46

Closed

ghost reviewed Jun 11, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ScopeManager #113

ScopeManager #113

pauldraper commented Aug 19, 2018 •

edited

Loading

coveralls commented Aug 19, 2018 •

edited

Loading

pauldraper commented Aug 19, 2018 •

edited

Loading

rmfitzpatrick Oct 9, 2018

pauldraper Oct 31, 2018

rochdev Oct 29, 2018

rochdev Oct 29, 2018

rochdev Oct 29, 2018

rochdev Oct 29, 2018

rochdev Oct 29, 2018

pauldraper Oct 31, 2018

rochdev Nov 28, 2018

rochdev Oct 29, 2018

pauldraper Oct 31, 2018

rochdev Nov 28, 2018

rochdev Nov 28, 2018

rochdev Nov 28, 2018

ghost Jun 11, 2021

	* ScopeManger using Node.js {@link async_hooks\|https://nodejs.org/api/async_hooks.html}
	* ScopeManager using Node.js {@link async_hooks\|https://nodejs.org/api/async_hooks.html}

ScopeManager #113

Are you sure you want to change the base?

ScopeManager #113

Conversation

pauldraper commented Aug 19, 2018 • edited Loading

Continuation passing style

Support

coveralls commented Aug 19, 2018 • edited Loading

pauldraper commented Aug 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pauldraper commented Aug 19, 2018 •

edited

Loading

coveralls commented Aug 19, 2018 •

edited

Loading

pauldraper commented Aug 19, 2018 •

edited

Loading