api(trace): change trace id to byte array. (open-telemetry#226)

* api(trace): change trace id to byte array. * fix lint errors * add helper to create trace id from hex and improve stdout exporter. * remove comma. * fix lint * change TraceIDFromHex to be compliant with w3 trace-context * revert remove of hex16 regex because its used to parse SpanID * lint * fix typo
ctsquires · Oct 23, 2019 · a6e139e · a6e139e
1 parent cf62d12
commit a6e139e
Show file tree

Hide file tree

Showing 21 changed files with 207 additions and 148 deletions.
diff --git a/api/core/span_context.go b/api/core/span_context.go
@@ -15,14 +15,13 @@
 package core
 
 import (
+	"bytes"
+	"encoding/hex"
+	"encoding/json"
+	"errors"
 	"fmt"
 )
 
-type TraceID struct {
-	High uint64
-	Low  uint64
-}
-
 const (
 	traceFlagsBitMaskSampled = byte(0x01)
 	traceFlagsBitMaskUnused  = byte(0xFE)
@@ -33,23 +32,81 @@ const (
 	TraceFlagsUnused  = traceFlagsBitMaskUnused
 )
 
+type TraceID [16]byte
+
+var nilTraceID TraceID
+
+func (t TraceID) isValid() bool {
+	return !bytes.Equal(t[:], nilTraceID[:])
+}
+
+// TraceIDFromHex returns a TraceID from a hex string if it is compliant
+// with the w3c trace-context specification.
+// See more at https://www.w3.org/TR/trace-context/#trace-id
+func TraceIDFromHex(h string) (TraceID, error) {
+	t := TraceID{}
+	if len(h) != 32 {
+		return t, errors.New("hex encoded trace-id must have length equals to 32")
+	}
+
+	for _, r := range h {
+		switch {
+		case 'a' <= r && r <= 'f':
+			continue
+		case '0' <= r && r <= '9':
+			continue
+		default:
+			return t, errors.New("trace-id can only contain [0-9a-f] characters, all lowercase")
+		}
+	}
+
+	b, err := hex.DecodeString(h)
+	if err != nil {
+		return t, err
+	}
+	copy(t[:], b)
+
+	if !t.isValid() {
+		return t, errors.New("trace-id can't be all zero")
+	}
+	return t, nil
+}
+
 type SpanContext struct {
 	TraceID    TraceID
 	SpanID     uint64
 	TraceFlags byte
 }
 
+var _ json.Marshaler = (*SpanContext)(nil)
+
 // EmptySpanContext is meant for internal use to return invalid span context during error conditions.
 func EmptySpanContext() SpanContext {
 	return SpanContext{}
 }
 
+// MarshalJSON implements a custom marshal function to encode SpanContext
+// in a human readable format with hex encoded TraceID and SpanID.
+func (sc SpanContext) MarshalJSON() ([]byte, error) {
+	type JSONSpanContext struct {
+		TraceID    string
+		SpanID     string
+		TraceFlags byte
+	}
+
+	return json.Marshal(JSONSpanContext{
+		TraceID:    sc.TraceIDString(),
+		SpanID:     sc.SpanIDString(),
+		TraceFlags: sc.TraceFlags,
+	})
+}
+
 func (sc SpanContext) IsValid() bool {
 	return sc.HasTraceID() && sc.HasSpanID()
 }
 
 func (sc SpanContext) HasTraceID() bool {
-	return sc.TraceID.High != 0 || sc.TraceID.Low != 0
+	return sc.TraceID.isValid()
 }
 
 func (sc SpanContext) HasSpanID() bool {
@@ -61,9 +118,7 @@ func (sc SpanContext) SpanIDString() string {
 }
 
 func (sc SpanContext) TraceIDString() string {
-	p1 := fmt.Sprintf("%.16x", sc.TraceID.High)
-	p2 := fmt.Sprintf("%.16x", sc.TraceID.Low)
-	return p1 + p2
+	return hex.EncodeToString(sc.TraceID[:])
 }
 
 func (sc SpanContext) IsSampled() bool {

diff --git a/api/core/span_context_test.go b/api/core/span_context_test.go
@@ -29,22 +29,22 @@ func TestIsValid(t *testing.T) {
 	}{
 		{
 			name: "SpanContext.IsValid() returns true if sc has both an Trace ID and Span ID",
-			tid:  core.TraceID{High: uint64(42)},
+			tid:  core.TraceID([16]byte{1}),
 			sid:  uint64(42),
 			want: true,
 		}, {
 			name: "SpanContext.IsValid() returns false if sc has neither an Trace ID nor Span ID",
-			tid:  core.TraceID{High: uint64(0)},
+			tid:  core.TraceID([16]byte{}),
 			sid:  uint64(0),
 			want: false,
 		}, {
 			name: "SpanContext.IsValid() returns false if sc has a Span ID but not a Trace ID",
-			tid:  core.TraceID{High: uint64(0)},
+			tid:  core.TraceID([16]byte{}),
 			sid:  uint64(42),
 			want: false,
 		}, {
 			name: "SpanContext.IsValid() returns false if sc has a Trace ID but not a Span ID",
-			tid:  core.TraceID{High: uint64(42)},
+			tid:  core.TraceID([16]byte{1}),
 			sid:  uint64(0),
 			want: false,
 		},
@@ -62,6 +62,50 @@ func TestIsValid(t *testing.T) {
 	}
 }
 
+func TestIsValidFromHex(t *testing.T) {
+	for _, testcase := range []struct {
+		name  string
+		hex   string
+		tid   core.TraceID
+		valid bool
+	}{
+		{
+			name:  "Valid TraceID",
+			tid:   core.TraceID([16]byte{128, 241, 152, 238, 86, 52, 59, 168, 100, 254, 139, 42, 87, 211, 239, 247}),
+			hex:   "80f198ee56343ba864fe8b2a57d3eff7",
+			valid: true,
+		}, {
+			name:  "Invalid TraceID with invalid length",
+			hex:   "80f198ee56343ba864fe8b2a57d3eff",
+			valid: false,
+		}, {
+			name:  "Invalid TraceID with invalid char",
+			hex:   "80f198ee56343ba864fe8b2a57d3efg7",
+			valid: false,
+		}, {
+			name:  "Invalid TraceID with uppercase",
+			hex:   "80f198ee56343ba864fe8b2a57d3efF7",
+			valid: false,
+		},
+	} {
+		t.Run(testcase.name, func(t *testing.T) {
+			tid, err := core.TraceIDFromHex(testcase.hex)
+
+			if testcase.valid && err != nil {
+				t.Errorf("Expected TraceID %s to be valid but end with error %s", testcase.hex, err.Error())
+			}
+
+			if !testcase.valid && err == nil {
+				t.Errorf("Expected TraceID %s to be invalid but end no error", testcase.hex)
+			}
+
+			if tid != testcase.tid {
+				t.Errorf("Want: %v, but have: %v", testcase.tid, tid)
+			}
+		})
+	}
+}
+
 func TestHasTraceID(t *testing.T) {
 	for _, testcase := range []struct {
 		name string
@@ -70,20 +114,12 @@ func TestHasTraceID(t *testing.T) {
 	}{
 		{
 			name: "SpanContext.HasTraceID() returns true if both Low and High are nonzero",
-			tid:  core.TraceID{High: uint64(42), Low: uint64(42)},
+			tid:  core.TraceID([16]byte{1}),
 			want: true,
 		}, {
 			name: "SpanContext.HasTraceID() returns false if neither Low nor High are nonzero",
 			tid:  core.TraceID{},
 			want: false,
-		}, {
-			name: "SpanContext.HasTraceID() returns true if High != 0",
-			tid:  core.TraceID{High: uint64(42)},
-			want: true,
-		}, {
-			name: "SpanContext.HasTraceID() returns true if Low != 0",
-			tid:  core.TraceID{Low: uint64(42)},
-			want: true,
 		},
 	} {
 		t.Run(testcase.name, func(t *testing.T) {
@@ -158,12 +194,9 @@ func TestTraceIDString(t *testing.T) {
 		{
 			name: "SpanContext.TraceIDString returns string representation of self.TraceID values > 0",
 			sc: core.SpanContext{
-				TraceID: core.TraceID{
-					High: uint64(42),
-					Low:  uint64(42),
-				},
+				TraceID: core.TraceID([16]byte{255}),
 			},
-			want: `000000000000002a000000000000002a`,
+			want: `ff000000000000000000000000000000`,
 		}, {
 			name: "SpanContext.TraceIDString returns string representation of self.TraceID values == 0",
 			sc:   core.SpanContext{TraceID: core.TraceID{}},
@@ -189,20 +222,14 @@ func TestSpanContextIsSampled(t *testing.T) {
 		{
 			name: "sampled",
 			sc: core.SpanContext{
-				TraceID: core.TraceID{
-					High: uint64(42),
-					Low:  uint64(42),
-				},
+				TraceID:    core.TraceID([16]byte{1}),
 				TraceFlags: core.TraceFlagsSampled,
 			},
 			want: true,
 		}, {
 			name: "sampled plus unused",
 			sc: core.SpanContext{
-				TraceID: core.TraceID{
-					High: uint64(42),
-					Low:  uint64(42),
-				},
+				TraceID:    core.TraceID([16]byte{1}),
 				TraceFlags: core.TraceFlagsSampled | core.TraceFlagsUnused,
 			},
 			want: true,

diff --git a/experimental/bridge/opentracing/bridge.go b/experimental/bridge/opentracing/bridge.go
@@ -31,7 +31,7 @@ import (
 	otelcore "go.opentelemetry.io/api/core"
 	oteltrace "go.opentelemetry.io/api/trace"
 
-	migration "go.opentelemetry.io/experimental/bridge/opentracing/migration"
+	"go.opentelemetry.io/experimental/bridge/opentracing/migration"
 )
 
 type bridgeSpanContext struct {
@@ -512,7 +512,7 @@ func (t *BridgeTracer) Inject(sm ot.SpanContext, format interface{}, carrier int
 	if !ok {
 		return ot.ErrInvalidCarrier
 	}
-	hhcarrier.Set(traceIDHeader, traceIDString(bridgeSC.otelSpanContext.TraceID))
+	hhcarrier.Set(traceIDHeader, bridgeSC.otelSpanContext.TraceIDString())
 	hhcarrier.Set(spanIDHeader, spanIDToString(bridgeSC.otelSpanContext.SpanID))
 	hhcarrier.Set(traceFlagsHeader, traceFlagsToString(bridgeSC.otelSpanContext.TraceFlags))
 	bridgeSC.ForeachBaggageItem(func(k, v string) bool {
@@ -523,12 +523,6 @@ func (t *BridgeTracer) Inject(sm ot.SpanContext, format interface{}, carrier int
 	return nil
 }
 
-// mostly copied from core/span_context.go, but I prefer not to rely
-// on some impl details
-func traceIDString(traceID otelcore.TraceID) string {
-	return fmt.Sprintf("%.16x%.16x", traceID.High, traceID.Low)
-}
-
 func spanIDToString(spanID uint64) string {
 	return fmt.Sprintf("%.16x", spanID)
 }
@@ -558,7 +552,7 @@ func (t *BridgeTracer) Extract(format interface{}, carrier interface{}) (ot.Span
 		ck := http.CanonicalHeaderKey(k)
 		switch ck {
 		case traceIDHeader:
-			traceID, err := traceIDFromString(v)
+			traceID, err := otelcore.TraceIDFromHex(v)
 			if err != nil {
 				return err
 			}
@@ -588,23 +582,6 @@ func (t *BridgeTracer) Extract(format interface{}, carrier interface{}) (ot.Span
 	return bridgeSC, nil
 }
 
-func traceIDFromString(s string) (otelcore.TraceID, error) {
-	traceID := otelcore.TraceID{}
-	if len(s) != 32 {
-		return traceID, fmt.Errorf("invalid trace ID")
-	}
-	high, err := strconv.ParseUint(s[0:16], 16, 64)
-	if err != nil {
-		return traceID, err
-	}
-	low, err := strconv.ParseUint(s[16:32], 16, 64)
-	if err != nil {
-		return traceID, err
-	}
-	traceID.High, traceID.Low = high, low
-	return traceID, nil
-}
-
 func spanIDFromString(s string) (uint64, error) {
 	if len(s) != 16 {
 		return 0, fmt.Errorf("invalid span ID")

diff --git a/experimental/bridge/opentracing/internal/mock.go b/experimental/bridge/opentracing/internal/mock.go
@@ -145,11 +145,7 @@ func (t *MockTracer) getTraceID(ctx context.Context, spanOpts *oteltrace.SpanOpt
 		}
 		return traceID
 	}
-	uints := t.getNRandUint64(2)
-	return otelcore.TraceID{
-		High: uints[0],
-		Low:  uints[1],
-	}
+	return t.getRandTraceID()
 }
 
 func (t *MockTracer) getParentSpanID(ctx context.Context, spanOpts *oteltrace.SpanOptions) uint64 {
@@ -183,17 +179,19 @@ func (t *MockTracer) getSpanID() uint64 {
 }
 
 func (t *MockTracer) getRandUint64() uint64 {
-	return t.getNRandUint64(1)[0]
+	t.randLock.Lock()
+	defer t.randLock.Unlock()
+	return t.rand.Uint64()
 }
 
-func (t *MockTracer) getNRandUint64(n int) []uint64 {
-	uints := make([]uint64, n)
+func (t *MockTracer) getRandTraceID() otelcore.TraceID {
 	t.randLock.Lock()
 	defer t.randLock.Unlock()
-	for i := 0; i < n; i++ {
-		uints[i] = t.rand.Uint64()
-	}
-	return uints
+
+	tid := otelcore.TraceID{}
+	t.rand.Read(tid[:])
+
+	return tid
 }
 
 func (t *MockTracer) DeferredContextSetupHook(ctx context.Context, span oteltrace.Span) context.Context {

diff --git a/experimental/bridge/opentracing/mix_test.go b/experimental/bridge/opentracing/mix_test.go
@@ -504,10 +504,7 @@ func reverse(length int, swap func(i, j int)) {
 }
 
 func simpleTraceID() otelcore.TraceID {
-	return otelcore.TraceID{
-		High: 1357,
-		Low:  2468,
-	}
+	return [16]byte{123, 42}
 }
 
 func simpleSpanIDs(count int) []uint64 {

diff --git a/exporter/trace/jaeger/jaeger.go b/exporter/trace/jaeger/jaeger.go
@@ -16,6 +16,7 @@ package jaeger
 
 import (
 	"context"
+	"encoding/binary"
 	"log"
 	"sync"
 
@@ -196,8 +197,8 @@ func spanDataToThrift(data *export.SpanData) *gen.Span {
 	var refs []*gen.SpanRef
 	for _, link := range data.Links {
 		refs = append(refs, &gen.SpanRef{
-			TraceIdHigh: int64(link.TraceID.High),
-			TraceIdLow:  int64(link.TraceID.Low),
+			TraceIdHigh: int64(binary.BigEndian.Uint64(link.TraceID[0:8])),
+			TraceIdLow:  int64(binary.BigEndian.Uint64(link.TraceID[8:16])),
 			SpanId:      int64(link.SpanID),
 			// TODO(paivagustavo): properly set the reference type when specs are defined
 			//  see https://github.com/open-telemetry/opentelemetry-specification/issues/65
@@ -206,8 +207,8 @@ func spanDataToThrift(data *export.SpanData) *gen.Span {
 	}
 
 	return &gen.Span{
-		TraceIdHigh:   int64(data.SpanContext.TraceID.High),
-		TraceIdLow:    int64(data.SpanContext.TraceID.Low),
+		TraceIdHigh:   int64(binary.BigEndian.Uint64(data.SpanContext.TraceID[0:8])),
+		TraceIdLow:    int64(binary.BigEndian.Uint64(data.SpanContext.TraceID[8:16])),
 		SpanId:        int64(data.SpanContext.SpanID),
 		ParentSpanId:  int64(data.ParentSpanID),
 		OperationName: data.Name, // TODO: if span kind is added then add prefix "Sent"/"Recv"