From 817773ad8edc7e9833d910aa2e3dd5a6fcb0f940 Mon Sep 17 00:00:00 2001 From: jmacd Date: Mon, 4 Nov 2019 10:55:39 -0800 Subject: [PATCH 1/3] Change SpanKind to an integer --- api/trace/api.go | 14 ++++++++------ bridge/opentracing/bridge.go | 13 +++++++++++-- bridge/opentracing/internal/mock.go | 9 ++++++++- bridge/opentracing/mix_test.go | 2 -- exporter/trace/stdout/stdout_test.go | 2 +- sdk/trace/span.go | 2 +- sdk/trace/trace_test.go | 18 ++++++++---------- 7 files changed, 37 insertions(+), 23 deletions(-) diff --git a/api/trace/api.go b/api/trace/api.go index b364af44a24..436c97cc103 100644 --- a/api/trace/api.go +++ b/api/trace/api.go @@ -142,14 +142,16 @@ type Link struct { // SpanKind represents the role of a Span inside a Trace. Often, this defines how a Span // will be processed and visualized by various backends. -type SpanKind string +type SpanKind int const ( - SpanKindInternal SpanKind = "internal" - SpanKindServer SpanKind = "server" - SpanKindClient SpanKind = "client" - SpanKindProducer SpanKind = "producer" - SpanKindConsumer SpanKind = "consumer" + // These match the proto definition, see opentelemetry/proto/trace/v1/trace.proto + SpanKindUnspecified SpanKind = 0 + SpanKindInternal SpanKind = 1 + SpanKindServer SpanKind = 2 + SpanKindClient SpanKind = 3 + SpanKindProducer SpanKind = 4 + SpanKindConsumer SpanKind = 5 ) // WithStartTime sets the start time of the span to provided time t, when it is started. diff --git a/bridge/opentracing/bridge.go b/bridge/opentracing/bridge.go index d76745789cd..034907bfd89 100644 --- a/bridge/opentracing/bridge.go +++ b/bridge/opentracing/bridge.go @@ -385,8 +385,17 @@ func otTagsToOtelAttributesKindAndError(tags map[string]interface{}) ([]otelcore for k, v := range tags { switch k { case string(otext.SpanKind): - if sk, ok := v.(string); ok { - kind = oteltrace.SpanKind(sk) + if s, ok := v.(string); ok { + switch strings.ToLower(s) { + case "client": + kind = oteltrace.SpanKindClient + case "server": + kind = oteltrace.SpanKindServer + case "producer": + kind = oteltrace.SpanKindProducer + case "consumer": + kind = oteltrace.SpanKindConsumer + } } case string(otext.Error): if b, ok := v.(bool); ok && b { diff --git a/bridge/opentracing/internal/mock.go b/bridge/opentracing/internal/mock.go index 20fd44d323e..4e3cf367e12 100644 --- a/bridge/opentracing/internal/mock.go +++ b/bridge/opentracing/internal/mock.go @@ -85,7 +85,14 @@ func (t *MockTracer) Start(ctx context.Context, name string, opts ...oteltrace.S startTime = time.Now() } spanKind := spanOpts.SpanKind - if spanKind == "" { + switch spanKind { + case oteltrace.SpanKindInternal, + oteltrace.SpanKindServer, + oteltrace.SpanKindClient, + oteltrace.SpanKindProducer, + oteltrace.SpanKindConsumer: + // valid + default: spanKind = oteltrace.SpanKindInternal } spanContext := otelcore.SpanContext{ diff --git a/bridge/opentracing/mix_test.go b/bridge/opentracing/mix_test.go index f623de2a555..70001099821 100644 --- a/bridge/opentracing/mix_test.go +++ b/bridge/opentracing/mix_test.go @@ -20,8 +20,6 @@ import ( "testing" ot "github.com/opentracing/opentracing-go" - //otext "github.com/opentracing/opentracing-go/ext" - //otlog "github.com/opentracing/opentracing-go/log" otelcore "go.opentelemetry.io/otel/api/core" oteltrace "go.opentelemetry.io/otel/api/trace" diff --git a/exporter/trace/stdout/stdout_test.go b/exporter/trace/stdout/stdout_test.go index 24252694cae..8c2938fb2f1 100644 --- a/exporter/trace/stdout/stdout_test.go +++ b/exporter/trace/stdout/stdout_test.go @@ -70,7 +70,7 @@ func TestExporter_ExportSpan(t *testing.T) { `"TraceID":"0102030405060708090a0b0c0d0e0f10",` + `"SpanID":"0102030405060708","TraceFlags":0},` + `"ParentSpanID":"0000000000000000",` + - `"SpanKind":"internal",` + + `"SpanKind":1,` + `"Name":"/foo",` + `"StartTime":` + string(expectedSerializedNow) + "," + `"EndTime":` + string(expectedSerializedNow) + "," + diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 6ca5d5b601a..1e592e940c8 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -322,7 +322,7 @@ func startSpanInternal(tr *tracer, name string, parent core.SpanContext, remoteP startTime = time.Now() } sk := o.SpanKind - if sk == "" { + if sk == apitrace.SpanKindUnspecified { sk = apitrace.SpanKindInternal } span.data = &export.SpanData{ diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 916314dc875..e28a2a111dd 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -285,8 +285,6 @@ func TestStartSpanWithFollowsFrom(t *testing.T) { } } -// TODO: [rghetia] Equivalent of SpanKind Test. - func TestSetSpanAttributes(t *testing.T) { te := &testExporter{} tp, _ := NewProvider(WithSyncer(te)) @@ -307,7 +305,7 @@ func TestSetSpanAttributes(t *testing.T) { Attributes: []core.KeyValue{ key.String("key1", "value1"), }, - SpanKind: "internal", + SpanKind: apitrace.SpanKindInternal, HasRemoteParent: true, } if diff := cmpDiff(got, want); diff != "" { @@ -341,7 +339,7 @@ func TestSetSpanAttributesOverLimit(t *testing.T) { key.Bool("key1", false), key.Int64("key4", 4), }, - SpanKind: "internal", + SpanKind: apitrace.SpanKindInternal, HasRemoteParent: true, DroppedAttributeCount: 1, } @@ -387,7 +385,7 @@ func TestEvents(t *testing.T) { {Message: "foo", Attributes: []core.KeyValue{k1v1}}, {Message: "bar", Attributes: []core.KeyValue{k2v2, k3v3}}, }, - SpanKind: "internal", + SpanKind: apitrace.SpanKindInternal, } if diff := cmpDiff(got, want); diff != "" { t.Errorf("Message Events: -got +want %s", diff) @@ -438,7 +436,7 @@ func TestEventsOverLimit(t *testing.T) { }, DroppedMessageEventCount: 2, HasRemoteParent: true, - SpanKind: "internal", + SpanKind: apitrace.SpanKindInternal, } if diff := cmpDiff(got, want); diff != "" { t.Errorf("Message Event over limit: -got +want %s", diff) @@ -478,7 +476,7 @@ func TestAddLinks(t *testing.T) { {SpanContext: sc1, Attributes: []core.KeyValue{k1v1}}, {SpanContext: sc2, Attributes: []core.KeyValue{k2v2}}, }, - SpanKind: "internal", + SpanKind: apitrace.SpanKindInternal, } if diff := cmpDiff(got, want); diff != "" { t.Errorf("AddLink: -got +want %s", diff) @@ -519,7 +517,7 @@ func TestLinks(t *testing.T) { {SpanContext: sc1, Attributes: []core.KeyValue{k1v1}}, {SpanContext: sc2, Attributes: []core.KeyValue{k2v2, k3v3}}, }, - SpanKind: "internal", + SpanKind: apitrace.SpanKindInternal, } if diff := cmpDiff(got, want); diff != "" { t.Errorf("Link: -got +want %s", diff) @@ -562,7 +560,7 @@ func TestLinksOverLimit(t *testing.T) { }, DroppedLinkCount: 1, HasRemoteParent: true, - SpanKind: "internal", + SpanKind: apitrace.SpanKindInternal, } if diff := cmpDiff(got, want); diff != "" { t.Errorf("Link over limit: -got +want %s", diff) @@ -609,7 +607,7 @@ func TestSetSpanStatus(t *testing.T) { }, ParentSpanID: sid, Name: "SpanStatus/span0", - SpanKind: "internal", + SpanKind: apitrace.SpanKindInternal, Status: codes.Canceled, HasRemoteParent: true, } From 8602ddcc15f4fd949c3d0c97bf370aa8a2d802a6 Mon Sep 17 00:00:00 2001 From: jmacd Date: Mon, 4 Nov 2019 14:35:38 -0800 Subject: [PATCH 2/3] Take suggestion --- api/trace/api.go | 23 ++++++++++++++++++++++- sdk/trace/span.go | 6 +----- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/api/trace/api.go b/api/trace/api.go index 408eb63bbc3..1172587aba0 100644 --- a/api/trace/api.go +++ b/api/trace/api.go @@ -145,7 +145,12 @@ type Link struct { type SpanKind int const ( - // These match the proto definition, see opentelemetry/proto/trace/v1/trace.proto + // As a convenience, these match the proto definition, see + // opentelemetry/proto/trace/v1/trace.proto + // + // The unspecified value is not a valid `SpanKind`. Use + // `ValidateSpanKind()` to coerce a span kind to a valid + // value. SpanKindUnspecified SpanKind = 0 SpanKindInternal SpanKind = 1 SpanKindServer SpanKind = 2 @@ -154,6 +159,22 @@ const ( SpanKindConsumer SpanKind = 5 ) +// ValidateSpanKind returns a valid span kind value. This will coerce +// invalid values into the default value, SpanKindInternal. +func ValidateSpanKind(spanKind SpanKind) SpanKind { + switch spanKind { + case SpanKindInternal, + SpanKindServer, + SpanKindClient, + SpanKindProducer, + SpanKindConsumer: + // valid + return spanKind + default: + return SpanKindInternal + } +} + // String returns the specified name of the SpanKind in lower-case. func (sk SpanKind) String() string { switch sk { diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 1e592e940c8..9607804f703 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -321,14 +321,10 @@ func startSpanInternal(tr *tracer, name string, parent core.SpanContext, remoteP if startTime.IsZero() { startTime = time.Now() } - sk := o.SpanKind - if sk == apitrace.SpanKindUnspecified { - sk = apitrace.SpanKindInternal - } span.data = &export.SpanData{ SpanContext: span.spanContext, StartTime: startTime, - SpanKind: sk, + SpanKind: apitrace.ValidateSpanKind(o.SpanKind), Name: name, HasRemoteParent: remoteParent, } From 14d35fde7b647c8f566ac45e570b4627ab8b51f4 Mon Sep 17 00:00:00 2001 From: jmacd Date: Mon, 4 Nov 2019 16:01:13 -0800 Subject: [PATCH 3/3] Use the helper from the bridge --- bridge/opentracing/internal/mock.go | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/bridge/opentracing/internal/mock.go b/bridge/opentracing/internal/mock.go index 4e3cf367e12..19dde5f5d87 100644 --- a/bridge/opentracing/internal/mock.go +++ b/bridge/opentracing/internal/mock.go @@ -84,17 +84,6 @@ func (t *MockTracer) Start(ctx context.Context, name string, opts ...oteltrace.S if startTime.IsZero() { startTime = time.Now() } - spanKind := spanOpts.SpanKind - switch spanKind { - case oteltrace.SpanKindInternal, - oteltrace.SpanKindServer, - oteltrace.SpanKindClient, - oteltrace.SpanKindProducer, - oteltrace.SpanKindConsumer: - // valid - default: - spanKind = oteltrace.SpanKindInternal - } spanContext := otelcore.SpanContext{ TraceID: t.getTraceID(ctx, &spanOpts), SpanID: t.getSpanID(), @@ -112,7 +101,7 @@ func (t *MockTracer) Start(ctx context.Context, name string, opts ...oteltrace.S EndTime: time.Time{}, ParentSpanID: t.getParentSpanID(ctx, &spanOpts), Events: nil, - SpanKind: spanKind, + SpanKind: oteltrace.ValidateSpanKind(spanOpts.SpanKind), } if !migration.SkipContextSetup(ctx) { ctx = oteltrace.SetCurrentSpan(ctx, span)