[DoNotMerge] sdk: implementations to embed noop instead of embedded

[pellared]

Fixes #6609

This allows users to use older versions of SDK and newer versions of API. This might be desired if e.g. newer versions of SDK has some bug or worse performance. This would also make bumping dependencies easier as bumping to a newer versions of API should not cause SDK compilation error.

[MrAlias]

How do we provide an implementation of all methods? Before we would get a compilation error, now it will silently just provide a no-op.

[pellared]

Before we would get a compilation error, now it will silently just provide a no-op.

Correct. This is the intention of this PR.

How do we provide an implementation of all methods?

Users that want an implementation for new methods would need to bump to newer version of the SDK which has the new methods implemented.

Do you have any other proposal?

[MrAlias]

Users that want an implementation for new methods would need to bump to newer version of the SDK which has the new methods implemented.

Right, but how do we know we have implemented all the known methods while we are developing? Before this change if I remove SetName from a span the build will fail. Afterwards it will compile and be released silently.

[MrAlias]

This feedback might also apply more generally. Do we really want users to be surprised when their telemetry doesn't show up the way they expect, or do we want to signal as soon as possible about the incompatibility of the SDK to implement the API fully?

It seems like embedding the noop lulls the user into a false sense of success. They will think the upgrade is successful, but their system will operate in a partially broken state.

[MrAlias]

Old versions of the SDK are not expected to support newer versions of the API.

https://github.com/open-telemetry/opentelemetry-specification/blob/9f803175fda2f9cca8a52524f4f2c8cf266827ef/specification/upgrading.md#api-changes

[pellared]

@MrAlias, your feedback makes totally sense. I think we should also get more feedback from end-users before making such change. Leaving the PR open for some time to encourage gathering feedback and adding "[Do not merge]" label. I am leaning towards closing the PR and referenced issue as "not planned".

[pellared]

@dmathieu, @XSAM, do you have any opinions?

[dmathieu]

This makes sense to me, and resolves many stability concerns that were raised before.

Regarding the full implementation question, how about adding a test checking that we implement noop?
That way, runtime keeps working if we're missing a method, but our test suite on main will fail.

[seankhliao]

As someone who writes code, I can't say I like the noop / grpc Unimplemented style very much.

As a consumer of the api, if I'm using the reference implementation in otel/sdk, I pretty much expect to not run in to silent / runtime failures from things being unimplemented, whether that's a new addition to an interface, or a new option that needs to be read / handled by the sdk.

I don't really have an opinion on what other implementations do.

While I understand why someone might want to allow version skew, IMO that should be explicit decisions handled and recorded through the standard go dependency constructs of replace and exclude directives, that is, they should be exceptional circumstances, and treated accordingly so.

Consider for example the addition of histogram boundary hints.
An application may remove their configuration of the sdk and switch to using the options in api, since it's much nicer to manage that way.
But a missed upgrade in sdk will leave them with no boundaries configured at all (api upgrade can't be missed for them to use the new options).

I think the only safe upgrade path should be sdk >= api, rather than api >= sdk, which is what this PR would promote.