Make TraceFlags spec-compliant (#1770)

* Make TraceFlags spec-compliant

* Remove `trace.FlagsDebug` and `trace.FlagsDeferred`
  * These are used only by the B3 propagator and will be handled there in the `context.Context`
* Make `trace.TraceFlags` a defined type, aliasing `byte`
* Move `IsSampled` method from `trace.SpanContext` to `trace.TraceFlags`
* Add `Sampled(bool)` method to `trace.TraceFlags`
* Implement `Stringer` and `json.Marshaler` for `trace.TraceFlags`

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* Rename `TraceFlags.Sampled()` to `TraceFlags.WithSampled()` for consistency

* Restore `SpanContext.IsSampled()` method.

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

CHANGELOG.md

@@ -28,6 +28,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
   - `OTEL_EXPORTER_OTLP_CERTIFICATE`
   - `OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE`
   - `OTEL_EXPORTER_OTLP_METRICS_CERTIFICATE`
+- `trace.TraceFlags` is now a defined type over `byte` and `WithSampled(bool) TraceFlags` and `IsSampled() bool` methods have been added to it. (#1770)
 
 ### Fixed
 
@@ -65,6 +66,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
   If needed, that Span's `SpanContext.IsRemote()` can then be used to determine if it is remote or not. (#1731)
 - The `HasRemoteParent` field of the `"go.opentelemetry.io/otel/sdk/trace".SamplingParameters` is removed.
   This field is redundant to the information returned from the `Remote` method of the `SpanContext` held in the `ParentContext` field. (#1749)
+- The `trace.FlagsDebug` and `trace.FlagsDeferred` constants have been removed and will be localized to the B3 propagator. (#1770)
 
 ## [0.19.0] - 2021-03-18
 

bridge/opencensus/utils/utils.go

@@ -15,11 +15,8 @@
 package utils // import "go.opentelemetry.io/otel/bridge/opencensus/utils"
 
 import (
-	"fmt"
-
 	octrace "go.opencensus.io/trace"
 
-	"go.opentelemetry.io/otel"
 	"go.opentelemetry.io/otel/trace"
 )
 
@@ -27,9 +24,6 @@ import (
 // OpenCensus SpanContext, and handles any incompatibilities with the global
 // error handler.
 func OTelSpanContextToOC(sc trace.SpanContext) octrace.SpanContext {
-	if sc.IsDebug() || sc.IsDeferred() {
-		otel.Handle(fmt.Errorf("ignoring OpenTelemetry Debug or Deferred trace flags for span %q because they are not supported by OpenCensus", sc.SpanID()))
-	}
 	var to octrace.TraceOptions
 	if sc.IsSampled() {
 		// OpenCensus doesn't expose functions to directly set sampled
@@ -45,7 +39,7 @@ func OTelSpanContextToOC(sc trace.SpanContext) octrace.SpanContext {
 // OCSpanContextToOTel converts from an OpenCensus SpanContext to an
 // OpenTelemetry SpanContext.
 func OCSpanContextToOTel(sc octrace.SpanContext) trace.SpanContext {
-	var traceFlags byte
+	var traceFlags trace.TraceFlags
 	if sc.IsSampled() {
 		traceFlags = trace.FlagsSampled
 	}

bridge/opencensus/utils/utils_test.go

@@ -58,19 +58,6 @@ func TestOTelSpanContextToOC(t *testing.T) {
 				TraceOptions: octrace.TraceOptions(0),
 			},
 		},
-		{
-			description: "debug flag",
-			input: trace.NewSpanContext(trace.SpanContextConfig{
-				TraceID:    trace.TraceID([16]byte{1}),
-				SpanID:     trace.SpanID([8]byte{2}),
-				TraceFlags: trace.FlagsDebug,
-			}),
-			expected: octrace.SpanContext{
-				TraceID:      octrace.TraceID([16]byte{1}),
-				SpanID:       octrace.SpanID([8]byte{2}),
-				TraceOptions: octrace.TraceOptions(0),
-			},
-		},
 	} {
 		t.Run(tc.description, func(t *testing.T) {
 			output := OTelSpanContextToOC(tc.input)

exporters/otlp/otlp_span_test.go

@@ -58,7 +58,7 @@ func TestExportSpans(t *testing.T) {
 					SpanContext: trace.NewSpanContext(trace.SpanContextConfig{
 						TraceID:    trace.TraceID([16]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}),
 						SpanID:     trace.SpanID([8]byte{0, 0, 0, 0, 0, 0, 0, 1}),
-						TraceFlags: byte(1),
+						TraceFlags: trace.FlagsSampled,
 					}),
 					SpanKind:  trace.SpanKindServer,
 					Name:      "parent process",
@@ -80,7 +80,7 @@ func TestExportSpans(t *testing.T) {
 					SpanContext: trace.NewSpanContext(trace.SpanContextConfig{
 						TraceID:    trace.TraceID([16]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2}),
 						SpanID:     trace.SpanID([8]byte{0, 0, 0, 0, 0, 0, 0, 1}),
-						TraceFlags: byte(1),
+						TraceFlags: trace.FlagsSampled,
 					}),
 					SpanKind:  trace.SpanKindServer,
 					Name:      "secondary parent process",
@@ -102,12 +102,12 @@ func TestExportSpans(t *testing.T) {
 					SpanContext: trace.NewSpanContext(trace.SpanContextConfig{
 						TraceID:    trace.TraceID([16]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}),
 						SpanID:     trace.SpanID([8]byte{0, 0, 0, 0, 0, 0, 0, 2}),
-						TraceFlags: byte(1),
+						TraceFlags: trace.FlagsSampled,
 					}),
 					Parent: trace.NewSpanContext(trace.SpanContextConfig{
 						TraceID:    trace.TraceID([16]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}),
 						SpanID:     trace.SpanID([8]byte{0, 0, 0, 0, 0, 0, 0, 1}),
-						TraceFlags: byte(1),
+						TraceFlags: trace.FlagsSampled,
 					}),
 					SpanKind:  trace.SpanKindInternal,
 					Name:      "internal process",
@@ -129,7 +129,7 @@ func TestExportSpans(t *testing.T) {
 					SpanContext: trace.NewSpanContext(trace.SpanContextConfig{
 						TraceID:    trace.TraceID([16]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2}),
 						SpanID:     trace.SpanID([8]byte{0, 0, 0, 0, 0, 0, 0, 1}),
-						TraceFlags: byte(1),
+						TraceFlags: trace.FlagsSampled,
 					}),
 					SpanKind:  trace.SpanKindServer,
 					Name:      "parent process",

exporters/stdout/trace_test.go

@@ -78,7 +78,7 @@ func TestExporter_ExportSpan(t *testing.T) {
 	got := b.String()
 	expectedOutput := `[{"SpanContext":{` +
 		`"TraceID":"0102030405060708090a0b0c0d0e0f10",` +
-		`"SpanID":"0102030405060708","TraceFlags":0,` +
+		`"SpanID":"0102030405060708","TraceFlags":"00",` +
 		`"TraceState":[` +
 		`{` +
 		`"Key":"key",` +
@@ -87,7 +87,7 @@ func TestExporter_ExportSpan(t *testing.T) {
 		`"Parent":{` +
 		`"TraceID":"00000000000000000000000000000000",` +
 		`"SpanID":"0000000000000000",` +
-		`"TraceFlags":0,` +
+		`"TraceFlags":"00",` +
 		`"TraceState":null,` +
 		`"Remote":false` +
 		`},` +

propagation/trace_context.go

@@ -54,11 +54,14 @@ func (tc TraceContext) Inject(ctx context.Context, carrier TextMapCarrier) {
 
 	carrier.Set(tracestateHeader, sc.TraceState().String())
 
-	h := fmt.Sprintf("%.2x-%s-%s-%.2x",
+	// Clear all flags other than the trace-context supported sampling bit.
+	flags := sc.TraceFlags() & trace.FlagsSampled
+
+	h := fmt.Sprintf("%.2x-%s-%s-%s",
 		supportedVersion,
 		sc.TraceID(),
 		sc.SpanID(),
-		sc.TraceFlags()&trace.FlagsSampled)
+		flags)
 	carrier.Set(traceparentHeader, h)
 }
 
@@ -134,7 +137,7 @@ func (tc TraceContext) extract(carrier TextMapCarrier) trace.SpanContext {
 		return trace.SpanContext{}
 	}
 	// Clear all flags other than the trace-context supported sampling bit.
-	scc.TraceFlags = opts[0] & trace.FlagsSampled
+	scc.TraceFlags = trace.TraceFlags(opts[0]) & trace.FlagsSampled
 
 	scc.TraceState = parseTraceState(carrier.Get(tracestateHeader))
 	scc.Remote = true

sdk/trace/simple_span_processor.go

@@ -49,7 +49,7 @@ func (ssp *simpleSpanProcessor) OnEnd(s ReadOnlySpan) {
 	ssp.exporterMu.RLock()
 	defer ssp.exporterMu.RUnlock()
 
-	if ssp.exporter != nil && s.SpanContext().IsSampled() {
+	if ssp.exporter != nil && s.SpanContext().TraceFlags().IsSampled() {
 		ss := s.Snapshot()
 		if err := ssp.exporter.ExportSpans(context.Background(), []*export.SpanSnapshot{ss}); err != nil {
 			otel.Handle(err)

trace/trace.go

@@ -30,13 +30,7 @@ import (
 const (
 	// FlagsSampled is a bitmask with the sampled bit set. A SpanContext
 	// with the sampling bit set means the span is sampled.
-	FlagsSampled = byte(0x01)
-	// FlagsDeferred is a bitmask with the deferred bit set. A SpanContext
-	// with the deferred bit set means the sampling decision has been
-	// defered to the receiver.
-	FlagsDeferred = byte(0x02)
-	// FlagsDebug is a bitmask with the debug bit set.
-	FlagsDebug = byte(0x04)
+	FlagsSampled = TraceFlags(0x01)
 
 	errInvalidHexID errorConst = "trace-id and span-id can only contain [0-9a-f] characters, all lowercase"
 
@@ -319,12 +313,40 @@ func isTraceStateKeyValueValid(kv attribute.KeyValue) bool {
 		valueFormatRegExp.MatchString(kv.Value.Emit())
 }
 
+// TraceFlags contains flags that can be set on a SpanContext
+type TraceFlags byte //nolint:golint
+
+// IsSampled returns if the sampling bit is set in the TraceFlags.
+func (tf TraceFlags) IsSampled() bool {
+	return tf&FlagsSampled == FlagsSampled
+}
+
+// WithSampled sets the sampling bit in a new copy of the TraceFlags.
+func (tf TraceFlags) WithSampled(sampled bool) TraceFlags {
+	if sampled {
+		return tf | FlagsSampled
+	}
+
+	return tf &^ FlagsSampled
+}
+
+// MarshalJSON implements a custom marshal function to encode TraceFlags
+// as a hex string.
+func (tf TraceFlags) MarshalJSON() ([]byte, error) {
+	return json.Marshal(tf.String())
+}
+
+// String returns the hex string representation form of TraceFlags
+func (tf TraceFlags) String() string {
+	return hex.EncodeToString([]byte{byte(tf)}[:])
+}
+
 // SpanContextConfig contains mutable fields usable for constructing
 // an immutable SpanContext.
 type SpanContextConfig struct {
 	TraceID    TraceID
 	SpanID     SpanID
-	TraceFlags byte
+	TraceFlags TraceFlags
 	TraceState TraceState
 	Remote     bool
 }
@@ -345,7 +367,7 @@ func NewSpanContext(config SpanContextConfig) SpanContext {
 type SpanContext struct {
 	traceID    TraceID
 	spanID     SpanID
-	traceFlags byte
+	traceFlags TraceFlags
 	traceState TraceState
 	remote     bool
 }
@@ -415,12 +437,17 @@ func (sc SpanContext) WithSpanID(spanID SpanID) SpanContext {
 }
 
 // TraceFlags returns the flags from the SpanContext.
-func (sc SpanContext) TraceFlags() byte {
+func (sc SpanContext) TraceFlags() TraceFlags {
 	return sc.traceFlags
 }
 
+// IsSampled returns if the sampling bit is set in the SpanContext's TraceFlags.
+func (sc SpanContext) IsSampled() bool {
+	return sc.traceFlags.IsSampled()
+}
+
 // WithTraceFlags returns a new SpanContext with the TraceFlags replaced.
-func (sc SpanContext) WithTraceFlags(flags byte) SpanContext {
+func (sc SpanContext) WithTraceFlags(flags TraceFlags) SpanContext {
 	return SpanContext{
 		traceID:    sc.traceID,
 		spanID:     sc.spanID,
@@ -430,21 +457,6 @@ func (sc SpanContext) WithTraceFlags(flags byte) SpanContext {
 	}
 }
 
-// IsDeferred returns if the deferred bit is set in the trace flags.
-func (sc SpanContext) IsDeferred() bool {
-	return sc.traceFlags&FlagsDeferred == FlagsDeferred
-}
-
-// IsDebug returns if the debug bit is set in the trace flags.
-func (sc SpanContext) IsDebug() bool {
-	return sc.traceFlags&FlagsDebug == FlagsDebug
-}
-
-// IsSampled returns if the sampling bit is set in the trace flags.
-func (sc SpanContext) IsSampled() bool {
-	return sc.traceFlags&FlagsSampled == FlagsSampled
-}
-
 // TraceState returns the TraceState from the SpanContext.
 func (sc SpanContext) TraceState() TraceState {
 	return sc.traceState

trace/trace_test.go

@@ -164,41 +164,71 @@ func TestHasSpanID(t *testing.T) {
 	}
 }
 
-func TestSpanContextIsSampled(t *testing.T) {
+func TestTraceFlagsIsSampled(t *testing.T) {
 	for _, testcase := range []struct {
 		name string
-		sc   SpanContext
+		tf   TraceFlags
 		want bool
 	}{
 		{
 			name: "sampled",
-			sc: SpanContext{
-				traceID:    TraceID([16]byte{1}),
-				traceFlags: FlagsSampled,
-			},
+			tf:   FlagsSampled,
 			want: true,
 		}, {
 			name: "unused bits are ignored, still not sampled",
-			sc: SpanContext{
-				traceID:    TraceID([16]byte{1}),
-				traceFlags: ^FlagsSampled,
-			},
+			tf:   ^FlagsSampled,
 			want: false,
 		}, {
 			name: "unused bits are ignored, still sampled",
-			sc: SpanContext{
-				traceID:    TraceID([16]byte{1}),
-				traceFlags: FlagsSampled | ^FlagsSampled,
-			},
+			tf:   FlagsSampled | ^FlagsSampled,
 			want: true,
 		}, {
 			name: "not sampled/default",
-			sc:   SpanContext{traceID: TraceID{}},
 			want: false,
 		},
 	} {
 		t.Run(testcase.name, func(t *testing.T) {
-			have := testcase.sc.IsSampled()
+			have := testcase.tf.IsSampled()
+			if have != testcase.want {
+				t.Errorf("Want: %v, but have: %v", testcase.want, have)
+			}
+		})
+	}
+}
+
+func TestTraceFlagsWithSampled(t *testing.T) {
+	for _, testcase := range []struct {
+		name   string
+		start  TraceFlags
+		sample bool
+		want   TraceFlags
+	}{
+		{
+			name:   "sampled unchanged",
+			start:  FlagsSampled,
+			want:   FlagsSampled,
+			sample: true,
+		}, {
+			name:   "become sampled",
+			want:   FlagsSampled,
+			sample: true,
+		}, {
+			name:   "unused bits are ignored, still not sampled",
+			start:  ^FlagsSampled,
+			want:   ^FlagsSampled,
+			sample: false,
+		}, {
+			name:   "unused bits are ignored, becomes sampled",
+			start:  ^FlagsSampled,
+			want:   FlagsSampled | ^FlagsSampled,
+			sample: true,
+		}, {
+			name:   "not sampled/default",
+			sample: false,
+		},
+	} {
+		t.Run(testcase.name, func(t *testing.T) {
+			have := testcase.start.WithSampled(testcase.sample)
 			if have != testcase.want {
 				t.Errorf("Want: %v, but have: %v", testcase.want, have)
 			}