TracerRegistry: set name to default

[mayurkale22]

Originates from #582 (review)

In case of TracerRegistry's getTracer(name, version) method, use default name when an invalid name (null or empty string) is specified. With this, we can remove default name from all the examples : .getTracer('default’); => .getTracer();.

Specs: /~https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#obtaining-a-tracer

[dyladan]

I'm not sure I agree that this is a good idea. For one thing, the specification states that name is a required property. For another, the obligatory nature of the name in a named tracer is what makes it useful. If you make it possible to acquire a tracer without providing a name, then library devs that integrate otel hooks might not provide one, which would render the original purpose impossible.

[mayurkale22]

Hmm, this is a valid concern. I think intention was to make .getTracer(name); usage simple and convenient, especially for the examples.

[dyladan]

I think if that is the concern, I would rather update the getTracer calls to use good names instead of "default". This would help people reading the examples understand what the name parameter is anyways.

[mayurkale22]

Just did the comparison with other libraries. Looks like good names should solve the issue.

Go: /~https://github.com/open-telemetry/opentelemetry-go/blob/master/example/basic/main.go#L73

Python: /~https://github.com/open-telemetry/opentelemetry-python/blob/master/examples/basic_tracer/tracer.py#L45 (name of the current module)

Java: /~https://github.com/open-telemetry/opentelemetry-java/blob/master/examples/jaeger/src/main/java/io/opentelemetry/example/JaegerExample.java#L18-L19

@obecny WDYT?

[obecny]

@dyladan the spec on one hand says that name is required but then it also says that when name is invalid null or empty string then it should still work. Translating this to js it means that you should make the param optional (but still highly recommended) and set a default name a working default Tracer implementation as a fallback is returned rather than returning null or throwing an exception..
With regards to having a much more friendly name "good name" this is fine as in other traces, but in the original version we had default in 95% cases which was misleading and looked like a boiler plate code rather then useful name for a tracer.
We should probably explain that very precisely in documentation explaining whats the benefit if you provide a name versus if you don't and then examples could provide much better name then default so people can understand this better, including us too :).

[dyladan]

the spec on one hand says that name is required but then it also says that when name is invalid null or empty string then it should still work. Translating this to js it means that you should make the param optional (but still highly recommended) and set a default name a working default Tracer implementation as a fallback is returned rather than returning null or throwing an exception..

"working default tracer implementation" could be noop tracer. This is just to prevent invalid use of the API from causing crashes. For example, if a js library implements OpenTelemetry but calls the API incorrectly and leaves out a name, the worst case should be that they get no traces. It should never crash the program. In my opinion, "required" is very unambiguous and making the property optional in the types will imply to most users they they really don't need it. 99% of users will not read the documentation and many that do won't read it thoroughly. We should keep the property non-optional in the types.

With regards to having a much more friendly name "good name" this is fine as in other traces, but in the original version we had default in 95% cases which was misleading and looked like a boiler plate code rather then useful name for a tracer.

Totally agree that "default" is a bad name and should not be in the examples. That's why I think we should use good names like "http-example-client".

We should probably explain that very precisely in documentation explaining whats the benefit if you provide a name versus if you don't and then examples could provide much better name then default so people can understand this better, including us too :).

I'm currently working on a denylist implementation where you disable instrumentation by name. I will add an example that shows how it is used.

[obecny]

whoops @dyladan I think I edited your response adding mine, not sure why is this even possible :/

[obecny]

"working default tracer implementation" could be noop tracer. This is just to prevent invalid use of the API from causing crashes.

Let me copy the whole sentence
A TracerFactory could also return a no-op Tracer here if application owners configure the SDK to suppress telemetry produced by this library.
So the case with no-op Tracer says it's the application owner should configure the SDK this way, not "us" so if name is not provided it should still work fine - returning the correct tracer not the no-op right ?

In case an invalid name (null or empty string) is specified, a working default Tracer implementation as a fallback is returned rather than returning null or throwing an exception

working default Tracer

[obecny]

@dyladan copied from email, should be fine now

[dyladan]

The No-Op tracer is a working tracer, it just accomplishes a different goal. Notice the examples of "not working" are null or throw. In any case, a name is not optional and we should do everything in our power to ensure that it is used correctly. Returning a no-op tracer if name is excluded would force library developers to include a name, while at the same time not causing crashes in applications which have misused it.