Update API configs. (open-telemetry#1921)

* Added Reason to Contributing and Updated TracerConfig

* PR comment fixup

* Changed how span Options work.

* Fix Markdown linting

* Added meter configs.

* Fixes from PR comments

* fix for missing instrument

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

CHANGELOG.md

@@ -50,6 +50,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Spans created by the global `Tracer` obtained from `go.opentelemetry.io/otel`, prior to a functioning `TracerProvider` being set, now propagate the span context from their parent if one exists. (#1901)
 - The `"go.opentelemetry.io/otel".Tracer` function now accepts tracer options. (#1902)
 - Move the `go.opentelemetry.io/otel/unit` package to `go.opentelemetry.io/otel/metric/unit`. (#1903)
+- Changed `go.opentelemetry.io/otel/trace.TracerConfig` to conform to the [Contributing guidelines](CONTRIBUTING.md#config.) (#1921)
+- Changed `go.opentelemetry.io/otel/trace.SpanConfig` to conform to the [Contributing guidelines](CONTRIBUTING.md#config). (#1921)
+- Changed `span.End()` now only accepts Options that are allowed at `end()`. (#1921)
+- Changed `go.opentelemetry.io/otel/metric.InstrumentConfig` to conform to the [Contributing guidelines](CONTRIBUTING.md#config). (#1921)
+- Changed `go.opentelemetry.io/otel/metric.MeterConfig` to conform to the [Contributing guidelines](CONTRIBUTING.md#config). (#1921)
 - Refactor option types according to the contribution style guide. (#1882)
 - Move the `go.opentelemetry.io/otel/trace.TraceStateFromKeyValues` function to the `go.opentelemetry.io/otel/oteltest` package.
   This function is preserved for testing purposes where it may be useful to create a `TraceState` from `attribute.KeyValue`s, but it is not intended for production use.

CONTRIBUTING.md

@@ -190,8 +190,16 @@ will likely want to build custom options for the configuration, the `config`
 should be exported. Please, include in the documentation for the `config`
 how the user can extend the configuration.
 
-It is important that `config` are not shared across package boundaries.
-Meaning a `config` from one package should not be directly used by another.
+It is important that internal `config` are not shared across package boundaries.
+Meaning a `config` from one package should not be directly used by another. The
+one exception is the API packages.  The configs from the base API, eg.
+`go.opentelemetry.io/otel/trace.TracerConfig` and
+`go.opentelemetry.io/otel/metric.InstrumentConfig`, are intended to be consumed
+by the SDK therefor it is expected that these are exported.
+
+When a config is exported we want to maintain forward and backward
+compatibility, to achieve this no fields should be exported but should
+instead be accessed by methods.
 
 Optionally, it is common to include a `newConfig` function (with the same
 naming scheme). This function wraps any defaults setting and looping over

bridge/opencensus/bridge.go

@@ -45,12 +45,12 @@ func (o *otelTracer) StartSpan(ctx context.Context, name string, s ...octrace.St
 	return ctx, octrace.NewSpan(&span{otSpan: sp})
 }
 
-func convertStartOptions(optFns []octrace.StartOption, name string) []trace.SpanOption {
+func convertStartOptions(optFns []octrace.StartOption, name string) []trace.SpanStartOption {
 	var ocOpts octrace.StartOptions
 	for _, fn := range optFns {
 		fn(&ocOpts)
 	}
-	otOpts := []trace.SpanOption{}
+	otOpts := []trace.SpanStartOption{}
 	switch ocOpts.SpanKind {
 	case octrace.SpanKindClient:
 		otOpts = append(otOpts, trace.WithSpanKind(trace.SpanKindClient))

bridge/opentracing/bridge.go

@@ -98,7 +98,7 @@ func (s *bridgeSpan) Finish() {
 }
 
 func (s *bridgeSpan) FinishWithOptions(opts ot.FinishOptions) {
-	var otelOpts []trace.SpanOption
+	var otelOpts []trace.SpanEndOption
 
 	if !opts.FinishTime.IsZero() {
 		otelOpts = append(otelOpts, trace.WithTimestamp(opts.FinishTime))

bridge/opentracing/internal/mock.go

@@ -70,9 +70,9 @@ func NewMockTracer() *MockTracer {
 	}
 }
 
-func (t *MockTracer) Start(ctx context.Context, name string, opts ...trace.SpanOption) (context.Context, trace.Span) {
-	config := trace.NewSpanConfig(opts...)
-	startTime := config.Timestamp
+func (t *MockTracer) Start(ctx context.Context, name string, opts ...trace.SpanStartOption) (context.Context, trace.Span) {
+	config := trace.NewSpanStartConfig(opts...)
+	startTime := config.Timestamp()
 	if startTime.IsZero() {
 		startTime = time.Now()
 	}
@@ -86,13 +86,13 @@ func (t *MockTracer) Start(ctx context.Context, name string, opts ...trace.SpanO
 		officialTracer: t,
 		spanContext:    spanContext,
 		Attributes: baggage.NewMap(baggage.MapUpdate{
-			MultiKV: config.Attributes,
+			MultiKV: config.Attributes(),
 		}),
 		StartTime:    startTime,
 		EndTime:      time.Time{},
 		ParentSpanID: t.getParentSpanID(ctx, config),
 		Events:       nil,
-		SpanKind:     trace.ValidateSpanKind(config.SpanKind),
+		SpanKind:     trace.ValidateSpanKind(config.SpanKind()),
 	}
 	if !migration.SkipContextSetup(ctx) {
 		ctx = trace.ContextWithSpan(ctx, span)
@@ -137,7 +137,7 @@ func (t *MockTracer) getParentSpanID(ctx context.Context, config *trace.SpanConf
 }
 
 func (t *MockTracer) getParentSpanContext(ctx context.Context, config *trace.SpanConfig) trace.SpanContext {
-	if !config.NewRoot {
+	if !config.NewRoot() {
 		return trace.SpanContextFromContext(ctx)
 	}
 	return trace.SpanContext{}
@@ -232,12 +232,12 @@ func (s *MockSpan) applyUpdate(update baggage.MapUpdate) {
 	s.Attributes = s.Attributes.Apply(update)
 }
 
-func (s *MockSpan) End(options ...trace.SpanOption) {
+func (s *MockSpan) End(options ...trace.SpanEndOption) {
 	if !s.EndTime.IsZero() {
 		return // already finished
 	}
-	config := trace.NewSpanConfig(options...)
-	endTime := config.Timestamp
+	config := trace.NewSpanEndConfig(options...)
+	endTime := config.Timestamp()
 	if endTime.IsZero() {
 		endTime = time.Now()
 	}
@@ -269,10 +269,10 @@ func (s *MockSpan) Tracer() trace.Tracer {
 func (s *MockSpan) AddEvent(name string, o ...trace.EventOption) {
 	c := trace.NewEventConfig(o...)
 	s.Events = append(s.Events, MockEvent{
-		Timestamp: c.Timestamp,
+		Timestamp: c.Timestamp(),
 		Name:      name,
 		Attributes: baggage.NewMap(baggage.MapUpdate{
-			MultiKV: c.Attributes,
+			MultiKV: c.Attributes(),
 		}),
 	})
 }

bridge/opentracing/wrapper.go

@@ -74,7 +74,7 @@ func (t *WrapperTracer) otelTracer() trace.Tracer {
 // Start forwards the call to the wrapped tracer. It also tries to
 // override the tracer of the returned span if the span implements the
 // OverrideTracerSpanExtension interface.
-func (t *WrapperTracer) Start(ctx context.Context, name string, opts ...trace.SpanOption) (context.Context, trace.Span) {
+func (t *WrapperTracer) Start(ctx context.Context, name string, opts ...trace.SpanStartOption) (context.Context, trace.Span) {
 	ctx, span := t.otelTracer().Start(ctx, name, opts...)
 	if spanWithExtension, ok := span.(migration.OverrideTracerSpanExtension); ok {
 		spanWithExtension.OverrideTracer(t)

internal/global/meter.go

@@ -153,7 +153,7 @@ func (p *meterProvider) Meter(instrumentationName string, opts ...metric.MeterOp
 
 	key := meterKey{
 		Name:    instrumentationName,
-		Version: metric.NewMeterConfig(opts...).InstrumentationVersion,
+		Version: metric.NewMeterConfig(opts...).InstrumentationVersion(),
 	}
 	entry, ok := p.meters[key]
 	if !ok {

internal/global/trace.go

@@ -92,7 +92,7 @@ func (p *tracerProvider) Tracer(name string, opts ...trace.TracerOption) trace.T
 
 	key := il{
 		name:    name,
-		version: trace.NewTracerConfig(opts...).InstrumentationVersion,
+		version: trace.NewTracerConfig(opts...).InstrumentationVersion(),
 	}
 
 	if p.tracers == nil {
@@ -139,7 +139,7 @@ func (t *tracer) setDelegate(provider trace.TracerProvider) {
 
 // Start implements trace.Tracer by forwarding the call to t.delegate if
 // set, otherwise it forwards the call to a NoopTracer.
-func (t *tracer) Start(ctx context.Context, name string, opts ...trace.SpanOption) (context.Context, trace.Span) {
+func (t *tracer) Start(ctx context.Context, name string, opts ...trace.SpanStartOption) (context.Context, trace.Span) {
 	delegate := t.delegate.Load()
 	if delegate != nil {
 		return delegate.(trace.Tracer).Start(ctx, name, opts...)
@@ -175,7 +175,7 @@ func (nonRecordingSpan) SetError(bool) {}
 func (nonRecordingSpan) SetAttributes(...attribute.KeyValue) {}
 
 // End does nothing.
-func (nonRecordingSpan) End(...trace.SpanOption) {}
+func (nonRecordingSpan) End(...trace.SpanEndOption) {}
 
 // RecordError does nothing.
 func (nonRecordingSpan) RecordError(error, ...trace.EventOption) {}

internal/global/trace_test.go

@@ -74,10 +74,10 @@ func (fn fnTracerProvider) Tracer(instrumentationName string, opts ...trace.Trac
 }
 
 type fnTracer struct {
-	start func(ctx context.Context, spanName string, opts ...trace.SpanOption) (context.Context, trace.Span)
+	start func(ctx context.Context, spanName string, opts ...trace.SpanStartOption) (context.Context, trace.Span)
 }
 
-func (fn fnTracer) Start(ctx context.Context, spanName string, opts ...trace.SpanOption) (context.Context, trace.Span) {
+func (fn fnTracer) Start(ctx context.Context, spanName string, opts ...trace.SpanStartOption) (context.Context, trace.Span) {
 	return fn.start(ctx, spanName, opts...)
 }
 
@@ -93,7 +93,6 @@ func TestTraceProviderDelegates(t *testing.T) {
 		tracer: func(name string, opts ...trace.TracerOption) trace.Tracer {
 			called = true
 			assert.Equal(t, "abc", name)
-			assert.Equal(t, []trace.TracerOption{trace.WithInstrumentationVersion("xyz")}, opts)
 			return trace.NewNoopTracerProvider().Tracer("")
 		},
 	})
@@ -131,7 +130,6 @@ func TestTraceProviderDelegatesConcurrentSafe(t *testing.T) {
 		tracer: func(name string, opts ...trace.TracerOption) trace.Tracer {
 			newVal := atomic.AddInt32(&called, 1)
 			assert.Equal(t, "abc", name)
-			assert.Equal(t, []trace.TracerOption{trace.WithInstrumentationVersion("xyz")}, opts)
 			if newVal == 10 {
 				// Signal the goroutine to finish.
 				close(quit)
@@ -175,9 +173,8 @@ func TestTracerDelegatesConcurrentSafe(t *testing.T) {
 	otel.SetTracerProvider(fnTracerProvider{
 		tracer: func(name string, opts ...trace.TracerOption) trace.Tracer {
 			assert.Equal(t, "abc", name)
-			assert.Equal(t, []trace.TracerOption{trace.WithInstrumentationVersion("xyz")}, opts)
 			return fnTracer{
-				start: func(ctx context.Context, spanName string, opts ...trace.SpanOption) (context.Context, trace.Span) {
+				start: func(ctx context.Context, spanName string, opts ...trace.SpanStartOption) (context.Context, trace.Span) {
 					newVal := atomic.AddInt32(&called, 1)
 					assert.Equal(t, "name", spanName)
 					if newVal == 10 {

metric/config.go

@@ -20,109 +20,122 @@ import (
 
 // InstrumentConfig contains options for metric instrument descriptors.
 type InstrumentConfig struct {
-	// Description describes the instrument in human-readable terms.
-	Description string
-	// Unit describes the measurement unit for a instrument.
-	Unit unit.Unit
-	// InstrumentationName is the name of the library providing
-	// instrumentation.
-	InstrumentationName string
-	// InstrumentationVersion is the version of the library providing
-	// instrumentation.
-	InstrumentationVersion string
+	description            string
+	unit                   unit.Unit
+	instrumentationName    string
+	instrumentationVersion string
+}
+
+// Description describes the instrument in human-readable terms.
+func (cfg InstrumentConfig) Description() string {
+	return cfg.description
+}
+
+// Unit describes the measurement unit for a instrument.
+func (cfg InstrumentConfig) Unit() unit.Unit {
+	return cfg.unit
+}
+
+// InstrumentationName is the name of the library providing
+// instrumentation.
+func (cfg InstrumentConfig) InstrumentationName() string {
+	return cfg.instrumentationName
+}
+
+// InstrumentationVersion is the version of the library providing
+// instrumentation.
+func (cfg InstrumentConfig) InstrumentationVersion() string {
+	return cfg.instrumentationVersion
 }
 
 // InstrumentOption is an interface for applying metric instrument options.
 type InstrumentOption interface {
 	// ApplyMeter is used to set a InstrumentOption value of a
 	// InstrumentConfig.
-	ApplyInstrument(*InstrumentConfig)
+	applyInstrument(*InstrumentConfig)
 }
 
 // NewInstrumentConfig creates a new InstrumentConfig
 // and applies all the given options.
 func NewInstrumentConfig(opts ...InstrumentOption) InstrumentConfig {
 	var config InstrumentConfig
 	for _, o := range opts {
-		o.ApplyInstrument(&config)
+		o.applyInstrument(&config)
 	}
 	return config
 }
 
-// WithDescription applies provided description.
-func WithDescription(desc string) InstrumentOption {
-	return descriptionOption(desc)
-}
+type instrumentOptionFunc func(*InstrumentConfig)
 
-type descriptionOption string
+func (fn instrumentOptionFunc) applyInstrument(cfg *InstrumentConfig) {
+	fn(cfg)
+}
 
-func (d descriptionOption) ApplyInstrument(config *InstrumentConfig) {
-	config.Description = string(d)
+// WithDescription applies provided description.
+func WithDescription(desc string) InstrumentOption {
+	return instrumentOptionFunc(func(cfg *InstrumentConfig) {
+		cfg.description = desc
+	})
 }
 
 // WithUnit applies provided unit.
 func WithUnit(unit unit.Unit) InstrumentOption {
-	return unitOption(unit)
-}
-
-type unitOption unit.Unit
-
-func (u unitOption) ApplyInstrument(config *InstrumentConfig) {
-	config.Unit = unit.Unit(u)
+	return instrumentOptionFunc(func(cfg *InstrumentConfig) {
+		cfg.unit = unit
+	})
 }
 
 // WithInstrumentationName sets the instrumentation name.
 func WithInstrumentationName(name string) InstrumentOption {
-	return instrumentationNameOption(name)
-}
-
-type instrumentationNameOption string
-
-func (i instrumentationNameOption) ApplyInstrument(config *InstrumentConfig) {
-	config.InstrumentationName = string(i)
+	return instrumentOptionFunc(func(cfg *InstrumentConfig) {
+		cfg.instrumentationName = name
+	})
 }
 
 // MeterConfig contains options for Meters.
 type MeterConfig struct {
-	// InstrumentationVersion is the version of the library providing
-	// instrumentation.
-	InstrumentationVersion string
+	instrumentationVersion string
+}
+
+// InstrumentationVersion is the version of the library providing instrumentation.
+func (cfg MeterConfig) InstrumentationVersion() string {
+	return cfg.instrumentationVersion
 }
 
 // MeterOption is an interface for applying Meter options.
 type MeterOption interface {
 	// ApplyMeter is used to set a MeterOption value of a MeterConfig.
-	ApplyMeter(*MeterConfig)
+	applyMeter(*MeterConfig)
 }
 
 // NewMeterConfig creates a new MeterConfig and applies
 // all the given options.
 func NewMeterConfig(opts ...MeterOption) MeterConfig {
 	var config MeterConfig
 	for _, o := range opts {
-		o.ApplyMeter(&config)
+		o.applyMeter(&config)
 	}
 	return config
 }
 
-// InstrumentationOption is an interface for applying instrumentation specific
-// options.
-type InstrumentationOption interface {
+// InstrumentMeterOption are options that can be used as both an InstrumentOption
+// and MeterOption
+type InstrumentMeterOption interface {
 	InstrumentOption
 	MeterOption
 }
 
 // WithInstrumentationVersion sets the instrumentation version.
-func WithInstrumentationVersion(version string) InstrumentationOption {
+func WithInstrumentationVersion(version string) InstrumentMeterOption {
 	return instrumentationVersionOption(version)
 }
 
 type instrumentationVersionOption string
 
-func (i instrumentationVersionOption) ApplyMeter(config *MeterConfig) {
-	config.InstrumentationVersion = string(i)
+func (i instrumentationVersionOption) applyMeter(config *MeterConfig) {
+	config.instrumentationVersion = string(i)
 }
 
-func (i instrumentationVersionOption) ApplyInstrument(config *InstrumentConfig) {
-	config.InstrumentationVersion = string(i)
+func (i instrumentationVersionOption) applyInstrument(config *InstrumentConfig) {
+	config.instrumentationVersion = string(i)
 }