API/SDK usage based on current implementation

[mayurkale22]

I tried to put one simple example together to get the initial feeling of current APIs and BasicTracer SDK.

1.  const opentelemetry = require('@opentelemetry/core');
2.  const { BasicTracer } = require('@opentelemetry/basic-tracer');
3.  const { NoopScopeManager } = require('@opentelemetry/scope-base');
4. 
5.  // Initialize the OpenTelemetry APIs to use the BasicTracer bindings
6.  opentelemetry.initGlobalTracer(new BasicTracer({
7.      scopeManager: new NoopScopeManager()
8.  }));
9. 
10. doWork();
11. 
12. function doWork() {
13.   const span = opentelemetry.getTracer().startSpan('doWork');
14.   span.addEvent('invoking doWork').setAttribute('value', 'some value');
15. 
16.   const childSpan = opentelemetry.getTracer().startSpan('anotherWork', {
17.     parent: span
18.   });
19.   childSpan.setAttribute('value', 'some value');
20.   childSpan.end();
21. 
22.   span.end();
23. 
24.   // Stop the delegate from using the provided tracer.
25.   opentelemetry.getTracer().stop();
26. 
27.   // This should fallback on NoopTracer.
28.   const noopSpan = opentelemetry.getTracer().startSpan('noop');
29.   noopSpan.addEvent('invoking doWork').end();
30. 
31.   // Begin using the user provided tracer.
32.   opentelemetry.getTracer().start();
33. 
34.   const span2 = opentelemetry.getTracer().startSpan('span2');
35.   span2.end();
36. }

Some key observations:

1. Personally, I don't like passing compulsory param (NoopScopeManager) See line # 7.
2. If I use NodeTracer, I should not have to manually pass the parent span it should fetch it internally from getCurrentSpan() - from the current context. See line # 16-18.
3. Should we allow users to pass TracerConfig while opentelemetry.getTracer().start()? See line # 32.
4. Do we still need this PR - fix: add start method on tracer and initialize PluginLoader #168?

Any thoughts?

[mayurkale22]

/cc @bg451 @danielkhan @hekike @OlivierAlbertini @rochdev @draffensperger @vmarchaud @markwolff

[vmarchaud]

1. I thought the BasicTracer would create a NoopScopeManager by default if none is given in options.

3/4. Would be needed to enable/disable/load specific plugins for the NodeTracer, so yeah the PR is needed

[mayurkale22]

I thought the BasicTracer would create a NoopScopeManager by default if none is given in options.

Per SIG and 41112dd, scopeManager is a required option. I think explicitly mentioning scopeManager eliminates the require for log, throw or swallow stuff. But personally, I would prefer optional param.

[draffensperger]

What if we made it an optional param with a default value of NoopScopeManager?

I know we discussed the global tracer stuff at length, but I personally would prefer something like this:

1. const { tracer } = require('@opentelemetry/basic-tracer');
2. const { NoopScopeManager } = require('@opentelemetry/scope-base');
3.  
4. // This specifies a scopeManager to show how to configure the tracer
5. // but if no call to `start` were made it would retain its default NoopScopeManager
6. tracer.start({scopeManager: new NoopScopeManager()}); 
7.  
8. doWork();
9. 
10. function doWork() {
11.   const span = tracer.startSpan('doWork');
12.   ... // same as Mayur's example above

In that case the basic tracer would be given a definite value of scope manager by default, but it could be overridden in the call to start and calling initGlobalTracer isn't needed.

That said, I'm open to the initGlobalTracer pattern if that's people's preference as a whole.

[bg451]

Looking at this I'd definitely like to add some global static methods so getTracer() won't have to be used so often.

I'll do a similar exercise with a toy http server and share the code here.

[danielkhan]

Looking at this I'd definitely like to add some global static methods so getTracer() won't have to be used so often.

I'll do a similar exercise with a toy HTTP server and share the code here.

@bg451 I'm interested to see your suggested approach but wouldn't a const tracer = opentelemetry.getTracer() solve the problem anyway?

[danielkhan]

@mayurkale22
I'm looking at it blindly from a usability perspective:
In # 32 you call .start() - where is the .start() for everything that came before the .stop() in # 25?

I kind of like the approach of explicitly having to define a ScopeManager as it removes ambiguity.
If I copy/paste the code from the documentation, I instantly know what it is doing. Otherwise, I would have to look up the default. With the current approach, my next step as an implementor would be to have a look at which scope managers are available.

I think the same question will arise with the exporter where we could default to ConsoleExporter.

[OlivierAlbertini]

I kind of like the approach of explicitly having to define a ScopeManager as it removes ambiguity.
If I copy/paste the code from the documentation, I instantly know what it is doing. Otherwise, I would have to look up the default. With the current approach, my next step as an implementor would be to have a look at which scope managers are available.

I think the same question will arise with the exporter where we could default to ConsoleExporter.

I think both approach is good and I prefer to make it optional (in the long term run).
But I think @danielkhan is right. People will be less frustrated to use the SDK at the beginning.

[mayurkale22]

Agreed.