diff --git a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go index 20d6f3bf29..4498cde081 100644 --- a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go +++ b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go @@ -464,14 +464,14 @@ func TestAuthorizeRequestFilter(t *testing.T) { allow_runtime_environment { opa.runtime().config.labels.environment == "test" } - + default allow_object := { "allowed": false, "headers": {"x-ext-auth-allow": "no"}, "body": "Unauthorized Request", "http_status": 401 } - + allow_object := response { input.parsed_path == [ "allow", "structured" ] response := { @@ -506,13 +506,13 @@ func TestAuthorizeRequestFilter(t *testing.T) { allow_body { input.parsed_body.target_id == "123456" - } - + } + decision_id := input.attributes.metadataContext.filterMetadata.open_policy_agent.decision_id allow_object_decision_id_in_header := response { input.parsed_path = ["allow", "structured"] - decision_id + decision_id response := { "allowed": true, "response_headers_to_add": { @@ -541,9 +541,9 @@ func TestAuthorizeRequestFilter(t *testing.T) { "environment": "test" }, "plugins": { - "envoy_ext_authz_grpc": { + "envoy_ext_authz_grpc": { "path": %q, - "dry-run": false + "dry-run": false } } }`, opaControlPlane.URL(), ti.regoQuery)) @@ -561,7 +561,7 @@ func TestAuthorizeRequestFilter(t *testing.T) { openpolicyagent.WithConfigTemplate(config), openpolicyagent.WithEnvoyMetadataBytes(envoyMetaDataConfig)) - opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry(openpolicyagent.WithTracer(&tracingtest.Tracer{})) + opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry(openpolicyagent.WithTracer(tracingtest.NewTracer())) ftSpec := NewOpaAuthorizeRequestSpec(opaFactory, opts...) fr.Register(ftSpec) ftSpec = NewOpaAuthorizeRequestWithBodySpec(opaFactory, opts...) @@ -769,11 +769,11 @@ func BenchmarkAuthorizeRequest(b *testing.B) { "cert": public_key_cert, "aud": "nqz3xhorr5" }) - + valid - + payload.sub == "5974934733" - } + } `, publicKey), }), ) @@ -863,9 +863,9 @@ func generateConfig(opaControlPlane *opasdktest.Server, path string) []byte { "environment": "test" }, "plugins": { - "envoy_ext_authz_grpc": { + "envoy_ext_authz_grpc": { "path": %q, - "dry-run": false + "dry-run": false } } }`, opaControlPlane.URL(), path)) diff --git a/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go b/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go index 6809ceb9b3..1a35f50b3b 100644 --- a/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go +++ b/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go @@ -176,15 +176,15 @@ func TestServerResponseFilter(t *testing.T) { allow { input.parsed_path == [ "allow" ] - } - + } + default allow_object := { "allowed": false, "headers": {"x-ext-auth-allow": "no"}, "body": "Unauthorized Request", "http_status": 403 } - + allow_object := response { input.parsed_path == [ "allow", "structured" ] response := { @@ -216,7 +216,7 @@ func TestServerResponseFilter(t *testing.T) { "http_status": 200 } } - + allow_object := response { input.parsed_path == [ "allow", "production" ] opa.runtime().config.labels.environment == "production" @@ -289,7 +289,7 @@ func TestServerResponseFilter(t *testing.T) { } }, "plugins": { - "envoy_ext_authz_grpc": { + "envoy_ext_authz_grpc": { "path": %q, "dry-run": false, "skip-request-body-parse": false @@ -300,7 +300,7 @@ func TestServerResponseFilter(t *testing.T) { } }`, opaControlPlane.URL(), ti.regoQuery)) - opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry(openpolicyagent.WithTracer(&tracingtest.Tracer{})) + opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry(openpolicyagent.WithTracer(tracingtest.NewTracer())) ftSpec := NewOpaServeResponseSpec(opaFactory, openpolicyagent.WithConfigTemplate(config)) fr.Register(ftSpec) ftSpec = NewOpaServeResponseWithReqBodySpec(opaFactory, openpolicyagent.WithConfigTemplate(config)) diff --git a/filters/openpolicyagent/openpolicyagent_test.go b/filters/openpolicyagent/openpolicyagent_test.go index 6c6dc7d923..8d38211bf9 100644 --- a/filters/openpolicyagent/openpolicyagent_test.go +++ b/filters/openpolicyagent/openpolicyagent_test.go @@ -24,6 +24,7 @@ import ( "github.com/open-policy-agent/opa/storage/inmem" "github.com/opentracing/opentracing-go" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/zalando/skipper/filters" "github.com/zalando/skipper/filters/filtertest" "github.com/zalando/skipper/filters/openpolicyagent/internal/envoy" @@ -385,16 +386,16 @@ func TestTracing(t *testing.T) { inst, err := registry.NewOpenPolicyAgentInstance("test", *cfg, "testfilter") assert.NoError(t, err) - tracer := &tracingtest.Tracer{} + tracer := tracingtest.NewTracer() parent := tracer.StartSpan("start_span") ctx := opentracing.ContextWithSpan(context.Background(), parent) span, _ := inst.StartSpanFromContext(ctx) span.Finish() parent.Finish() - recspan, ok := tracer.FindSpan("open-policy-agent") - assert.True(t, ok, "No span was created for open policy agent") - assert.Equal(t, map[string]interface{}{"opa.bundle_name": "test", "opa.label.id": inst.manager.Labels()["id"], "opa.label.version": inst.manager.Labels()["version"]}, recspan.Tags) + recspan := tracer.FindSpan("open-policy-agent") + require.NotNil(t, recspan, "No span was created for open policy agent") + assert.Equal(t, map[string]interface{}{"opa.bundle_name": "test", "opa.label.id": inst.manager.Labels()["id"], "opa.label.version": inst.manager.Labels()["version"]}, recspan.Tags()) } func TestEval(t *testing.T) { @@ -408,7 +409,7 @@ func TestEval(t *testing.T) { inst, err := registry.NewOpenPolicyAgentInstance("test", *cfg, "testfilter") assert.NoError(t, err) - tracer := &tracingtest.Tracer{} + tracer := tracingtest.NewTracer() span := tracer.StartSpan("open-policy-agent") ctx := opentracing.ContextWithSpan(context.Background(), span) @@ -426,9 +427,9 @@ func TestEval(t *testing.T) { assert.False(t, allowed) span.Finish() - testspan, ok := tracer.FindSpan("open-policy-agent") - assert.True(t, ok) - assert.Equal(t, result.DecisionID, testspan.Tags["opa.decision_id"]) + testspan := tracer.FindSpan("open-policy-agent") + require.NotNil(t, testspan) + assert.Equal(t, result.DecisionID, testspan.Tags()["opa.decision_id"]) } func TestResponses(t *testing.T) { @@ -442,7 +443,7 @@ func TestResponses(t *testing.T) { inst, err := registry.NewOpenPolicyAgentInstance("test", *cfg, "testfilter") assert.NoError(t, err) - tracer := &tracingtest.Tracer{} + tracer := tracingtest.NewTracer() span := tracer.StartSpan("open-policy-agent") metrics := &metricstest.MockMetrics{} @@ -455,8 +456,8 @@ func TestResponses(t *testing.T) { assert.Equal(t, int64(1), counters["decision.err.test"]) }) span.Finish() - testspan, ok := tracer.FindSpan("open-policy-agent") - assert.True(t, ok, "span not found") + testspan := tracer.FindSpan("open-policy-agent") + require.NotNil(t, testspan, "span not found") assert.Contains(t, testspan.Tags, "error") fc = &filtertest.Context{FMetrics: metrics} diff --git a/filters/openpolicyagent/tracing_test.go b/filters/openpolicyagent/tracing_test.go index 92f295eea3..535a2ec600 100644 --- a/filters/openpolicyagent/tracing_test.go +++ b/filters/openpolicyagent/tracing_test.go @@ -10,6 +10,7 @@ import ( "github.com/open-policy-agent/opa/plugins" "github.com/opentracing/opentracing-go" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/zalando/skipper/proxy" "github.com/zalando/skipper/tracing/tracingtest" ) @@ -24,13 +25,13 @@ func (t *MockTransport) RoundTrip(*http.Request) (*http.Response, error) { } func TestTracingFactory(t *testing.T) { - tracer := &tracingtest.Tracer{} + tracer := tracingtest.NewTracer() testCases := []struct { name string req *http.Request tracer opentracing.Tracer - parentSpan opentracing.Span + parentSpan string resp *http.Response resperr error }{ @@ -43,11 +44,11 @@ func TestTracingFactory(t *testing.T) { URL: &url.URL{Path: "/test", Scheme: "http", Host: "example.com"}, }, tracer: nil, - parentSpan: tracer.StartSpan("open-policy-agent"), + parentSpan: "open-policy-agent", resp: &http.Response{StatusCode: http.StatusOK}, }, { - name: "Sub-span created with parent span without tracer set", + name: "Sub-span created with parent span with tracer set", req: &http.Request{ Header: map[string][]string{}, Method: "GET", @@ -55,7 +56,7 @@ func TestTracingFactory(t *testing.T) { URL: &url.URL{Path: "/test", Scheme: "http", Host: "example.com"}, }, tracer: tracer, - parentSpan: tracer.StartSpan("open-policy-agent"), + parentSpan: "open-policy-agent", resp: &http.Response{StatusCode: http.StatusOK}, }, { @@ -96,7 +97,15 @@ func TestTracingFactory(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { f := &tracingFactory{} - tracer.Reset("") + tracer.Reset() + + if tc.parentSpan != "" { + parentSpan := tracer.StartSpan(tc.parentSpan) + defer parentSpan.Finish() + + ctx := opentracing.ContextWithSpan(context.Background(), parentSpan) + tc.req = tc.req.WithContext(ctx) + } tr := f.NewTransport(&MockTransport{tc.resp, tc.resperr}, buildTracingOptions(tc.tracer, "bundle", &plugins.Manager{ ID: "manager-id", @@ -105,41 +114,35 @@ func TestTracingFactory(t *testing.T) { }, })) - if tc.parentSpan != nil { - ctx := opentracing.ContextWithSpan(context.Background(), tc.parentSpan) - tc.req = tc.req.WithContext(ctx) - } - resp, err := tr.RoundTrip(tc.req) - if tc.parentSpan != nil { - tc.parentSpan.Finish() - } - createdSpan, ok := tracer.FindSpan("open-policy-agent.http") - assert.True(t, ok, "No span was created") + createdSpan := tracer.FindSpan("open-policy-agent.http") + require.NotNil(t, createdSpan, "No span was created") + + createdSpanTags := createdSpan.Tags() if tc.resperr == nil { assert.NoError(t, err) if tc.resp.StatusCode > 399 { - assert.Equal(t, true, createdSpan.Tags["error"], "Error tag was not set") + assert.Equal(t, true, createdSpanTags["error"], "Error tag was not set") } - assert.Equal(t, tc.resp.StatusCode, createdSpan.Tags[proxy.HTTPStatusCodeTag], "http status tag was not set") + assert.Equal(t, tc.resp.StatusCode, createdSpanTags[proxy.HTTPStatusCodeTag], "http status tag was not set") } else { - assert.Equal(t, true, createdSpan.Tags["error"], "Error tag was not set") + assert.Equal(t, true, createdSpanTags["error"], "Error tag was not set") assert.Equal(t, tc.resperr, err, "Error was not propagated") } assert.Equal(t, tc.resp, resp, "Response was not propagated") - assert.Equal(t, tc.req.Method, createdSpan.Tags["http.method"]) - assert.Equal(t, tc.req.URL.String(), createdSpan.Tags["http.url"]) - assert.Equal(t, tc.req.Host, createdSpan.Tags["hostname"]) - assert.Equal(t, tc.req.URL.Path, createdSpan.Tags["http.path"]) - assert.Equal(t, "skipper", createdSpan.Tags["component"]) - assert.Equal(t, "client", createdSpan.Tags["span.kind"]) - assert.Equal(t, "bundle", createdSpan.Tags["opa.bundle_name"]) - assert.Equal(t, "value", createdSpan.Tags["opa.label.label"]) + assert.Equal(t, tc.req.Method, createdSpanTags["http.method"]) + assert.Equal(t, tc.req.URL.String(), createdSpanTags["http.url"]) + assert.Equal(t, tc.req.Host, createdSpanTags["hostname"]) + assert.Equal(t, tc.req.URL.Path, createdSpanTags["http.path"]) + assert.Equal(t, "skipper", createdSpanTags["component"]) + assert.Equal(t, "client", createdSpanTags["span.kind"]) + assert.Equal(t, "bundle", createdSpanTags["opa.bundle_name"]) + assert.Equal(t, "value", createdSpanTags["opa.label.label"]) }) } } diff --git a/filters/tracing/baggagetotag_test.go b/filters/tracing/baggagetotag_test.go index a6909c6f15..5e85271880 100644 --- a/filters/tracing/baggagetotag_test.go +++ b/filters/tracing/baggagetotag_test.go @@ -25,7 +25,8 @@ func TestBaggageItemNameToTag(t *testing.T) { t.Run(ti.msg, func(t *testing.T) { req := &http.Request{Header: http.Header{}} - span := tracingtest.NewSpan("start_span") + tracer := tracingtest.NewTracer() + span := tracer.StartSpan("start_span") span.SetBaggageItem(ti.baggageItemName, ti.baggageItemValue) req = req.WithContext(opentracing.ContextWithSpan(req.Context(), span)) ctx := &filtertest.Context{FRequest: req} @@ -38,7 +39,9 @@ func TestBaggageItemNameToTag(t *testing.T) { f.Request(ctx) - if tagValue := span.Tags[ti.tagName]; ti.baggageItemValue != tagValue { + span.Finish() + + if tagValue := span.(*tracingtest.MockSpan).Tags()[ti.tagName]; ti.baggageItemValue != tagValue { t.Error("couldn't set span tag from baggage item") } }) @@ -99,7 +102,8 @@ func TestFallbackToBaggageNameForTag(t *testing.T) { t.Run(ti.msg, func(t *testing.T) { req := &http.Request{Header: http.Header{}} - span := tracingtest.NewSpan("start_span") + tracer := tracingtest.NewTracer() + span := tracer.StartSpan("start_span") span.SetBaggageItem(ti.baggageItemName, ti.baggageItemValue) req = req.WithContext(opentracing.ContextWithSpan(req.Context(), span)) ctx := &filtertest.Context{FRequest: req} @@ -112,7 +116,9 @@ func TestFallbackToBaggageNameForTag(t *testing.T) { f.Request(ctx) - if tagValue := span.Tags[ti.baggageItemName]; ti.baggageItemValue != tagValue { + span.Finish() + + if tagValue := span.(*tracingtest.MockSpan).Tags()[ti.baggageItemName]; ti.baggageItemValue != tagValue { t.Error("couldn't set span tag from baggage item") } }) diff --git a/filters/tracing/statebagtotag_test.go b/filters/tracing/statebagtotag_test.go index 66b1a4c3b7..5788fcd936 100644 --- a/filters/tracing/statebagtotag_test.go +++ b/filters/tracing/statebagtotag_test.go @@ -16,7 +16,9 @@ import ( func TestStateBagToTag(t *testing.T) { req := &http.Request{Header: http.Header{}} - span := tracingtest.NewSpan("start_span") + tracer := tracingtest.NewTracer() + span := tracer.StartSpan("start_span") + req = req.WithContext(opentracing.ContextWithSpan(req.Context(), span)) ctx := &filtertest.Context{FRequest: req, FStateBag: map[string]interface{}{"item": "val"}} @@ -25,13 +27,18 @@ func TestStateBagToTag(t *testing.T) { f.Request(ctx) - assert.Equal(t, "val", span.Tags["tag"]) + span.Finish() + + assert.Equal(t, "val", span.(*tracingtest.MockSpan).Tags()["tag"]) } func TestStateBagToTagAllocs(t *testing.T) { req := &http.Request{Header: http.Header{}} - span := tracingtest.NewSpan("start_span") + tracer := tracingtest.NewTracer() + span := tracer.StartSpan("start_span") + defer span.Finish() + req = req.WithContext(opentracing.ContextWithSpan(req.Context(), span)) ctx := &filtertest.Context{FRequest: req, FStateBag: map[string]interface{}{"item": "val"}} @@ -100,7 +107,9 @@ func BenchmarkStateBagToTag_StringValue(b *testing.B) { f, err := NewStateBagToTag().CreateFilter([]interface{}{"item", "tag"}) require.NoError(b, err) - span := tracingtest.NewSpan("start_span") + tracer := tracingtest.NewTracer() + span := tracer.StartSpan("start_span") + defer span.Finish() req := &http.Request{Header: http.Header{}} req = req.WithContext(opentracing.ContextWithSpan(req.Context(), span)) @@ -108,7 +117,7 @@ func BenchmarkStateBagToTag_StringValue(b *testing.B) { ctx := &filtertest.Context{FRequest: req, FStateBag: map[string]interface{}{"item": "val"}} f.Request(ctx) - require.Equal(b, "val", span.Tags["tag"]) + require.Equal(b, "val", span.(*tracingtest.MockSpan).Tags()["tag"]) b.ReportAllocs() b.ResetTimer() diff --git a/filters/tracing/tag_test.go b/filters/tracing/tag_test.go index 529b7000ef..6135618a87 100644 --- a/filters/tracing/tag_test.go +++ b/filters/tracing/tag_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/opentracing/opentracing-go" - "github.com/opentracing/opentracing-go/mocktracer" "github.com/zalando/skipper/eskip" "github.com/zalando/skipper/filters" "github.com/zalando/skipper/filters/filtertest" @@ -306,7 +305,7 @@ func TestTracingTag(t *testing.T) { }, } { t.Run(ti.name, func(t *testing.T) { - span := tracer.StartSpan("proxy").(*mocktracer.MockSpan) + span := tracer.StartSpan("proxy").(*tracingtest.MockSpan) defer span.Finish() requestContext := &filtertest.Context{ diff --git a/proxy/tracing_test.go b/proxy/tracing_test.go index 6e1ba1ae85..92d3ad3ff0 100644 --- a/proxy/tracing_test.go +++ b/proxy/tracing_test.go @@ -1,7 +1,6 @@ package proxy import ( - "crypto/md5" "fmt" "io" "math/rand" @@ -20,60 +19,6 @@ import ( "github.com/stretchr/testify/require" ) -const traceHeader = "X-Trace-Header" - -func TestTracingFromWire(t *testing.T) { - traceContent := fmt.Sprintf("%x", md5.New().Sum([]byte(time.Now().String()))) - s := startTestServer(nil, 0, func(r *http.Request) { - th, ok := r.Header[traceHeader] - if !ok { - t.Errorf("missing %s request header", traceHeader) - } else { - if th[0] != traceContent { - t.Errorf("wrong X-Trace-Header content: %s", th[0]) - } - } - }) - defer s.Close() - - u, _ := url.ParseRequestURI("https://www.example.org/hello") - r := &http.Request{ - URL: u, - Method: "GET", - Header: make(http.Header), - } - r.Header.Set(traceHeader, traceContent) - w := httptest.NewRecorder() - - doc := fmt.Sprintf(`hello: Path("/hello") -> "%s"`, s.URL) - tracer := &tracingtest.Tracer{} - params := Params{ - OpenTracing: &OpenTracingParams{ - Tracer: tracer, - }, - Flags: FlagsNone, - } - - tp, err := newTestProxyWithParams(doc, params) - if err != nil { - t.Error(err) - return - } - defer tp.close() - - tp.proxy.ServeHTTP(w, r) - - if len(tracer.RecordedSpans) == 0 { - t.Fatal("no span recorded...") - } - if tracer.RecordedSpans[0].Trace != traceContent { - t.Errorf("trace not found, got `%s` instead", tracer.RecordedSpans[0].Trace) - } - if len(tracer.RecordedSpans[0].Refs) == 0 { - t.Errorf("no references found, this is a root span") - } -} - func TestTracingIngressSpan(t *testing.T) { s := startTestServer(nil, 0, func(r *http.Request) { p := &mocktracer.TextMapPropagator{} @@ -117,8 +62,8 @@ func TestTracingIngressSpan(t *testing.T) { t.Fatal(err) } - span, ok := findSpan(tracer, "ingress") - if !ok { + span := tracer.FindSpan("ingress") + if span == nil { t.Fatal("ingress span not found") } @@ -172,8 +117,8 @@ func TestTracingIngressSpanShunt(t *testing.T) { defer rsp.Body.Close() io.Copy(io.Discard, rsp.Body) - span, ok := findSpan(tracer, "ingress") - if !ok { + span := tracer.FindSpan("ingress") + if span == nil { t.Fatal("ingress span not found") } @@ -264,17 +209,7 @@ func TestTracingIngressSpanLoopback(t *testing.T) { } func TestTracingSpanName(t *testing.T) { - traceContent := fmt.Sprintf("%x", md5.New().Sum([]byte(time.Now().String()))) - s := startTestServer(nil, 0, func(r *http.Request) { - th, ok := r.Header[traceHeader] - if !ok { - t.Errorf("missing %s request header", traceHeader) - } else { - if th[0] != traceContent { - t.Errorf("wrong X-Trace-Header content: %s", th[0]) - } - } - }) + s := startTestServer(nil, 0, func(r *http.Request) {}) defer s.Close() u, _ := url.ParseRequestURI("https://www.example.org/hello") @@ -286,7 +221,7 @@ func TestTracingSpanName(t *testing.T) { w := httptest.NewRecorder() doc := fmt.Sprintf(`hello: Path("/hello") -> tracingSpanName("test-span") -> "%s"`, s.URL) - tracer := &tracingtest.Tracer{TraceContent: traceContent} + tracer := tracingtest.NewTracer() params := Params{ OpenTracing: &OpenTracingParams{ Tracer: tracer, @@ -303,23 +238,13 @@ func TestTracingSpanName(t *testing.T) { tp.proxy.ServeHTTP(w, r) - if _, ok := tracer.FindSpan("test-span"); !ok { + if span := tracer.FindSpan("test-span"); span == nil { t.Error("setting the span name failed") } } func TestTracingInitialSpanName(t *testing.T) { - traceContent := fmt.Sprintf("%x", md5.New().Sum([]byte(time.Now().String()))) - s := startTestServer(nil, 0, func(r *http.Request) { - th, ok := r.Header[traceHeader] - if !ok { - t.Errorf("missing %s request header", traceHeader) - } else { - if th[0] != traceContent { - t.Errorf("wrong X-Trace-Header content: %s", th[0]) - } - } - }) + s := startTestServer(nil, 0, func(r *http.Request) {}) defer s.Close() u, _ := url.ParseRequestURI("https://www.example.org/hello") @@ -331,7 +256,7 @@ func TestTracingInitialSpanName(t *testing.T) { w := httptest.NewRecorder() doc := fmt.Sprintf(`hello: Path("/hello") -> "%s"`, s.URL) - tracer := &tracingtest.Tracer{TraceContent: traceContent} + tracer := tracingtest.NewTracer() params := Params{ OpenTracing: &OpenTracingParams{ Tracer: tracer, @@ -349,7 +274,7 @@ func TestTracingInitialSpanName(t *testing.T) { tp.proxy.ServeHTTP(w, r) - if _, ok := tracer.FindSpan("test-initial-span"); !ok { + if span := tracer.FindSpan("test-initial-span"); span == nil { t.Error("setting the span name failed") } } @@ -390,8 +315,8 @@ func TestTracingProxySpan(t *testing.T) { t.Fatal(err) } - span, ok := findSpan(tracer, "proxy") - if !ok { + span := tracer.FindSpan("proxy") + if span == nil { t.Fatal("proxy span not found") } @@ -437,7 +362,7 @@ func TestTracingProxySpanWithRetry(t *testing.T) { const docFmt = `r: * -> ;` doc := fmt.Sprintf(docFmt, s0.URL, s1.URL) - tracer := &tracingtest.Tracer{} + tracer := tracingtest.NewTracer() tp, err := newTestProxyWithParams(doc, Params{OpenTracing: &OpenTracingParams{Tracer: tracer}}) if err != nil { t.Fatal(err) @@ -445,7 +370,7 @@ func TestTracingProxySpanWithRetry(t *testing.T) { defer tp.close() testFallback := func() bool { - tracer.Reset("") + tracer.Reset() req, err := http.NewRequest("GET", "https://www.example.org", nil) if err != nil { t.Fatal(err) @@ -453,7 +378,13 @@ func TestTracingProxySpanWithRetry(t *testing.T) { tp.proxy.ServeHTTP(httptest.NewRecorder(), req) - proxySpans := tracer.FindAllSpans("proxy") + var proxySpans []*tracingtest.MockSpan + for _, span := range tracer.FinishedSpans() { + if span.OperationName == "proxy" { + proxySpans = append(proxySpans, span) + } + } + if len(proxySpans) != 2 { t.Log("invalid count of proxy spans", len(proxySpans)) return false @@ -551,7 +482,7 @@ func TestFilterTracing(t *testing.T) { } } -func spanLogs(span *mocktracer.MockSpan) string { +func spanLogs(span *tracingtest.MockSpan) string { var logs []string for _, e := range span.Logs() { for _, f := range e.Fields { @@ -573,7 +504,7 @@ func TestEnabledLogStreamEvents(t *testing.T) { tracing.logStreamEvent(span, "test-filter", StartEvent) tracing.logStreamEvent(span, "test-filter", EndEvent) - mockSpan := span.(*mocktracer.MockSpan) + mockSpan := span.(*tracingtest.MockSpan) if len(mockSpan.Logs()) != 2 { t.Errorf("filter lifecycle events were not logged although it was enabled") @@ -592,7 +523,7 @@ func TestDisabledLogStreamEvents(t *testing.T) { tracing.logStreamEvent(span, "test-filter", StartEvent) tracing.logStreamEvent(span, "test-filter", EndEvent) - mockSpan := span.(*mocktracer.MockSpan) + mockSpan := span.(*tracingtest.MockSpan) if len(mockSpan.Logs()) != 0 { t.Errorf("filter lifecycle events were logged although it was disabled") @@ -611,7 +542,7 @@ func TestSetEnabledTags(t *testing.T) { tracing.setTag(span, HTTPStatusCodeTag, 200) tracing.setTag(span, ComponentTag, "skipper") - mockSpan := span.(*mocktracer.MockSpan) + mockSpan := span.(*tracingtest.MockSpan) tags := mockSpan.Tags() @@ -638,7 +569,7 @@ func TestSetDisabledTags(t *testing.T) { tracing.setTag(span, ComponentTag, "skipper") tracing.setTag(span, SkipperRouteIDTag, "long_route_id") - mockSpan := span.(*mocktracer.MockSpan) + mockSpan := span.(*tracingtest.MockSpan) tags := mockSpan.Tags() @@ -676,16 +607,7 @@ func TestSetTagWithEmptySpan(t *testing.T) { tracing.setTag(nil, "test", "val") } -func findSpan(tracer *tracingtest.MockTracer, name string) (*mocktracer.MockSpan, bool) { - for _, s := range tracer.FinishedSpans() { - if s.OperationName == name { - return s, true - } - } - return nil, false -} - -func findSpanByRouteID(tracer *tracingtest.MockTracer, routeID string) (*mocktracer.MockSpan, bool) { +func findSpanByRouteID(tracer *tracingtest.MockTracer, routeID string) (*tracingtest.MockSpan, bool) { for _, s := range tracer.FinishedSpans() { if s.Tag(SkipperRouteIDTag) == routeID { return s, true @@ -694,21 +616,21 @@ func findSpanByRouteID(tracer *tracingtest.MockTracer, routeID string) (*mocktra return nil, false } -func verifyTag(t *testing.T, span *mocktracer.MockSpan, name string, expected interface{}) { +func verifyTag(t *testing.T, span *tracingtest.MockSpan, name string, expected interface{}) { t.Helper() if got := span.Tag(name); got != expected { t.Errorf("unexpected '%s' tag value: '%v' != '%v'", name, got, expected) } } -func verifyNoTag(t *testing.T, span *mocktracer.MockSpan, name string) { +func verifyNoTag(t *testing.T, span *tracingtest.MockSpan, name string) { t.Helper() if got, ok := span.Tags()[name]; ok { t.Errorf("unexpected '%s' tag: '%v'", name, got) } } -func verifyHasTag(t *testing.T, span *mocktracer.MockSpan, name string) { +func verifyHasTag(t *testing.T, span *tracingtest.MockSpan, name string) { t.Helper() if got, ok := span.Tags()[name]; !ok || got == "" { t.Errorf("expected '%s' tag", name) diff --git a/redis_test.go b/redis_test.go index e4429763a2..d495a07189 100644 --- a/redis_test.go +++ b/redis_test.go @@ -126,7 +126,7 @@ spec: rt := routing.New(ro) defer rt.Close() <-rt.FirstLoad() - tracer := &tracingtest.Tracer{} + tracer := tracingtest.NewTracer() pr := proxy.WithParams(proxy.Params{ Routing: rt, OpenTracing: &proxy.OpenTracingParams{Tracer: tracer}, @@ -301,7 +301,7 @@ spec: rt := routing.New(ro) defer rt.Close() <-rt.FirstLoad() - tracer := &tracingtest.Tracer{} + tracer := tracingtest.NewTracer() pr := proxy.WithParams(proxy.Params{ Routing: rt, OpenTracing: &proxy.OpenTracingParams{Tracer: tracer}, diff --git a/skipper_test.go b/skipper_test.go index c47bc979b5..051434ab29 100644 --- a/skipper_test.go +++ b/skipper_test.go @@ -132,7 +132,7 @@ func TestOptionsFilterRegistry(t *testing.T) { } func TestOptionsOpenTracingTracerInstanceOverridesOpenTracing(t *testing.T) { - tracer := &tracingtest.Tracer{} + tracer := tracingtest.NewTracer() o := Options{ OpenTracingTracer: tracer, OpenTracing: []string{"noop"}, @@ -574,7 +574,7 @@ func TestDataClients(t *testing.T) { rt := routing.New(ro) defer rt.Close() <-rt.FirstLoad() - tracer := &tracingtest.Tracer{} + tracer := tracingtest.NewTracer() pr := proxy.WithParams(proxy.Params{ Routing: rt, OpenTracing: &proxy.OpenTracingParams{Tracer: tracer}, diff --git a/tracing/tracingtest/mocktracer.go b/tracing/tracingtest/mocktracer.go index 9082c81a9f..f848d61f95 100644 --- a/tracing/tracingtest/mocktracer.go +++ b/tracing/tracingtest/mocktracer.go @@ -1,6 +1,7 @@ package tracingtest import ( + "fmt" "sync/atomic" "time" @@ -13,6 +14,11 @@ type MockTracer struct { spans atomic.Int32 } +type MockSpan struct { + *mocktracer.MockSpan + t *MockTracer +} + func NewTracer() *MockTracer { return &MockTracer{mockTracer: mocktracer.New()} } @@ -24,24 +30,37 @@ func (t *MockTracer) Reset() { func (t *MockTracer) StartSpan(operationName string, opts ...opentracing.StartSpanOption) opentracing.Span { t.spans.Add(1) - return t.mockTracer.StartSpan(operationName, opts...) + return &MockSpan{MockSpan: t.mockTracer.StartSpan(operationName, opts...).(*mocktracer.MockSpan), t: t} } -func (t *MockTracer) FinishedSpans() []*mocktracer.MockSpan { +func (t *MockTracer) FinishedSpans() []*MockSpan { timeout := time.After(1 * time.Second) retry := time.NewTicker(100 * time.Millisecond) defer retry.Stop() for { finished := t.mockTracer.FinishedSpans() if len(finished) == int(t.spans.Load()) { - return finished + result := make([]*MockSpan, len(finished)) + for i, s := range finished { + result[i] = &MockSpan{MockSpan: s, t: t} + } + return result } select { case <-retry.C: case <-timeout: - return nil + panic(fmt.Sprintf("Timeout waiting for %d finished spans, got: %d", t.spans.Load(), len(finished))) + } + } +} + +func (t *MockTracer) FindSpan(operationName string) *MockSpan { + for _, s := range t.FinishedSpans() { + if s.OperationName == operationName { + return s } } + return nil } func (t *MockTracer) Inject(sm opentracing.SpanContext, format any, carrier any) error { @@ -51,3 +70,7 @@ func (t *MockTracer) Inject(sm opentracing.SpanContext, format any, carrier any) func (t *MockTracer) Extract(format any, carrier any) (opentracing.SpanContext, error) { return t.mockTracer.Extract(format, carrier) } + +func (s *MockSpan) Tracer() opentracing.Tracer { + return s.t +} diff --git a/tracing/tracingtest/mocktracer_test.go b/tracing/tracingtest/mocktracer_test.go new file mode 100644 index 0000000000..902f4ae49d --- /dev/null +++ b/tracing/tracingtest/mocktracer_test.go @@ -0,0 +1,17 @@ +package tracingtest_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/zalando/skipper/tracing/tracingtest" +) + +func TestMockTracerSpanTracer(t *testing.T) { + tracer := tracingtest.NewTracer() + + span := tracer.StartSpan("test") + span.Finish() + + assert.Same(t, tracer, span.Tracer()) +} diff --git a/tracing/tracingtest/testtracer.go b/tracing/tracingtest/testtracer.go index 4da0a6d4ba..0227f74387 100644 --- a/tracing/tracingtest/testtracer.go +++ b/tracing/tracingtest/testtracer.go @@ -13,6 +13,8 @@ import ( // Tracer is an implementation of opentracing.Tracer for testing. It records // the defined spans during a series of operations. +// +// Deprecated: use [NewTracer] instead. type Tracer struct { // TraceContent represents the tracing content passed along the wire. @@ -44,6 +46,7 @@ type Span struct { tracer *Tracer } +// Deprecated: use [NewTracer] and [MockTracer.StartSpan] instead. func NewSpan(operation string) *Span { return &Span{ operationName: operation,