Expose a noop implementation for each interface in API that can be embedded

[bogdandrutu]

Looking at the metrics API, looks like we made lots of progress in terms of the "scary" messages around the interfaces "this can be extended".

I see this in the API (even though the comment on Synchronous is related to grouping also allows you to extend the interface, since every implementation MUST embed the interface):

API:

// Synchronous instruments are updated in line with application code.
//
// This interface is used as a grouping mechanism.
type Synchronous interface {
	synchronous()
}

// Float64Counter is an instrument that records increasing float64 values.
//
// Warning: methods may be added to this interface in minor releases.
type Float64Counter interface {
	// Add records a change to the counter.
	Add(ctx context.Context, incr float64, attrs ...attribute.KeyValue)

	Synchronous
}

type nonrecordingSyncFloat64Instrument struct {
	instrument.Float64Counter
}

SDK:

type instrumentImpl[N int64 | float64] struct {
	instrument.Synchronous

	aggregators []internal.Aggregator[N]
}

This is good "on paper" since you somehow achieve the goal to be able to extend the interface (based on your implementation only instrument.Synchronous can be extended) and still compile but not necessary able to run successfully the binary. This problem was present in Java versions before Java 1.8 when interfaces could not have default implementation for funcs, and you will hit the same problem, if a new func is added to the interface and someone calls it but the implementation does not override it you will get a panic.

The solution in Java < 1.8 was to use abstract classes, which we can/should implement here as well. We will ask the users to embed the "abstract class" instead of the struct.

The suggestion would be to do:

API

// No need for Sync.

// Float64Counter is an instrument that records increasing float64 values.
//
// Warning: methods may be added to this interface in minor releases.
type Float64Counter interface {
	// Add records a change to the counter.
	Add(ctx context.Context, incr float64, attrs ...attribute.KeyValue)

	internalFloat64Counter()
}

var _ instrument.Float64Counter = (*NoopFloat64Counter)(nil

// Noop implementation of the Float64Counter
type NoopFloat64Counter struct {}

// .. implement all funcs as no-op

SDK:
```golang

type instrumentImpl[N int64 | float64] struct {
	aggregators []internal.Aggregator[N]
}

type float64CounterImpl struct {
	instrument.NoopFloat64Counter
	instrumentImpl[float64]
}

This way every func added to Float64Counter will have a "no-op" or default behavior in any impl, even not updated.

[MrAlias]

I think this is a good idea. I think we will need to continue commenting our interface, but this allows implementers nervous of the accretion causing panics to embedded the no-op and inserted default to dropping.

I'll put together a PR tomorrow.

[MrAlias]

This issue touches on two ideas:

1. Include a private method on all exported API interfaces
2. Include a No-Op implementation users can embed in their implementations

In discussing this during last week's SIG meeting the synopsis was that (2) is desirable even without (1), but there is hesitation around (1). The main concern was that by requiring all implementations to embed an implementation or the interface itself it changed the compatibility problem where a user would get a compile time error to one of a runtime panic or a runtime ignoring.

Add private methods to all exported API interfaces

• Pros
  • Forces SDK authors to make an explicit choice about how they want unimplemented API methods to be handled from the start
  • Backwards compatible if we decide to switch.
    • If there is overwhelming SDK author feedback that they do not want this, it can be removed in a backwards compatible manner.
  • Not implementing an API method is not a compile error.
    • If the API updates without an SDK updating an end-user will still be able to compile their code.
• Cons
  • Allows lazy SDK developers to be surprised (though less surprised than if we do not add private methods).
    • A default response to needing to implement an unexported method is to just embed the interface in their implementation. If they don't look at the docs explaining about what this choice means and how there are alternatives they can "surprisingly" include runtime panics in their SDK.
  • Not implementing an API method is not a compile error.
    • Developers will need additional testing to ensure when new methods are added they add an implementation.
      • note: we can provide a generic testing framework to solve this.
    • If an SDK decides to just embed the interface itself they can easily ship an SDK that has runtime panics or unintended no-ops in it.

Do not add private methods to all exported API interfaces

• Pros
  • Not implementing an API method is a compile error.
    • SDK developers will be notified during their SDK build after upgrading the dependency on the new API that something broke. They will not need to build testing to check this.
• Cons
  • Allows lazy SDK developers to be surprised.
    • They can build, ship, and abandon an SDK thinking it will comply with the Go stability guarantees because they did not read the documentation. Their users will get stuck and not be able to upgrade their instrumentation unless the SDK devs update the SDK.
  • Not backwards compatible (according to Go stability guarantees) if we decide to switch.
    • If we want to switch to including private methods later on it would be a breaking change (though allowed by the compatibility policy).
  • Not implementing an API method is a compile error.
    • If the API updates without an SDK updating an end-user will not be able to compile their code if an instrumentation library upgrade uses the new API.

@Aneurysm9 please feel free to edit this or comment below with things I missed

[MrAlias]

Open alternatives to look into

Provide an implementation similar No-Op but it logs an error if an unimplemented method is called.

[MrAlias]

Another idea could be:

type MeterProviderKey interface {
	meterProvider()
}

type MeterProvider interface {
	MeterProviderKey
	
	// All the exported methods ...
}

type MeterKey interface {
	meter()
}

type Meter interface {
	MeterKey
	
	// All the exported methods ...
}

type InstrumentKey interface {
	instrument()
}

type Int64Counter interface {
	InstrumentKey
	
	// All the exported methods ...
}

// All the other instruments ...

This would separate the unexported method from the usable interface. This means that SDK authors could embed say MeterProviderKey if they wanted a compilation error, and MeterProvider or an implementation of MeterProvider if they wanted a panic or alt default behavior.

The (rather large) downside here is that there will need to be all these Key interfaces in the API that instrumentation authors will not use nor care about. These key interfaces could (I think) all be contained in sub-package, but they would need to be exported from somewhere for SDK devs to access.

[MrAlias]

Another idea could be: [...]

This actually isn't too bad if the "key" types are contained in their own package. Here is the proof-of-concept.

It provides an option that means SDK authors will still need to make an explicit choice (not embedding a type will not compile), but they can choose to embed a type from otel/metric/embed that will have their SDK not compile if a new method is added to an interface from otel/metric.

[MadVikingGod]

Would we have this same problem if only the SDK were to embed the NoOp implementation of the Interface in question? In theory, when we add a method to the interface and the NoOp, being part of the API package would get updated, makes the SDK not break.

This would mean that if an API user (library author) uses the new API they won't get any data. But I think this is better than an SDK user (application author) getting a cryptic message about &meterProvider not being a metric.MeterProvider when they update some unrelated dependency.

We could leave a warning to SDK authors (both us and others) that they should do the same to avoid breaking SDKs when the API updates.

[MrAlias]

Would we have this same problem if only the SDK were to embed the NoOp implementation of the Interface in question? In theory, when we add a method to the interface and the NoOp, being part of the API package would get updated, makes the SDK not break.

This would mean that if an API user (library author) uses the new API they won't get any data. But I think this is better than an SDK user (application author) getting a cryptic message about &meterProvider not being a metric.MeterProvider when they update some unrelated dependency.

We could leave a warning to SDK authors (both us and others) that they should do the same to avoid breaking SDKs when the API updates.

I'm not sure I follow.

Yes, the intention of adding a private method is to make SDK authors choose how they want unimplemented methods to be handled and embedding the No-Op in their implementation is an option. Is this what you mean?

[MadVikingGod]

Don't change the API interface; create in the API package a public NoOp implementation.

In the SDK, embed that implementation. When a new method is added, and you use an old SDK, it will fall back to the NoOp. This doesn't solve for a different SDK author not embedding, but we don't need to.

What this will solve is someone updating an dependency, which updates the API, and the application not compiling because sdk/*metric.meterProvider is no longer a metric.MeterProvider; and the solution is go get go.opentelemetry.io/otel/sdk/metric@latest but nothing tells our users that.

[MrAlias]

Don't change the API interface; create in the API package a public NoOp implementation.

In the SDK, embed that implementation. When a new method is added, and you use an old SDK, it will fall back to the NoOp. This doesn't solve for a different SDK author not embedding, but we don't need to.

What this will solve is someone updating an dependency, which updates the API, and the application not compiling because sdk/*metric.meterProvider is no longer a metric.MeterProvider; and the solution is go get go.opentelemetry.io/otel/sdk/metric@latest but nothing tells our users that.

Sure, this is the option broken down in the "Do not add private methods to all exported API interfaces" section of #3865 (comment) and using the No-Op from idea (1).

The question is do we want to signal to SDK authors that they need to make this choice by including a private method.

[MrAlias]

Possibly superseded by #3920