From 5e3a2105b94622c005b0261bbc22793d628ede8c Mon Sep 17 00:00:00 2001 From: Gustavo Silva Paiva Date: Wed, 23 Oct 2019 20:25:14 -0300 Subject: [PATCH] add WithSpanKind option to span creation (#234) * add WithSpanKind option to span creation * change SpanKind to string alias and add support for SpanKind on ot bridge * fix tests * fix import order * fix nits --- api/trace/api.go | 22 ++++++++- experimental/bridge/opentracing/bridge.go | 15 +++--- .../bridge/opentracing/internal/mock.go | 8 +++- experimental/bridge/opentracing/mix_test.go | 17 +++++-- exporter/trace/stackdriver/trace_proto.go | 2 +- exporter/trace/stdout/stdout_test.go | 6 ++- plugin/httptrace/clienttrace.go | 2 +- plugin/othttp/handler.go | 1 + sdk/export/span.go | 2 +- sdk/trace/span.go | 11 +++-- sdk/trace/trace_test.go | 46 +++++++++++++++++++ 11 files changed, 109 insertions(+), 23 deletions(-) diff --git a/api/trace/api.go b/api/trace/api.go index 2e680b3299b..c4a03f70039 100644 --- a/api/trace/api.go +++ b/api/trace/api.go @@ -110,6 +110,7 @@ type SpanOptions struct { StartTime time.Time Relation Relation Record bool + SpanKind SpanKind } // Relation is used to establish relationship between newly created span and the @@ -143,8 +144,20 @@ type Link struct { Attributes []core.KeyValue } +// 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 + +const ( + SpanKindInternal SpanKind = "internal" + SpanKindServer SpanKind = "server" + SpanKindClient SpanKind = "client" + SpanKindProducer SpanKind = "producer" + SpanKindConsumer SpanKind = "consumer" +) + // WithStartTime sets the start time of the span to provided time t, when it is started. -// In absensce of this option, wall clock time is used as start time. +// In absence of this option, wall clock time is used as start time. // This option is typically used when starting of the span is delayed. func WithStartTime(t time.Time) SpanOption { return func(o *SpanOptions) { @@ -188,3 +201,10 @@ func FollowsFrom(sc core.SpanContext) SpanOption { } } } + +// WithSpanKind specifies the role a Span on a Trace. +func WithSpanKind(sk SpanKind) SpanOption { + return func(o *SpanOptions) { + o.SpanKind = sk + } +} diff --git a/experimental/bridge/opentracing/bridge.go b/experimental/bridge/opentracing/bridge.go index a32fa110ced..f9d1d85b52d 100644 --- a/experimental/bridge/opentracing/bridge.go +++ b/experimental/bridge/opentracing/bridge.go @@ -312,14 +312,14 @@ func (t *BridgeTracer) StartSpan(operationName string, opts ...ot.StartSpanOptio } // TODO: handle links, needs SpanData to be in the API first? bRelation, _ := otSpanReferencesToBridgeRelationAndLinks(sso.References) - // TODO: handle span kind, needs SpanData to be in the API first? - attributes, _, hadTrueErrorTag := otTagsToOtelAttributesKindAndError(sso.Tags) + attributes, kind, hadTrueErrorTag := otTagsToOtelAttributesKindAndError(sso.Tags) checkCtx := migration.WithDeferredSetup(context.Background()) checkCtx2, otelSpan := t.setTracer.tracer().Start(checkCtx, operationName, func(opts *oteltrace.SpanOptions) { opts.Attributes = attributes opts.StartTime = sso.StartTime opts.Relation = bRelation.ToOtelRelation() opts.Record = true + opts.SpanKind = kind }) if checkCtx != checkCtx2 { t.warnOnce.Do(func() { @@ -379,17 +379,16 @@ func (t *BridgeTracer) ContextWithSpanHook(ctx context.Context, span ot.Span) co return ctx } -type spanKindTODO struct{} - -func otTagsToOtelAttributesKindAndError(tags map[string]interface{}) ([]otelcore.KeyValue, spanKindTODO, bool) { - kind := spanKindTODO{} +func otTagsToOtelAttributesKindAndError(tags map[string]interface{}) ([]otelcore.KeyValue, oteltrace.SpanKind, bool) { + kind := oteltrace.SpanKindInternal error := false var pairs []otelcore.KeyValue for k, v := range tags { switch k { case string(otext.SpanKind): - // TODO: java has some notion of span kind, it - // probably is related to some proto stuff + if sk, ok := v.(string); ok { + kind = oteltrace.SpanKind(sk) + } case string(otext.Error): if b, ok := v.(bool); ok && b { error = true diff --git a/experimental/bridge/opentracing/internal/mock.go b/experimental/bridge/opentracing/internal/mock.go index 83bcf54a0c1..4d5bb422312 100644 --- a/experimental/bridge/opentracing/internal/mock.go +++ b/experimental/bridge/opentracing/internal/mock.go @@ -27,7 +27,7 @@ import ( otelkey "go.opentelemetry.io/api/key" oteltrace "go.opentelemetry.io/api/trace" - migration "go.opentelemetry.io/experimental/bridge/opentracing/migration" + "go.opentelemetry.io/experimental/bridge/opentracing/migration" ) var ( @@ -97,6 +97,10 @@ func (t *MockTracer) Start(ctx context.Context, name string, opts ...oteltrace.S if startTime.IsZero() { startTime = time.Now() } + spanKind := spanOpts.SpanKind + if spanKind == "" { + spanKind = oteltrace.SpanKindInternal + } spanContext := otelcore.SpanContext{ TraceID: t.getTraceID(ctx, &spanOpts), SpanID: t.getSpanID(), @@ -112,6 +116,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, } if !migration.SkipContextSetup(ctx) { ctx = oteltrace.SetCurrentSpan(ctx, span) @@ -209,6 +214,7 @@ type MockSpan struct { mockTracer *MockTracer officialTracer oteltrace.Tracer spanContext otelcore.SpanContext + SpanKind oteltrace.SpanKind recording bool Attributes oteldctx.Map diff --git a/experimental/bridge/opentracing/mix_test.go b/experimental/bridge/opentracing/mix_test.go index 4c82f2d2268..dda0297b836 100644 --- a/experimental/bridge/opentracing/mix_test.go +++ b/experimental/bridge/opentracing/mix_test.go @@ -478,6 +478,12 @@ func checkTraceAndSpans(t *testing.T, tracer *internal.MockTracer, expectedTrace // the last finished span has no parent parentSpanIDs := append(spanIDs[1:], 0) + sks := map[uint64]oteltrace.SpanKind{ + 3456: oteltrace.SpanKindProducer, + 2345: oteltrace.SpanKindInternal, + 1234: oteltrace.SpanKindClient, + } + if len(tracer.FinishedSpans) != expectedSpanCount { t.Errorf("Expected %d finished spans, got %d", expectedSpanCount, len(tracer.FinishedSpans)) } @@ -494,6 +500,9 @@ func checkTraceAndSpans(t *testing.T, tracer *internal.MockTracer, expectedTrace if span.ParentSpanID != expectedParentSpanID { t.Errorf("Expected finished span %d (span ID: %d) to have parent span ID %d, but got %d", idx, sctx.SpanID, expectedParentSpanID, span.ParentSpanID) } + if span.SpanKind != sks[span.SpanContext().SpanID] { + t.Errorf("Expected finished span %d (span ID: %d) to have span.kind to be '%v' but was '%v'", idx, sctx.SpanID, sks[span.SpanContext().SpanID], span.SpanKind) + } } } @@ -535,7 +544,7 @@ func min(a, b int) int { func runOtelOTOtel(t *testing.T, ctx context.Context, name string, callback func(*testing.T, context.Context)) { tr := oteltrace.GlobalProvider().GetTracer("") - ctx, span := tr.Start(ctx, fmt.Sprintf("%s_Otel_OTOtel", name)) + ctx, span := tr.Start(ctx, fmt.Sprintf("%s_Otel_OTOtel", name), oteltrace.WithSpanKind(oteltrace.SpanKindClient)) defer span.End() callback(t, ctx) func(ctx2 context.Context) { @@ -543,7 +552,7 @@ func runOtelOTOtel(t *testing.T, ctx context.Context, name string, callback func defer span.Finish() callback(t, ctx2) func(ctx3 context.Context) { - ctx3, span := tr.Start(ctx3, fmt.Sprintf("%sOtelOT_Otel_", name)) + ctx3, span := tr.Start(ctx3, fmt.Sprintf("%sOtelOT_Otel_", name), oteltrace.WithSpanKind(oteltrace.SpanKindProducer)) defer span.End() callback(t, ctx3) }(ctx2) @@ -552,7 +561,7 @@ func runOtelOTOtel(t *testing.T, ctx context.Context, name string, callback func func runOTOtelOT(t *testing.T, ctx context.Context, name string, callback func(*testing.T, context.Context)) { tr := oteltrace.GlobalProvider().GetTracer("") - span, ctx := ot.StartSpanFromContext(ctx, fmt.Sprintf("%s_OT_OtelOT", name)) + span, ctx := ot.StartSpanFromContext(ctx, fmt.Sprintf("%s_OT_OtelOT", name), ot.Tag{Key: "span.kind", Value: "client"}) defer span.Finish() callback(t, ctx) func(ctx2 context.Context) { @@ -560,7 +569,7 @@ func runOTOtelOT(t *testing.T, ctx context.Context, name string, callback func(* defer span.End() callback(t, ctx2) func(ctx3 context.Context) { - span, ctx3 := ot.StartSpanFromContext(ctx3, fmt.Sprintf("%sOTOtel_OT_", name)) + span, ctx3 := ot.StartSpanFromContext(ctx3, fmt.Sprintf("%sOTOtel_OT_", name), ot.Tag{Key: "span.kind", Value: "producer"}) defer span.Finish() callback(t, ctx3) }(ctx2) diff --git a/exporter/trace/stackdriver/trace_proto.go b/exporter/trace/stackdriver/trace_proto.go index db97bcc9d87..bb8033ceb85 100644 --- a/exporter/trace/stackdriver/trace_proto.go +++ b/exporter/trace/stackdriver/trace_proto.go @@ -71,7 +71,7 @@ func protoFromSpanData(s *export.SpanData, projectID string) *tracepb.Span { switch s.SpanKind { // TODO(ymotongpoo): add cases for "Send" and "Recv". default: - name = fmt.Sprintf("Span.%d-%s", s.SpanKind, name) + name = fmt.Sprintf("Span.%s-%s", s.SpanKind, name) } sp := &tracepb.Span{ diff --git a/exporter/trace/stdout/stdout_test.go b/exporter/trace/stdout/stdout_test.go index d2416ee432c..101018345fd 100644 --- a/exporter/trace/stdout/stdout_test.go +++ b/exporter/trace/stdout/stdout_test.go @@ -24,6 +24,7 @@ import ( "google.golang.org/grpc/codes" "go.opentelemetry.io/api/core" + "go.opentelemetry.io/api/trace" "go.opentelemetry.io/sdk/export" ) @@ -62,7 +63,8 @@ func TestExporter_ExportSpan(t *testing.T) { Value: core.Value{Type: core.FLOAT64, Float64: doubleValue}, }, }, - Status: codes.Unknown, + SpanKind: trace.SpanKindInternal, + Status: codes.Unknown, } ex.ExportSpan(context.Background(), testSpan) @@ -73,7 +75,7 @@ func TestExporter_ExportSpan(t *testing.T) { `"TraceID":"0102030405060708090a0b0c0d0e0f10",` + `"SpanID":"0102030405060708","TraceFlags":0},` + `"ParentSpanID":0,` + - `"SpanKind":0,` + + `"SpanKind":"internal",` + `"Name":"/foo",` + `"StartTime":` + string(expectedSerializedNow) + "," + `"EndTime":` + string(expectedSerializedNow) + "," + diff --git a/plugin/httptrace/clienttrace.go b/plugin/httptrace/clienttrace.go index b1ee3b0f0af..519f611fc25 100644 --- a/plugin/httptrace/clienttrace.go +++ b/plugin/httptrace/clienttrace.go @@ -60,7 +60,7 @@ func newClientTracer(ctx context.Context) *clientTracer { } func (ct *clientTracer) open(name string, attrs ...core.KeyValue) { - _, sp := ct.tr.Start(ct.Context, name, trace.WithAttributes(attrs...)) + _, sp := ct.tr.Start(ct.Context, name, trace.WithAttributes(attrs...), trace.WithSpanKind(trace.SpanKindClient)) ct.mtx.Lock() defer ct.mtx.Unlock() if ct.root == nil { diff --git a/plugin/othttp/handler.go b/plugin/othttp/handler.go index 137135e9eac..49d957a261e 100644 --- a/plugin/othttp/handler.go +++ b/plugin/othttp/handler.go @@ -131,6 +131,7 @@ func NewHandler(handler http.Handler, operation string, opts ...Option) http.Han defaultOpts := []Option{ WithTracer(trace.GlobalProvider().GetTracer("go.opentelemtry.io/plugin/othttp")), WithPropagator(prop.HTTPTraceContextPropagator{}), + WithSpanOptions(trace.WithSpanKind(trace.SpanKindServer)), } for _, opt := range append(defaultOpts, opts...) { diff --git a/sdk/export/span.go b/sdk/export/span.go index a010e69e0b9..4d2d513d016 100644 --- a/sdk/export/span.go +++ b/sdk/export/span.go @@ -27,7 +27,7 @@ import ( type SpanData struct { SpanContext core.SpanContext ParentSpanID uint64 - SpanKind int + SpanKind apitrace.SpanKind Name string StartTime time.Time // The wall clock time of EndTime will be adjusted to always be offset diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 6d339f7d065..f7a2b88b365 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -330,11 +330,14 @@ func startSpanInternal(tr *tracer, name string, parent core.SpanContext, remoteP if startTime.IsZero() { startTime = time.Now() } + sk := o.SpanKind + if sk == "" { + sk = apitrace.SpanKindInternal + } span.data = &export.SpanData{ - SpanContext: span.spanContext, - StartTime: startTime, - // TODO;[rghetia] : fix spanKind - //SpanKind: o.SpanKind, + SpanContext: span.spanContext, + StartTime: startTime, + SpanKind: sk, Name: name, HasRemoteParent: remoteParent, } diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 56b4fc9848e..e30e6421b56 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -242,6 +242,7 @@ func TestSetSpanAttributes(t *testing.T) { Key: core.Key("key1"), Value: core.Value{Type: core.STRING, String: "value1"}, }}, + SpanKind: "internal", HasRemoteParent: true, } if diff := cmp.Diff(got, want); diff != "" { @@ -281,6 +282,7 @@ func TestSetSpanAttributesOverLimit(t *testing.T) { Value: core.Value{Type: core.STRING, String: "value4"}, }, }, + SpanKind: "internal", HasRemoteParent: true, DroppedAttributeCount: 1, } @@ -326,6 +328,7 @@ func TestEvents(t *testing.T) { {Message: "foo", Attributes: []core.KeyValue{k1v1}}, {Message: "bar", Attributes: []core.KeyValue{k2v2, k3v3}}, }, + SpanKind: "internal", } if diff := cmp.Diff(got, want, cmp.AllowUnexported(export.Event{})); diff != "" { t.Errorf("Message Events: -got +want %s", diff) @@ -376,6 +379,7 @@ func TestEventsOverLimit(t *testing.T) { }, DroppedMessageEventCount: 2, HasRemoteParent: true, + SpanKind: "internal", } if diff := cmp.Diff(got, want, cmp.AllowUnexported(export.Event{})); diff != "" { t.Errorf("Message Event over limit: -got +want %s", diff) @@ -415,6 +419,7 @@ func TestAddLinks(t *testing.T) { {SpanContext: sc1, Attributes: []core.KeyValue{k1v1}}, {SpanContext: sc2, Attributes: []core.KeyValue{k2v2}}, }, + SpanKind: "internal", } if diff := cmp.Diff(got, want, cmp.AllowUnexported(export.Event{})); diff != "" { t.Errorf("AddLink: -got +want %s", diff) @@ -455,6 +460,7 @@ func TestLinks(t *testing.T) { {SpanContext: sc1, Attributes: []core.KeyValue{k1v1}}, {SpanContext: sc2, Attributes: []core.KeyValue{k2v2, k3v3}}, }, + SpanKind: "internal", } if diff := cmp.Diff(got, want, cmp.AllowUnexported(export.Event{})); diff != "" { t.Errorf("Link: -got +want %s", diff) @@ -497,6 +503,7 @@ func TestLinksOverLimit(t *testing.T) { }, DroppedLinkCount: 1, HasRemoteParent: true, + SpanKind: "internal", } if diff := cmp.Diff(got, want, cmp.AllowUnexported(export.Event{})); diff != "" { t.Errorf("Link over limit: -got +want %s", diff) @@ -543,6 +550,7 @@ func TestSetSpanStatus(t *testing.T) { }, ParentSpanID: sid, Name: "SpanStatus/span0", + SpanKind: "internal", Status: codes.Canceled, HasRemoteParent: true, } @@ -801,3 +809,41 @@ func TestCustomStartEndTime(t *testing.T) { t.Errorf("expected end time to be %s, got %s", endTime, got.EndTime) } } + +func TestWithSpanKind(t *testing.T) { + var te testExporter + tp, _ := NewProvider(WithSyncer(&te), WithConfig(Config{DefaultSampler: AlwaysSample()})) + tr := tp.GetTracer("withSpanKind") + + _, span := tr.Start(context.Background(), "WithoutSpanKind") + spanData, err := endSpan(&te, span) + if err != nil { + t.Error(err.Error()) + } + + if spanData.SpanKind != apitrace.SpanKindInternal { + t.Errorf("Default value of Spankind should be Internal: got %+v, want %+v\n", spanData.SpanKind, apitrace.SpanKindInternal) + } + + sks := []apitrace.SpanKind{ + apitrace.SpanKindInternal, + apitrace.SpanKindServer, + apitrace.SpanKindClient, + apitrace.SpanKindProducer, + apitrace.SpanKindConsumer, + } + + for _, sk := range sks { + te.spans = nil + + _, span := tr.Start(context.Background(), fmt.Sprintf("SpanKind-%v", sk), apitrace.WithSpanKind(sk)) + spanData, err := endSpan(&te, span) + if err != nil { + t.Error(err.Error()) + } + + if spanData.SpanKind != sk { + t.Errorf("WithSpanKind check: got %+v, want %+v\n", spanData.SpanKind, sks) + } + } +}