Merge branch 'feature/spann-allocation-revert-tests'

jaeger_thrift_span_test.go

@@ -52,17 +52,12 @@ func TestBuildJaegerThrift(t *testing.T) {
 	ext.PeerService.Set(sp1, "svc")
 	sp2 := tracer.StartSpan("sp2", opentracing.ChildOf(sp1.Context())).(*Span)
 	ext.SpanKindRPCClient.Set(sp2)
-
-	// NOTE: method Finish releas the memory or return span into the pool
-	sp2.Retain().Finish()
-	sp1.Retain().Finish()
+	sp2.Finish()
+	sp1.Finish()
 
 	jaegerSpan1 := BuildJaegerThrift(sp1)
 	jaegerSpan2 := BuildJaegerThrift(sp2)
 
-	sp2.Release()
-	sp1.Release()
-
 	assert.Equal(t, "sp1", jaegerSpan1.OperationName)
 	assert.Equal(t, "sp2", jaegerSpan2.OperationName)
 	assert.EqualValues(t, 0, jaegerSpan1.ParentSpanId)
@@ -92,8 +87,7 @@ func TestBuildJaegerProcessThrift(t *testing.T) {
 	defer closer.Close()
 
 	sp := tracer.StartSpan("sp1").(*Span)
-	sp.Retain().Finish() // Need to release the memory after the converting into the Thrift object
-	defer sp.Release()
+	sp.Finish() // Need to release the memory after the converting into the Thrift object
 
 	process := BuildJaegerProcessThrift(sp)
 	assert.Equal(t, process.ServiceName, "DOOP")
@@ -247,16 +241,6 @@ func TestBuildLogs(t *testing.T) {
 	for i, test := range tests {
 		testName := fmt.Sprintf("test-%02d", i)
 		sp := tracer.StartSpan(testName, opentracing.ChildOf(root.Context())).(*Span)
-
-		// Note: some of the tests run span Finish function which releases the allocated
-		// span object So we have to retain the object do not destroy it in memory prematurely
-		sp.Retain(1)
-
-		// Before release reduce the counter
-		defer func() {
-			sp.Retain(-1).Release()
-		}()
-
 		if test.disableSampling {
 			ext.SamplingPriority.Set(sp, 0)
 		}
@@ -335,8 +319,7 @@ func TestJaegerSpanBaggageLogs(t *testing.T) {
 	sp := tracer.StartSpan("s1").(*Span)
 	sp.SetBaggageItem("auth.token", "token")
 	ext.SpanKindRPCServer.Set(sp)
-	sp.Retain().Finish()
-	defer sp.Release()
+	sp.Finish()
 
 	jaegerSpan := BuildJaegerThrift(sp)
 	require.Len(t, jaegerSpan.Logs, 1)
@@ -362,14 +345,13 @@ func TestJaegerMaxTagValueLength(t *testing.T) {
 		t.Run(strconv.Itoa(test.tagValueLength), func(t *testing.T) {
 			tracer, closer := NewTracer("DOOP",
 				NewConstSampler(true),
-				NewNullReporter(),
+				NewInMemoryReporter(),
 				TracerOptions.MaxTagValueLength(test.tagValueLength))
 			defer closer.Close()
 			sp := tracer.StartSpan("s1").(*Span)
 			sp.SetTag("tag.string", string(test.value))
 			sp.SetTag("tag.bytes", test.value)
-			sp.Retain().Finish()
-			defer sp.Release()
+			sp.Finish()
 			thriftSpan := BuildJaegerThrift(sp)
 			stringTag := findTag(thriftSpan, "tag.string")
 			assert.Equal(t, j.TagType_STRING, stringTag.VType)

span.go

@@ -288,12 +288,12 @@ func (s *Span) serviceName() string {
 
 // setSamplingPriority returns true if the flag was updated successfully, false otherwise.
 func setSamplingPriority(s *Span, value interface{}) bool {
-	s.Lock()
-	defer s.Unlock()
 	val, ok := value.(uint16)
 	if !ok {
 		return false
 	}
+	s.Lock()
+	defer s.Unlock()
 	if val == 0 {
 		s.context.flags = s.context.flags & (^flagSampled)
 		return true

span_allocator.go

@@ -55,7 +55,8 @@ func (pool spanSimpleAllocator) Get() *Span {
 }
 
 func (pool spanSimpleAllocator) Put(span *Span) {
-	span.reset()
+	// https://github.com/jaegertracing/jaeger-client-go/pull/381#issuecomment-475904351
+	// span.reset()
 }
 
 func (pool spanSimpleAllocator) IsPool() bool {

span_test.go

@@ -16,9 +16,10 @@ package jaeger
 
 import (
 	"sync"
+	"sync/atomic"
 	"testing"
 
-	"github.com/opentracing/opentracing-go"
+	opentracing "github.com/opentracing/opentracing-go"
 	"github.com/opentracing/opentracing-go/ext"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -88,7 +89,7 @@ func TestSpanOperationName(t *testing.T) {
 
 	sp1 := tracer.StartSpan("s1").(*Span)
 	sp1.SetOperationName("s2")
-	defer sp1.Finish()
+	sp1.Finish()
 
 	assert.Equal(t, "s2", sp1.OperationName())
 }
@@ -146,3 +147,22 @@ func TestBaggageContextRace(t *testing.T) {
 	startWg.Done()
 	endWg.Wait()
 }
+
+func TestSpanLifecycle(t *testing.T) {
+	service := "DOOP"
+	tracer, closer := NewTracer(service, NewConstSampler(true), NewNullReporter(), TracerOptions.PoolSpans(true))
+	// After closing all contexts will be released
+	defer closer.Close()
+
+	sp1 := tracer.StartSpan("s1").(*Span)
+	assert.True(t, sp1.tracer != nil, "invalid span initalisation")
+
+	sp1.Retain() // After this we are responsible for the span releasing
+	assert.Equal(t, int32(1), atomic.LoadInt32(&sp1.retainCounter))
+
+	sp1.Finish() // The span is still alive
+	assert.True(t, sp1.tracer != nil, "invalid span finishing")
+
+	sp1.Release() // Now we will kill the object and return it in the pool
+	assert.True(t, sp1.tracer == nil, "span must be released")
+}

tracer_test.go

@@ -45,7 +45,7 @@ func (s *tracerSuite) SetupTest() {
 
 	s.tracer, s.closer = NewTracer("DOOP", // respect the classics, man!
 		NewConstSampler(true),
-		NewNullReporter(),
+		NewInMemoryReporter(),
 		TracerOptions.Metrics(metrics),
 		TracerOptions.ZipkinSharedRPCSpan(true),
 		TracerOptions.BaggageRestrictionManager(baggage.NewDefaultRestrictionManager(0)),
@@ -126,56 +126,50 @@ func (c nonJaegerSpanContext) ForeachBaggageItem(handler func(k, v string) bool)
 
 func (s *tracerSuite) TestStartSpanWithMultipleReferences() {
 	s.metricsFactory.Clear()
-	sp1 := s.tracer.StartSpan("A").(*Span)
-	sp2 := s.tracer.StartSpan("B").(*Span)
-	sp3 := s.tracer.StartSpan("C").(*Span)
+	sp1 := s.tracer.StartSpan("A")
+	sp2 := s.tracer.StartSpan("B")
+	sp3 := s.tracer.StartSpan("C")
 	sp4 := s.tracer.StartSpan(
 		"D",
 		opentracing.ChildOf(sp1.Context()),
 		opentracing.ChildOf(sp2.Context()),
 		opentracing.FollowsFrom(sp3.Context()),
 		opentracing.FollowsFrom(nonJaegerSpanContext{}),
 		opentracing.FollowsFrom(SpanContext{}), // Empty span context should be excluded
-	).(*Span)
+	)
 	// Should use the first ChildOf ref span as the parent
-	s.Equal(sp1.context.spanID, sp4.context.parentID)
-	sp4.Retain().Finish()
-	s.NotNil(sp4.duration)
-	sp3.Retain().Finish()
-	sp2.Retain().Finish()
-	sp1.Retain().Finish()
+	s.Equal(sp1.(*Span).context.spanID, sp4.(*Span).context.parentID)
+	sp4.Finish()
+	s.NotNil(sp4.(*Span).duration)
+	sp3.Finish()
+	sp2.Finish()
+	sp1.Finish()
 	s.metricsFactory.AssertCounterMetrics(s.T(), []metricstest.ExpectedMetric{
 		{Name: "jaeger.tracer.started_spans", Tags: map[string]string{"sampled": "y"}, Value: 4},
 		{Name: "jaeger.tracer.traces", Tags: map[string]string{"sampled": "y", "state": "started"}, Value: 3},
 		{Name: "jaeger.tracer.finished_spans", Value: 4},
 	}...)
-	assert.Len(s.T(), sp4.references, 3)
-	sp4.Release()
-	sp3.Release()
-	sp2.Release()
-	sp1.Release()
+	assert.Len(s.T(), sp4.(*Span).references, 3)
 }
 
 func (s *tracerSuite) TestStartSpanWithOnlyFollowFromReference() {
 	s.metricsFactory.Clear()
-	sp1 := s.tracer.StartSpan("A").(*Span)
+	sp1 := s.tracer.StartSpan("A")
 	sp2 := s.tracer.StartSpan(
 		"B",
 		opentracing.FollowsFrom(sp1.Context()),
-	).(*Span)
+	)
 	// Should use the first ChildOf ref span as the parent
-	s.Equal(sp1.context.spanID, sp2.context.parentID)
-	sp2.Retain().Finish()
-	s.NotNil(sp2.duration)
-	sp1.Retain().Finish()
+	s.Equal(sp1.(*Span).context.spanID, sp2.(*Span).context.parentID)
+	sp2.Finish()
+	s.NotNil(sp2.(*Span).duration)
+	sp1.Finish()
 	s.metricsFactory.AssertCounterMetrics(s.T(), []metricstest.ExpectedMetric{
 		{Name: "jaeger.tracer.started_spans", Tags: map[string]string{"sampled": "y"}, Value: 2},
 		{Name: "jaeger.tracer.traces", Tags: map[string]string{"sampled": "y", "state": "started"}, Value: 1},
 		{Name: "jaeger.tracer.finished_spans", Value: 2},
 	}...)
-	assert.Len(s.T(), sp2.references, 1)
-	sp2.Release()
-	sp1.Release()
+	assert.Len(s.T(), sp2.(*Span).references, 1)
 }
 
 func (s *tracerSuite) TestTraceStartedOrJoinedMetrics() {
@@ -189,19 +183,16 @@ func (s *tracerSuite) TestTraceStartedOrJoinedMetrics() {
 	for _, test := range tests {
 		s.metricsFactory.Clear()
 		s.tracer.(*Tracer).sampler = NewConstSampler(test.sampled)
-		sp1 := s.tracer.StartSpan("parent", ext.RPCServerOption(nil)).(*Span)
-		sp2 := s.tracer.StartSpan("child1", opentracing.ChildOf(sp1.Context())).(*Span)
-		sp3 := s.tracer.StartSpan("child2", ext.RPCServerOption(sp2.Context())).(*Span)
-		s.Equal(sp2.context.spanID, sp3.context.spanID)
-		s.Equal(sp2.context.parentID, sp3.context.parentID)
-		sp3.Retain().Finish()
-		sp2.Retain().Finish()
-		sp1.Retain().Finish()
+		sp1 := s.tracer.StartSpan("parent", ext.RPCServerOption(nil))
+		sp2 := s.tracer.StartSpan("child1", opentracing.ChildOf(sp1.Context()))
+		sp3 := s.tracer.StartSpan("child2", ext.RPCServerOption(sp2.Context()))
+		s.Equal(sp2.(*Span).context.spanID, sp3.(*Span).context.spanID)
+		s.Equal(sp2.(*Span).context.parentID, sp3.(*Span).context.parentID)
+		sp3.Finish()
+		sp2.Finish()
+		sp1.Finish()
 		s.Equal(test.sampled, sp1.Context().(SpanContext).IsSampled())
 		s.Equal(test.sampled, sp2.Context().(SpanContext).IsSampled())
-		sp3.Release()
-		sp2.Release()
-		sp1.Release()
 
 		s.metricsFactory.AssertCounterMetrics(s.T(), []metricstest.ExpectedMetric{
 			{Name: "jaeger.tracer.started_spans", Tags: map[string]string{"sampled": test.label}, Value: 3},
@@ -302,25 +293,23 @@ func TestInjectorExtractorOptions(t *testing.T) {
 }
 
 func TestEmptySpanContextAsParent(t *testing.T) {
-	tracer, tc := NewTracer("x", NewConstSampler(true), NewNullReporter())
+	tracer, tc := NewTracer("x", NewConstSampler(true), NewInMemoryReporter())
 	defer tc.Close()
 
 	span := tracer.StartSpan("test", opentracing.ChildOf(emptyContext)).(*Span)
-	span.Retain().Finish()
+	span.Finish()
 	ctx := span.Context().(SpanContext)
-	span.Release()
 	assert.True(t, ctx.traceID.IsValid())
 	assert.True(t, ctx.IsValid())
 }
 
 func TestGen128Bit(t *testing.T) {
-	tracer, tc := NewTracer("x", NewConstSampler(true), NewNullReporter(), TracerOptions.Gen128Bit(true))
+	tracer, tc := NewTracer("x", NewConstSampler(true), NewInMemoryReporter(), TracerOptions.Gen128Bit(true))
 	defer tc.Close()
 
 	span := tracer.StartSpan("test", opentracing.ChildOf(emptyContext)).(*Span)
-	span.Retain().Finish()
+	span.Finish()
 	traceID := span.Context().(SpanContext).TraceID()
-	span.Release()
 	assert.True(t, traceID.High != 0)
 	assert.True(t, traceID.Low != 0)
 }

transport/http_test.go

@@ -46,8 +46,7 @@ func TestHTTPTransport(t *testing.T) {
 	defer closer.Close()
 
 	span := tracer.StartSpan("root").(*jaeger.Span)
-	span.Retain().Finish()
-	defer span.Release()
+	span.Finish()
 
 	// Need to yield to the select loop to accept the send request, and then
 	// yield again to the send operation to write to the socket. I think the

zipkin_thrift_span_test.go

@@ -39,10 +39,8 @@ func TestThriftFirstInProcessSpan(t *testing.T) {
 
 	sp1 := tracer.StartSpan("s1").(*Span)
 	sp2 := tracer.StartSpan("sp2", opentracing.ChildOf(sp1.Context())).(*Span)
-	sp2.Retain().Finish()
-	sp1.Retain().Finish()
-	defer sp2.Release()
-	defer sp1.Release()
+	sp2.Finish()
+	sp1.Finish()
 
 	tests := []struct {
 		span     *Span
@@ -84,10 +82,8 @@ func Test128bitTraceIDs(t *testing.T) {
 
 	sp1 := tracer128.StartSpan("s1").(*Span)
 	sp2 := tracer128.StartSpan("sp2", opentracing.ChildOf(sp1.Context())).(*Span)
-	sp2.Retain().Finish()
-	sp1.Retain().Finish()
-	defer sp2.Release()
-	defer sp1.Release()
+	sp2.Finish()
+	sp1.Finish()
 
 	thriftSpan1 := BuildZipkinThrift(sp1)
 	assert.NotNil(t, thriftSpan1.TraceIDHigh)
@@ -96,8 +92,7 @@ func Test128bitTraceIDs(t *testing.T) {
 	assert.NotNil(t, thriftSpan2.TraceIDHigh)
 
 	sp3 := tracer64.StartSpan("s3").(*Span)
-	sp3.Retain().Finish()
-	defer sp3.Release()
+	sp3.Finish()
 	thriftSpan3 := BuildZipkinThrift(sp3)
 	assert.Nil(t, thriftSpan3.TraceIDHigh)
 }
@@ -251,12 +246,7 @@ func TestThriftSpanLogs(t *testing.T) {
 
 	for i, test := range tests {
 		testName := fmt.Sprintf("test-%02d", i)
-		sp := tracer.StartSpan(testName, opentracing.ChildOf(root.Context())).(*Span)
-		sp.Retain()
-		defer func() {
-			sp.Retain(-1).Release()
-		}()
-
+		sp := tracer.StartSpan(testName, opentracing.ChildOf(root.Context()))
 		if test.disableSampling {
 			ext.SamplingPriority.Set(sp, 0)
 		}
@@ -265,7 +255,7 @@ func TestThriftSpanLogs(t *testing.T) {
 		} else if len(test.fields) > 0 {
 			sp.LogFields(test.fields...)
 		}
-		thriftSpan := BuildZipkinThrift(sp)
+		thriftSpan := BuildZipkinThrift(sp.(*Span))
 		if test.disableSampling {
 			assert.Equal(t, 0, len(thriftSpan.Annotations), testName)
 			continue
@@ -297,9 +287,8 @@ func TestThriftLocalComponentSpan(t *testing.T) {
 		if test.addComponentTag {
 			ext.Component.Set(sp, "c1")
 		}
-		sp.Retain().Finish()
+		sp.Finish()
 		thriftSpan := BuildZipkinThrift(sp)
-		sp.Release()
 
 		anno := findBinaryAnnotation(thriftSpan, "lc")
 		require.NotNil(t, anno)
@@ -318,8 +307,7 @@ func TestSpecialTags(t *testing.T) {
 	ext.PeerService.Set(sp, "peer")
 	ext.PeerPort.Set(sp, 80)
 	ext.PeerHostIPv4.Set(sp, 2130706433)
-	sp.Retain().Finish()
-	defer sp.Release()
+	sp.Finish()
 
 	thriftSpan := BuildZipkinThrift(sp)
 	// Special tags should not be copied over to binary annotations
@@ -349,8 +337,7 @@ func TestBaggageLogs(t *testing.T) {
 	sp := tracer.StartSpan("s1").(*Span)
 	sp.SetBaggageItem("auth.token", "token")
 	ext.SpanKindRPCServer.Set(sp)
-	sp.Retain().Finish()
-	defer sp.Release()
+	sp.Finish()
 
 	thriftSpan := BuildZipkinThrift(sp)
 	assert.NotNil(t, findAnnotation(thriftSpan, `{"event":"baggage","key":"auth.token","value":"token"}`))
@@ -377,8 +364,7 @@ func TestMaxTagValueLength(t *testing.T) {
 			sp := tracer.StartSpan("s1").(*Span)
 			sp.SetTag("tag.string", string(test.value))
 			sp.SetTag("tag.bytes", test.value)
-			sp.Retain().Finish()
-			defer sp.Release()
+			sp.Finish()
 			thriftSpan := BuildZipkinThrift(sp)
 			assert.Equal(t, test.expected, findBinaryAnnotation(thriftSpan, "tag.string").Value)
 			assert.Equal(t, test.expected, findBinaryAnnotation(thriftSpan, "tag.bytes").Value)