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

Update API configs. #1921

Merged
merged 9 commits into from
May 27, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ 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)
- Refactor option types according to the contribution style guide. (#1882)

### Deprecated
Expand Down
12 changes: 10 additions & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion bridge/opentracing/bridge.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
20 changes: 10 additions & 10 deletions bridge/opentracing/internal/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ 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
config := trace.NewSpanStartConfig(opts...)
startTime := config.Timestamp()
if startTime.IsZero() {
startTime = time.Now()
}
Expand All @@ -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)
Expand Down Expand Up @@ -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{}
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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(),
}),
})
}
Expand Down
4 changes: 2 additions & 2 deletions internal/global/trace.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {}
Expand Down
3 changes: 0 additions & 3 deletions internal/global/trace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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("")
},
})
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -175,7 +173,6 @@ 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) {
newVal := atomic.AddInt32(&called, 1)
Expand Down
4 changes: 2 additions & 2 deletions oteltest/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,15 @@ func (p *TracerProvider) Tracer(instName string, opts ...trace.TracerOption) tra

inst := instrumentation{
Name: instName,
Version: conf.InstrumentationVersion,
Version: conf.InstrumentationVersion(),
}
p.tracersMu.Lock()
defer p.tracersMu.Unlock()
t, ok := p.tracers[inst]
if !ok {
t = &Tracer{
Name: instName,
Version: conf.InstrumentationVersion,
Version: conf.InstrumentationVersion(),
config: &p.config,
}
p.tracers[inst] = t
Expand Down
12 changes: 6 additions & 6 deletions oteltest/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,17 +49,17 @@ type Span struct {
// End ends s. If the Tracer that created s was configured with a
// SpanRecorder, that recorder's OnEnd method is called as the final part of
// this method.
func (s *Span) End(opts ...trace.SpanOption) {
func (s *Span) End(opts ...trace.SpanEndOption) {
s.lock.Lock()
defer s.lock.Unlock()

if s.ended {
return
}

c := trace.NewSpanConfig(opts...)
c := trace.NewSpanEndConfig(opts...)
s.endTime = time.Now()
if endTime := c.Timestamp; !endTime.IsZero() {
if endTime := c.Timestamp(); !endTime.IsZero() {
s.endTime = endTime
}

Expand Down Expand Up @@ -101,15 +101,15 @@ func (s *Span) AddEvent(name string, o ...trace.EventOption) {
c := trace.NewEventConfig(o...)

var attributes map[attribute.Key]attribute.Value
if l := len(c.Attributes); l > 0 {
if l := len(c.Attributes()); l > 0 {
attributes = make(map[attribute.Key]attribute.Value, l)
for _, attr := range c.Attributes {
for _, attr := range c.Attributes() {
attributes[attr.Key] = attr.Value
}
}

s.events = append(s.events, Event{
Timestamp: c.Timestamp,
Timestamp: c.Timestamp(),
Name: name,
Attributes: attributes,
})
Expand Down
12 changes: 6 additions & 6 deletions oteltest/tracer.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ type Tracer struct {
// Start creates a span. If t is configured with a SpanRecorder its OnStart
// method will be called after the created Span has been initialized.
func (t *Tracer) Start(ctx context.Context, name string, opts ...trace.SpanOption) (context.Context, trace.Span) {
c := trace.NewSpanConfig(opts...)
c := trace.NewSpanStartConfig(opts...)
startTime := time.Now()
if st := c.Timestamp; !st.IsZero() {
if st := c.Timestamp(); !st.IsZero() {
startTime = st
}

Expand All @@ -48,10 +48,10 @@ func (t *Tracer) Start(ctx context.Context, name string, opts ...trace.SpanOptio
startTime: startTime,
attributes: make(map[attribute.Key]attribute.Value),
links: []trace.Link{},
spanKind: c.SpanKind,
spanKind: c.SpanKind(),
}

if c.NewRoot {
if c.NewRoot() {
span.spanContext = trace.SpanContext{}
} else {
span.spanContext = t.config.SpanContextFunc(ctx)
Expand All @@ -61,7 +61,7 @@ func (t *Tracer) Start(ctx context.Context, name string, opts ...trace.SpanOptio
}
}

for _, link := range c.Links {
for _, link := range c.Links() {
for i, sl := range span.links {
if sl.SpanContext.SpanID() == link.SpanContext.SpanID() &&
sl.SpanContext.TraceID() == link.SpanContext.TraceID() &&
Expand All @@ -75,7 +75,7 @@ func (t *Tracer) Start(ctx context.Context, name string, opts ...trace.SpanOptio
}

span.SetName(name)
span.SetAttributes(c.Attributes...)
span.SetAttributes(c.Attributes()...)

if t.config.SpanRecorder != nil {
t.config.SpanRecorder.OnStart(span)
Expand Down
2 changes: 1 addition & 1 deletion sdk/trace/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ func (p *TracerProvider) Tracer(name string, opts ...trace.TracerOption) trace.T
}
il := instrumentation.Library{
Name: name,
Version: c.InstrumentationVersion,
Version: c.InstrumentationVersion(),
}
t, ok := p.namedTracer[il]
if !ok {
Expand Down
32 changes: 16 additions & 16 deletions sdk/trace/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ func (s *span) SetAttributes(attributes ...attribute.KeyValue) {
//
// If this method is called while panicking an error event is added to the
// Span before ending it and the panic is continued.
func (s *span) End(options ...trace.SpanOption) {
func (s *span) End(options ...trace.SpanEndOption) {
// Do not start by checking if the span is being recorded which requires
// acquiring a lock. Make a minimal check that the span is not nil.
if s == nil {
Expand Down Expand Up @@ -247,14 +247,14 @@ func (s *span) End(options ...trace.SpanOption) {
s.executionTracerTaskEnd()
}

config := trace.NewSpanConfig(options...)
config := trace.NewSpanEndConfig(options...)

s.mu.Lock()
// Setting endTime to non-zero marks the span as ended and not recording.
if config.Timestamp.IsZero() {
if config.Timestamp().IsZero() {
s.endTime = et
} else {
s.endTime = config.Timestamp
s.endTime = config.Timestamp()
}
s.mu.Unlock()

Expand Down Expand Up @@ -305,19 +305,19 @@ func (s *span) addEvent(name string, o ...trace.EventOption) {
c := trace.NewEventConfig(o...)

// Discard over limited attributes
attributes := c.Attributes()
var discarded int
if len(c.Attributes) > s.spanLimits.AttributePerEventCountLimit {
discarded = len(c.Attributes) - s.spanLimits.AttributePerEventCountLimit
c.Attributes = c.Attributes[:s.spanLimits.AttributePerEventCountLimit]
if len(attributes) > s.spanLimits.AttributePerEventCountLimit {
discarded = len(attributes) - s.spanLimits.AttributePerEventCountLimit
attributes = attributes[:s.spanLimits.AttributePerEventCountLimit]
}

s.mu.Lock()
defer s.mu.Unlock()
s.events.add(Event{
Name: name,
Attributes: c.Attributes,
Attributes: attributes,
DroppedAttributeCount: discarded,
Time: c.Timestamp,
Time: c.Timestamp(),
})
}

Expand Down Expand Up @@ -549,7 +549,7 @@ func startSpanInternal(ctx context.Context, tr *tracer, name string, o *trace.Sp
// If told explicitly to make this a new root use a zero value SpanContext
// as a parent which contains an invalid trace ID and is not remote.
var psc trace.SpanContext
if !o.NewRoot {
if !o.NewRoot() {
psc = trace.SpanContextFromContext(ctx)
}

Expand All @@ -575,9 +575,9 @@ func startSpanInternal(ctx context.Context, tr *tracer, name string, o *trace.Sp
ParentContext: ctx,
TraceID: tid,
Name: name,
Kind: o.SpanKind,
Attributes: o.Attributes,
Links: o.Links,
Kind: o.SpanKind(),
Attributes: o.Attributes(),
Links: o.Links(),
})

scc := trace.SpanContextConfig{
Expand All @@ -596,13 +596,13 @@ func startSpanInternal(ctx context.Context, tr *tracer, name string, o *trace.Sp
return span
}

startTime := o.Timestamp
startTime := o.Timestamp()
if startTime.IsZero() {
startTime = time.Now()
}
span.startTime = startTime

span.spanKind = trace.ValidateSpanKind(o.SpanKind)
span.spanKind = trace.ValidateSpanKind(o.SpanKind())
span.name = name
span.parent = psc
span.resource = provider.resource
Expand Down
6 changes: 3 additions & 3 deletions sdk/trace/tracer.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ var _ trace.Tracer = &tracer{}
// configured appropriately by any SpanOption passed. Any Timestamp option
// passed will be used as the start time of the Span's life-cycle.
func (tr *tracer) Start(ctx context.Context, name string, options ...trace.SpanOption) (context.Context, trace.Span) {
config := trace.NewSpanConfig(options...)
config := trace.NewSpanStartConfig(options...)

// For local spans created by this SDK, track child span count.
if p := trace.SpanFromContext(ctx); p != nil {
Expand All @@ -47,10 +47,10 @@ func (tr *tracer) Start(ctx context.Context, name string, options ...trace.SpanO
}

span := startSpanInternal(ctx, tr, name, config)
for _, l := range config.Links {
for _, l := range config.Links() {
span.addLink(l)
}
span.SetAttributes(config.Attributes...)
span.SetAttributes(config.Attributes()...)

span.tracer = tr

Expand Down
Loading