Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Develop and implement a plan to extend interfaces #1714

Closed
2 of 3 tasks
MrAlias opened this issue Mar 19, 2021 · 5 comments · Fixed by #2012
Closed
2 of 3 tasks

Develop and implement a plan to extend interfaces #1714

MrAlias opened this issue Mar 19, 2021 · 5 comments · Fixed by #2012
Assignees
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Mar 19, 2021

The specification SIG has stated that they will consider additions to API/SDK interfaces that are not expected for instrumentation or operator users of OpenTelemetry to not be backwards incompatible. In Go adding a method to an Exported interface is a breaking change.

There are a few ways we can support these changes while remaining compatible:

  • Add a private method to the interfaces and require implementation to embed the interface. This topic is also explored in more detail in Jonathan Amsterdam's "Detecting Incompatible API Changes" talk. The take away is that you allow for compile time compatability, but will cause the client to panic at runtime if they don't implement the added method.
  • Add a private method to the interface and export a NoOp implementation that will need to be embedded. This is similar to the previous approach, but will have a silent failure or no action instead of a panic. This could be worse in cases where these silent failures are important and unnoticed by the user.
  • Plan to add interfaces as new methods are needed. Also talked about in Jonathan Amsterdam's talk and the Go blog.

To resolve this we need to:

  • Inventory what interfaces have the possibility to be extended by the specification.
  • Define a plan for how we will handle these situations.
    • If it is something where we need to update interfaces, we need to do that before a stable release.
  • Document this decision in our contributing guidelines.
@MrAlias MrAlias added this to the RC1 milestone Mar 19, 2021
@seh

This comment has been minimized.

@MrAlias MrAlias changed the title Update all interfaces we do not expect end users to implement to contain private methods Develop and implement a plan to extend interfaces Mar 19, 2021
@seh
Copy link
Contributor

seh commented Apr 8, 2021

Per our discussion on this week's SIG call, see the following two issues from the grpc/grpc-go project for background on a couple of pertinent techniques:

@MrAlias

This comment has been minimized.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jun 15, 2021

The way I understand the groups decision on how we want to handle interface changes in the future is conditional on what should implement the interface is and how it will be used.

  • Interfaces that we provide for end-users to implement and call.
  • Interfaces that we provide which will be implemented internally only.
  • Interfaces that we provide which will be implemented by SDK developers.

End-User Interfaces

These interfaces will be called by the SDK and can be user provided. Adding or changing methods for these interfaces will break user implementations and therefore MUST NOT be done.

These interfaces may also be called by the user themselves. Removing methods that users call from these interface definitions will break users' code and therefore MUST NOT be done.

These interfaces MUST NOT change once they are made stable.

If new functionality is needed for these interfaces it MUST be added by including an additional interface. That interface can be a simple interface for the specific functionality that you want to add or it can be a super-set of the original interface. For example, if you wanted to a Close method to the Exporter interface:

type Exporter interface {
	Export()
}

A new interface, Closer, can be added:

type Closer interface {
	Close()
}

Code that is passed the Exporter interface can now check to see if the passed value also satisfies the new interface. E.g.

func caller(e Exporter) {
	/* ... */
	if c, ok := e.(Closer); ok {
		c.Close()
	}
	/* ... */
}

Alternatively, a new type that is the super-set of an Exporter can be created.

type ClosingExporter struct {
	Exporter
	Close()
}

This new type can be used similar to the simple interface above in that a passed Exporter type can be asserted to satisfy the ClosingExporter type and the Close method called.

This super-set approach can be useful if there is explicit behavior that needs to be coupled with the original type and passed as a unified type to a new function, but, because of this coupling, it also limits the applicability of the added functionality. If there exist other interfaces where this functionality should be added, each one will need their own super-set interfaces and will duplicate the pattern. For this reason, the simple targeted interface that defines the specific functionality should be preferred.

Interfaces

Internal Only Interface Implementations

These interfaces should be guarded from external implementations. While not perfect, this should be done using an unexported method.

Interfaces

SDK Implemented Interfaces

These interfaces can only accrete, methods MUST NOT be removed or changed. Users are expected to call the methods of these interfaces.

These interfaces SHOULD NOT be guarded, a build failure should occur when things are added to the interface and SDK developers have not satisfied the new methods. This is an assumed responsibility that comes with development of an SDK.

The default SDK will be released in lock-step with any changes to the interfaces such that it satisfies the interface.

This divergence from the standard backwards compatibility guarantee of a Go API is part of the OpenTelemetry versioning policy and is meant to provide room to evolve the OpenTelemetry specification.

Interfaces

* these interfaces are also internal only implementation.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jun 15, 2021

An interesting point to consider in the already decided configuration policy is that if we instead configure by just passing a configuration struct type we would not have any "Internal Only Interface" and all code written by users could be checked by the compiler without exception.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants