Implementations MUST NOT allow callers to create Spans directly

[codeboten]

To be compliant with the tracing spec, implementations MUST NOT allow callers to create Spans directly

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#span

[owais]

Do we provide any APIs other than the tracer to create spans currently? I'd be nice to point that out for anyone taking this on as the first issue.

[ahlaw]

Hi, can I take this issue?

[ahlaw]

I believe the most Pythonic way would be to simply document that the caller does not instantiate Spans themselves and have them face the consequences of breaking the trust rather than strictly enforcing it in this case. The sdk Span already documents this. We could also add this to the Span api docstring or just close this issue outright?

[lzchen]

@ahlaw
Perhaps we can use something like this to enforce it.

[ahlaw]

Hmm it won't strictly enforce it, but it would definitely make it almost impossible to unintentionally instantiate Span directly. Sounds good to me.

[lzchen]

Looking at the implementation now, I'm wondering if we would have to add this for all classes that we'd expect to instantiate via the api rather than directly the constructor (all classes in metrics pretty much). Would it be worth it to do all that or should we simply go with the documentation route?

Thoughts? @codeboten @aabmass

[aabmass]

I just want to raise another alternative of using Python "private" underscores (which is essentially documentation) to tell the user not to instantiate directly, but still allow easily overriding this for our testing purposes. For type annotations, we can expose a NewType

# opentelemetry/sdk/trace/__init__.py

from typing import NewType

# make it "private"
class _Span(trace_api.Span):
    # everything the same as before
    # ...
    pass

# "public" API for typing purposes
Span = NewType('Span', _Span)

class Tracer(trace_api.Tracer):
    def start_span(self, ...) -> Span:
        # .. same as before except
        return Span(_Span(...))

A user trying to call Span(name, ...) won't work (typing-wise or at runtime) and if the user really wants to override this, they'll have to use _Span which I think most python users know to use at your own risk. Thoughts?

[lzchen]

@aabmass
I like this approach. It seems much less invasive then to all these methods to Span. Also +1 because it will be much less code for all the other classes we should enforce as well.

[ahlaw]

There is a problem with handling typing with NewType. An instance of type _Span cannot be used in places where the newtype Span is expected since NewType declares Span to be a subclass of _Span. On the flip side, casting an instance of _Span to Span raises PyLint errors when members of that instance are referenced. In addition, the newtype cannot be used at runtime, such as in isinstance calls. Any thoughts on this?

[aabmass]

Looking at that pylint issue, it looks like it gives false positives for all NewTypes? By casting do you mean Span(_Span(...)) or typing.cast?

In addition, the newtype cannot be used at runtime, such as in isinstance calls.

Hmm that sounds like a deal breaker for NewType 🙁

[aabmass]

Another option might be to have the Span class be abstract, then have actual implementation _Span subclass it and be used internally?

import abc

class Span(trace_api.Span, ABC):
    # current span class we have + this to make it uninstantiable
    @abc.abstractmethod
    def __dummy(self):
        pass

# This one is used internally and for tests, can actually be instantiated
class _Span(Span):
    def __dummy(self):
        pass

[ahlaw]

That could work, the abstract Span class wouldn't need any abstract methods either since trace_api.Span is also an abstract base class. Users attempting to instantiate Span from opentelemetry.sdk.trace will be met with this error: TypeError: Can't instantiate abstract class Span with abstract methods add_event, end, get_context, is_recording, record_exception, set_attribute, set_status, update_name. All internal and test instantiations will use _Span, and all private static methods under Span will also be moved under the _Span namespace. Both static and runtime typechecks can continue to check _Span instances as type Span. How does that sound?

[aabmass]

Span class wouldn't need any abstract methods either since trace_api.Span is also an abstract base class.

If you move the implementation of those methods to _Span then that's correct. I was thinking it might be cleaner to not split the implementation across two classes and instead just have it all in _Span. Then public Span interface would never need to be touched. Might be easier to discuss in the PR if that approach is feasible