ScopeManager for in-process context propagation

sebnow commented 6 years ago

The JavaScript implementation should follow suite of languages like Java and Python and implement the proposed ScopeManager specification. This would make managing the active span significantly easier.

RFC: https://github.com/opentracing/specification/blob/master/rfc/scope_manager.md Python: https://github.com/opentracing/opentracing-python/pull/63 Java: https://github.com/opentracing/opentracing-java/pull/188

yurishkuro commented 6 years ago

It's a bit tough given that there are no thread locals in Javascript. There might be something coming up from https://github.com/nodejs/diagnostics

pauldraper commented 6 years ago

Node.js diagnostics are limited to the Node.js runtime.

AWS X-Ray uses https://github.com/othiym23/node-continuation-local-storage which in turn is built on https://github.com/othiym23/async-listener. It has built-in support for Node.js APIs.

Though I would recommend using https://github.com/angular/zone.js which is robust with a long history. It also has an associated (early stage) TC39 proposal https://github.com/domenic/zones. If zones/thread locals become a part of the ES standard, this is the likely form. Zone.js has built-in support for both web browser and Node.js APIs.

yurishkuro commented 6 years ago

Good to have domain experts 🙂

Are there minimum Node version limitations in those cls libraries?

junminstorage commented 6 years ago

I used node-contination-local-storage in the tutorial a while ago: https://github.com/junminstorage/opentracing-tutorial/blob/master/nodejs/lesson04/solution/hello-context.js

junminstorage commented 6 years ago

node-cls support was developed in 2012, supported 0.12.x, and is very lower level library, The latest nodejs has async hook for any application level library we want to use like request, or promise lib e.g. bluebird, you need to patch it with a cls version e.g. cls-bluebird which is what I did in the tutorial. Back then I was trying to mimic the python contextManager in nodeJS, but didn't like sample code I ended up with, I forgot the reasons why though.

Puneeth-n commented 5 years ago

Also https://www.npmjs.com/package/cls-hooked

rochdev commented 5 years ago

Here is a summary of the situation, along with the version support information asked above.

Node modules

The following Node modules exist and are popular enough to be mentioned here.

async-listener

Overview

This is the oldest one and probably the most used one for Node applications. It is used by the very popular continuation-local-storage.

Node support

Partial Node support is available for >=0.10 <=9. Since it’s implemented in pure JavaScript, it does not support the new native async/await construct from Node 7.6+ however. We can then say that effective version support is basically >=0.10 <=7.5.

Browser support

N/A

Issues

This library has had an unfixed memory leak for over a year. The main reason it’s not fixed yet from my understanding is the maintainers of the library have since moved on to local implementations in their respective projects, meaning they no longer actively use/maintain the library.

Another issue is that this is the only library in the list that binds promises on resolve() instead of on then().

What this means for our purpose is that we would have to maintain a fork of the project that fixes the memory leak, alters promises behavior, and doesn’t attach to the process to preserve compatibility with the non-forked module.

async_hooks

Overview

This is the officially supported and built-in solution from Node. It is still marked as "experimental" but the API has been stable for a while now.

Node support

It supports Node >=8.

Browser support

N/A

async-hook-jl

Overview

This is a wrapper around AsyncWrap to provide functionality similar to async_hooks.

Node support

It supports Node ^4.7 || >=6.9.

Browser support

N/A

zone.js

Overview

This is the reference implementation of Zones, which is an attempt to standardize context propagation.

Node support

Node support is limited to Node 8, and is incomplete as async/await is not currently supported. This means that in its current state, we can consider that the library effectively doesn't support Node.

Browser support

This is the only project on the list that supports the browser. The project is led by Angular so it makes sense.

The following browsers are supported:

Android 4.4+
Chrome 48+
Edge 14+
Firefox 52+
Internet Explorer 9+
Safari 8+

Node Domain

Overview

This was one of the many attempts to standardize context propagation in Node. It went directly from unstable to deprecated and never landed proper.

Node support

Node >=0.8.

Issues

The feature is deprecated, and was never officially supported outside of being marked as unstable. The behavior of promises binding has also changed over time, and the implementation had some dealbreaker drawbacks.

Proposed solution

Given all of the above, the scope manager should be implemented with the following underlying implementations.

async_hooks for Node >=8
Fork of async-listener with the following fixes for Node <8
- Bind promises on then() instead of resolve().
- Fix the memory leak when an error is thrown from a promise callback.
- Remove any globals to avoid conflicts with the original async-listener.
zone.js for browser support.
Optionally, use async-hook-jl for Node ^4.7 || >=6.9 <8. This would probably make sense only if there is a performance benefit over async-listener. In my experience, it's actually slower, and comes with its own set of problems which are avoidable by simply relying on async-listener for these versions instead.

All of the above should be automatically selected based on the environment.

The implementation should also be based on continuation passing style as explained in https://github.com/opentracing/specification/issues/126, with the following API:

// `activate` could be renamed to something like `run` or `execute`
//  if this is considered an acceptable deviation from the spec.
interface ScopeManger {
  active(): Span;
  activate(Span span, f: () => void): void; 
}

@yurishkuro A lot of this work has already been done by @pauldraper in https://github.com/opentracing/opentracing-javascript/pull/113. How can we get this PR to move forward?

On our end, we've implemented the aforementioned async-listener fixes in our fork.

tedsuo commented 5 years ago

Thanks for the writeup @rochdev. We can devote some time to looking at this.

pauldraper commented 5 years ago

Thanks @rochdev for the summary! That is a good and AFAIK complete summary of JS tracing tech.

(1) The most confusing and hard to implement thing is autoclosing scopes. And I'm not sure they really solve a meaningful problem.

(2) CPS is a dead-simple API, to understand, to implement, and to use. It is possible that it lacks some important ability, but so far, not examples have been provided.

pauldraper commented 5 years ago

Also FYI Node 8 is the earliest version in active LTS, and as of Apr 2019, it's the earliest in maintenance LTS.

rochdev commented 5 years ago

The most confusing and hard to implement thing is autoclosing scopes. And I'm not sure they really solve a meaningful problem.

I don't think this is necessary at all with a CPS implementation.

CPS is a dead-simple API, to understand, to implement, and to use. It is possible that it lacks some important ability, but so far, not examples have been provided.

Since internally even CPS usually has some kind of enter/exit logic, this could be exposed eventually if warranted.

Also FYI Node 8 is the earliest version in active LTS, and as of Apr 2019, it's the earliest in maintenance LTS.

Unfortunately, it's not so simple for APM providers. On our end, we have customers that are still using Node 4, and I know some OpenTracing users are still on 0.10. Node 6 is still officially supported at the moment as well.

However, this is not really a problem since as you've demonstrated in your PR it's possible to have multiple ScopeManager implementations in very few lines.

rochdev commented 5 years ago

After thinking about this a bit more, right now the current proposal stores a span, but it doesn't use any method from the span, meaning it could be used to store any arbitrary value. What I'm wondering is if we should create the scope manager as a generic external library and simply import it in opentracing-javascript.

Then it would in theory be possible to even create a new universal implementation of continuation-local-storage on top of the scope manager which would support all platforms. I know this would work for everything except for Zone.js which I'm not sure about. Maybe @pauldraper can confirm if Zone.js would work with the CLS API?

If Zone.js can support the CLS API as well, then maybe it could even be the other way around: build a universal CLS and then have the ScopeManager use it. The benefit of this approach is that users could also use the CLS for their app and effectively reuse the same context propagation mechanism. Since context propagation is very heavy in JavaScript, this would be a huge performance benefit.

See the API here: https://github.com/othiym23/node-continuation-local-storage. Also note that it's missing 2 public methods enter(context) and exit(context) which are for advanced use cases only and generally not needed.

okonon commented 5 years ago

wondering if there was any progress on this issue?

opentracing / opentracing-javascript