Consider using ABC

[c24t]

Consider using abstract base classes and the various abc.abstract* decorators in the API package.

From https://github.com/open-telemetry/opentelemetry-python/pull/61/files#r307940312.

[carlosalberto]

I was thinking it would be a good idea to use them in an uniform way - either we use them everywhere, or we don't use them (and remove their usage in the API).

(I'm up for using in the API side, btw ;) )

[Oberon00]

My thinking was (and I thought we decided that somewhere, but can't find it) that since the APIpackage must be usable on its own as a no-op implementation, we would make the API interface the no-op implementation instead of raising exceptions (that would require a separate NoOp version of each class in the API which is a lot of boilerplate code).

[toumorokoshi]

Agreed that we have to be uniform regardless of what path we take.

I worry about the no-op implementations because that doesn't provide a way to let sdk authors (or consumers of sdk's) know that a particular implementation has not implemented a method.

Using ABCs surfaces the warning as soon as the object is imported, which means that the missing methods will raise an error as soon as the application has started.

We could potentially revisit this once we have a working system, or have a unit test that just uses the API exclusively. Some way to better understand the ramifications.

[Oberon00]

When using ABCs, we still have to create no-op versions of every class in the API (or maybe we could autogenerate these with "reflection"). I don't think the additional warnings are worth the complications and/or boilerplate code though. As a vendor I would still think twice whether I want to risk crashing the app (even if only at startup) by deriving from an ABC that got a method added since I wrote my implementation or whether to derive from that no-op version instead, so that users get (worst-case) incomplete telemetry instead of crash-looping PODs, etc.

Or put another way: If we make the base classes ABCs, if we want to be backwards compatible as far as API-implementing vendors are concerned, we must never add new methods to them.

[carlosalberto]

we would make the API interface the no-op implementation instead of raising exceptions

This is how we do it in OpenTracing. Not having this as a no-op implementation would mean creating (and maintaining) a DefaultTracer and related siblings, which effectively is the no-op.

As a vendor I would still think twice whether I want to risk crashing the app (even if only at startup) by deriving from an ABC ...

My same feeling.

we must never add new methods to them.

... and this has been a recurring talk, how to not break vendors for every new (minor) version ;)

[Oberon00]

Reopening, as #311 seems to be implementing this. Please close as soon as #311 is resolved.

[codeboten]

ABC's are useful in enforcing anyone implementing the interface to at least understand the scope of what needs to be implemented up front. I would hope that a vendor wouldn't ship a crashing implementation to a user, but maybe that's me being optimistic that everyone tests their code :D. If the recommended behaviour is to extend the Default implementation anyways, then I agree that using ABCs is an unnecessary complication here.

It's true that using ABCs has the disadvantage of crashing if a new abstract method is added to an interface and is not implemented, which means we have two choices when adding methods to the interface:

1. we add abstract methods and create pain for anyone that hasn't pinned the version of the API
2. we add non-abstract methods, which defeats the purpose of using ABCs in the first place

On the upside, this ensures that new methods in the API are auto-discoverable via crashes :) Crashing applications on upgrades of an underlying library definitely doesn't feel very python-esque though.

[Oberon00]

I would hope that a vendor wouldn't ship a crashing implementation to a user,

The problem occurs if the user upgrades the API and the vendor hasn't tested the newer API version yet.

[ocelotl]

Just a question here: adding a new abc.abstractmethod decorated method to an abc.ABC class is a non backwards compatible change and should require a major release as per Semantic Versioning. Why isn't this the approach to follow when a new abstract method is added?

From the discussion it kinda seems like we don't want to do a major release when this happens. I understand the issues that may come with a major release, but why not treat this kind of non backwards compatible change like any other one?

[Oberon00]

It should be a goal to be able to make as many changes as possible without requiring a new major version. Otherwise you end up having a library that either requires users to upgrade to new major releases all the time or you need to port bug fixes back to a multitude of major versions.

[ocelotl]

I completely agree, @Oberon00, breaking changes are to be kept as few as possible for the impact they have in users and developers. My point is just that using an ABC does not cause more breaking changes because of the ABC itself. An abstractmethod decorated method just means that subclasses must implement that method, and that decorator should only be applied if calling that method is needed elsewhere: if we use abstractmethod the user application crashes because the method is not implemented. If not, the user application crashes because the attribute (the method) can't be found. The first scenario is better because the crash happens earlier with a failure that is easier to understand. But at the end, the cause of the crash is not because of the ABC itself, but because something else called that particular method that should be implemented in the subclasses. That something else is the actual cause of the non-backwards compatible change.

I believe there is no problem with using ABCs as long as we use the abstractmethod decorator only when an implementation of a particular method is required elsewhere and only when it is necessary for the subclasses to provide an implementation by themselves. That means that some methods in the ABC may be decorated with abstractmethod and some not, and that is fine. It all depends on the necessity of the subclasses to implement or not.

[Oberon00]

There won't be a crash if there is a default implementation of the method, even if that default method does nothing (it must return something of the right type of course, if applicable).

[c24t]

Summarizing a conversation with @ocelotl.

Here's the problem as I understand it, written out in the form of an example:

Part 1

Suppose v1 of opentelemetry defines Tracer with a single method, start_span:

class api.Tracer(ABC):
    @abstractmethod
    def start_span(self, *args) -> api.Span:
        pass

class api.DefaultTracer(api.Tracer):
    def start_span(self, *args) -> api.Span:
        return INVALID_SPAN

class sdk.Tracer(api.Tracer):
    def start_span(self, *args) -> api.Span:
        ...  # start the span, make it active
        return span

An APM-vendor ships their own Tracer implementation in the v1 alt package:

class alt.Tracer(api.Tracer):
    def start_span(self, *args):
        ...  # do secret APM-specific stuff
        return actual_span

And a third party library xyz decides to add instrumentation via opentelemetry. Our API doesn't make it easy, but they manage to make it work:

class XYZer:
    def startup(self):
        span = tracer.start_span("xyz")
        self.span_cm = Tracer.use_span(span, end_on_exit=True)
        self.span_cm.__enter__()

    def shutdown(self):
        self.span_cm.__exit__(None, None, None)

Some application uses the xyz library and the alt SDK. Everything works as intended, and everyone is happy.

Part 2

Later, we decide to release opentelemetry v2 and add a new Tracer.end_span method:

class api.Tracer(ABC):
    @abstractmethod
    def start_span(self, *args) -> api.Span:
        pass

    @abstractmethod
    def end_span(self, span) -> bool:
        """End and deactivate `span` if it's currently active.

        Returns:
            Whether `span` was active at the time of the call.
        """
        pass


class api.DefaultTracer(api.Tracer):
    def start_span(self, *args) -> api.Span:
        return INVALID_SPAN

    def end_span(self, span) -> bool:
        return False


class sdk.Tracer(api.Tracer):
    def start_span(self, *args) -> api.Span:
        ...  # start the span, make it active
        return span

    def end_span(self, span) -> bool:
        ok = span == self.get_current_span()
        ...  # end the span, pop the context stack, etc.
        return ok

The xyz dev team loves the new API method, and updates to opentelemetry v2. They make a quick change to their code and release a patch-level update of xyz:

class XYZer:
    def startup(self):
        self.span = tracer.start_span()

    def shutdown(self):
        if not tracer.end_span():
            logger.warning("Unexpected active span")

alt does not update to the new opentelemetry version. Because alt didn't pin opentelemetry to v1, our app developer doesn't see an "incompatible versions" error when they update xyz. As far as the app developer is concerned, none of the app's dependencies have undergone a major version change.

So what happens when they run the app?

If alt.Tracer extends api.Tracer: the app will crash with a TypeError as soon as it tries to instantiate a Tracer.

If alt.Tracer extends api.DefaultTracer: the app will run, but xyz never closes the spans it opens.

---

It's not clear to me that calling combination of custom and no-op methods is better than refusing to instantiate the tracer here.

There won't be a crash if there is a default implementation of the method, even if that default method does nothing (it must return something of the right type of course, if applicable).

It won't crash, but could produce some pretty surprising results. Especially when default classes return placeholders like INVALID_SPAN.

It should be a goal to be able to make as many changes as possible without requiring a new major version.

Some new API methods represent breaking changes whether or not they're abstractmethods. The method in the example is arguably one of these. Also any method that we ourselves call from code in the API package. Not using ABCs means we can't easily distinguish between breaking changes (which require a change in the implementation) and non-breaking changes (which don't).

Some new API methods have safe Default-class equivalents, and only represent breaking changes if we insist on using ABCs. Using ABCs forces us to make major version updates for each such change.

I don't think there's an obviously correct answer here, and any policy we come up with is going to rely on some value judgements:

• How serious an error is it if a user accidentally calls a no-op method? I.e. is it safe to extend no-op DefaultFoo classes? Should we write these classes with extensibility in mind?

• Is it better for the application to crash if the telemetry layer is broken/misconfigured, or for the application to run without working telemetry?

• How useful are ABCs/interfaces as documentation? How much do we care about having a clean separation between the public-facing API (of both the API and SDK packages) and the implementation?

• Which configuration errors is it our responsibility to defend against?

• How much should we optimize for backwards compatibility with old versions?

I still like ABCs for this use case, but I agree that a telemetry library crashing a user's application because of version mismatches in transitive dependencies is an extremely bad look.

[codeboten]

Thanks @c24t for the summary. One of the nice advantages of using ABCs along with a Default no-op implementation is that it gives implementors the option of either implementing the interface that will crash if a method is not implemented, or extend the no-op default if that option is preferred. This would mean deferring the decision to our users rather than limiting them one way or another.

Though I suppose this could also create confusion.

[ocelotl]

It could cause confusion, yes, but that can be fixed with proper documentation 🙂 .

[c24t]

We discussed this again in the SIG meeting today and decided to go ahead with ABCs (and "default" no-op implementations) in #311. This does mean some code duplication, and raises the possibility of errors as described above. Hopefully the benefits outweigh the risks.