Which JS SDK should be used in manual instrumentation docs?

[cartermp]

The manual instrumentation docs today use sdk-trace-base: https://opentelemetry.io/docs/instrumentation/js/instrumentation/

This is helpful insofar as the code here is useful both in Node and in the browser.

However, sdk-trace-base doesn't propagate context, and so creating nested spans is kind of awful: https://opentelemetry.io/docs/instrumentation/js/instrumentation/#create-nested-spans

Contrast this with when using the Node SDK (code is from a basic express app):

function doot() {
    tracer.startActiveSpan('doot', span => {
        console.log('doot');
        span.end();
    });
}

app.get('/', (_req, _res) => {
    tracer.startActiveSpan('toot', span => {
        _res.send("toot");
        doot();
        span.end();
    })
});

No needing to set the current span in context, get the new context, and use that awful overload to create a nested span. Just creating a span like any other language automatically gives you that parent-child relationship. Nice!

But not everyone reading this document is going to be a Node developer, so the code to initialize tracing, such that you can just use the OTel APIs happily, is not applicable for everyone.

My proposal would be to create this content twice on this page: https://opentelemetry.io/docs/instrumentation/js/instrumentation/#initializing-a-tracer

One for node, and one for browser JS. This could be as tabbed content, although we may want to use tabs later on to show JS vs. TS code. I'm fine with whatever folks think works best. But mainly, I'd like to not document the hard way to create nested spans.

@open-telemetry/javascript-approvers what do you think?

[austinlparker]

Let's split them out. I think it'd make sense to actually break node and browser into separate files entirely?

[cartermp]

So I think that the majority of content can stay the same for both SDKs, including nested spans. Both propagate context (at least within the same process!) and so creating nested spans is easy peasy. E.g. in web:

  const onClick = () => {
    webTracerWithZone.startActiveSpan('files-series-info', singleSpan => {
      for (let i = 0, j = 5; i < j; i += 1) {
        webTracerWithZone.startActiveSpan(`files-series-info-${i}`, fileSeriesSpan => {
          traceGetData(url);
          fileSeriesSpan.addEvent(`fetching-span-${i}-completed`)
          fileSeriesSpan.end();
        });
      }
      singleSpan.end();
    });
  };

This creates child spans just as you'd expect:

[file-series-info]
    [file-series-info-0]
        [FETCH]
    [file-series-info-1]    
        [FETCH]
...

Maybe this can be the layout?

Initialize tracing

words

Node.js

words and code sample

Browser

words and code sample

Create spans

...
etc.

And then maybe a separate doc or section about when you're using neither, which requires you to manually put spans into context if you want a parent-child relationship.

[vmarchaud]

I would think that it's fine to keep in one file (just to avoid duplicating example) however there is a request to support deno (and ongoing work to do so) and i would bet that workers (like cloudflare) might follow at some point so we need to think about how that you go, in other sections too ?

[dharesign]

I was reading the docs, which state:

Don't bother using startActiveSpan because it won't do this for you.

and looking into why ended up here as the issue which prompted this addition to the docs.

Why doesn't startActiveSpan work? It seems to call the same methods as the example in the docs calls manually.

[cartermp]

Not quite - startActiveSpan does not set the parent span in the active context. And since sdk-trace-base doesn't propagate context, the span created by it will be a sibling rather than a child of whatever the active span was prior to calling startActiveSpan. I suppose if you were to use it in conjunction with api.context.with you could get it to be a child span? I haven't tried, but my gut feel is that what's written today is easier for people who are in a more niche scenario where they aren't in a Node or Web context.

[dharesign]

api.context.with is what startActiveSpan does though:
https://github.com/open-telemetry/opentelemetry-js/blob/3290b25d0a1f28ad0df7efd30ed3ff15dc1c2189/packages/opentelemetry-sdk-trace-base/src/Tracer.ts#L224-L228

It also does api.trace.setSpan(parentContext, span), where parentContext is api.context.active() if one wasn't passed into startActiveSpan, which is what the docs say to do.

Full disclosure, I haven't used the API at all, I'm just browsing the documentation at this point, so I very definitely might be overlooking something 😅

[cartermp]

I think what you're missing is the subtlety in the API calls -- startActiveSpan doesn't explicitly set the parent span as active; it creates a new span and sets it as the active one. That's not normally an issue because in the Node and Web SDKs, the parent is already set as the active span.