[refactor] Clean up span.kind tag handling (#6390)

## Description of the changes
- Introduce strongly typed constants and keys for some standard tags

---------

Signed-off-by: Yuri Shkuro <[email protected]>

cmd/agent/app/reporter/grpc/reporter_test.go

@@ -67,7 +67,7 @@ func TestReporter_EmitZipkinBatch(t *testing.T) {
 			expected: model.Batch{
 				Spans: []*model.Span{{
 					TraceID: model.NewTraceID(0, 1), SpanID: model.NewSpanID(2), OperationName: "jonatan", Duration: time.Microsecond * 1,
-					Tags:    model.KeyValues{{Key: "span.kind", VStr: "client", VType: model.StringType}},
+					Tags:    model.KeyValues{model.SpanKindTag(model.SpanKindClient)},
 					Process: &model.Process{ServiceName: "spring"}, StartTime: tm.UTC(),
 				}},
 			},

cmd/anonymizer/app/anonymizer/anonymizer.go

@@ -21,12 +21,12 @@ import (
 )
 
 var allowedTags = map[string]bool{
-	"error":            true,
-	"span.kind":        true,
-	"http.method":      true,
-	"http.status_code": true,
-	"sampler.type":     true,
-	"sampler.param":    true,
+	"error":               true,
+	"http.method":         true,
+	"http.status_code":    true,
+	model.SpanKindKey:     true,
+	model.SamplerTypeKey:  true,
+	model.SamplerParamKey: true,
 }
 
 const PermUserRW = 0o600 // Read-write for owner only

cmd/anonymizer/app/uiconv/extractor_test.go

@@ -43,7 +43,7 @@ func TestExtractorTraceSuccess(t *testing.T) {
 
 	for i := range trace.Data {
 		for j := range trace.Data[i].Spans {
-			assert.Equal(t, "span.kind", trace.Data[i].Spans[j].Tags[0].Key)
+			assert.Equal(t, model.SpanKindKey, trace.Data[i].Spans[j].Tags[0].Key)
 		}
 	}
 }

cmd/anonymizer/app/uiconv/module_test.go

@@ -10,6 +10,8 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/zap"
+
+	"github.com/jaegertracing/jaeger/model"
 )
 
 func TestModule_TraceSuccess(t *testing.T) {
@@ -30,7 +32,7 @@ func TestModule_TraceSuccess(t *testing.T) {
 
 	for i := range trace.Data {
 		for j := range trace.Data[i].Spans {
-			assert.Equal(t, "span.kind", trace.Data[i].Spans[j].Tags[0].Key)
+			assert.Equal(t, model.SpanKindKey, trace.Data[i].Spans[j].Tags[0].Key)
 		}
 	}
 }

cmd/query/app/apiv3/gateway_test.go

@@ -90,7 +90,7 @@ func makeTestTrace() (*model.Trace, spanstore.GetTraceParameters) {
 				SpanID:        model.NewSpanID(180),
 				OperationName: "foobar",
 				Tags: []model.KeyValue{
-					model.String("span.kind", "server"),
+					model.SpanKindTag(model.SpanKindServer),
 					model.Bool("error", true),
 				},
 			},

model/adjuster/span_hash_deduper_test.go

@@ -8,7 +8,6 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"go.opentelemetry.io/otel/trace"
 
 	"github.com/jaegertracing/jaeger/model"
 )
@@ -22,15 +21,15 @@ func newDuplicatedSpansTrace() *model.Trace {
 				SpanID:  clientSpanID,
 				Tags: model.KeyValues{
 					// span.kind = server
-					model.String(keySpanKind, trace.SpanKindServer.String()),
+					model.SpanKindTag(model.SpanKindServer),
 				},
 			},
 			{
 				TraceID: traceID,
 				SpanID:  clientSpanID, // shared span ID
 				Tags: model.KeyValues{
 					// span.kind = server
-					model.String(keySpanKind, trace.SpanKindServer.String()),
+					model.SpanKindTag(model.SpanKindServer),
 				},
 			},
 			{
@@ -52,7 +51,7 @@ func newUniqueSpansTrace() *model.Trace {
 				SpanID:  anotherSpanID,
 				Tags: model.KeyValues{
 					// span.kind = server
-					model.String(keySpanKind, trace.SpanKindServer.String()),
+					model.SpanKindTag(model.SpanKindServer),
 				},
 			},
 			{

model/adjuster/zipkin_span_id_uniquify_test.go

@@ -9,15 +9,13 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"go.opentelemetry.io/otel/trace"
 
 	"github.com/jaegertracing/jaeger/model"
 )
 
 var (
 	clientSpanID  = model.NewSpanID(1)
 	anotherSpanID = model.NewSpanID(11)
-	keySpanKind   = "span.kind"
 )
 
 func newZipkinTrace() *model.Trace {
@@ -30,7 +28,7 @@ func newZipkinTrace() *model.Trace {
 				SpanID:  clientSpanID,
 				Tags: model.KeyValues{
 					// span.kind = client
-					model.String(keySpanKind, trace.SpanKindClient.String()),
+					model.SpanKindTag(model.SpanKindClient),
 				},
 			},
 			{
@@ -39,7 +37,7 @@ func newZipkinTrace() *model.Trace {
 				SpanID:  clientSpanID, // shared span ID
 				Tags: model.KeyValues{
 					// span.kind = server
-					model.String(keySpanKind, trace.SpanKindServer.String()),
+					model.SpanKindTag(model.SpanKindServer),
 				},
 			},
 			{

model/converter/thrift/zipkin/to_domain.go

@@ -21,7 +21,6 @@ import (
 const (
 	// UnknownServiceName is serviceName we give to model.Process if we cannot find it anywhere in a Zipkin span
 	UnknownServiceName = "unknown-service-name"
-	keySpanKind        = "span.kind"
 	component          = "component"
 	peerservice        = "peer.service"
 	peerHostIPv4       = "peer.ipv4"
@@ -166,15 +165,15 @@ func (td toDomain) transformSpan(zSpan *zipkincore.Span) []*model.Span {
 		}
 		// if the first span is a client span we create server span and vice-versa.
 		if result[0].IsRPCClient() {
-			s.Tags = []model.KeyValue{model.String(keySpanKind, trace.SpanKindServer.String())}
+			s.Tags = []model.KeyValue{model.SpanKindTag(model.SpanKindServer)}
 			//nolint: gosec // G115
 			s.StartTime = model.EpochMicrosecondsAsTime(uint64(sr.Timestamp))
 			if ss := td.findAnnotation(zSpan, zipkincore.SERVER_SEND); ss != nil {
 				//nolint: gosec // G115
 				s.Duration = model.MicrosecondsAsDuration(uint64(ss.Timestamp - sr.Timestamp))
 			}
 		} else {
-			s.Tags = []model.KeyValue{model.String(keySpanKind, trace.SpanKindClient.String())}
+			s.Tags = []model.KeyValue{model.SpanKindTag(model.SpanKindClient)}
 			//nolint: gosec // G115
 			s.StartTime = model.EpochMicrosecondsAsTime(uint64(cs.Timestamp))
 			if cr := td.findAnnotation(zSpan, zipkincore.CLIENT_RECV); cr != nil {
@@ -393,7 +392,7 @@ func (toDomain) getLogFields(annotation *zipkincore.Annotation) []model.KeyValue
 func (toDomain) getSpanKindTag(annotations []*zipkincore.Annotation) (model.KeyValue, bool) {
 	for _, a := range annotations {
 		if spanKind, ok := coreAnnotations[a.Value]; ok {
-			return model.String(keySpanKind, spanKind), true
+			return model.SpanKindTag(model.SpanKind(spanKind)), true
 		}
 	}
 	return model.KeyValue{}, false

model/converter/thrift/zipkin/to_domain_test.go

@@ -20,7 +20,6 @@ import (
 	"github.com/kr/pretty"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"go.opentelemetry.io/otel/trace"
 
 	"github.com/jaegertracing/jaeger/model"
 	z "github.com/jaegertracing/jaeger/thrift-gen/zipkincore"
@@ -107,30 +106,30 @@ func TestToDomainMultipleSpanKinds(t *testing.T) {
 		json         string
 		tagFirstKey  string
 		tagSecondKey string
-		tagFirstVal  trace.SpanKind
-		tagSecondVal trace.SpanKind
+		tagFirstVal  model.SpanKind
+		tagSecondVal model.SpanKind
 	}{
 		{
 			json: `[{ "trace_id": -1, "id": 31, "annotations": [
 		{"value": "cs", "host": {"service_name": "bar", "ipv4": 23456}},
 		{"value": "sr", "timestamp": 1, "host": {"service_name": "bar", "ipv4": 23456}},
 		{"value": "ss", "timestamp": 2, "host": {"service_name": "bar", "ipv4": 23456}}
 		]}]`,
-			tagFirstKey:  keySpanKind,
-			tagSecondKey: keySpanKind,
-			tagFirstVal:  trace.SpanKindClient,
-			tagSecondVal: trace.SpanKindServer,
+			tagFirstKey:  model.SpanKindKey,
+			tagSecondKey: model.SpanKindKey,
+			tagFirstVal:  model.SpanKindClient,
+			tagSecondVal: model.SpanKindServer,
 		},
 		{
 			json: `[{ "trace_id": -1, "id": 31, "annotations": [
 		{"value": "sr", "host": {"service_name": "bar", "ipv4": 23456}},
 		{"value": "cs", "timestamp": 1, "host": {"service_name": "bar", "ipv4": 23456}},
 		{"value": "cr", "timestamp": 2, "host": {"service_name": "bar", "ipv4": 23456}}
 		]}]`,
-			tagFirstKey:  keySpanKind,
-			tagSecondKey: keySpanKind,
-			tagFirstVal:  trace.SpanKindServer,
-			tagSecondVal: trace.SpanKindClient,
+			tagFirstKey:  model.SpanKindKey,
+			tagSecondKey: model.SpanKindKey,
+			tagFirstVal:  model.SpanKindServer,
+			tagSecondVal: model.SpanKindClient,
 		},
 	}
 
@@ -141,12 +140,12 @@ func TestToDomainMultipleSpanKinds(t *testing.T) {
 		assert.Len(t, trc.Spans, 2)
 		assert.Len(t, trc.Spans[0].Tags, 1)
 		assert.Equal(t, test.tagFirstKey, trc.Spans[0].Tags[0].Key)
-		assert.Equal(t, test.tagFirstVal.String(), trc.Spans[0].Tags[0].VStr)
+		assert.EqualValues(t, test.tagFirstVal, trc.Spans[0].Tags[0].VStr)
 
 		assert.Len(t, trc.Spans[1].Tags, 1)
 		assert.Equal(t, test.tagSecondKey, trc.Spans[1].Tags[0].Key)
 		assert.Equal(t, time.Duration(1000), trc.Spans[1].Duration)
-		assert.Equal(t, test.tagSecondVal.String(), trc.Spans[1].Tags[0].VStr)
+		assert.EqualValues(t, test.tagSecondVal, trc.Spans[1].Tags[0].VStr)
 	}
 }
 

model/keyvalue.go

@@ -25,8 +25,32 @@ const (
 	Float64Type = ValueType_FLOAT64
 	// BinaryType indicates the value is binary blob stored as a byte array
 	BinaryType = ValueType_BINARY
+
+	SpanKindKey     = "span.kind"
+	SamplerTypeKey  = "sampler.type"
+	SamplerParamKey = "sampler.param"
+)
+
+type SpanKind string
+
+const (
+	SpanKindClient      SpanKind = "client"
+	SpanKindServer      SpanKind = "server"
+	SpanKindProducer    SpanKind = "producer"
+	SpanKindConsumer    SpanKind = "consumer"
+	SpanKindInternal    SpanKind = "internal"
+	SpanKindUnspecified SpanKind = ""
 )
 
+func SpanKindFromString(kind string) (SpanKind, error) {
+	switch SpanKind(kind) {
+	case SpanKindClient, SpanKindServer, SpanKindProducer, SpanKindConsumer, SpanKindInternal, SpanKindUnspecified:
+		return SpanKind(kind), nil
+	default:
+		return SpanKindUnspecified, fmt.Errorf("unknown span kind %q", kind)
+	}
+}
+
 // KeyValues is a type alias that exposes convenience functions like Sort, FindByKey.
 type KeyValues []KeyValue
 

model/keyvalues_test.go

@@ -40,6 +40,29 @@ func TestKeyValuesSort(t *testing.T) {
 	assert.Equal(t, expected, input)
 }
 
+func TestSpanKindFromString(t *testing.T) {
+	testCases := []struct {
+		input  string
+		output model.SpanKind
+		err    string
+	}{
+		{input: "client", output: model.SpanKindClient},
+		{input: "", output: model.SpanKindUnspecified},
+		{input: "foobar", output: model.SpanKindUnspecified, err: "unknown span kind"},
+	}
+	for _, test := range testCases {
+		t.Run(fmt.Sprintf("%+v", test), func(t *testing.T) {
+			v, err := model.SpanKindFromString(test.input)
+			if test.err != "" {
+				require.ErrorContains(t, err, test.err)
+			} else {
+				require.NoError(t, err)
+			}
+			assert.Equal(t, test.output, v)
+		})
+	}
+}
+
 func TestKeyValuesFindByKey(t *testing.T) {
 	input := model.KeyValues{
 		model.String("x", "z"),

model/span.go

@@ -9,7 +9,6 @@ import (
 	"io"
 	"strconv"
 
-	"go.opentelemetry.io/otel/trace"
 	"go.uber.org/zap"
 )
 
@@ -28,23 +27,11 @@ const (
 	DebugFlag = Flags(2)
 	// FirehoseFlag is the bit in Flags in order to define a span as a firehose span
 	FirehoseFlag = Flags(8)
-
-	keySamplerType  = "sampler.type"
-	keySpanKind     = "span.kind"
-	keySamplerParam = "sampler.param"
 )
 
 // Flags is a bit map of flags for a span
 type Flags uint32
 
-var toSpanKind = map[string]trace.SpanKind{
-	"client":   trace.SpanKindClient,
-	"server":   trace.SpanKindServer,
-	"producer": trace.SpanKindProducer,
-	"consumer": trace.SpanKindConsumer,
-	"internal": trace.SpanKindInternal,
-}
-
 var toSamplerType = map[string]SamplerType{
 	"unrecognized":  SamplerTypeUnrecognized,
 	"probabilistic": SamplerTypeProbabilistic,
@@ -70,6 +57,10 @@ func (s SamplerType) String() string {
 	}
 }
 
+func SpanKindTag(kind SpanKind) KeyValue {
+	return String(SpanKindKey, string(kind))
+}
+
 // Hash implements Hash from Hashable.
 func (s *Span) Hash(w io.Writer) (err error) {
 	// gob is not the most efficient way, but it ensures we don't miss any fields.
@@ -79,27 +70,27 @@ func (s *Span) Hash(w io.Writer) (err error) {
 }
 
 // HasSpanKind returns true if the span has a `span.kind` tag set to `kind`.
-func (s *Span) HasSpanKind(kind trace.SpanKind) bool {
-	if tag, ok := KeyValues(s.Tags).FindByKey(keySpanKind); ok {
-		return tag.AsString() == kind.String()
+func (s *Span) HasSpanKind(kind SpanKind) bool {
+	if tag, ok := KeyValues(s.Tags).FindByKey(SpanKindKey); ok {
+		return tag.AsString() == string(kind)
 	}
 	return false
 }
 
 // GetSpanKind returns value of `span.kind` tag and whether the tag can be found
-func (s *Span) GetSpanKind() (spanKind trace.SpanKind, found bool) {
-	if tag, ok := KeyValues(s.Tags).FindByKey(keySpanKind); ok {
-		if kind, ok := toSpanKind[tag.AsString()]; ok {
+func (s *Span) GetSpanKind() (spanKind SpanKind, found bool) {
+	if tag, ok := KeyValues(s.Tags).FindByKey(SpanKindKey); ok {
+		if kind, err := SpanKindFromString(tag.AsString()); err == nil {
 			return kind, true
 		}
 	}
-	return trace.SpanKindUnspecified, false
+	return SpanKindUnspecified, false
 }
 
 // GetSamplerType returns the sampler type for span
 func (s *Span) GetSamplerType() SamplerType {
 	// There's no corresponding opentelemetry tag label corresponding to sampler.type
-	if tag, ok := KeyValues(s.Tags).FindByKey(keySamplerType); ok {
+	if tag, ok := KeyValues(s.Tags).FindByKey(SamplerTypeKey); ok {
 		if s, ok := toSamplerType[tag.VStr]; ok {
 			return s
 		}
@@ -110,13 +101,13 @@ func (s *Span) GetSamplerType() SamplerType {
 // IsRPCClient returns true if the span represents a client side of an RPC,
 // as indicated by the `span.kind` tag set to `client`.
 func (s *Span) IsRPCClient() bool {
-	return s.HasSpanKind(trace.SpanKindClient)
+	return s.HasSpanKind(SpanKindClient)
 }
 
 // IsRPCServer returns true if the span represents a server side of an RPC,
 // as indicated by the `span.kind` tag set to `server`.
 func (s *Span) IsRPCServer() bool {
-	return s.HasSpanKind(trace.SpanKindServer)
+	return s.HasSpanKind(SpanKindServer)
 }
 
 // NormalizeTimestamps changes all timestamps in this span to UTC.
@@ -168,7 +159,7 @@ func (s *Span) GetSamplerParams(logger *zap.Logger) (SamplerType, float64) {
 	if samplerType == SamplerTypeUnrecognized {
 		return SamplerTypeUnrecognized, 0
 	}
-	tag, ok := KeyValues(s.Tags).FindByKey(keySamplerParam)
+	tag, ok := KeyValues(s.Tags).FindByKey(SamplerParamKey)
 	if !ok {
 		return SamplerTypeUnrecognized, 0
 	}