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.