Remove WithRecord() option from trace.SpanOption when starting a span (…

…open-telemetry#1660) * Remove `WithRecord()` option from SpanConfig options This brings the trace API into conformance with the specification. * Add entry to CHANGELOG Fixes #192 * Updated CHANGELOG with PR# * Cleaned up CHANGELOG notes * fixup! Merge remote-tracking branch 'upstream/main' into remove-with-record * Use new spanContext API to set traceflags, tracestate Co-authored-by: Tyler Yahn <[email protected]>
atlassian-forks · Mar 9, 2021 · 3684191 · 3684191
1 parent 65c7de2
commit 3684191
Show file tree

Hide file tree

Showing 8 changed files with 22 additions and 64 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,7 +10,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Added
 
-- Added `Marshler` config option to `otlphttp` to enable otlp over json or protobufs. (#1586)
+- Added `Marshaler` config option to `otlphttp` to enable otlp over json or protobufs. (#1586)
 - A `ForceFlush` method to the `"go.opentelemetry.io/otel/sdk/trace".TracerProvider` to flush all registered `SpanProcessor`s. (#1608)
 
 ### Changed
@@ -26,6 +26,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Removed
 
+- Removed `WithRecord()` from `trace.SpanOption` when creating a span. (#1660)
 - Removed the exported `SimpleSpanProcessor` and `BatchSpanProcessor` structs.
    These are now returned as a SpanProcessor interface from their respective constructors. (#1638)
 - Removed setting status to `Error` while recording an error as a span event in `RecordError`. (#1663)
@@ -97,7 +98,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - The sequential timing check of timestamps in the stdout exporter are now setup explicitly to be sequential (#1571). (#1572)
 - Windows build of Jaeger tests now compiles with OS specific functions (#1576). (#1577)
 - The sequential timing check of timestamps of go.opentelemetry.io/otel/sdk/metric/aggregator/lastvalue are now setup explicitly to be sequential (#1578). (#1579)
-- Validate tracestate header keys with vedors according to the W3C TraceContext specification (#1475). (#1581)
+- Validate tracestate header keys with vendors according to the W3C TraceContext specification (#1475). (#1581)
 - The OTLP exporter includes related labels for translations of a GaugeArray (#1563). (#1570)
 - Jaeger Exporter: Ensure mapping between OTEL and Jaeger span data complies with the specification. (#1626)
 

diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go
@@ -411,7 +411,6 @@ func (t *BridgeTracer) StartSpan(operationName string, opts ...ot.StartSpanOptio
 		trace.WithAttributes(attributes...),
 		trace.WithTimestamp(sso.StartTime),
 		trace.WithLinks(links...),
-		trace.WithRecord(),
 		trace.WithSpanKind(kind),
 	)
 	if checkCtx != checkCtx2 {

diff --git a/bridge/opentracing/internal/mock.go b/bridge/opentracing/internal/mock.go
@@ -85,7 +85,6 @@ func (t *MockTracer) Start(ctx context.Context, name string, opts ...trace.SpanO
 		mockTracer:     t,
 		officialTracer: t,
 		spanContext:    spanContext,
-		recording:      config.Record,
 		Attributes: baggage.NewMap(baggage.MapUpdate{
 			MultiKV: config.Attributes,
 		}),

diff --git a/oteltest/harness.go b/oteltest/harness.go
@@ -152,17 +152,6 @@ func (h *Harness) TestTracer(subjectFactory func() trace.Tracer) {
 			e.Expect(sc1.SpanID()).NotToEqual(sc2.SpanID())
 		})
 
-		t.Run("records the span if specified", func(t *testing.T) {
-			t.Parallel()
-
-			e := matchers.NewExpecter(t)
-			subject := subjectFactory()
-
-			_, span := subject.Start(context.Background(), "span", trace.WithRecord())
-
-			e.Expect(span.IsRecording()).ToBeTrue()
-		})
-
 		t.Run("propagates a parent's trace ID through the context", func(t *testing.T) {
 			t.Parallel()
 

diff --git a/sdk/trace/span.go b/sdk/trace/span.go
@@ -558,10 +558,15 @@ func startSpanInternal(ctx context.Context, tr *tracer, name string, parent trac
 		links:        o.Links,
 		kind:         o.SpanKind,
 	}
-	sampled := makeSamplingDecision(data)
-	span.spanContext = span.spanContext.WithTraceState(sampled.Tracestate)
+	samplingResult := makeSamplingDecision(data)
+	if isSampled(samplingResult) {
+		span.spanContext = span.spanContext.WithTraceFlags(span.spanContext.TraceFlags() | trace.FlagsSampled)
+	} else {
+		span.spanContext = span.spanContext.WithTraceFlags(span.spanContext.TraceFlags() &^ trace.FlagsSampled)
+	}
+	span.spanContext = span.spanContext.WithTraceState(samplingResult.Tracestate)
 
-	if !span.spanContext.IsSampled() && !o.Record {
+	if !isRecording(samplingResult) {
 		return span
 	}
 
@@ -577,7 +582,7 @@ func startSpanInternal(ctx context.Context, tr *tracer, name string, parent trac
 	span.resource = cfg.Resource
 	span.instrumentationLibrary = tr.instrumentationLibrary
 
-	span.SetAttributes(sampled.Attributes...)
+	span.SetAttributes(samplingResult.Attributes...)
 
 	span.parent = parent
 
@@ -602,7 +607,7 @@ type samplingData struct {
 
 func makeSamplingDecision(data samplingData) SamplingResult {
 	sampler := data.cfg.DefaultSampler
-	sampled := sampler.ShouldSample(SamplingParameters{
+	return sampler.ShouldSample(SamplingParameters{
 		ParentContext:   data.parent,
 		TraceID:         data.span.spanContext.TraceID(),
 		Name:            data.name,
@@ -611,10 +616,12 @@ func makeSamplingDecision(data samplingData) SamplingResult {
 		Attributes:      data.attributes,
 		Links:           data.links,
 	})
-	if sampled.Decision == RecordAndSample {
-		data.span.spanContext = data.span.spanContext.WithTraceFlags(data.span.spanContext.TraceFlags() | trace.FlagsSampled)
-	} else {
-		data.span.spanContext = data.span.spanContext.WithTraceFlags(data.span.spanContext.TraceFlags() &^ trace.FlagsSampled)
-	}
-	return sampled
+}
+
+func isRecording(s SamplingResult) bool {
+	return s.Decision == RecordOnly || s.Decision == RecordAndSample
+}
+
+func isSampled(s SamplingResult) bool {
+	return s.Decision == RecordAndSample
 }
diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go
@@ -864,7 +864,6 @@ func startSpan(tp *TracerProvider, trName string, args ...trace.SpanOption) trac
 func startNamedSpan(tp *TracerProvider, trName, name string, args ...trace.SpanOption) trace.Span {
 	ctx := context.Background()
 	ctx = trace.ContextWithRemoteSpanContext(ctx, remoteSpanContext())
-	args = append(args, trace.WithRecord())
 	_, span := tp.Tracer(trName).Start(
 		ctx,
 		name,
@@ -878,7 +877,6 @@ func startNamedSpan(tp *TracerProvider, trName, name string, args ...trace.SpanO
 // along with the span so this parent can be used to create child
 // spans.
 func startLocalSpan(tp *TracerProvider, ctx context.Context, trName, name string, args ...trace.SpanOption) (context.Context, trace.Span) {
-	args = append(args, trace.WithRecord())
 	ctx, span := tp.Tracer(trName).Start(
 		ctx,
 		name,
@@ -1326,7 +1324,7 @@ func TestWithInstrumentationVersion(t *testing.T) {
 	_, span := tp.Tracer(
 		"WithInstrumentationVersion",
 		trace.WithInstrumentationVersion("v0.1.0"),
-	).Start(ctx, "span0", trace.WithRecord())
+	).Start(ctx, "span0")
 	got, err := endSpan(te, span)
 	if err != nil {
 		t.Error(err.Error())
@@ -1357,7 +1355,6 @@ func TestSpanCapturesPanic(t *testing.T) {
 	_, span := tp.Tracer("CatchPanic").Start(
 		context.Background(),
 		"span",
-		trace.WithRecord(),
 	)
 
 	f := func() {

diff --git a/trace/config.go b/trace/config.go
@@ -54,8 +54,6 @@ type SpanConfig struct {
 	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.
@@ -165,18 +163,6 @@ func WithLinks(links ...Link) SpanOption {
 	return linksSpanOption(links)
 }
 
-type recordSpanOption bool
-
-func (o recordSpanOption) ApplySpan(c *SpanConfig) { c.Record = bool(o) }
-func (recordSpanOption) private()                  {}
-
-// 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) ApplySpan(c *SpanConfig) { c.NewRoot = bool(o) }

diff --git a/trace/config_test.go b/trace/config_test.go
@@ -115,24 +115,6 @@ func TestNewSpanConfig(t *testing.T) {
 				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(),
@@ -175,15 +157,13 @@ func TestNewSpanConfig(t *testing.T) {
 				WithAttributes(k1v1),
 				WithTimestamp(timestamp0),
 				WithLinks(link1, link2),
-				WithRecord(),
 				WithNewRoot(),
 				WithSpanKind(SpanKindConsumer),
 			},
 			&SpanConfig{
 				Attributes: []attribute.KeyValue{k1v1},
 				Timestamp:  timestamp0,
 				Links:      []Link{link1, link2},
-				Record:     true,
 				NewRoot:    true,
 				SpanKind:   SpanKindConsumer,
 			},