Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/agent/app/reporter/grpc/reporter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
}},
},
Expand Down
12 changes: 6 additions & 6 deletions cmd/anonymizer/app/anonymizer/anonymizer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion cmd/anonymizer/app/uiconv/extractor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion cmd/anonymizer/app/uiconv/module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/query/app/apiv3/gateway_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
},
},
Expand Down
7 changes: 3 additions & 4 deletions model/adjuster/span_hash_deduper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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),
},
},
{
Expand All @@ -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),
},
},
{
Expand Down
6 changes: 2 additions & 4 deletions model/adjuster/zipkin_span_id_uniquify_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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),
},
},
{
Expand All @@ -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),
},
},
{
Expand Down
7 changes: 3 additions & 4 deletions model/converter/thrift/zipkin/to_domain.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
25 changes: 12 additions & 13 deletions model/converter/thrift/zipkin/to_domain_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
},
}

Expand All @@ -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)
}
}

Expand Down
24 changes: 24 additions & 0 deletions model/keyvalue.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ""
Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be "unspecified" - looks like some tests have expectations around that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unspecified is an OTEL value, it did not exist in OpenTracing, so I left it blank.

And in OTLP it's technically undefined which strings should be used because the values are numeric enums.

)

func SpanKindFromString(kind string) (SpanKind, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add unit tests for this function?

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

Expand Down
39 changes: 15 additions & 24 deletions model/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"io"
"strconv"

"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"
)

Expand All @@ -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,
Expand All @@ -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.
Expand All @@ -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
}
Expand All @@ -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.
Expand Down Expand Up @@ -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
}
Expand Down
Loading
Loading