From 03b768f10773f50da3e0e874c807356f5ab8548a Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Thu, 19 Dec 2024 20:38:11 -0500 Subject: [PATCH] Clean up span.kind tag handling Signed-off-by: Yuri Shkuro --- cmd/agent/app/reporter/grpc/reporter_test.go | 2 +- cmd/anonymizer/app/anonymizer/anonymizer.go | 12 +++--- cmd/anonymizer/app/uiconv/extractor_test.go | 2 +- cmd/anonymizer/app/uiconv/module_test.go | 3 +- cmd/query/app/apiv3/gateway_test.go | 2 +- model/adjuster/span_hash_deduper_test.go | 7 ++-- .../adjuster/zipkin_span_id_uniquify_test.go | 6 +-- model/converter/thrift/zipkin/to_domain.go | 7 ++-- .../converter/thrift/zipkin/to_domain_test.go | 25 ++++++------ model/keyvalue.go | 24 ++++++++++++ model/span.go | 39 +++++++------------ model/span_test.go | 10 ++--- plugin/storage/cassandra/spanstore/writer.go | 4 +- plugin/storage/memory/memory.go | 4 +- plugin/storage/memory/memory_test.go | 6 +-- 15 files changed, 81 insertions(+), 72 deletions(-) diff --git a/cmd/agent/app/reporter/grpc/reporter_test.go b/cmd/agent/app/reporter/grpc/reporter_test.go index 32ed9253033..e5f89dd3266 100644 --- a/cmd/agent/app/reporter/grpc/reporter_test.go +++ b/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(), }}, }, diff --git a/cmd/anonymizer/app/anonymizer/anonymizer.go b/cmd/anonymizer/app/anonymizer/anonymizer.go index 1a9a5a8305c..9d22f5e6cd3 100644 --- a/cmd/anonymizer/app/anonymizer/anonymizer.go +++ b/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 diff --git a/cmd/anonymizer/app/uiconv/extractor_test.go b/cmd/anonymizer/app/uiconv/extractor_test.go index 5f06f338462..9588850a9cd 100644 --- a/cmd/anonymizer/app/uiconv/extractor_test.go +++ b/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) } } } diff --git a/cmd/anonymizer/app/uiconv/module_test.go b/cmd/anonymizer/app/uiconv/module_test.go index 02b541cfb3c..2e8afbb3fc9 100644 --- a/cmd/anonymizer/app/uiconv/module_test.go +++ b/cmd/anonymizer/app/uiconv/module_test.go @@ -7,6 +7,7 @@ import ( "os" "testing" + "github.com/jaegertracing/jaeger/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/zap" @@ -30,7 +31,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) } } } diff --git a/cmd/query/app/apiv3/gateway_test.go b/cmd/query/app/apiv3/gateway_test.go index 22a08dc2641..64d71fa73cf 100644 --- a/cmd/query/app/apiv3/gateway_test.go +++ b/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), }, }, diff --git a/model/adjuster/span_hash_deduper_test.go b/model/adjuster/span_hash_deduper_test.go index 6fd898d264a..d8bffd5bf82 100644 --- a/model/adjuster/span_hash_deduper_test.go +++ b/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,7 +21,7 @@ func newDuplicatedSpansTrace() *model.Trace { SpanID: clientSpanID, Tags: model.KeyValues{ // span.kind = server - model.String(keySpanKind, trace.SpanKindServer.String()), + model.SpanKindTag(model.SpanKindServer), }, }, { @@ -30,7 +29,7 @@ func newDuplicatedSpansTrace() *model.Trace { 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), }, }, { diff --git a/model/adjuster/zipkin_span_id_uniquify_test.go b/model/adjuster/zipkin_span_id_uniquify_test.go index 13a2114ebc5..c0b92799c21 100644 --- a/model/adjuster/zipkin_span_id_uniquify_test.go +++ b/model/adjuster/zipkin_span_id_uniquify_test.go @@ -9,7 +9,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "go.opentelemetry.io/otel/trace" "github.com/jaegertracing/jaeger/model" ) @@ -17,7 +16,6 @@ import ( 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), }, }, { diff --git a/model/converter/thrift/zipkin/to_domain.go b/model/converter/thrift/zipkin/to_domain.go index c287d958c1a..00ca20408fd 100644 --- a/model/converter/thrift/zipkin/to_domain.go +++ b/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,7 +165,7 @@ 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 { @@ -174,7 +173,7 @@ func (td toDomain) transformSpan(zSpan *zipkincore.Span) []*model.Span { 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 diff --git a/model/converter/thrift/zipkin/to_domain_test.go b/model/converter/thrift/zipkin/to_domain_test.go index d70f24e09c2..316a3a8c8ed 100644 --- a/model/converter/thrift/zipkin/to_domain_test.go +++ b/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,8 +106,8 @@ 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": [ @@ -116,10 +115,10 @@ func TestToDomainMultipleSpanKinds(t *testing.T) { {"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": [ @@ -127,10 +126,10 @@ func TestToDomainMultipleSpanKinds(t *testing.T) { {"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) } } diff --git a/model/keyvalue.go b/model/keyvalue.go index 68fc91ea3a4..72304eeee91 100644 --- a/model/keyvalue.go +++ b/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 diff --git a/model/span.go b/model/span.go index 51d96cae40a..3ec5a9431a2 100644 --- a/model/span.go +++ b/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 } diff --git a/model/span_test.go b/model/span_test.go index 1c86746df42..3f8ba3e6b18 100644 --- a/model/span_test.go +++ b/model/span_test.go @@ -106,8 +106,6 @@ var ( } ) -const keySpanKind = "span.kind" - func TestSpanIDMarshalJSON(t *testing.T) { for _, testCase := range testCasesSpanID { expected := fmt.Sprintf(`{"traceId":"AAAAAAAAAAAAAAAAAAAAAA==","spanId":"%s"}`, testCase.b64) @@ -167,7 +165,7 @@ func TestSpanIDUnmarshalJSONErrors(t *testing.T) { func TestIsRPCClientServer(t *testing.T) { span1 := &model.Span{ Tags: model.KeyValues{ - model.String(keySpanKind, trace.SpanKindClient.String()), + model.String(model.SpanKindKey, trace.SpanKindClient.String()), }, } assert.True(t, span1.IsRPCClient()) @@ -206,12 +204,12 @@ func TestIsFirehoseEnabled(t *testing.T) { func TestGetSpanKind(t *testing.T) { span := makeSpan(model.String("sampler.type", "lowerbound")) spanKind, found := span.GetSpanKind() - assert.Equal(t, "unspecified", spanKind.String()) + assert.Equal(t, model.SpanKindUnspecified, spanKind) assert.False(t, found) - span = makeSpan(model.String("span.kind", "client")) + span = makeSpan(model.SpanKindTag("client")) spanKind, found = span.GetSpanKind() - assert.Equal(t, "client", spanKind.String()) + assert.Equal(t, model.SpanKindClient, spanKind) assert.True(t, found) } diff --git a/plugin/storage/cassandra/spanstore/writer.go b/plugin/storage/cassandra/spanstore/writer.go index 24bfd58062b..1dc201ef306 100644 --- a/plugin/storage/cassandra/spanstore/writer.go +++ b/plugin/storage/cassandra/spanstore/writer.go @@ -168,10 +168,10 @@ func (s *SpanWriter) writeSpan(_ *model.Span, ds *dbmodel.Span) error { } func (s *SpanWriter) writeIndexes(span *model.Span, ds *dbmodel.Span) error { - spanKind, _ := span.GetSpanKind() + spanKind, _ := span.GetSpanKind() // if not found it returns "" if err := s.saveServiceNameAndOperationName(dbmodel.Operation{ ServiceName: ds.ServiceName, - SpanKind: spanKind.String(), + SpanKind: string(spanKind), OperationName: ds.OperationName, }); err != nil { // should this be a soft failure? diff --git a/plugin/storage/memory/memory.go b/plugin/storage/memory/memory.go index a20e1a5afc6..23ec0bd2d39 100644 --- a/plugin/storage/memory/memory.go +++ b/plugin/storage/memory/memory.go @@ -147,10 +147,10 @@ func (st *Store) WriteSpan(ctx context.Context, span *model.Span) error { m.operations[span.Process.ServiceName] = map[spanstore.Operation]struct{}{} } - spanKind, _ := span.GetSpanKind() + spanKind, _ := span.GetSpanKind() // if not found it returns Unspecified operation := spanstore.Operation{ Name: span.OperationName, - SpanKind: spanKind.String(), + SpanKind: string(spanKind), } if _, ok := m.operations[span.Process.ServiceName][operation]; !ok { diff --git a/plugin/storage/memory/memory_test.go b/plugin/storage/memory/memory_test.go index af130edd566..480c934b9e1 100644 --- a/plugin/storage/memory/memory_test.go +++ b/plugin/storage/memory/memory_test.go @@ -38,7 +38,7 @@ var childSpan1 = &model.Span{ OperationName: "childOperationName", Tags: model.KeyValues{ model.String("tagKey", "tagValue"), - model.String("span.kind", "server"), + model.SpanKindTag(model.SpanKindServer), }, Logs: []model.Log{ { @@ -63,7 +63,7 @@ var childSpan2 = &model.Span{ OperationName: "childOperationName", Tags: model.KeyValues{ model.String("tagKey", "tagValue"), - model.String("span.kind", "internal"), + model.SpanKindTag(model.SpanKindInternal), }, Logs: []model.Log{ { @@ -504,7 +504,7 @@ func makeTestingSpan(traceID model.TraceID, suffix string) *model.Span { OperationName: "operationName" + suffix, Tags: model.KeyValues{ model.String("tagKey", "tagValue"+suffix), - model.String("span.kind", "client"+suffix), + model.SpanKindTag(model.SpanKindClient), }, Logs: []model.Log{ {