From d143b8fbf809225ab2b07f6ed0486e9e1974d549 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Sep 2020 07:34:36 -0700 Subject: [PATCH] Unify API Span Start/End Options (#1108) * Unify API Span Start/End Options Replace both with `SpanOption`. Add a unified `SpanConfig` to match and a `SpanConfigure` function to parse a `SpanConfig` from `SpanOption`s. Update all the related options to use new `SpanOption`s. * No non-zero SpanConfig defaults The SDK uses an internal clock for the current time that cannot be use if it does not know the time has not been set. * Append attributes for WithAttributes This preserves existing behavior. * Add unit test for SpanConfigure * Propagate changes * Update append option documentation * Update testing comments * Move comments on guarantees to appropriate function * Add documentation for SDK methods Include SDK implementation specific information in the Tracer Start method and Span End method. * Add changes to Changelog * Apply suggestions from code review Co-authored-by: ET * Update the SpanKind comment in the SpanConfig Try for a less tautological comment. Co-authored-by: ET --- CHANGELOG.md | 5 + api/global/internal/trace.go | 2 +- api/trace/api.go | 182 ++++++++++++++------------ api/trace/context_test.go | 2 +- api/trace/noop_span.go | 2 +- api/trace/noop_trace.go | 2 +- api/trace/span_test.go | 195 ++++++++++++++++++++++++++++ api/trace/tracetest/mock_span.go | 2 +- api/trace/tracetest/mock_tracer.go | 9 +- api/trace/tracetest/span.go | 11 +- api/trace/tracetest/span_test.go | 4 +- api/trace/tracetest/tracer.go | 10 +- api/trace/tracetest/tracer_test.go | 8 +- bridge/opentracing/bridge.go | 21 +-- bridge/opentracing/internal/mock.go | 42 +++--- bridge/opentracing/wrapper.go | 2 +- sdk/trace/span.go | 23 ++-- sdk/trace/trace_test.go | 39 +++--- sdk/trace/tracer.go | 22 ++-- 19 files changed, 393 insertions(+), 190 deletions(-) create mode 100644 api/trace/span_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 23b3fde9fca..bfe8bc803d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Added +- A `SpanConfigure` function in `go.opentelemetry.io/otel/api/trace` to create a new `SpanConfig` from `SpanOption`s. (#1108) - In the `go.opentelemetry.io/otel/api/trace` package a new `TracerConfigure` function was added to configure a new `TracerConfig`. This addition was made to conform with our project option conventions. (#1109) - Instrumentation library information was added to the Zipkin exporter. (#1119) @@ -19,6 +20,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add reconnecting udp connection type to Jaeger exporter. This change adds a new optional implementation of the udp conn interface used to detect changes to an agent's host dns record. It then adopts the new destination address to ensure the exporter doesn't get stuck. This change was ported from jaegertracing/jaeger-client-go#520. (#1063) +- Replace `StartOption` and `EndOption` in `go.opentelemetry.io/otel/api/trace` with `SpanOption`. + This change is matched by replacing the `StartConfig` and `EndConfig` with a unified `SpanConfig`. (#1108) +- Replace the `LinkedTo` span option in `go.opentelemetry.io/otel/api/trace` with `WithLinks`. + This is be more consistent with our other option patterns, i.e. passing the item to be configured directly instead of its component parts, and provides a cleaner function signature. (#1108) - The `go.opentelemetry.io/otel/api/trace` `TracerOption` was changed to an interface to conform to project option conventions. (#1109) - Rename Jaeger tags used for instrumentation library information to reflect changes in OpenTelemetry specification. (#1119) diff --git a/api/global/internal/trace.go b/api/global/internal/trace.go index d9371fd2ea2..9c97fc735da 100644 --- a/api/global/internal/trace.go +++ b/api/global/internal/trace.go @@ -116,7 +116,7 @@ func (t *tracer) setDelegate(provider trace.Provider) { // 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.StartOption) (context.Context, trace.Span) { +func (t *tracer) Start(ctx context.Context, name string, opts ...trace.SpanOption) (context.Context, trace.Span) { if t.delegate != nil { return t.delegate.Start(ctx, name, opts...) } diff --git a/api/trace/api.go b/api/trace/api.go index c55fcb31d93..731fe6479a7 100644 --- a/api/trace/api.go +++ b/api/trace/api.go @@ -68,23 +68,7 @@ func WithInstrumentationVersion(version string) TracerOption { type Tracer interface { // Start a span. - Start(ctx context.Context, spanName string, opts ...StartOption) (context.Context, Span) -} - -// EndConfig provides options to set properties of span at the time of ending -// the span. -type EndConfig struct { - EndTime time.Time -} - -// EndOption applies changes to EndConfig that sets options when the span is ended. -type EndOption func(*EndConfig) - -// WithEndTime sets the end time of the span to provided time t, when it is ended. -func WithEndTime(t time.Time) EndOption { - return func(c *EndConfig) { - c.EndTime = t - } + Start(ctx context.Context, spanName string, opts ...SpanOption) (context.Context, Span) } // ErrorConfig provides options to set properties of an error event at the time it is recorded. @@ -116,7 +100,7 @@ type Span interface { // End completes the span. No updates are allowed to span after it // ends. The only exception is setting status of the span. - End(options ...EndOption) + End(options ...SpanOption) // AddEvent adds an event to the span. AddEvent(ctx context.Context, name string, attrs ...label.KeyValue) @@ -153,18 +137,106 @@ type Span interface { SetAttribute(string, interface{}) } -// StartOption applies changes to StartConfig that sets options at span start time. -type StartOption func(*StartConfig) - -// StartConfig provides options to set properties of span at the time of starting -// a new span. -type StartConfig struct { +// SpanConfig is a group of options for a Span. +type SpanConfig struct { + // Attributes describe the associated qualities of a Span. Attributes []label.KeyValue - StartTime time.Time - Links []Link - Record bool - NewRoot bool - SpanKind SpanKind + // Timestamp is a time in a Span life-cycle. + Timestamp time.Time + // Links are the associations a Span has with other Spans. + Links []Link + // Record is the recording state of a Span. + Record bool + // NewRoot identifies a Span as the root Span for a new trace. This is + // commonly used when an existing trace crosses trust boundaries and the + // remote parent span context should be ignored for security. + NewRoot bool + // SpanKind is the role a Span has in a trace. + SpanKind SpanKind +} + +// SpanConfigure applies all the options to a returned SpanConfig. +// The default value for all the fields of the returned SpanConfig are the +// default zero value of the type. Also, this does not perform any validation +// on the returned SpanConfig (e.g. no uniqueness checking or bounding of +// data). Instead, it is left to the implementations of the SDK to perform this +// action. +func SpanConfigure(options []SpanOption) *SpanConfig { + config := new(SpanConfig) + for _, option := range options { + option.Apply(config) + } + return config +} + +// SpanOption applies an option to a SpanConfig. +type SpanOption interface { + Apply(*SpanConfig) +} + +type attributeSpanOption []label.KeyValue + +func (o attributeSpanOption) Apply(c *SpanConfig) { + c.Attributes = append(c.Attributes, []label.KeyValue(o)...) +} + +// WithAttributes adds the attributes to a span. These attributes are meant to +// provide additional information about the work the Span represents. The +// attributes are added to the existing Span attributes, i.e. this does not +// overwrite. +func WithAttributes(attributes ...label.KeyValue) SpanOption { + return attributeSpanOption(attributes) +} + +type timestampSpanOption time.Time + +func (o timestampSpanOption) Apply(c *SpanConfig) { c.Timestamp = time.Time(o) } + +// WithTimestamp sets the time of a Span life-cycle moment (e.g. started or +// stopped). +func WithTimestamp(t time.Time) SpanOption { + return timestampSpanOption(t) +} + +type linksSpanOption []Link + +func (o linksSpanOption) Apply(c *SpanConfig) { c.Links = append(c.Links, []Link(o)...) } + +// WithLinks adds links to a Span. The links are added to the existing Span +// links, i.e. this does not overwrite. +func WithLinks(links ...Link) SpanOption { + return linksSpanOption(links) +} + +type recordSpanOption bool + +func (o recordSpanOption) Apply(c *SpanConfig) { c.Record = bool(o) } + +// WithRecord specifies that the span should be recorded. It is important to +// note that implementations may override this option, i.e. if the span is a +// child of an un-sampled trace. +func WithRecord() SpanOption { + return recordSpanOption(true) +} + +type newRootSpanOption bool + +func (o newRootSpanOption) Apply(c *SpanConfig) { c.NewRoot = bool(o) } + +// WithNewRoot specifies that the Span should be treated as a root Span. Any +// existing parent span context will be ignored when defining the Span's trace +// identifiers. +func WithNewRoot() SpanOption { + return newRootSpanOption(true) +} + +type spanKindSpanOption SpanKind + +func (o spanKindSpanOption) Apply(c *SpanConfig) { c.SpanKind = SpanKind(o) } + +// WithSpanKind sets the SpanKind of a Span. +func WithSpanKind(kind SpanKind) SpanOption { + return spanKindSpanOption(kind) } // Link is used to establish relationship between two spans within the same Trace or @@ -235,55 +307,3 @@ func (sk SpanKind) String() string { return "unspecified" } } - -// WithStartTime sets the start time of the span to provided time t, when it is started. -// In absence of this option, wall clock time is used as start time. -// This option is typically used when starting of the span is delayed. -func WithStartTime(t time.Time) StartOption { - return func(c *StartConfig) { - c.StartTime = t - } -} - -// WithAttributes sets attributes to span. These attributes provides additional -// data about the span. -// Multiple `WithAttributes` options appends the attributes preserving the order. -func WithAttributes(attrs ...label.KeyValue) StartOption { - return func(c *StartConfig) { - c.Attributes = append(c.Attributes, attrs...) - } -} - -// WithRecord specifies that the span should be recorded. -// Note that the implementation may still override this preference, -// e.g., if the span is a child in an unsampled trace. -func WithRecord() StartOption { - return func(c *StartConfig) { - c.Record = true - } -} - -// WithNewRoot specifies that the current span or remote span context -// in context passed to `Start` should be ignored when deciding about -// a parent, which effectively means creating a span with new trace -// ID. The current span and the remote span context may be added as -// links to the span by the implementation. -func WithNewRoot() StartOption { - return func(c *StartConfig) { - c.NewRoot = true - } -} - -// LinkedTo allows instantiating a Span with initial Links. -func LinkedTo(sc SpanContext, attrs ...label.KeyValue) StartOption { - return func(c *StartConfig) { - c.Links = append(c.Links, Link{sc, attrs}) - } -} - -// WithSpanKind specifies the role a Span on a Trace. -func WithSpanKind(sk SpanKind) StartOption { - return func(c *StartConfig) { - c.SpanKind = sk - } -} diff --git a/api/trace/context_test.go b/api/trace/context_test.go index 57c1b0a04d0..4af89292641 100644 --- a/api/trace/context_test.go +++ b/api/trace/context_test.go @@ -101,7 +101,7 @@ func (mockSpan) SetAttribute(k string, v interface{}) { } // End does nothing. -func (mockSpan) End(options ...trace.EndOption) { +func (mockSpan) End(options ...trace.SpanOption) { } // RecordError does nothing. diff --git a/api/trace/noop_span.go b/api/trace/noop_span.go index 27f0d319f3d..d205daf887c 100644 --- a/api/trace/noop_span.go +++ b/api/trace/noop_span.go @@ -54,7 +54,7 @@ func (NoopSpan) SetAttribute(k string, v interface{}) { } // End does nothing. -func (NoopSpan) End(options ...EndOption) { +func (NoopSpan) End(options ...SpanOption) { } // RecordError does nothing. diff --git a/api/trace/noop_trace.go b/api/trace/noop_trace.go index ffdb913185e..9b67ea3d884 100644 --- a/api/trace/noop_trace.go +++ b/api/trace/noop_trace.go @@ -23,7 +23,7 @@ type NoopTracer struct{} var _ Tracer = NoopTracer{} // Start starts a noop span. -func (NoopTracer) Start(ctx context.Context, name string, opts ...StartOption) (context.Context, Span) { +func (NoopTracer) Start(ctx context.Context, name string, opts ...SpanOption) (context.Context, Span) { span := NoopSpan{} return ContextWithSpan(ctx, span), span } diff --git a/api/trace/span_test.go b/api/trace/span_test.go new file mode 100644 index 00000000000..6cd35528bfc --- /dev/null +++ b/api/trace/span_test.go @@ -0,0 +1,195 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package trace + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + + "go.opentelemetry.io/otel/label" +) + +func TestSpanConfigure(t *testing.T) { + k1v1 := label.String("key1", "value1") + k1v2 := label.String("key1", "value2") + k2v2 := label.String("key2", "value2") + + timestamp0 := time.Unix(0, 0) + timestamp1 := time.Unix(0, 0) + + link1 := Link{ + SpanContext: SpanContext{TraceID: ID([16]byte{1, 1}), SpanID: SpanID{3}}, + Attributes: []label.KeyValue{k1v1}, + } + link2 := Link{ + SpanContext: SpanContext{TraceID: ID([16]byte{1, 1}), SpanID: SpanID{3}}, + Attributes: []label.KeyValue{k1v2, k2v2}, + } + + tests := []struct { + options []SpanOption + expected *SpanConfig + }{ + { + // No non-zero-values should be set. + []SpanOption{}, + new(SpanConfig), + }, + { + []SpanOption{ + WithAttributes(k1v1), + }, + &SpanConfig{ + Attributes: []label.KeyValue{k1v1}, + }, + }, + { + // Multiple calls should append not overwrite. + []SpanOption{ + WithAttributes(k1v1), + WithAttributes(k1v2), + WithAttributes(k2v2), + }, + &SpanConfig{ + // No uniqueness is guaranteed by the API. + Attributes: []label.KeyValue{k1v1, k1v2, k2v2}, + }, + }, + { + []SpanOption{ + WithAttributes(k1v1, k1v2, k2v2), + }, + &SpanConfig{ + // No uniqueness is guaranteed by the API. + Attributes: []label.KeyValue{k1v1, k1v2, k2v2}, + }, + }, + { + []SpanOption{ + WithTimestamp(timestamp0), + }, + &SpanConfig{ + Timestamp: timestamp0, + }, + }, + { + []SpanOption{ + // Multiple calls overwrites with last-one-wins. + WithTimestamp(timestamp0), + WithTimestamp(timestamp1), + }, + &SpanConfig{ + Timestamp: timestamp1, + }, + }, + { + []SpanOption{ + WithLinks(link1), + }, + &SpanConfig{ + Links: []Link{link1}, + }, + }, + { + []SpanOption{ + // Multiple calls should append not overwrite. + WithLinks(link1), + WithLinks(link1, link2), + }, + &SpanConfig{ + // No uniqueness is guaranteed by the API. + Links: []Link{link1, link1, link2}, + }, + }, + { + []SpanOption{ + WithRecord(), + }, + &SpanConfig{ + Record: true, + }, + }, + { + []SpanOption{ + // Multiple calls should not change Record state. + WithRecord(), + WithRecord(), + }, + &SpanConfig{ + Record: true, + }, + }, + { + []SpanOption{ + WithNewRoot(), + }, + &SpanConfig{ + NewRoot: true, + }, + }, + { + []SpanOption{ + // Multiple calls should not change NewRoot state. + WithNewRoot(), + WithNewRoot(), + }, + &SpanConfig{ + NewRoot: true, + }, + }, + { + []SpanOption{ + WithSpanKind(SpanKindConsumer), + }, + &SpanConfig{ + SpanKind: SpanKindConsumer, + }, + }, + { + []SpanOption{ + // Multiple calls overwrites with last-one-wins. + WithSpanKind(SpanKindClient), + WithSpanKind(SpanKindConsumer), + }, + &SpanConfig{ + SpanKind: SpanKindConsumer, + }, + }, + { + // Everything should work together. + []SpanOption{ + WithAttributes(k1v1), + WithTimestamp(timestamp0), + WithLinks(link1, link2), + WithRecord(), + WithNewRoot(), + WithSpanKind(SpanKindConsumer), + }, + &SpanConfig{ + Attributes: []label.KeyValue{k1v1}, + Timestamp: timestamp0, + Links: []Link{link1, link2}, + Record: true, + NewRoot: true, + SpanKind: SpanKindConsumer, + }, + }, + } + for _, test := range tests { + assert.Equal(t, test.expected, SpanConfigure(test.options)) + } +} diff --git a/api/trace/tracetest/mock_span.go b/api/trace/tracetest/mock_span.go index 6f7b4915709..a8f40652340 100644 --- a/api/trace/tracetest/mock_span.go +++ b/api/trace/tracetest/mock_span.go @@ -67,7 +67,7 @@ func (ms *MockSpan) SetAttribute(k string, v interface{}) { } // End does nothing. -func (ms *MockSpan) End(options ...apitrace.EndOption) { +func (ms *MockSpan) End(options ...apitrace.SpanOption) { } // RecordError does nothing. diff --git a/api/trace/tracetest/mock_tracer.go b/api/trace/tracetest/mock_tracer.go index e65b5e38610..c29e6858195 100644 --- a/api/trace/tracetest/mock_tracer.go +++ b/api/trace/tracetest/mock_tracer.go @@ -47,15 +47,12 @@ var _ apitrace.Tracer = (*MockTracer)(nil) // TraceID is used from Parent Span Context and SpanID is assigned. // If Parent SpanContext option is not specified then random TraceID is used. // No other options are supported. -func (mt *MockTracer) Start(ctx context.Context, name string, o ...apitrace.StartOption) (context.Context, apitrace.Span) { - var opts apitrace.StartConfig - for _, op := range o { - op(&opts) - } +func (mt *MockTracer) Start(ctx context.Context, name string, o ...apitrace.SpanOption) (context.Context, apitrace.Span) { + config := apitrace.SpanConfigure(o) var span *MockSpan var sc apitrace.SpanContext - parentSpanContext, _, _ := otelparent.GetSpanContextAndLinks(ctx, opts.NewRoot) + parentSpanContext, _, _ := otelparent.GetSpanContextAndLinks(ctx, config.NewRoot) if !parentSpanContext.IsValid() { sc = apitrace.SpanContext{} diff --git a/api/trace/tracetest/span.go b/api/trace/tracetest/span.go index c60e8525c83..8ea113497b8 100644 --- a/api/trace/tracetest/span.go +++ b/api/trace/tracetest/span.go @@ -55,7 +55,7 @@ func (s *Span) Tracer() trace.Tracer { return s.tracer } -func (s *Span) End(opts ...trace.EndOption) { +func (s *Span) End(opts ...trace.SpanOption) { s.lock.Lock() defer s.lock.Unlock() @@ -63,14 +63,9 @@ func (s *Span) End(opts ...trace.EndOption) { return } - var c trace.EndConfig - for _, opt := range opts { - opt(&c) - } - + c := trace.SpanConfigure(opts) s.endTime = time.Now() - - if endTime := c.EndTime; !endTime.IsZero() { + if endTime := c.Timestamp; !endTime.IsZero() { s.endTime = endTime } diff --git a/api/trace/tracetest/span_test.go b/api/trace/tracetest/span_test.go index 11fa68b988e..a0f8642fa4c 100644 --- a/api/trace/tracetest/span_test.go +++ b/api/trace/tracetest/span_test.go @@ -101,7 +101,7 @@ func TestSpan(t *testing.T) { e.Expect(endTime).ToEqual(expectedEndTime) }) - t.Run("uses the time from WithEndTime", func(t *testing.T) { + t.Run("uses the time from WithTimestamp", func(t *testing.T) { t.Parallel() e := matchers.NewExpecter(t) @@ -113,7 +113,7 @@ func TestSpan(t *testing.T) { e.Expect(ok).ToBeTrue() expectedEndTime := time.Now().AddDate(5, 0, 0) - subject.End(trace.WithEndTime(expectedEndTime)) + subject.End(trace.WithTimestamp(expectedEndTime)) e.Expect(subject.Ended()).ToBeTrue() diff --git a/api/trace/tracetest/tracer.go b/api/trace/tracetest/tracer.go index 9662c106898..5b25a110225 100644 --- a/api/trace/tracetest/tracer.go +++ b/api/trace/tracetest/tracer.go @@ -34,14 +34,10 @@ type Tracer struct { config *config } -func (t *Tracer) Start(ctx context.Context, name string, opts ...trace.StartOption) (context.Context, trace.Span) { - var c trace.StartConfig - for _, opt := range opts { - opt(&c) - } - +func (t *Tracer) Start(ctx context.Context, name string, opts ...trace.SpanOption) (context.Context, trace.Span) { + c := trace.SpanConfigure(opts) startTime := time.Now() - if st := c.StartTime; !st.IsZero() { + if st := c.Timestamp; !st.IsZero() { startTime = st } diff --git a/api/trace/tracetest/tracer_test.go b/api/trace/tracetest/tracer_test.go index f2999e2c515..43e246e9201 100644 --- a/api/trace/tracetest/tracer_test.go +++ b/api/trace/tracetest/tracer_test.go @@ -47,7 +47,7 @@ func TestTracer(t *testing.T) { return span, nil }) - t.Run("uses the start time from WithStartTime", func(t *testing.T) { + t.Run("uses the start time from WithTimestamp", func(t *testing.T) { t.Parallel() e := matchers.NewExpecter(t) @@ -55,7 +55,7 @@ func TestTracer(t *testing.T) { expectedStartTime := time.Now().AddDate(5, 0, 0) subject := tp.Tracer(t.Name()) - _, span := subject.Start(context.Background(), "test", trace.WithStartTime(expectedStartTime)) + _, span := subject.Start(context.Background(), "test", trace.WithTimestamp(expectedStartTime)) testSpan, ok := span.(*tracetest.Span) e.Expect(ok).ToBeTrue() @@ -223,7 +223,7 @@ func TestTracer(t *testing.T) { e.Expect(gotLinks).ToMatchInAnyOrder(expectedLinks) }) - t.Run("uses the links provided through LinkedTo", func(t *testing.T) { + t.Run("uses the links provided through WithLinks", func(t *testing.T) { t.Parallel() e := matchers.NewExpecter(t) @@ -246,7 +246,7 @@ func TestTracer(t *testing.T) { }, } - _, span = subject.Start(context.Background(), "test", trace.LinkedTo(link1.SpanContext, link1.Attributes...), trace.LinkedTo(link2.SpanContext, link2.Attributes...)) + _, span = subject.Start(context.Background(), "test", trace.WithLinks(link1, link2)) testSpan, ok := span.(*tracetest.Span) e.Expect(ok).ToBeTrue() diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go index 97d6786ee33..6fbe9bbac54 100644 --- a/bridge/opentracing/bridge.go +++ b/bridge/opentracing/bridge.go @@ -99,10 +99,10 @@ func (s *bridgeSpan) Finish() { } func (s *bridgeSpan) FinishWithOptions(opts ot.FinishOptions) { - var otelOpts []oteltrace.EndOption + var otelOpts []oteltrace.SpanOption if !opts.FinishTime.IsZero() { - otelOpts = append(otelOpts, oteltrace.WithEndTime(opts.FinishTime)) + otelOpts = append(otelOpts, oteltrace.WithTimestamp(opts.FinishTime)) } for _, record := range opts.LogRecords { s.logRecord(record) @@ -389,14 +389,15 @@ func (t *BridgeTracer) StartSpan(operationName string, opts ...ot.StartSpanOptio if parentBridgeSC != nil { checkCtx = oteltrace.ContextWithRemoteSpanContext(checkCtx, parentBridgeSC.otelSpanContext) } - checkCtx2, otelSpan := t.setTracer.tracer().Start(checkCtx, operationName, func(opts *oteltrace.StartConfig) { - opts.Attributes = attributes - opts.StartTime = sso.StartTime - opts.Links = links - opts.Record = true - opts.NewRoot = false - opts.SpanKind = kind - }) + checkCtx2, otelSpan := t.setTracer.tracer().Start( + checkCtx, + operationName, + oteltrace.WithAttributes(attributes...), + oteltrace.WithTimestamp(sso.StartTime), + oteltrace.WithLinks(links...), + oteltrace.WithRecord(), + oteltrace.WithSpanKind(kind), + ) if checkCtx != checkCtx2 { t.warnOnce.Do(func() { t.warningHandler("SDK should have deferred the context setup, see the documentation of go.opentelemetry.io/otel/bridge/opentracing/migration\n") diff --git a/bridge/opentracing/internal/mock.go b/bridge/opentracing/internal/mock.go index f346ffa33ac..59861ea09d2 100644 --- a/bridge/opentracing/internal/mock.go +++ b/bridge/opentracing/internal/mock.go @@ -70,17 +70,14 @@ func NewMockTracer() *MockTracer { } } -func (t *MockTracer) Start(ctx context.Context, name string, opts ...oteltrace.StartOption) (context.Context, oteltrace.Span) { - spanOpts := oteltrace.StartConfig{} - for _, opt := range opts { - opt(&spanOpts) - } - startTime := spanOpts.StartTime +func (t *MockTracer) Start(ctx context.Context, name string, opts ...oteltrace.SpanOption) (context.Context, oteltrace.Span) { + config := oteltrace.SpanConfigure(opts) + startTime := config.Timestamp if startTime.IsZero() { startTime = time.Now() } spanContext := oteltrace.SpanContext{ - TraceID: t.getTraceID(ctx, &spanOpts), + TraceID: t.getTraceID(ctx, config), SpanID: t.getSpanID(), TraceFlags: 0, } @@ -88,15 +85,15 @@ func (t *MockTracer) Start(ctx context.Context, name string, opts ...oteltrace.S mockTracer: t, officialTracer: t, spanContext: spanContext, - recording: spanOpts.Record, + recording: config.Record, Attributes: otelcorrelation.NewMap(otelcorrelation.MapUpdate{ - MultiKV: spanOpts.Attributes, + MultiKV: config.Attributes, }), StartTime: startTime, EndTime: time.Time{}, - ParentSpanID: t.getParentSpanID(ctx, &spanOpts), + ParentSpanID: t.getParentSpanID(ctx, config), Events: nil, - SpanKind: oteltrace.ValidateSpanKind(spanOpts.SpanKind), + SpanKind: oteltrace.ValidateSpanKind(config.SpanKind), } if !migration.SkipContextSetup(ctx) { ctx = oteltrace.ContextWithSpan(ctx, span) @@ -118,8 +115,8 @@ func (t *MockTracer) addSpareContextValue(ctx context.Context) context.Context { return ctx } -func (t *MockTracer) getTraceID(ctx context.Context, spanOpts *oteltrace.StartConfig) oteltrace.ID { - if parent := t.getParentSpanContext(ctx, spanOpts); parent.IsValid() { +func (t *MockTracer) getTraceID(ctx context.Context, config *oteltrace.SpanConfig) oteltrace.ID { + if parent := t.getParentSpanContext(ctx, config); parent.IsValid() { return parent.TraceID } if len(t.SpareTraceIDs) > 0 { @@ -133,15 +130,15 @@ func (t *MockTracer) getTraceID(ctx context.Context, spanOpts *oteltrace.StartCo return t.getRandTraceID() } -func (t *MockTracer) getParentSpanID(ctx context.Context, spanOpts *oteltrace.StartConfig) oteltrace.SpanID { - if parent := t.getParentSpanContext(ctx, spanOpts); parent.IsValid() { +func (t *MockTracer) getParentSpanID(ctx context.Context, config *oteltrace.SpanConfig) oteltrace.SpanID { + if parent := t.getParentSpanContext(ctx, config); parent.IsValid() { return parent.SpanID } return oteltrace.SpanID{} } -func (t *MockTracer) getParentSpanContext(ctx context.Context, spanOpts *oteltrace.StartConfig) oteltrace.SpanContext { - spanCtx, _, _ := otelparent.GetSpanContextAndLinks(ctx, spanOpts.NewRoot) +func (t *MockTracer) getParentSpanContext(ctx context.Context, config *oteltrace.SpanConfig) oteltrace.SpanContext { + spanCtx, _, _ := otelparent.GetSpanContextAndLinks(ctx, config.NewRoot) return spanCtx } @@ -239,17 +236,12 @@ func (s *MockSpan) applyUpdate(update otelcorrelation.MapUpdate) { s.Attributes = s.Attributes.Apply(update) } -func (s *MockSpan) End(options ...oteltrace.EndOption) { +func (s *MockSpan) End(options ...oteltrace.SpanOption) { if !s.EndTime.IsZero() { return // already finished } - endOpts := oteltrace.EndConfig{} - - for _, opt := range options { - opt(&endOpts) - } - - endTime := endOpts.EndTime + config := oteltrace.SpanConfigure(options) + endTime := config.Timestamp if endTime.IsZero() { endTime = time.Now() } diff --git a/bridge/opentracing/wrapper.go b/bridge/opentracing/wrapper.go index f133c116edc..f5070695bdc 100644 --- a/bridge/opentracing/wrapper.go +++ b/bridge/opentracing/wrapper.go @@ -74,7 +74,7 @@ func (t *WrapperTracer) otelTracer() oteltrace.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 ...oteltrace.StartOption) (context.Context, oteltrace.Span) { +func (t *WrapperTracer) Start(ctx context.Context, name string, opts ...oteltrace.SpanOption) (context.Context, oteltrace.Span) { ctx, span := t.otelTracer().Start(ctx, name, opts...) if spanWithExtension, ok := span.(migration.OverrideTracerSpanExtension); ok { spanWithExtension.OverrideTracer(t) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 9e93bfe215b..46c055c4391 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -108,8 +108,14 @@ func (s *span) SetAttribute(k string, v interface{}) { } } -// End ends the span adding an error event if it was called while panicking. -func (s *span) End(options ...apitrace.EndOption) { +// End ends the span. +// +// The only SpanOption currently supported is WithTimestamp which will set the +// end time for a Span's life-cycle. +// +// 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 ...apitrace.SpanOption) { if s == nil { return } @@ -131,19 +137,16 @@ func (s *span) End(options ...apitrace.EndOption) { if !s.IsRecording() { return } - opts := apitrace.EndConfig{} - for _, opt := range options { - opt(&opts) - } + config := apitrace.SpanConfigure(options) s.endOnce.Do(func() { sps, _ := s.tracer.provider.spanProcessors.Load().(spanProcessorMap) mustExportOrProcess := len(sps) > 0 if mustExportOrProcess { sd := s.makeSpanData() - if opts.EndTime.IsZero() { + if config.Timestamp.IsZero() { sd.EndTime = internal.MonotonicEndTime(sd.StartTime) } else { - sd.EndTime = opts.EndTime + sd.EndTime = config.Timestamp } for sp := range sps { sp.OnEnd(sd) @@ -324,7 +327,7 @@ func (s *span) addChild() { s.mu.Unlock() } -func startSpanInternal(tr *tracer, name string, parent apitrace.SpanContext, remoteParent bool, o apitrace.StartConfig) *span { +func startSpanInternal(tr *tracer, name string, parent apitrace.SpanContext, remoteParent bool, o *apitrace.SpanConfig) *span { var noParent bool span := &span{} span.spanContext = parent @@ -355,7 +358,7 @@ func startSpanInternal(tr *tracer, name string, parent apitrace.SpanContext, rem return span } - startTime := o.StartTime + startTime := o.Timestamp if startTime.IsZero() { startTime = time.Now() } diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index c7ac11a5284..f86776b48a5 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -500,13 +500,11 @@ func TestLinks(t *testing.T) { sc1 := apitrace.SpanContext{TraceID: apitrace.ID([16]byte{1, 1}), SpanID: apitrace.SpanID{3}} sc2 := apitrace.SpanContext{TraceID: apitrace.ID([16]byte{1, 1}), SpanID: apitrace.SpanID{3}} - span := startSpan(tp, "Links", - apitrace.LinkedTo(sc1, label.String("key1", "value1")), - apitrace.LinkedTo(sc2, - label.String("key2", "value2"), - label.String("key3", "value3"), - ), - ) + links := []apitrace.Link{ + {SpanContext: sc1, Attributes: []label.KeyValue{k1v1}}, + {SpanContext: sc2, Attributes: []label.KeyValue{k2v2, k3v3}}, + } + span := startSpan(tp, "Links", apitrace.WithLinks(links...)) got, err := endSpan(te, span) if err != nil { @@ -518,13 +516,10 @@ func TestLinks(t *testing.T) { TraceID: tid, TraceFlags: 0x1, }, - ParentSpanID: sid, - Name: "span0", - HasRemoteParent: true, - Links: []apitrace.Link{ - {SpanContext: sc1, Attributes: []label.KeyValue{k1v1}}, - {SpanContext: sc2, Attributes: []label.KeyValue{k2v2, k3v3}}, - }, + ParentSpanID: sid, + Name: "span0", + HasRemoteParent: true, + Links: links, SpanKind: apitrace.SpanKindInternal, InstrumentationLibrary: instrumentation.Library{Name: "Links"}, } @@ -544,9 +539,11 @@ func TestLinksOverLimit(t *testing.T) { tp, _ := NewProvider(WithConfig(cfg), WithSyncer(te)) span := startSpan(tp, "LinksOverLimit", - apitrace.LinkedTo(sc1, label.String("key1", "value1")), - apitrace.LinkedTo(sc2, label.String("key2", "value2")), - apitrace.LinkedTo(sc3, label.String("key3", "value3")), + apitrace.WithLinks( + apitrace.Link{SpanContext: sc1, Attributes: []label.KeyValue{label.String("key1", "value1")}}, + apitrace.Link{SpanContext: sc2, Attributes: []label.KeyValue{label.String("key2", "value2")}}, + apitrace.Link{SpanContext: sc3, Attributes: []label.KeyValue{label.String("key3", "value3")}}, + ), ) k2v2 := label.String("key2", "value2") @@ -668,7 +665,7 @@ func checkChild(p apitrace.SpanContext, apiSpan apitrace.Span) error { // startSpan starts a span with a name "span0". See startNamedSpan for // details. -func startSpan(tp *Provider, trName string, args ...apitrace.StartOption) apitrace.Span { +func startSpan(tp *Provider, trName string, args ...apitrace.SpanOption) apitrace.Span { return startNamedSpan(tp, trName, "span0", args...) } @@ -676,7 +673,7 @@ func startSpan(tp *Provider, trName string, args ...apitrace.StartOption) apitra // passed name and with remote span context as parent. The remote span // context contains TraceFlags with sampled bit set. This allows the // span to be automatically sampled. -func startNamedSpan(tp *Provider, trName, name string, args ...apitrace.StartOption) apitrace.Span { +func startNamedSpan(tp *Provider, trName, name string, args ...apitrace.SpanOption) apitrace.Span { ctx := context.Background() ctx = apitrace.ContextWithRemoteSpanContext(ctx, remoteSpanContext()) args = append(args, apitrace.WithRecord()) @@ -906,9 +903,9 @@ func TestCustomStartEndTime(t *testing.T) { _, span := tp.Tracer("Custom Start and End time").Start( context.Background(), "testspan", - apitrace.WithStartTime(startTime), + apitrace.WithTimestamp(startTime), ) - span.End(apitrace.WithEndTime(endTime)) + span.End(apitrace.WithTimestamp(endTime)) if len(te.spans) != 1 { t.Fatalf("got exported spans %#v, want one span", te.spans) diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index 8d395c58efb..ec785999e8b 100644 --- a/sdk/trace/tracer.go +++ b/sdk/trace/tracer.go @@ -29,14 +29,16 @@ type tracer struct { var _ apitrace.Tracer = &tracer{} -func (tr *tracer) Start(ctx context.Context, name string, o ...apitrace.StartOption) (context.Context, apitrace.Span) { - var opts apitrace.StartConfig - - for _, op := range o { - op(&opts) - } +// Start starts a Span and returns it along with a context containing it. +// +// The Span is created with the provided name and as a child of any existing +// span context found in the passed context. The created Span will be +// 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 ...apitrace.SpanOption) (context.Context, apitrace.Span) { + config := apitrace.SpanConfigure(options) - parentSpanContext, remoteParent, links := parent.GetSpanContextAndLinks(ctx, opts.NewRoot) + parentSpanContext, remoteParent, links := parent.GetSpanContextAndLinks(ctx, config.NewRoot) if p := apitrace.SpanFromContext(ctx); p != nil { if sdkSpan, ok := p.(*span); ok { @@ -44,14 +46,14 @@ func (tr *tracer) Start(ctx context.Context, name string, o ...apitrace.StartOpt } } - span := startSpanInternal(tr, name, parentSpanContext, remoteParent, opts) + span := startSpanInternal(tr, name, parentSpanContext, remoteParent, config) for _, l := range links { span.addLink(l) } - for _, l := range opts.Links { + for _, l := range config.Links { span.addLink(l) } - span.SetAttributes(opts.Attributes...) + span.SetAttributes(config.Attributes...) span.tracer = tr