From c6b92d5b20a1f4d6d2ed15f6f9d1d12b0b145fed Mon Sep 17 00:00:00 2001 From: Anthony Mirabella Date: Mon, 5 Apr 2021 13:21:42 -0400 Subject: [PATCH] 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 * Rename `TraceFlags.Sampled()` to `TraceFlags.WithSampled()` for consistency * Restore `SpanContext.IsSampled()` method. Signed-off-by: Anthony J Mirabella --- CHANGELOG.md | 2 + bridge/opencensus/utils/utils.go | 8 +--- bridge/opencensus/utils/utils_test.go | 13 ------ exporters/otlp/otlp_span_test.go | 10 ++--- exporters/stdout/trace_test.go | 4 +- propagation/trace_context.go | 9 ++-- sdk/trace/simple_span_processor.go | 2 +- trace/trace.go | 64 ++++++++++++++++----------- trace/trace_test.go | 62 +++++++++++++++++++------- 9 files changed, 101 insertions(+), 73 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d5723b31b31..a059c08cd1f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/bridge/opencensus/utils/utils.go b/bridge/opencensus/utils/utils.go index c5abc6728cb..91f586d57a7 100644 --- a/bridge/opencensus/utils/utils.go +++ b/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 } diff --git a/bridge/opencensus/utils/utils_test.go b/bridge/opencensus/utils/utils_test.go index 9e7a7741b67..cc9674c7fdd 100644 --- a/bridge/opencensus/utils/utils_test.go +++ b/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) diff --git a/exporters/otlp/otlp_span_test.go b/exporters/otlp/otlp_span_test.go index 7e71df21d41..c5ca21fed1d 100644 --- a/exporters/otlp/otlp_span_test.go +++ b/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", diff --git a/exporters/stdout/trace_test.go b/exporters/stdout/trace_test.go index 7c0f7e91714..e9208c1dc36 100644 --- a/exporters/stdout/trace_test.go +++ b/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` + `},` + diff --git a/propagation/trace_context.go b/propagation/trace_context.go index 014d190caec..82de416bea6 100644 --- a/propagation/trace_context.go +++ b/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 diff --git a/sdk/trace/simple_span_processor.go b/sdk/trace/simple_span_processor.go index 9efffb01895..3efe54d54bf 100644 --- a/sdk/trace/simple_span_processor.go +++ b/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) diff --git a/trace/trace.go b/trace/trace.go index 893c0c8fe75..4a5e1201370 100644 --- a/trace/trace.go +++ b/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 diff --git a/trace/trace_test.go b/trace/trace_test.go index 40422b6f579..dc8afe47e50 100644 --- a/trace/trace_test.go +++ b/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) }