Only expose interfaces in API

[anuraaga]

We have a use case for completely reimplementing the API - auto instrumentation which does so too provide safety to an app in the face of version skew. Migrating span context in #1935 is helpful, but there are still classes like attributes which can only be bridged with copies. How about we go ahead and expose only interfaces to support this sort of use case better? It does allow a user to implement an interface themselves, but if this actually solves a use case for them, I don't see why we need to proactively block this, and it will be rare.

[bogdandrutu]

From one of the PR that generated this discussion:

@jkwatson:

I've heard this comment a lot, but I've never heard of an actual example of this happening in the real world. Can you show a case where you've been actually impacted by this in the past? Java 8 default methods let us evolve interfaces in a similar way that we've been able to evolve abstract classes, pre Java-8.

@bogdandrutu:

FYI: This is not about evolving the API, but about dealing with third-party implementation of the interface. I am advocating to not allow other implementations for classes when we don't need to offer that capability.

Yes, we had the problem in OpenCensus. We had an interface that represented the Tags (a.k.a baggage) that did not expose publicly a get method, and have our own implementation that exposed (package protected) a get method. Now because that was an interface and everyone can extend the interface it was not trivial to convert to the internal implementation because we had a path where we have to deal with another implementation that does not have our functionality.

You may argue that all functionality must be public, but that is not necessary true if you don't want user to have access to that or abuse your API. By restricting who can implement an interface/abstract class you can rely on package protected functionality to optimize performance etc.

So I am fine with using final classes or auto-value classes with package protected ctor, because it means we control the implementation.

[bogdandrutu]

Also to use @jkwatson expression :), I've heard this comment a lot, that instrumentation needs to re-implement all the classes, but a clear documentation and design document (why is this really required) was never documented in the repository. I saw some documents in google docs (please move that to the repo so others can see and propose better alternatives if possible) which did not convince me that this is a real requirement (mostly like I am missing stuff).

So I suggest that initially we prove that the hypothesis is clear and understood, before taking actions.

[jkwatson]

My opinion on this:

If what you are exposing is pure data, then having a final class is fine. If you're exposing functionality, I will always prefer to have an interface for that functionality.

For example, Baggage is pure, immutable data. Having a final class for that is great. A Span, however, is a combination of data and function, so we should use an interface for it (which we do!).

[bogdandrutu]

@jkwatson same logic applies to SpanContext

[bogdandrutu]

Also there are 2 independent issues that I am hearing:

1. Everything in our API must be extensible, including data only objects.
2. Make all current autovalue generated classes final (using the argument from the guide line). This is wrong because an abstract class with a package protected ctor achieves almost the same behavior as a final class, which is that it cannot be extended by our API users.

[jkwatson]

I don't love AutoValue (mostly because I think the code it generates and requires is not particularly idiomatic), but yes, AutoValue + package private constructors is effectively the same as a final class.

But, given that lombok is such a bad word, AutoValue is the next best thing. And, I'm good with AutoValue data classes, since I'd definitely prefer AutoValue to having to maintain the boilerplate equals/hashCode/toString.

So, my opinion, I think, stands that data classes as final (or AutoValue) is good. Classes with functionality should be interfaces, rather than classes.

[anuraaga]

@trask can elaborate on the agent's shading constraints. I think the short answer is that agents have broken user apps in the past due to version issues and the shading of the API is important to maintain the agent's safety guarantees. It'd definitely be good to get that into markdown.

@tylerbenson also has a use case for extending spancontext it seems and maybe can explain it.

There is some double edged sword to interface vs class, but I think we can always come up with examples of both helpful and harmful. Another fun thing to do with SpanContext as an interface would be to have the Span implementations implement it and inline the data to reduce an allocation. It's impossible to make such changes in the future with classes.

I think it's fair to assume that the burden of something working is on the implementer if they do something custom. It's possible to document this, and point at it if something goes wrong with something custom.

We can also look at our current API to see what could break with something custom. I don't think we have anything - this is probably because the fact that the API needs to be used by SDKs tends to prevent package private tricks.

So I would still consider more interfaces, precisely to have more flexibility in the future, not less. I think this is possible and reasonable precisely because we have such a strict separation between API and SDK.

[bogdandrutu]

I think changing the implementation for a final class is backwards compatible. Maybe this is yet a missing defined thing, what is considered to be a breaking change and what is not after the GA.

[bogdandrutu]

@anuraaga i would really like to see the requirement of having to re-implement all classes documented and explain, would make a lot of the discussions easier if we agree on that.

[anuraaga]

I think changing the implementation for a final class is backwards compatible.

Yeah, but it's not possible to, for example, inline it into a different class unless it's an interface. A class can't be changed to an interface and preserve ABI.

[bogdandrutu]

Yet another concept that is not documented, do we really need to guarantee ABI compatibility?

PS: I am trying to identity all the missing requirements that some of us have in their minds and are not documented and clarify between devs. I would say that we definitely need to document these before making a decision, so that we can have all pros/cons.

[trask]

do we really need to guarantee ABI compatibility?

I would think yes, due to libraries depending on otel api, which won't get recompiled when application developer bumps otel api version.

PS: I am trying to identity all the missing requirements that some of us have in their minds and are not documented and clarify between devs

💯👍

[tylerbenson]

I suggest that making SpanContext (and any other core class) not extensible makes it very difficult for alternate implementations of the API that have different requirements for what belongs in the SpanContext.
For example, with OpenTracing, the Datadog Javaagent is able to create a bridge that interfaces with our internal core api. Currently, this is done via wrapper classes (but considering other options to improve performance). I'm trying to do the same for OpenTelemetry, but having core non-interface classes that can't be extended makes that very difficult.

I understand the desire for limiting the extension points of the API, but it's a difficult balance and by being too restrictive it guarantees that only one implementation will ever work. At that point, it feels like we're back at OpenCensus without the separation between api and sdk.

In general, I think our API should be simple and extensible. Right now I feel it's neither.

[Oberon00]

I suggest that making SpanContext (and any other core class) not extensible makes it very difficult for alternate implementations of the API that have different requirements for what belongs in the SpanContext.

I fully agree, but unfortunately that ship has long sailed: open-telemetry/oteps#58 (see also the many related issues)

[Oberon00]

In general, I think our API should be simple and extensible. Right now I feel it's neither.

To me, the API feels more like something split from the default SDK than something that is really meant for vendors to implement. EDIT: This is just how the spec is designed. It is not Java's fault.

[tylerbenson]

I think we have enough reasons to push back on it at this point to overturn the decision.

[anuraaga]

@Oberon00 Didn't we already change it in the spec?

open-telemetry/opentelemetry-specification#969

I'm a bit sad after going through that effort, back at the drawing board

One note though is that I believe it is OK to change ABI pre-GA, even if we declare a version like 0.10.0 (relatively) API stable, since it doesn't effect the ability for an end user to upgrade without changes to their code, and dependency classpath issues should be minor before 1.0. I'm really hoping so at least :)

[carlosalberto]

I think we have enough reasons to push back on it at this point to overturn the decision.

As @anuraaga said, this is now allowed, Specification-wise. Whether we want to support it for Java, that's a different (tricky) discussion.

[anuraaga]

I think the ability to inline interfaces into the same class can't be understated. It actually becomes possible to, e.g., embed all of Context, Span, SpanContext, TraceState, Baggage into the same allocated object. Perhaps it's a maintenance nightmare, or overkill - but it may be a demonstrable win for end users to reduce those allocations, so it's nice to have that option open.

[Oberon00]

If we do not implement the "SHOULD NOT be overridable" we should have a good, language-specific reason. I don't think any of the reasons above was language-specific. I mean, there should be no problem with making SpanContext an interface, but putting the mechanisms in place to make it truly overridable (without bytecode manipulation) is another story (e.g. moving creation of SpanContext to instance methods of TracerProvider).

[trask]

Also to use @jkwatson expression :), I've heard this comment a lot, that instrumentation needs to re-implement all the classes, but a clear documentation and design document (why is this really required) was never documented in the repository. I saw some documents in google docs (please move that to the repo so others can see and propose better alternatives if possible) which did not convince me that this is a real requirement (mostly like I am missing stuff).

@bogdandrutu good call out. check out and please comment on open-telemetry/opentelemetry-java-instrumentation#1597. also this would be a good discussion topic on Thu if you are able to join either the 9a or 6p Pacific Time meeting.

[trask]

@bogdandrutu are you ok with this change now that we have officially published the design document explaining the java agent bridging problems and associated design?

For the specific difference of bridging interfaces and classes:

Without interfaces

We have to copy data structures back and forth across the bridge.

With interfaces

At worst, we can use a wrapper to bridge, e.g.

class SpanContextImpl implements io.opentelemetry.api.trace.SpanContext {

  private final io.opentelemetry.javaagent.shaded.io.opentelemetry.api.trace.SpanContext delegate;
  
  ...
}

At best, we can have a single implementation that satisfies both the shaded and unshaded interfaces, e.g.

class SpanContextImpl implements io.opentelemetry.api.trace.SpanContext,
                                 io.opentelemetry.javaagent.shaded.io.opentelemetry.api.trace.SpanContext {

  // covariant return is important here
  public TraceStateImpl getTraceState() {
    ...
  }
}

class TraceStateImpl implements io.opentelemetry.api.trace.TraceState,
                                io.opentelemetry.javaagent.shaded.io.opentelemetry.api.trace.TraceState {
  ...
}

[trask]

@bogdandrutu any feedback? currently you are blocking #1935, and we would like to convert SpanContext, Labels, and Attributes to interfaces for agent bridging performance reasons. with default and static methods those interfaces will look and feel the same for API users (source code compatible but not bytecode compatible with prior versions).

[jkwatson]

Can we close this issue, @anuraaga ?

[Oberon00]

Maybe an ArchUnit test could be added?

[jkwatson]

closing, as this is done for now. Feel free to submit a PR with some sort of archunit verification as a separate concern.