From a99f87275966a21a7d7376aa0c1945d163580bd1 Mon Sep 17 00:00:00 2001 From: Vineeth Reddy Date: Tue, 26 Nov 2019 13:03:07 -0800 Subject: [PATCH] Span interface should only allow LinkedTo at creation. (#349) * Remove AddLink & Link from Span Interface I have remove AddLink and Link from the interface and all it refereneces and replaced AddLink with addlink, Also Removed respective unit tests Signed-off-by: vineeth * removing the unused code from unit tests Signed-off-by: VineethReddy02 --- api/testharness/harness.go | 6 --- api/trace/api.go | 7 ---- api/trace/current_test.go | 8 ---- api/trace/noop_span.go | 10 +---- bridge/opentracing/internal/mock.go | 8 ---- internal/trace/mock_span.go | 8 ---- sdk/trace/span.go | 25 +----------- sdk/trace/trace_test.go | 63 ++++++----------------------- sdk/trace/tracer.go | 2 +- 9 files changed, 16 insertions(+), 121 deletions(-) diff --git a/api/testharness/harness.go b/api/testharness/harness.go index cb928b877b1..1edc3ac0ca8 100644 --- a/api/testharness/harness.go +++ b/api/testharness/harness.go @@ -304,12 +304,6 @@ func (h *Harness) testSpan(tracerFactory func() trace.Tracer) { "#AddEventWithTimestamp": func(span trace.Span) { span.AddEventWithTimestamp(context.Background(), time.Now(), "test event") }, - "#AddLink": func(span trace.Span) { - span.AddLink(trace.Link{}) - }, - "#Link": func(span trace.Span) { - span.Link(core.SpanContext{}) - }, "#SetStatus": func(span trace.Span) { span.SetStatus(codes.Internal) }, diff --git a/api/trace/api.go b/api/trace/api.go index 8fe74a1e64b..2f2ab8f962b 100644 --- a/api/trace/api.go +++ b/api/trace/api.go @@ -72,13 +72,6 @@ type Span interface { // IsRecording returns true if the span is active and recording events is enabled. IsRecording() bool - // AddLink adds a link to the span. - AddLink(link Link) - - // Link creates a link between this span and the other span specified by the SpanContext. - // It then adds the newly created Link to the span. - Link(sc core.SpanContext, attrs ...core.KeyValue) - // SpanContext returns span context of the span. Returned SpanContext is usable // even after the span ends. SpanContext() core.SpanContext diff --git a/api/trace/current_test.go b/api/trace/current_test.go index d4dfd925ad6..9e8bcdddeac 100644 --- a/api/trace/current_test.go +++ b/api/trace/current_test.go @@ -103,11 +103,3 @@ func (mockSpan) AddEvent(ctx context.Context, msg string, attrs ...core.KeyValue // AddEventWithTimestamp does nothing. func (mockSpan) AddEventWithTimestamp(ctx context.Context, timestamp time.Time, msg string, attrs ...core.KeyValue) { } - -// AddLink does nothing. -func (mockSpan) AddLink(link trace.Link) { -} - -// Link does nothing. -func (mockSpan) Link(sc core.SpanContext, attrs ...core.KeyValue) { -} diff --git a/api/trace/noop_span.go b/api/trace/noop_span.go index cfc4bc2002a..1e518ef785c 100644 --- a/api/trace/noop_span.go +++ b/api/trace/noop_span.go @@ -73,12 +73,4 @@ func (NoopSpan) AddEventWithTimestamp(ctx context.Context, timestamp time.Time, // SetName does nothing. func (NoopSpan) SetName(name string) { -} - -// AddLink does nothing. -func (NoopSpan) AddLink(link Link) { -} - -// Link does nothing. -func (NoopSpan) Link(sc core.SpanContext, attrs ...core.KeyValue) { -} +} \ No newline at end of file diff --git a/bridge/opentracing/internal/mock.go b/bridge/opentracing/internal/mock.go index 19dde5f5d87..51fedfbb6dd 100644 --- a/bridge/opentracing/internal/mock.go +++ b/bridge/opentracing/internal/mock.go @@ -289,14 +289,6 @@ func (s *MockSpan) AddEventWithTimestamp(ctx context.Context, timestamp time.Tim }) } -func (s *MockSpan) AddLink(link oteltrace.Link) { - // TODO -} - -func (s *MockSpan) Link(sc otelcore.SpanContext, attrs ...otelcore.KeyValue) { - // TODO -} - func (s *MockSpan) OverrideTracer(tracer oteltrace.Tracer) { s.officialTracer = tracer } diff --git a/internal/trace/mock_span.go b/internal/trace/mock_span.go index 12e356f3a56..7cc63da0ac4 100644 --- a/internal/trace/mock_span.go +++ b/internal/trace/mock_span.go @@ -82,11 +82,3 @@ func (ms *MockSpan) AddEvent(ctx context.Context, msg string, attrs ...core.KeyV // AddEvent does nothing. func (ms *MockSpan) AddEventWithTimestamp(ctx context.Context, timestamp time.Time, msg string, attrs ...core.KeyValue) { } - -// AddLink does nothing. -func (ms *MockSpan) AddLink(link apitrace.Link) { -} - -// Link does nothing. -func (ms *MockSpan) Link(sc core.SpanContext, attrs ...core.KeyValue) { -} diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 53ebebcb42b..906126366f6 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -189,33 +189,10 @@ func (s *span) SetName(name string) { makeSamplingDecision(data) } -// AddLink implements Span interface. Specified link is added to the span. -// If the total number of links associated with the span exceeds the limit -// then the oldest link is removed to create space for the link being added. -func (s *span) AddLink(link apitrace.Link) { - if !s.IsRecording() { - return - } - s.addLink(link) -} - -// Link implements Span interface. It is similar to AddLink but it excepts -// SpanContext and attributes as arguments instead of Link. It first creates -// a Link object and then adds to the span. -func (s *span) Link(sc core.SpanContext, attrs ...core.KeyValue) { +func (s *span) addLink(link apitrace.Link) { if !s.IsRecording() { return } - attrsCopy := attrs - if attrs != nil { - attrsCopy = make([]core.KeyValue, len(attrs)) - copy(attrsCopy, attrs) - } - link := apitrace.Link{SpanContext: sc, Attributes: attrsCopy} - s.addLink(link) -} - -func (s *span) addLink(link apitrace.Link) { s.mu.Lock() defer s.mu.Unlock() s.links.add(link) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 094360bad6e..8b3f6814038 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -482,51 +482,10 @@ func TestEventsOverLimit(t *testing.T) { } } -func TestAddLinks(t *testing.T) { - te := &testExporter{} - tp, _ := NewProvider(WithSyncer(te)) - - span := startSpan(tp, "AddLinks") - k1v1 := key.New("key1").String("value1") - k2v2 := key.New("key2").String("value2") - - sc1 := core.SpanContext{TraceID: core.TraceID([16]byte{1, 1}), SpanID: core.SpanID{3}} - sc2 := core.SpanContext{TraceID: core.TraceID([16]byte{1, 1}), SpanID: core.SpanID{3}} - - link1 := apitrace.Link{SpanContext: sc1, Attributes: []core.KeyValue{k1v1}} - link2 := apitrace.Link{SpanContext: sc2, Attributes: []core.KeyValue{k2v2}} - span.AddLink(link1) - span.AddLink(link2) - - got, err := endSpan(te, span) - if err != nil { - t.Fatal(err) - } - - want := &export.SpanData{ - SpanContext: core.SpanContext{ - TraceID: tid, - TraceFlags: 0x1, - }, - ParentSpanID: sid, - Name: "AddLinks/span0", - HasRemoteParent: true, - Links: []apitrace.Link{ - {SpanContext: sc1, Attributes: []core.KeyValue{k1v1}}, - {SpanContext: sc2, Attributes: []core.KeyValue{k2v2}}, - }, - SpanKind: apitrace.SpanKindInternal, - } - if diff := cmpDiff(got, want); diff != "" { - t.Errorf("AddLink: -got +want %s", diff) - } -} - func TestLinks(t *testing.T) { te := &testExporter{} tp, _ := NewProvider(WithSyncer(te)) - span := startSpan(tp, "Links") k1v1 := key.New("key1").String("value1") k2v2 := key.New("key2").String("value2") k3v3 := key.New("key3").String("value3") @@ -534,11 +493,14 @@ func TestLinks(t *testing.T) { sc1 := core.SpanContext{TraceID: core.TraceID([16]byte{1, 1}), SpanID: core.SpanID{3}} sc2 := core.SpanContext{TraceID: core.TraceID([16]byte{1, 1}), SpanID: core.SpanID{3}} - span.Link(sc1, key.New("key1").String("value1")) - span.Link(sc2, - key.New("key2").String("value2"), - key.New("key3").String("value3"), + span := startSpan(tp, "Links", + apitrace.LinkedTo(sc1, key.New("key1").String("value1")), + apitrace.LinkedTo(sc2, + key.New("key2").String("value2"), + key.New("key3").String("value3"), + ), ) + got, err := endSpan(te, span) if err != nil { t.Fatal(err) @@ -572,15 +534,16 @@ func TestLinksOverLimit(t *testing.T) { sc3 := core.SpanContext{TraceID: core.TraceID([16]byte{1, 1}), SpanID: core.SpanID{3}} tp, _ := NewProvider(WithConfig(cfg), WithSyncer(te)) - span := startSpan(tp, "LinksOverLimit") + + span := startSpan(tp, "LinksOverLimit", + apitrace.LinkedTo(sc1, key.New("key1").String("value1")), + apitrace.LinkedTo(sc2, key.New("key2").String("value2")), + apitrace.LinkedTo(sc3, key.New("key3").String("value3")), + ) k2v2 := key.New("key2").String("value2") k3v3 := key.New("key3").String("value3") - span.Link(sc1, key.New("key1").String("value1")) - span.Link(sc2, key.New("key2").String("value2")) - span.Link(sc3, key.New("key3").String("value3")) - got, err := endSpan(te, span) if err != nil { t.Fatal(err) diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index c367956000e..4e783958c62 100644 --- a/sdk/trace/tracer.go +++ b/sdk/trace/tracer.go @@ -59,7 +59,7 @@ func (tr *tracer) Start(ctx context.Context, name string, o ...apitrace.SpanOpti spanName := tr.spanNameWithPrefix(name) span := startSpanInternal(tr, spanName, parent, remoteParent, opts) for _, l := range opts.Links { - span.AddLink(l) + span.addLink(l) } span.SetAttributes(opts.Attributes...)