Removed all direct mocktracer.New() calls and explicit waits for fini…

…shed spans Signed-off-by: Roman Zavodskikh <[email protected]>
zalando · Nov 26, 2024 · 98e895d · 98e895d
1 parent adf38fc
commit 98e895d
Show file tree

Hide file tree

Showing 7 changed files with 36 additions and 62 deletions.
diff --git a/filters/auth/jwt_metrics_test.go b/filters/auth/jwt_metrics_test.go
@@ -8,9 +8,7 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"testing"
-	"time"
 
-	"github.com/opentracing/opentracing-go/mocktracer"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/zalando/skipper/eskip"
@@ -20,6 +18,7 @@ import (
 	"github.com/zalando/skipper/proxy"
 	"github.com/zalando/skipper/proxy/proxytest"
 	"github.com/zalando/skipper/routing"
+	"github.com/zalando/skipper/tracing/tracingtest"
 )
 
 func TestJwtMetrics(t *testing.T) {
@@ -275,7 +274,7 @@ func TestJwtMetrics(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			m := &metricstest.MockMetrics{}
 			defer m.Close()
-			tracer := mocktracer.New()
+			tracer := tracingtest.NewTracer()
 
 			fr := builtin.MakeRegistry()
 			fr.Register(auth.NewJwtMetrics())
@@ -304,8 +303,7 @@ func TestJwtMetrics(t *testing.T) {
 			resp.Body.Close()
 			require.Equal(t, tc.status, resp.StatusCode)
 
-			// wait for the span to be finished
-			require.Eventually(t, func() bool { return len(tracer.FinishedSpans()) == 1 }, time.Second, 100*time.Millisecond)
+			require.Equal(t, 1, len(tracer.FinishedSpans()))
 
 			span := tracer.FinishedSpans()[0]
 			if tc.expectedTag == "" {

diff --git a/filters/scheduler/lifo_test.go b/filters/scheduler/lifo_test.go
@@ -474,7 +474,7 @@ func TestLifoErrors(t *testing.T) {
 
 	<-rt.FirstLoad()
 
-	tracer := tracingtest.NewMockTracer()
+	tracer := tracingtest.NewTracer()
 	pr := proxy.WithParams(proxy.Params{
 		Routing:     rt,
 		OpenTracing: &proxy.OpenTracingParams{Tracer: tracer},

diff --git a/filters/tracing/tag_test.go b/filters/tracing/tag_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/zalando/skipper/eskip"
 	"github.com/zalando/skipper/filters"
 	"github.com/zalando/skipper/filters/filtertest"
+	"github.com/zalando/skipper/tracing/tracingtest"
 )
 
 func TestTracingTagNil(t *testing.T) {
@@ -196,7 +197,7 @@ func TestTagCreateFilter(t *testing.T) {
 }
 
 func TestTracingTag(t *testing.T) {
-	tracer := mocktracer.New()
+	tracer := tracingtest.NewTracer()
 
 	for _, ti := range []struct {
 		name     string

diff --git a/net/httpclient_example_test.go b/net/httpclient_example_test.go
@@ -11,23 +11,12 @@ import (
 	"github.com/lightstep/lightstep-tracer-go"
 	"github.com/opentracing/opentracing-go"
 	"github.com/opentracing/opentracing-go/ext"
-	"github.com/opentracing/opentracing-go/mocktracer"
 	"github.com/sirupsen/logrus"
 	"github.com/zalando/skipper/net"
 	"github.com/zalando/skipper/secrets"
+	"github.com/zalando/skipper/tracing/tracingtest"
 )
 
-func waitForSpanViaMockTracer(mockTracer *mocktracer.MockTracer) {
-	for i := 0; i < 20; i++ {
-		if n := len(mockTracer.FinishedSpans()); n > 0 {
-			logrus.Printf("found %d spans", n)
-			return
-		}
-		time.Sleep(100 * time.Millisecond)
-	}
-	logrus.Println("no span found")
-}
-
 func ExampleTransport() {
 	tracer := lightstep.NewTracer(lightstep.Options{})
 
@@ -221,7 +210,7 @@ func (t *customTracer) StartSpan(operationName string, opts ...opentracing.Start
 }
 
 func ExampleClient_customTracer() {
-	mockTracer := mocktracer.New()
+	mockTracer := tracingtest.NewTracer()
 	cli := net.NewClient(net.Options{
 		Tracer:              &customTracer{mockTracer},
 		OpentracingSpanName: "clientSpan",
@@ -232,10 +221,6 @@ func ExampleClient_customTracer() {
 	defer srv.Close()
 
 	cli.Get("http://" + srv.Listener.Addr().String() + "/")
-
-	// wait for the span to be finished
-	waitForSpanViaMockTracer(mockTracer)
-
 	fmt.Printf("customtag: %s", mockTracer.FinishedSpans()[0].Tags()["customtag"])
 
 	// Output:
@@ -343,7 +328,7 @@ func ExampleClient_hostSecret() {
 }
 
 func ExampleClient_withBeforeSendHook() {
-	mockTracer := mocktracer.New()
+	mockTracer := tracingtest.NewTracer()
 	peerService := "my-peer-service"
 	cli := net.NewClient(net.Options{
 		Tracer:                  &customTracer{mockTracer},
@@ -369,10 +354,6 @@ func ExampleClient_withBeforeSendHook() {
 	defer srv.Close()
 
 	cli.Get("http://" + srv.Listener.Addr().String() + "/")
-
-	// wait for the span to be finished
-	waitForSpanViaMockTracer(mockTracer)
-
 	fmt.Printf("request tag %q set to %q", string(ext.PeerService), mockTracer.FinishedSpans()[0].Tags()[string(ext.PeerService)])
 
 	// Output:
@@ -381,7 +362,7 @@ func ExampleClient_withBeforeSendHook() {
 }
 
 func ExampleClient_withAfterResponseHook() {
-	mockTracer := mocktracer.New()
+	mockTracer := tracingtest.NewTracer()
 	cli := net.NewClient(net.Options{
 		Tracer:                     &customTracer{mockTracer},
 		OpentracingComponentTag:    "testclient",
@@ -411,9 +392,6 @@ func ExampleClient_withAfterResponseHook() {
 		log.Fatalf("Failed to get: %v", err)
 	}
 
-	// wait for the span to be finished
-	waitForSpanViaMockTracer(mockTracer)
-
 	fmt.Printf("response code: %d\n", rsp.StatusCode)
 	fmt.Printf("span status.code: %d", mockTracer.FinishedSpans()[0].Tags()["status.code"])
 

diff --git a/net/httpclient_test.go b/net/httpclient_test.go
@@ -13,9 +13,9 @@ import (
 	"github.com/AlexanderYastrebov/noleak"
 	"github.com/opentracing/opentracing-go"
 	"github.com/opentracing/opentracing-go/ext"
-	"github.com/opentracing/opentracing-go/mocktracer"
 	"github.com/zalando/skipper/secrets"
 	"github.com/zalando/skipper/tracing/tracers/basic"
+	"github.com/zalando/skipper/tracing/tracingtest"
 )
 
 var testToken = []byte("mytoken1")
@@ -208,7 +208,7 @@ func TestClient(t *testing.T) {
 }
 
 func TestTransport(t *testing.T) {
-	mtracer := mocktracer.New()
+	mtracer := tracingtest.NewTracer()
 	tracer, err := basic.InitTracer(nil)
 	if err != nil {
 		t.Fatalf("Failed to get a tracer: %v", err)

diff --git a/proxy/tracing_test.go b/proxy/tracing_test.go
@@ -87,7 +87,7 @@ func TestTracingIngressSpan(t *testing.T) {
 	routeID := "ingressRoute"
 	doc := fmt.Sprintf(`%s: Path("/hello") -> setPath("/bye") -> setQuery("void") -> "%s"`, routeID, s.URL)
 
-	tracer := mocktracer.New()
+	tracer := tracingtest.NewTracer()
 	params := Params{
 		OpenTracing: &OpenTracingParams{
 			Tracer: tracer,
@@ -117,9 +117,6 @@ func TestTracingIngressSpan(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// client may get response before proxy finishes span
-	time.Sleep(10 * time.Millisecond)
-
 	span, ok := findSpan(tracer, "ingress")
 	if !ok {
 		t.Fatal("ingress span not found")
@@ -143,7 +140,7 @@ func TestTracingIngressSpanShunt(t *testing.T) {
 	routeID := "ingressShuntRoute"
 	doc := fmt.Sprintf(`%s: Path("/hello") -> setPath("/bye") -> setQuery("void") -> status(205) -> <shunt>`, routeID)
 
-	tracer := mocktracer.New()
+	tracer := tracingtest.NewTracer()
 	params := Params{
 		OpenTracing: &OpenTracingParams{
 			Tracer: tracer,
@@ -175,9 +172,6 @@ func TestTracingIngressSpanShunt(t *testing.T) {
 	defer rsp.Body.Close()
 	io.Copy(io.Discard, rsp.Body)
 
-	// client may get response before proxy finishes span
-	time.Sleep(10 * time.Millisecond)
-
 	span, ok := findSpan(tracer, "ingress")
 	if !ok {
 		t.Fatal("ingress span not found")
@@ -214,7 +208,7 @@ func TestTracingIngressSpanLoopback(t *testing.T) {
 %s: Path("/loop2") -> setPath("/loop1") -> <loopback>;
 `, shuntRouteID, loop1RouteID, loop2RouteID)
 
-	tracer := mocktracer.New()
+	tracer := tracingtest.NewTracer()
 	params := Params{
 		OpenTracing: &OpenTracingParams{
 			Tracer: tracer,
@@ -247,9 +241,6 @@ func TestTracingIngressSpanLoopback(t *testing.T) {
 	io.Copy(io.Discard, rsp.Body)
 	t.Logf("got response %d", rsp.StatusCode)
 
-	// client may get response before proxy finishes span
-	time.Sleep(10 * time.Millisecond)
-
 	sp, ok := findSpanByRouteID(tracer, loop2RouteID)
 	if !ok {
 		t.Fatalf("span for route %q not found", loop2RouteID)
@@ -375,7 +366,7 @@ func TestTracingProxySpan(t *testing.T) {
 	defer s.Close()
 
 	doc := fmt.Sprintf(`hello: Path("/hello") -> setPath("/bye") -> setQuery("void") -> "%s"`, s.URL)
-	tracer := mocktracer.New()
+	tracer := tracingtest.NewTracer()
 
 	t.Setenv("HOSTNAME", "proxy.tracing.test")
 
@@ -399,9 +390,6 @@ func TestTracingProxySpan(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// client may get response before proxy finishes span
-	time.Sleep(10 * time.Millisecond)
-
 	span, ok := findSpan(tracer, "proxy")
 	if !ok {
 		t.Fatal("proxy span not found")
@@ -532,7 +520,7 @@ func TestFilterTracing(t *testing.T) {
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			tracer := mocktracer.New()
+			tracer := tracingtest.NewTracer()
 			tc.params.Tracer = tracer
 			tracing := newProxyTracing(tc.params)
 
@@ -574,7 +562,7 @@ func spanLogs(span *mocktracer.MockSpan) string {
 }
 
 func TestEnabledLogStreamEvents(t *testing.T) {
-	tracer := mocktracer.New()
+	tracer := tracingtest.NewTracer()
 	tracing := newProxyTracing(&OpenTracingParams{
 		Tracer:          tracer,
 		LogStreamEvents: true,
@@ -593,7 +581,7 @@ func TestEnabledLogStreamEvents(t *testing.T) {
 }
 
 func TestDisabledLogStreamEvents(t *testing.T) {
-	tracer := mocktracer.New()
+	tracer := tracingtest.NewTracer()
 	tracing := newProxyTracing(&OpenTracingParams{
 		Tracer:          tracer,
 		LogStreamEvents: false,
@@ -612,7 +600,7 @@ func TestDisabledLogStreamEvents(t *testing.T) {
 }
 
 func TestSetEnabledTags(t *testing.T) {
-	tracer := mocktracer.New()
+	tracer := tracingtest.NewTracer()
 	tracing := newProxyTracing(&OpenTracingParams{
 		Tracer:      tracer,
 		ExcludeTags: []string{},
@@ -636,7 +624,7 @@ func TestSetEnabledTags(t *testing.T) {
 }
 
 func TestSetDisabledTags(t *testing.T) {
-	tracer := mocktracer.New()
+	tracer := tracingtest.NewTracer()
 	tracing := newProxyTracing(&OpenTracingParams{
 		Tracer: tracer,
 		ExcludeTags: []string{
@@ -668,7 +656,7 @@ func TestSetDisabledTags(t *testing.T) {
 }
 
 func TestLogEventWithEmptySpan(t *testing.T) {
-	tracer := mocktracer.New()
+	tracer := tracingtest.NewTracer()
 	tracing := newProxyTracing(&OpenTracingParams{
 		Tracer: tracer,
 	})
@@ -679,7 +667,7 @@ func TestLogEventWithEmptySpan(t *testing.T) {
 }
 
 func TestSetTagWithEmptySpan(t *testing.T) {
-	tracer := mocktracer.New()
+	tracer := tracingtest.NewTracer()
 	tracing := newProxyTracing(&OpenTracingParams{
 		Tracer: tracer,
 	})
@@ -688,7 +676,16 @@ func TestSetTagWithEmptySpan(t *testing.T) {
 	tracing.setTag(nil, "test", "val")
 }
 
-func findSpan(tracer *mocktracer.MockTracer, name string) (*mocktracer.MockSpan, bool) {
+func findSpanOld(tracer *mocktracer.MockTracer, name string) (*mocktracer.MockSpan, bool) {
+	for _, s := range tracer.FinishedSpans() {
+		if s.OperationName == name {
+			return s, true
+		}
+	}
+	return nil, false
+}
+
+func findSpan(tracer *tracingtest.MockTracer, name string) (*mocktracer.MockSpan, bool) {
 	for _, s := range tracer.FinishedSpans() {
 		if s.OperationName == name {
 			return s, true
@@ -697,7 +694,7 @@ func findSpan(tracer *mocktracer.MockTracer, name string) (*mocktracer.MockSpan,
 	return nil, false
 }
 
-func findSpanByRouteID(tracer *mocktracer.MockTracer, routeID string) (*mocktracer.MockSpan, bool) {
+func findSpanByRouteID(tracer *tracingtest.MockTracer, routeID string) (*mocktracer.MockSpan, bool) {
 	for _, s := range tracer.FinishedSpans() {
 		if s.Tag(SkipperRouteIDTag) == routeID {
 			return s, true

diff --git a/tracing/tracingtest/mocktracer.go b/tracing/tracingtest/mocktracer.go
@@ -13,8 +13,8 @@ type MockTracer struct {
 	spans      atomic.Int32
 }
 
-func NewMockTracer() *MockTracer {
-	return &MockTracer{mockTracer: &mocktracer.MockTracer{}}
+func NewTracer() *MockTracer {
+	return &MockTracer{mockTracer: mocktracer.New()}
 }
 
 func (t *MockTracer) Reset() {