From 21671286a6d2d004a2f33022a919a76237e9c7b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandor=20Sz=C3=BCcs?= Date: Wed, 21 Aug 2024 17:49:07 +0200 Subject: [PATCH] feature: add filter that sets a span tag on response status condition (#3202) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feature: add filter that sets a span tag on response status condition Signed-off-by: Sandor Szücs --- docs/reference/filters.md | 9 ++ filters/builtin/builtin.go | 1 + filters/filters.go | 1 + filters/tracing/tag.go | 76 +++++++++-- filters/tracing/tag_test.go | 248 ++++++++++++++++++++++++++++++++---- 5 files changed, 301 insertions(+), 34 deletions(-) diff --git a/docs/reference/filters.md b/docs/reference/filters.md index 2a2f223fc3..c203d0919a 100644 --- a/docs/reference/filters.md +++ b/docs/reference/filters.md @@ -3352,6 +3352,15 @@ tracingTag("http.flow_id", "${request.header.X-Flow-Id}") This filter works just like [tracingTag](#tracingtag), but is applied after the request was processed. In particular, [template placeholders](#template-placeholders) referencing the response can be used in the parameters. +### tracingTagFromResponseIfStatus + +Example: set error tag to true in case response status code is `>= 500` and `<= 599` + +``` +tracingTagFromResponseIfStatus("error", "true", 500, 599) +``` + + ### tracingSpanName This filter sets the name of the outgoing (client span) in opentracing. The default name is "proxy". Example: diff --git a/filters/builtin/builtin.go b/filters/builtin/builtin.go index 78e046eb21..1003756c60 100644 --- a/filters/builtin/builtin.go +++ b/filters/builtin/builtin.go @@ -214,6 +214,7 @@ func Filters() []filters.Spec { tracing.NewBaggageToTagFilter(), tracing.NewTag(), tracing.NewTagFromResponse(), + tracing.NewTagFromResponseIfStatus(), tracing.NewStateBagToTag(), //lint:ignore SA1019 due to backward compatibility accesslog.NewAccessLogDisabled(), diff --git a/filters/filters.go b/filters/filters.go index 1c554a95db..358e666b17 100644 --- a/filters/filters.go +++ b/filters/filters.go @@ -351,6 +351,7 @@ const ( StateBagToTagName = "stateBagToTag" TracingTagName = "tracingTag" TracingTagFromResponseName = "tracingTagFromResponse" + TracingTagFromResponseIfStatusName = "tracingTagFromResponseIfStatus" TracingSpanNameName = "tracingSpanName" OriginMarkerName = "originMarker" FadeInName = "fadeIn" diff --git a/filters/tracing/tag.go b/filters/tracing/tag.go index b80de4bea0..107662eb77 100644 --- a/filters/tracing/tag.go +++ b/filters/tracing/tag.go @@ -1,6 +1,8 @@ package tracing import ( + "net/http" + opentracing "github.com/opentracing/opentracing-go" "github.com/zalando/skipper/eskip" "github.com/zalando/skipper/filters" @@ -10,11 +12,21 @@ type tagSpec struct { typ string } +type tagFilterType int + +const ( + tagRequest tagFilterType = iota + 1 + tagResponse + tagResponseCondition +) + type tagFilter struct { - tagFromResponse bool + typ tagFilterType tagName string tagValue *eskip.Template + + condition func(*http.Response) bool } // NewTag creates a filter specification for the tracingTag filter. @@ -27,12 +39,34 @@ func NewTagFromResponse() filters.Spec { return &tagSpec{typ: filters.TracingTagFromResponseName} } +func NewTagFromResponseIfStatus() filters.Spec { + return &tagSpec{typ: filters.TracingTagFromResponseIfStatusName} +} + func (s *tagSpec) Name() string { return s.typ } func (s *tagSpec) CreateFilter(args []interface{}) (filters.Filter, error) { - if len(args) != 2 { + var typ tagFilterType + + switch s.typ { + case filters.TracingTagName: + if len(args) != 2 { + return nil, filters.ErrInvalidFilterParameters + } + typ = tagRequest + case filters.TracingTagFromResponseName: + if len(args) != 2 { + return nil, filters.ErrInvalidFilterParameters + } + typ = tagResponse + case filters.TracingTagFromResponseIfStatusName: + if len(args) != 4 { + return nil, filters.ErrInvalidFilterParameters + } + typ = tagResponseCondition + default: return nil, filters.ErrInvalidFilterParameters } @@ -46,23 +80,49 @@ func (s *tagSpec) CreateFilter(args []interface{}) (filters.Filter, error) { return nil, filters.ErrInvalidFilterParameters } - return &tagFilter{ - tagFromResponse: s.typ == filters.TracingTagFromResponseName, - + f := &tagFilter{ + typ: typ, tagName: tagName, tagValue: eskip.NewTemplate(tagValue), - }, nil + } + + if len(args) == 4 { + minValue, ok := args[2].(float64) + if !ok { + return nil, filters.ErrInvalidFilterParameters + } + maxValue, ok := args[3].(float64) + if !ok { + return nil, filters.ErrInvalidFilterParameters + } + minVal := int(minValue) + maxVal := int(maxValue) + if minVal < 0 || maxVal > 599 || minVal > maxVal { + return nil, filters.ErrInvalidFilterParameters + } + + f.condition = func(rsp *http.Response) bool { + return minVal <= rsp.StatusCode && rsp.StatusCode <= maxVal + } + } + + return f, nil } func (f *tagFilter) Request(ctx filters.FilterContext) { - if !f.tagFromResponse { + if f.typ == tagRequest { f.setTag(ctx) } } func (f *tagFilter) Response(ctx filters.FilterContext) { - if f.tagFromResponse { + switch f.typ { + case tagResponse: f.setTag(ctx) + case tagResponseCondition: + if f.condition(ctx.Response()) { + f.setTag(ctx) + } } } diff --git a/filters/tracing/tag_test.go b/filters/tracing/tag_test.go index 4947e3f2fd..266b2158d3 100644 --- a/filters/tracing/tag_test.go +++ b/filters/tracing/tag_test.go @@ -6,6 +6,7 @@ import ( "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" ) @@ -37,20 +38,160 @@ func TestTagName(t *testing.T) { if NewTagFromResponse().Name() != filters.TracingTagFromResponseName { t.Error("Wrong tag spec name") } + if NewTagFromResponseIfStatus().Name() != filters.TracingTagFromResponseIfStatusName { + t.Error("Wrong tag spec name") + } } func TestTagCreateFilter(t *testing.T) { - spec := tagSpec{} - if _, err := spec.CreateFilter(nil); err != filters.ErrInvalidFilterParameters { - t.Errorf("Create filter without args should return error") - } - - if _, err := spec.CreateFilter([]interface{}{3, "foo"}); err != filters.ErrInvalidFilterParameters { - t.Errorf("Create filter without first arg is string should return error") - } + for _, tt := range []struct { + name string + args []interface{} + spec filters.Spec + want error + }{ + { + name: "create filter with unknown filter", + spec: &tagSpec{}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with no args", + spec: NewTag(), + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with no args", + spec: NewTagFromResponse(), + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with no args", + spec: NewTagFromResponseIfStatus(), + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with one args", + spec: NewTag(), + args: []interface{}{"foo"}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with one args", + spec: NewTagFromResponse(), + args: []interface{}{"foo"}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with one args", + spec: NewTagFromResponseIfStatus(), + args: []interface{}{"foo"}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with two args one wrong", + spec: NewTag(), + args: []interface{}{"foo", 3}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with two args one wrong", + spec: NewTagFromResponse(), + args: []interface{}{"foo", 3}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with two args one wrong", + spec: NewTagFromResponseIfStatus(), + args: []interface{}{"foo", 3}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with two args one wrong", + spec: NewTag(), + args: []interface{}{3, "foo"}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with two args one wrong", + spec: NewTagFromResponse(), + args: []interface{}{3, "foo"}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with two args one wrong", + spec: NewTagFromResponseIfStatus(), + args: []interface{}{3, "foo"}, + want: filters.ErrInvalidFilterParameters, + }, - if _, err := spec.CreateFilter([]interface{}{"foo", 3}); err != filters.ErrInvalidFilterParameters { - t.Errorf("Create filter without second arg is string should return error") + // special + { + name: "create filter with three args want 2", + spec: NewTag(), + args: []interface{}{"foo", "bar", "qux"}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with three args want 2", + spec: NewTagFromResponse(), + args: []interface{}{"foo", "bar", "qux"}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with two args want 4 args", + spec: NewTagFromResponseIfStatus(), + args: []interface{}{"foo", "bar"}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with three args want 4", + spec: NewTagFromResponseIfStatus(), + args: []interface{}{"foo", "bar", 300}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with four args wrong args", + spec: NewTagFromResponseIfStatus(), + args: []interface{}{"foo", "bar", 300, "500"}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with four args wrong args", + spec: NewTagFromResponseIfStatus(), + args: []interface{}{"foo", "bar", "300", 500}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with four args wrong args", + spec: NewTagFromResponseIfStatus(), + args: []interface{}{"foo", "bar", "300", "500"}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with four args wrong args", + spec: NewTagFromResponseIfStatus(), + args: []interface{}{"foo", "bar", 300, 600}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with four args wrong args", + spec: NewTagFromResponseIfStatus(), + args: []interface{}{"foo", "bar", 500, 400}, + want: filters.ErrInvalidFilterParameters, + }, + { + name: "create filter with four args wrong args", + spec: NewTagFromResponseIfStatus(), + args: []interface{}{"foo", "bar", -1, 400}, + want: filters.ErrInvalidFilterParameters, + }, + } { + t.Run(tt.name, func(t *testing.T) { + if _, err := tt.spec.CreateFilter(tt.args); err != tt.want { + t.Errorf("Failed to create filter: Want %v, got %v", tt.want, err) + } + }) } } @@ -59,22 +200,21 @@ func TestTracingTag(t *testing.T) { for _, ti := range []struct { name string - spec filters.Spec - value string + doc string context *filtertest.Context + tagName string expected interface{} }{{ "plain key value", - NewTag(), - "test_value", + `tracingTag("test_tag", "test_value")`, &filtertest.Context{ FRequest: &http.Request{}, }, + "test_tag", "test_value", }, { "tag from header", - NewTag(), - "${request.header.X-Flow-Id}", + `tracingTag("test_tag", "${request.header.X-Flow-Id}")`, &filtertest.Context{ FRequest: &http.Request{ Header: http.Header{ @@ -82,11 +222,11 @@ func TestTracingTag(t *testing.T) { }, }, }, + "test_tag", "foo", }, { "tag from response", - NewTagFromResponse(), - "${response.header.X-Fallback}", + `tracingTagFromResponse("test_tag", "${response.header.X-Fallback}")`, &filtertest.Context{ FRequest: &http.Request{}, FResponse: &http.Response{ @@ -95,19 +235,63 @@ func TestTracingTag(t *testing.T) { }, }, }, + "test_tag", "true", + }, { + "tag from response if condition is true", + `tracingTagFromResponseIfStatus("error", "true", 500, 599)`, + &filtertest.Context{ + FRequest: &http.Request{}, + FResponse: &http.Response{ + StatusCode: 500, + }, + }, + "error", + "true", + }, { + "tag from response if condition is true", + `tracingTagFromResponseIfStatus("error", "true", 500, 500)`, + &filtertest.Context{ + FRequest: &http.Request{}, + FResponse: &http.Response{ + StatusCode: 500, + }, + }, + "error", + "true", + }, { + "tag from response if condition is true", + `tracingTagFromResponseIfStatus("error", "true", 500, 599)`, + &filtertest.Context{ + FRequest: &http.Request{}, + FResponse: &http.Response{ + StatusCode: 599, + }, + }, + "error", + "true", + }, { + "do not tag from response if condition is false", + `tracingTagFromResponseIfStatus("error", "true", 500, 599)`, + &filtertest.Context{ + FRequest: &http.Request{}, + FResponse: &http.Response{ + StatusCode: 499, + }, + }, + "error", + nil, }, { "tag from missing header", - NewTag(), - "${request.header.missing}", + `tracingTag("test_tag", "${request.header.missing}")`, &filtertest.Context{ FRequest: &http.Request{}, }, + "test_tag", nil, }, { "tracingTag is not processed on response", - NewTag(), - "${response.header.X-Fallback}", + `tracingTag("test_tag", "${request.header.X-Fallback}")`, &filtertest.Context{ FRequest: &http.Request{}, FResponse: &http.Response{ @@ -116,6 +300,7 @@ func TestTracingTag(t *testing.T) { }, }, }, + "test_tag", nil, }, } { @@ -127,9 +312,20 @@ func TestTracingTag(t *testing.T) { FRequest: ti.context.FRequest.WithContext(opentracing.ContextWithSpan(ti.context.FRequest.Context(), span)), } - f, err := ti.spec.CreateFilter([]interface{}{"test_tag", ti.value}) + fEskip := eskip.MustParseFilters(ti.doc)[0] + + fr := make(filters.Registry) + fr.Register(NewTag()) + fr.Register(NewTagFromResponse()) + fr.Register(NewTagFromResponseIfStatus()) + + spec, ok := fr[fEskip.Name] + if !ok { + t.Fatalf("Failed to find filter spec: %q", fEskip.Name) + } + f, err := spec.CreateFilter(fEskip.Args) if err != nil { - t.Fatal(err) + t.Fatalf("Failed to create filter %q with %v: %v", fEskip.Name, fEskip.Args, err) } f.Request(requestContext) @@ -138,8 +334,8 @@ func TestTracingTag(t *testing.T) { f.Response(requestContext) - if got := span.Tag("test_tag"); got != ti.expected { - t.Errorf("unexpected tag value '%v' != '%v'", got, ti.expected) + if got := span.Tag(ti.tagName); got != ti.expected { + t.Errorf("unexpected tag %q value '%v' != '%v'", ti.tagName, got, ti.expected) } }) }