Why this function instead of just having the env var point to the module that implements Tracer?

It's just more flexible. E.g. the tracer implementation's constructor could require some arguments but when using get_opentelemetry_backend_impl, a tracer could be constructed that is configured by reading environment variables or configuration files, etc.
Also, with this we allow the tracer to be named differently e.g. MyVendorTracer instead of just Tracer.

The class naming is a good point, but my complaint is about requiring this specific function in code to get a tracer.

@reyang did some work in OC to configure tracing (and etc.) from static config where the config looked a lot like executable python code: census-instrumentation/opencensus-python#621 (comment).

Since we're going to have to do the same thing for multiple components I think there's good reason to do something like this. That is the env var could point to a config file instead of pointing to a file that has a factory function to create the component.

The default SDK loader can still access different environment variables or load some config files in its get_opentelemetry_backend_impl function. I wanted to keep the basic loader as simple as possible, but maybe it does make sense to have a more complex logic that allows for easier configuration.

I could imagine a config file that looks like this (JSON):

{
 "trace": {
    "constructor": "my.vendor.trace:load_from_config", // Note the colon to separate module and function name; leave it out to use the SDK implementation
    "config": // Any JSON. The parsed object (dict, list, whatever) is passed on as the single argument to the constructor function
    }
}

Or like this:

{
 "vendor": "my.vendor"
 "trace": // Any JSON. The parsed object (dict, list, whatever) is passed on as the second argument to the constructor function, which must be named `my.vendor.configparser.load_from_config`. The first argument would be the API's tracer type object.
}

I thought we might want to be able to use the same config file between the different language clients, but after talking to @songy23, @mayurkale22, and @rghetia it looks like there's likely to be enough language-specific config that this won't be feasible. We have some flexibility in the static config format.

I think it would make sense to separate the loader and "backend" modules -- the first to get a SDK class given an API class based on env vars, the second to hold the global Tracer, Meter, etc.

I also assumed the global tracer would go in the trace package, as e.g. opentelemetry.trace.tracer. Is there an argument for keeping the tracer and meter (and etc.) in the same module instead?

The academic argument against putting the global objects alongside the API class definitions is that this makes these modules contain global mutable state. I don't know if that would be a real problem in practice. Now we have the problem that the backend module needs to implement each API module for which a global instance should be provided.

One idea I have that solves both problems but is probably over-engineered is the following:

1. Move the existing immutable modules to an api subpackage.
2. Create new modules with the name of the old ones that do a wildcard import from the api submodule and additionally provide the global instance.

That way, we have immutable API modules and conveniently accessible global tracers. The dependencies would be (with the example of tracer):

1. trace depends on api.trace and the backend/loader module
2. api.trace depends on nothing (nothing backend-related at least)
3. The loader module depends on nothing (nothing api related at least)

As far as quarantining the global mutable state, I think it would be fine to use a singleton (like java's OpenTelemetry) or dedicated container modules (e.g. opentelemetry.trace.tracer.tracer, where the first tracer is the module and the second is the object), but I think we have different intuitions about how "bad" it is to put the global tracer in the same module as the API class. One problem I can imagine is that it would prevent us from creating the tracer from config when the module is first imported; we'd have to modify it after the module was loaded since the implementation would need to import it first. What do you see as other problems?

The wildcard import is interesting, but it's not clear that the extra complexity is less bad than having an ugly import path for the tracer.

What do you see as other problems?

I haven't been able to think of any particular ones tbh, that's why I wrote "academic". 😃 Let's try putting the tracer getter in the api module. The "create from config" problem you mention doesn't apply now because we would create the tracer when the getter is first called.

Actually I have found an annoyance: See comment in unit test.

What's the use case for swapping out the global tracer?

I'm fine with disallowing that after a Tracer has been initially selected. It is mostly there to explicitly set a particular instance (maybe configured in an app-specific way) of the Tracer.

I now only provide a setter for the factory function.

Actually I like offering both a Tracer and a factory, as long as they cannot be re-set.

Another thing is that usually code has higher priority than environment variables (hence, if the user were to use this function explicitly, it would have precedence over the rest). Opinions on this?

Do you mean to leave this in? And why bother calling the type "untrusted"?

It's also not clear why we need object instead of the generic type here, but I'm willing to believe that we do. FWIW I don't think we should use type annotations outside of the overridable API components unless we have reason to believe that that bit of code in particular would benefit from type checks.

It's called untrusted and returns an object because we don't trust it to return the type we expect, since it is set by code that is out of our control. Using object forces us to check the type (as object has basically no methods, is not convertible to anything, etc.).

ImplementationFactory was meant for the other side, the set_preferred_tracer_implementation*_impl functions. But due to bug python/mypy#7092, I had to repeat the definition in the setter.

I'd actually prefer to leave it here as documentation, but I should add a comment explaining this.

I added comments, is that OK?

FWIW I don't think we should use type annotations outside of the overridable API components

Presently we run mypy with disallow_untyped_defs = True so it wouldn't even pass travis. And personally I like static typing.

And personally I like static typing

No accounting for taste :P

Why two levels of indentation here? Also I think it's fine to omit the types in this line since they're in the signature.

I fixed the indentation. I also made the description more terse, is that what you meant?

Yup, looks good.

Setting the tracer factory in the trace class modifies the callbacks in loader? I'd expect it to do something like set a module-level constant instead and only fall back to the loader if that was never set. But that only works if we treat configuration in code as higher priority than env vars.

Since the tracer is set lazily it's possible to set this factory multiple times, the last call would win. Is that intentional?

Actually a good point. Thinking about it, the dictionary in loader.py is uselessly complicated. The only advantage is that one can call loader._load_impl directly instead of using trace.tracer() but I guess that isn't really buying us much.

I removed the dictionary.

Regarding calling the setter multiple times: I have no strong opinion on that. I thought maybe some vendor may call set_preferred_impl themselves and then the application would have the final say if it called that setter again. Besides, the current behavior is the simplest to implement. 😄