Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate Context Propagation from Tracer API #9

Closed
hekike opened this issue May 31, 2019 · 7 comments
Closed

Separate Context Propagation from Tracer API #9

hekike opened this issue May 31, 2019 · 7 comments

Comments

@hekike
Copy link
Member

hekike commented May 31, 2019

Context

Context Propagation/Continues Local Storage (CLS) is not part of the language, JavaScript or the runtime Node's offering. Although there is some work in Node Core to support distributed tracing use-cases, the solutions today like node-continuation-local-storage come with significant performance overhead and issues, especially with using third-party libraries and a larger amount of async operations like Promises in user-land.

The current practice among APM providers is to hook the Node's module system, monkey patch core modules and bind async primitives via CLS in Node to achieve one line automatic tracing for their customers. This solution is acceptable for most of the Node users, but not for everyone, it's also not compatible with browser environments.

I believe the new OpenTelemetry initiative would give us the opportunity to create a simple synchronous tracer API like OpenTracing and the ability to handle CLS as a separate extension of this API.

Example API

(based on current OpenCensus APIs)

Synchronous Tracer APIs:

  /**
   * Start a new RootSpan to currentRootSpan
   * @param options Options for tracer instance
   * @returns {Span} A root span
   */
  startRootSpan(options: TraceOptions): Span;

  /**
   * Start a new Span instance to the currentRootSpan
   * @param childOf Span
   * @param [options] A TraceOptions object to start a root span.
   * @returns The new Span instance started
   */
  startChildSpan(options?: SpanOptions): Span;

CLS interface to set the context (must be async):

  /**
   * Sets span to current context
   * @param span Span to setup in context.
   * @param fn A callback function that runs after context setup.
   */
  withSpan<T>(span: Span, fn: () => T): T;

Tracer API combined with CLS:

  /**
   * Starts a root span and sets it to context.
   * @param options Options for tracer instance
   * @param fn A callback function to run after starting a root span.
   */
  startWithRootSpan<T>(options: TraceOptions, fn: (root: Span) => T): T;

  /**
   * Start a new Span instance to the currentRootSpan
   * @param childOf Span
   * @param [options] A TraceOptions object to start a root span.
   * @param fn A callback function to run after starting a root span.
   */
  startWithChildSpan<T>(options: TraceOptions, fn: (root: Span) => T): T;

An additional benefit of this API is that using withSpan developer could chain spans in CLS and client RPCs like gRPC would start from the correct contextual child span. OpenCensus today doesn't support context propagation for child spans.

census-instrumentation/opencensus-node#498

@hekike hekike changed the title Separate Context Propagation from Agent Separate Context Propagation from Tracer API May 31, 2019
@hekike
Copy link
Member Author

hekike commented May 31, 2019

I see the conversation already started about separation in #6.

@hekike
Copy link
Member Author

hekike commented May 31, 2019

I learned today that the proposed API won't fit more complex uses-cases probably where operations are put in a queue and will run later in a different context. However, I still believe we should have a conversation about how to handle API separation.

@rochdev
Copy link
Member

rochdev commented May 31, 2019

Synchronous Tracer APIs

Why the distinction between a root span and a child span? I can't find this in the spec and it doesn't seem necessary. A span is automatically a root span if it has no parent. It also makes it more difficult to use as if you don't know whether there is a parent you need an additional condition instead of just passing null or undefined as the parent.

CLS interface to set the context (must be async)

As discussed in #6 I think the context propagation itself is not a tracing concern and the implementation should be in a completely separate project that can be used for any arbitrary data.

I learned today that the proposed API won't fit more complex uses-cases probably where operations are put in a queue and will run later in a different context.

This can only be solved by monkey patching or by within the library. There is an incomplete list of the most popular affected modules here. Other modules known to be affected include Knex and most Promise libraries.

@hekike
Copy link
Member Author

hekike commented May 31, 2019

@rochdev

  1. I used the current OpenCensus APIs as an example, I think the API distinction between starting child and parent span should be a separate conversation. Personally, I liked the single API in OpenTracing.

  2. I think we are on the same page 👍

  3. Looks like you are familiar with these use-cases. What do you think, how would a suitable API look like?

@rochdev
Copy link
Member

rochdev commented Jun 3, 2019

It's not really possible to do it in the library natively without using AsyncResource, which some libraries have already started implementing. However, this wouldn't support older versions of Node (which may not be necessary but that's another discussion).

From the user's perspective, I think a simple bind(target, span) method is usually enough, where target is any kind of bindable object/function like functions, promises, event emitters, etc. This is what we use in our integrations and it works really well for us.

@vmarchaud
Copy link
Member

I believe we've implemented that with the different ScopeManager available so it's safe to close this one

@mayurkale22
Copy link
Member

Agreed, closed via #103

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants