From a01f2fb6d9f9ce5ddce75f0ad658a90da2179efd Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 17 Jun 2021 11:55:56 -0700 Subject: [PATCH 1/5] Interface stability documentation --- VERSIONING.md | 12 +++++++++++- error_handler.go | 5 +++++ exporters/otlp/otlptrace/clients.go | 11 +++++++++++ propagation/propagation.go | 22 ++++++++++++++++++++++ sdk/resource/auto.go | 5 +++++ sdk/trace/id_generator.go | 10 ++++++++++ sdk/trace/sampling.go | 11 +++++++++++ sdk/trace/span.go | 4 ++++ sdk/trace/span_exporter.go | 8 ++++++++ sdk/trace/span_processor.go | 11 +++++++++++ trace/trace.go | 6 ++++++ 11 files changed, 104 insertions(+), 1 deletion(-) diff --git a/VERSIONING.md b/VERSIONING.md index 3579b794ee9..d9415b8b830 100644 --- a/VERSIONING.md +++ b/VERSIONING.md @@ -12,7 +12,17 @@ is designed so the following goals can be achieved. * [Semantic import versioning](https://github.com/golang/go/wiki/Modules#semantic-import-versioning) will be used. - * Versions will comply with [semver 2.0](https://semver.org/spec/v2.0.0.html). + * Versions will comply with [semver 2.0](https://semver.org/spec/v2.0.0.html) with the following exceptions. + * New methods may be added to exported API interfaces. These + interfaces are exported for the sole purpose that SDKs will + implement them. Any and all SDKs assume the responsibility that + they will resolve this backwards-incompatible change when they + upgrade the SDK to support a newer API. All exported interfaces that + fall within this exception will include the following paragraph in + their public documentation. + + > Warning: methods may be added to this interface in minor releases. + * If a module is version `v2` or higher, the major version of the module must be included as a `/vN` at the end of the module paths used in `go.mod` files (e.g., `module go.opentelemetry.io/otel/v2`, `require diff --git a/error_handler.go b/error_handler.go index ac42f8be072..a51b2dee4e9 100644 --- a/error_handler.go +++ b/error_handler.go @@ -16,7 +16,12 @@ package otel // import "go.opentelemetry.io/otel" // ErrorHandler handles irremediable events. type ErrorHandler interface { + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // Handle handles any error deemed irremediable by an OpenTelemetry // component. Handle(error) + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. } diff --git a/exporters/otlp/otlptrace/clients.go b/exporters/otlp/otlptrace/clients.go index 896069fc821..dbb40cf5820 100644 --- a/exporters/otlp/otlptrace/clients.go +++ b/exporters/otlp/otlptrace/clients.go @@ -24,10 +24,16 @@ import ( // transformation of data into wire format, and the transmission of that // data to the collector. type Client interface { + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // Start should establish connection(s) to endpoint(s). It is // called just once by the exporter, so the implementation // does not need to worry about idempotence and locking. Start(ctx context.Context) error + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // Stop should close the connections. The function is called // only once by the exporter, so the implementation does not // need to worry about idempotence, but it may be called @@ -36,8 +42,13 @@ type Client interface { // synchronization point - after the function returns, the // process of closing connections is assumed to be finished. Stop(ctx context.Context) error + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // UploadTraces should transform the passed traces to the wire // format and send it to the collector. May be called // concurrently. UploadTraces(ctx context.Context, protoSpans []*tracepb.ResourceSpans) error + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. } diff --git a/propagation/propagation.go b/propagation/propagation.go index 9cfeb347a37..9fb6de66cd7 100644 --- a/propagation/propagation.go +++ b/propagation/propagation.go @@ -21,12 +21,23 @@ import ( // TextMapCarrier is the storage medium used by a TextMapPropagator. type TextMapCarrier interface { + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // Get returns the value associated with the passed key. Get(key string) string + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // Set stores the key-value pair. Set(key string, value string) + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // Keys lists the keys stored in this carrier. Keys() []string + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. } // HeaderCarrier adapts http.Header to satisfy the TextMapCarrier interface. @@ -54,12 +65,23 @@ func (hc HeaderCarrier) Keys() []string { // TextMapPropagator propagates cross-cutting concerns as key-value text // pairs within a carrier that travels in-band across process boundaries. type TextMapPropagator interface { + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // Inject set cross-cutting concerns from the Context into the carrier. Inject(ctx context.Context, carrier TextMapCarrier) + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // Extract reads cross-cutting concerns from the carrier into a Context. Extract(ctx context.Context, carrier TextMapCarrier) context.Context + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // Fields returns the keys who's values are set with Inject. Fields() []string + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. } type compositeTextMapPropagator []TextMapPropagator diff --git a/sdk/resource/auto.go b/sdk/resource/auto.go index b1c9b302d21..a5eaa7e5d39 100644 --- a/sdk/resource/auto.go +++ b/sdk/resource/auto.go @@ -29,12 +29,17 @@ var ( // Detector detects OpenTelemetry resource information type Detector interface { + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // Detect returns an initialized Resource based on gathered information. // If the source information to construct a Resource contains invalid // values, a Resource is returned with the valid parts of the source // information used for initialization along with an appropriately // wrapped ErrPartialResource error. Detect(ctx context.Context) (*Resource, error) + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. } // Detect calls all input detectors sequentially and merges each result with the previous one. diff --git a/sdk/trace/id_generator.go b/sdk/trace/id_generator.go index e60a421cde9..c9e2802ac53 100644 --- a/sdk/trace/id_generator.go +++ b/sdk/trace/id_generator.go @@ -26,8 +26,18 @@ import ( // IDGenerator allows custom generators for TraceID and SpanID. type IDGenerator interface { + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + + // NewIDs returns a new trace and span ID. NewIDs(ctx context.Context) (trace.TraceID, trace.SpanID) + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + + // NewSpanID returns a ID for a new span in the trace with traceID. NewSpanID(ctx context.Context, traceID trace.TraceID) trace.SpanID + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. } type randomIDGenerator struct { diff --git a/sdk/trace/sampling.go b/sdk/trace/sampling.go index 1fd3e9c465b..849248638c4 100644 --- a/sdk/trace/sampling.go +++ b/sdk/trace/sampling.go @@ -25,8 +25,19 @@ import ( // Sampler decides whether a trace should be sampled and exported. type Sampler interface { + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + + // ShouldSample returns a SamplingResult based on a decision made from the + // passed parameters. ShouldSample(parameters SamplingParameters) SamplingResult + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + + // Description returns information describing the Sampler. Description() string + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. } // SamplingParameters contains the values passed to a Sampler. diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 73f94d46a52..349ab4a9668 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -34,6 +34,8 @@ import ( // ReadOnlySpan allows reading information from the data structure underlying a // trace.Span. It is used in places where reading information from a span is // necessary but changing the span isn't necessary or allowed. +// +// Warning: methods may be added to this interface in minor releases. type ReadOnlySpan interface { // Name returns the name of the span. Name() string @@ -88,6 +90,8 @@ type ReadOnlySpan interface { // This interface exposes the union of the methods of trace.Span (which is a // "write-only" span) and ReadOnlySpan. New methods for writing or reading span // information should be added under trace.Span or ReadOnlySpan, respectively. +// +// Warning: methods may be added to this interface in minor releases. type ReadWriteSpan interface { trace.Span ReadOnlySpan diff --git a/sdk/trace/span_exporter.go b/sdk/trace/span_exporter.go index 2d5400223e5..9fb3d6eac3b 100644 --- a/sdk/trace/span_exporter.go +++ b/sdk/trace/span_exporter.go @@ -19,6 +19,9 @@ import "context" // SpanExporter handles the delivery of spans to external receivers. This is // the final component in the trace export pipeline. type SpanExporter interface { + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // ExportSpans exports a batch of spans. // // This function is called synchronously, so there is no concurrency @@ -31,9 +34,14 @@ type SpanExporter interface { // returned by this function are considered unrecoverable and will be // reported to a configured error Handler. ExportSpans(ctx context.Context, spans []ReadOnlySpan) error + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // Shutdown notifies the exporter of a pending halt to operations. The // exporter is expected to preform any cleanup or synchronization it // requires while honoring all timeouts and cancellations contained in // the passed context. Shutdown(ctx context.Context) error + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. } diff --git a/sdk/trace/span_processor.go b/sdk/trace/span_processor.go index 73f49815e8e..b649a2ff049 100644 --- a/sdk/trace/span_processor.go +++ b/sdk/trace/span_processor.go @@ -24,13 +24,20 @@ import ( // and end of a Span's lifecycle, and are called in the order they are // registered. type SpanProcessor interface { + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. + // OnStart is called when a span is started. It is called synchronously // and should not block. OnStart(parent context.Context, s ReadWriteSpan) + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. // OnEnd is called when span is finished. It is called synchronously and // hence not block. OnEnd(s ReadOnlySpan) + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. // Shutdown is called when the SDK shuts down. Any cleanup or release of // resources held by the processor should be done in this call. @@ -41,12 +48,16 @@ type SpanProcessor interface { // All timeouts and cancellations contained in ctx must be honored, this // should not block indefinitely. Shutdown(ctx context.Context) error + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. // ForceFlush exports all ended spans to the configured Exporter that have not yet // been exported. It should only be called when absolutely necessary, such as when // using a FaaS provider that may suspend the process after an invocation, but before // the Processor can export the completed spans. ForceFlush(ctx context.Context) error + // DO NOT CHANGE: any modification will not be backwards compatible and + // must never be done outside of a new major release. } type spanProcessorState struct { diff --git a/trace/trace.go b/trace/trace.go index a4b612341a6..15eb4673b41 100644 --- a/trace/trace.go +++ b/trace/trace.go @@ -337,6 +337,8 @@ func (sc SpanContext) MarshalJSON() ([]byte, error) { // and timed operation of a workflow that is traced. A Tracer is used to // create a Span and it is then up to the operation the Span represents to // properly end the Span when the operation itself ends. +// +// Warning: methods may be added to this interface in minor releases. type Span interface { // End completes the Span. The Span is considered complete and ready to be // delivered through the rest of the telemetry pipeline after this method @@ -478,6 +480,8 @@ func (sk SpanKind) String() string { } // Tracer is the creator of Spans. +// +// Warning: methods may be added to this interface in minor releases. type Tracer interface { // Start creates a span and a context.Context containing the newly-created span. // @@ -496,6 +500,8 @@ type Tracer interface { } // TracerProvider provides access to instrumentation Tracers. +// +// Warning: methods may be added to this interface in minor releases. type TracerProvider interface { // Tracer creates an implementation of the Tracer interface. // The instrumentationName must be the name of the library providing From 319d9ce2633ef9ce73a6024dfdf2ca14a41f6853 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 17 Jun 2021 12:33:58 -0700 Subject: [PATCH 2/5] Update versioning policy --- VERSIONING.md | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/VERSIONING.md b/VERSIONING.md index d9415b8b830..84996503e0e 100644 --- a/VERSIONING.md +++ b/VERSIONING.md @@ -13,13 +13,9 @@ is designed so the following goals can be achieved. versioning](https://github.com/golang/go/wiki/Modules#semantic-import-versioning) will be used. * Versions will comply with [semver 2.0](https://semver.org/spec/v2.0.0.html) with the following exceptions. - * New methods may be added to exported API interfaces. These - interfaces are exported for the sole purpose that SDKs will - implement them. Any and all SDKs assume the responsibility that - they will resolve this backwards-incompatible change when they - upgrade the SDK to support a newer API. All exported interfaces that - fall within this exception will include the following paragraph in - their public documentation. + * New methods may be added to exported API interfaces. All exported + interfaces that fall within this exception will include the following + paragraph in their public documentation. > Warning: methods may be added to this interface in minor releases. From c5b5ae64bc3ce7bf575380c9b496df570325971c Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 17 Jun 2021 12:58:47 -0700 Subject: [PATCH 3/5] Update CONTRIBUTING --- CONTRIBUTING.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index eec0975eb5a..d652b5acfa8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -380,7 +380,7 @@ func NewDog(name string, o ...DogOption) Dog {…} func NewBird(name string, o ...BirdOption) Bird {…} ``` -### Interface Type +### Interfaces To allow other developers to better comprehend the code, it is important to ensure it is sufficiently documented. One simple measure that contributes @@ -388,6 +388,15 @@ to this aim is self-documenting by naming method parameters. Therefore, where appropriate, methods of every exported interface type should have their parameters appropriately named. +#### Interface Stability + +All exported stable interfaces that include the following warning in their +doumentation are allowed to be extended with additional methods. + +> Warning: methods may be added to this interface in minor releases. + +Otherwise, stable interfaces MUST NOT be modified. + ## Approvers and Maintainers Approvers: From 59306e987ac2c41ff6808c5603fb03e297231e03 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 17 Jun 2021 16:45:02 -0700 Subject: [PATCH 4/5] Document how to extend immutable interfaces --- CONTRIBUTING.md | 54 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d652b5acfa8..d631339a5e6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -397,6 +397,60 @@ doumentation are allowed to be extended with additional methods. Otherwise, stable interfaces MUST NOT be modified. +If new functionality is needed for an interface that cannot be changed it MUST +be added by including an additional interface. That added 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: + +```go +type Exporter interface { + Export() +} +``` + +A new interface, `Closer`, can be added: + +```go +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. + +```go +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. + +```go +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. + ## Approvers and Maintainers Approvers: From 37b9eb7cb2d449dbc5b7882a7970abbec10248b8 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 17 Jun 2021 16:45:35 -0700 Subject: [PATCH 5/5] Markdown format VERSIONING changes --- VERSIONING.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/VERSIONING.md b/VERSIONING.md index 84996503e0e..fba712f6233 100644 --- a/VERSIONING.md +++ b/VERSIONING.md @@ -12,7 +12,8 @@ is designed so the following goals can be achieved. * [Semantic import versioning](https://github.com/golang/go/wiki/Modules#semantic-import-versioning) will be used. - * Versions will comply with [semver 2.0](https://semver.org/spec/v2.0.0.html) with the following exceptions. + * Versions will comply with [semver + 2.0](https://semver.org/spec/v2.0.0.html) with the following exceptions. * New methods may be added to exported API interfaces. All exported interfaces that fall within this exception will include the following paragraph in their public documentation.