Opentelemetry-go: Encapsulate OTel API

[MrAlias]

The recent move to make the OTel API more front-and-center and ultimately discoverable has also consequentially introduced issues.

1. Portions of the API (e.g. tracing, metrics) are not able to be versioned independently or be given different levels of stability. This split in versioning, while not desirable, may be needed if the specification states that different parts are going to be released at different times for GA.
2. The combination of the APIs into a single location had increased the learning burden on new instrumenters. While it is now much clearer the possible constructs and types a new users should be using as they are top level, it is harder to wade through the large volume of objects.

Proposal

Revised otel package structure:

├── registry
└── number
trace
propagation
baggage
internal
bridge
codes
example
exporters
label
oteltest
sdk
semconv
unit

metric

This package will contain the Metric API.

• ErrSDKReturnedNilImpl
• AsyncBatchRunner
• AsyncImpl
• AsyncRunner
• AsyncSingleRunner
• BatchObserver
• BatchObserverFunc
• BatchObserverMust
• BatchObserverResult
• BoundFloat64Counter
• BoundFloat64UpDownCounter
• BoundFloat64ValueRecorder
• BoundInt64Counter
• BoundInt64UpDownCounter
• BoundInt64ValueRecorder
• BoundSyncImpl
• Descriptor
• Float64Counter
• Float64ObserverFunc
• Float64ObserverResult
• Float64SumObserver
• Float64UpDownCounter
• Float64UpDownSumObserver
• Float64ValueObserver
• Float64ValueRecorder
• InstrumentConfig
• NewInstrumentConfig
• InstrumentImpl
• InstrumentKind
• InstrumentOption
• WithDescription
• WithInstrumentationName
• WithUnit
• InstrumentationOption
• WithInstrumentationVersion
• Int64Counter
• Int64ObserverFunc
• Int64ObserverResult
• Int64SumObserver
• Int64UpDownCounter
• Int64UpDownSumObserver
• Int64ValueObserver
• Int64ValueRecorder
• Measurement
• Meter
• WrapMeterImpl
• MeterConfig
• MeterImpl
• MeterMust
• MeterOption
• MeterProvider
• NoopAsync
• NoopMeterProvider
• NoopSync
• NumberKind
• Observation
• Must
• NewDescriptor
• SyncImpl
• NewMeterConfig

A follow on issue should track work to redesign the API closer to this proposal

registry

This package will be moved from the otel package to within the metric package unchanged.

number

This package will contain the otel number type.

• Number
• NewFloat64Number
• NewInt64Number
• NewNumberFromRaw
• NewNumberSignChange
• NumberKind

trace

This package will contain the Trace and part of the Context OpenTelemetry API.

• FlagsSampled
• FlagsDeferred
• FlagsDebug
• TracerProvider
• Tracer
• Span
• Link
• SpanID
• TraceID
• TraceIDFromHex
• SpanIDFromHex
• SpanContext
• ContextWithSpan
• SpanFromContext
• SpanContextFromContext
• ContextWithRemoteSpanContext
• RemoteSpanContextFromContext
• ValidateSpanKind
• LifeCycleOption
• WithAttributes
• WithTimestamp
• EventOption
• SpanKind
• NewEventConfig
• NewSpanConfig
• NewTracerConfig
• SpanConfig
• SpanOption
• TracerConfig
• TracerOption
• WithLinks
• WithRecord
• WithSpanKind
• NewNoopTracerProvider

propagation

This package will contain the propagation portions of the OpenTelemetry context API and the implementation of the TextMapPropagator.

• TextMapCarrier
• TextMapPropagator
• NewCompositeTextMapPropagator
• Baggage. This is the W3C baggage propagator.
• TraceContext

baggage

This package will contain the Baggage API.

• Baggage. This is the function func Baggage(ctx context.Context) label.Set
• BaggageValue
• ContextWithBaggageValues
• ContextWithoutBaggage
• ContextWithoutBaggageValues

internal

This package will continue to contain internal packages and functionality used.

bridge

The bridge package will be unchanged.

codes

The codes package will be unchanged.

example

The example package will be unchanged.

exporters

The exporters package will be unchanged.

label

The label package will be unchanged.

oteltest

The oteltest package will be unchanged.

sdk

The sdk package will be unchanged.

semconv

The semconv package will be unchanged.

unit

The unit package will be unchanged.

global

The current global package functionality will be moved into the otel package and this package will be removed.

• ErrorHandler renamed to GetErrorHandler. The ErrorHandler type will remain in the otel package.
• SetErrorHandler
• Handle.
• Meter.
• MeterProvider renamed to GetMeterProvider.
• SetMeterProvider.
• SetTextMapPropagator.
• TextMapPropagator renamed to GetTextMapPropagator.
• Tracer.
• TracerProvider renamed to GetTracerProvider.
• SetTracerProvider.

Versioning

The otel package will contain an version function that returns the project version.

• func Version() string

[krnowak]

I'm not sure if the listings above meant to be comprehensive (they looked like that), but I went through the exported types and functions in otel and I have some notes:

I'd add the following to the metric package:

• Must
• NewDescriptor
• SyncImpl
• NewMeterConfig

I'm not sure I'd put InstrumentationOption and WithInstrumentationVersion into metric package, though. InstrumentationOption brings in TracerOption which belongs to the trace package. Not sure if metric package should depend on trace.

There's also WithInstrumentationName which is confusing. Its name talks about Instrumentation, but the function actually returns InstrumentOption, not InstrumentationOption. I suppose it either should be name WithInstrumentName or should be fixed to return InstrumentationOption.

Also, I would not put NumberKind into metric - this should belong to the number package.

I'd add the following to the trace package:

• SpanKind
• NewEventConfig
• NewSpanConfig
• NewTracerConfig
• SpanConfig
• SpanOption
• TracerConfig
• TracerOption
• WithLinks
• WithRecord
• WithSpanKind
• NewNoopTracerProvider

The number package would be just the whole number.go file, so it would add the following:

• NewFloat64Number
• NewInt64Number
• NewNumberFromRaw
• NewNumberSignChange
• NumberKind (not in metric)

propagation and baggage confused me for a second, because both mention the Baggage type. But propagation wants the propagators.Baggage type, while baggage wants otel.Baggage.

So otel package would have contents of the global package and the Version function from sdk/opentelemetry.go, right? I'd add there also the following:

• InstrumentationOption
• WithInstrumentationName (if it should actually return InstrumentationOption)
• WithInstrumentationVersion

[MrAlias]

I'm not sure if the listings above meant to be comprehensive (they looked like that), but I went through the exported types and functions in otel and I have some notes:

Thanks for thoroughly double checking this.

I'd add the following to the metric package:

• Must
• NewDescriptor
• SyncImpl
• NewMeterConfig

Agreed, I've updated the description to included these in the proposal.

I'm not sure I'd put InstrumentationOption and WithInstrumentationVersion into metric package, though. InstrumentationOption brings in TracerOption which belongs to the trace package. Not sure if metric package should depend on trace.

Yes, I should have explained this better. I think we will need to add an InstrumentationOption and WithInstrumentationVersion back into both the metric and trace package. Effectively splitting this back into two different types in the different packages. The metric one passed to a NewMeterProvider and the trace one passed to a NewTracerProvider.

There's also WithInstrumentationName which is confusing. Its name talks about Instrumentation, but the function actually returns InstrumentOption, not InstrumentationOption. I suppose it either should be name WithInstrumentName or should be fixed to return InstrumentationOption.

Indeed this is a bit confusing. I seem to remember this being a hold over from our implementation of the wrapping utilities in the metrics package. I'm not sure if this is needed any more as this information should be coming from the MeterProvider not the creator of the instruments. I think we should move this as is and tackle cleaning this up in a separate issue. Thoughts?

Also, I would not put NumberKind into metric - this should belong to the number package.

Good catch.

I'd add the following to the trace package:

• SpanKind
• NewEventConfig
• NewSpanConfig
• NewTracerConfig
• SpanConfig
• SpanOption
• TracerConfig
• TracerOption
• WithLinks
• WithRecord
• WithSpanKind
• NewNoopTracerProvider

Agreed. Updated the description with this.

The number package would be just the whole number.go file, so it would add the following:

• NewFloat64Number
• NewInt64Number
• NewNumberFromRaw
• NewNumberSignChange
• NumberKind (not in metric)

Agreed. I've updated the description with this.

propagation and baggage confused me for a second, because both mention the Baggage type. But propagation wants the propagators.Baggage type, while baggage wants otel.Baggage.

Correct. This separation seems to organize code by their functional responsibilities. At lease that was the goal.

So otel package would have contents of the global package and the Version function from sdk/opentelemetry.go, right?

I had initially considered having an additional Version function from the SDK, but now that you point it out it would probably make more sense to do like you describe and move the Version function from the sdk to the otel package. I don't see a case where the SDK version will differ from the otel/API package and if that happens we can add that in a backwards compatible way.

I'd add there also the following:

• InstrumentationOption
• WithInstrumentationName (if it should actually return InstrumentationOption)
• WithInstrumentationVersion

I think we should split these up like mentioned above. I understand that it would be nice to not have repeated code, but I think trying to preserve this will only lead to an overall more complex code base and will require an import of the otel package in addition to a metric or trace import`.

[krnowak]

I'm not sure I'd put InstrumentationOption and WithInstrumentationVersion into metric package, though. InstrumentationOption brings in TracerOption which belongs to the trace package. Not sure if metric package should depend on trace.

Yes, I should have explained this better. I think we will need to add an InstrumentationOption and WithInstrumentationVersion back into both the metric and trace package. Effectively splitting this back into two different types in the different packages. The metric one passed to a NewMeterProvider and the trace one passed to a NewTracerProvider.

Cool, thanks for making this clear.

There's also WithInstrumentationName which is confusing. Its name talks about Instrumentation, but the function actually returns InstrumentOption, not InstrumentationOption. I suppose it either should be name WithInstrumentName or should be fixed to return InstrumentationOption.

Indeed this is a bit confusing. I seem to remember this being a hold over from our implementation of the wrapping utilities in the metrics package. I'm not sure if this is needed any more as this information should be coming from the MeterProvider not the creator of the instruments. I think we should move this as is and tackle cleaning this up in a separate issue. Thoughts?

Agreed to solve this in a separate issue. For now I'll just move it to the metric package without changing it in any way then.

Also, I would not put NumberKind into metric - this should belong to the number package.

Good catch.

One fallout from this reorganization of API will the return of stuttering types. number.Number, trace.TraceID, baggage.Baggage and likely more (mostly in trace and number packages). Is this something we should solve in this issue too? I remember naming issues taking a rather long time to be resolved…

One more: metric or metrics? It used to be the former I think, you propose the latter.

[MrAlias]

One fallout from this reorganization of API will the return of stuttering types. number.Number, trace.TraceID, baggage.Baggage and likely more (mostly in trace and number packages). Is this something we should solve in this issue too? I remember naming issues taking a rather long time to be resolved…

This is a good question. I think it is tractable to address here based on an inventory of the listed move, but agree if there is contention I think we should address in a separate issue.

For the number package:

• type Number struct. I do not see us being able to change this. It correctly describes the contents of the structure and this type of "stuttering" is common in Go.
• type NumberKind int8. It makes sense to rename this to Kind.

For the baggage package:

• func Baggage(ctx context.Context) label.Set. This seems like a good candidate to be renamed to Set as that is the returned value.
• BaggageValue. It makes sense to rename this to Value.

For the trace package there is only going to be the TraceID and related TraceIDFromHex function. If I recall correctly, this is a former point of contention. I would suggest just moving these as is and we can discuss in a follow-on issue if there is a need.

Personally, I think preserving this name is more descriptive of the type and points out that the package trace might not be the best name (given it contains more than just a "trace" structure or functionality, rather _tracing_ structures and functionality). That and there has been confusion in the past what the ID was for given this package is almost always renamed to something other than `trace.

@open-telemetry/go-approvers thoughts?

One more: metric or metrics? It used to be the former I think, you propose the latter.

Good catch. I had copied it from this, but we should remain consistent with the sdk and export package here and stick with metric. I've updated the description.

[XSAM]

For the baggage package:

• func Baggage(ctx context.Context) label.Set
• BaggageValue

Agreed.

I think the rest of the baggage package should have a better naming:

• ContextWithBaggageValues. Rename to ContextWithValues seems concise.
• ContextWithoutBaggage. Rename to ContextWithEmpty might be a good choice.
• ContextWithoutBaggageValues. Rename to ContextWithoutValues.

[krnowak]

So there is a PR for moving the trace code to trace package (#1307). The rest of the changes are in my new-api-2 branch (https://github.com/kinvolk/opentelemetry-go/tree/krnowak/new-api-2).