From 02a0c6d1286dc784bad7b7ed19d9e0ff848c52a3 Mon Sep 17 00:00:00 2001
From: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
Date: Wed, 20 Sep 2023 13:34:17 +0200
Subject: [PATCH 1/8] new filter tracingTagOnResponse

This change introduces a new filter tracingTagOnResponse that works just like the tracingTag filter, but is applied only after ther request has been processed. This allows using properties of the response (a header value) in the tag.

The use case for this is that users might want to consider an operation as failed even if it technically succeeds, e.g. because fallbacks were used. With this change, a response header can be sent, e.g. `"X-Fallback": "true"`, and then the filter `tracingTag("error", "${response.header.X-Fallback}")` would result in the span being marked as error.

Another use cases could be that metrics should be captured by use case, but the use case is not apparent from the request and instead only defined by the result of the request processing.

Signed-off-by: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
---
 docs/reference/filters.md             |  22 ++++++
 filters/filters.go                    |   1 +
 filters/tracing/tagonresponse.go      |  59 ++++++++++++++
 filters/tracing/tagonresponse_test.go | 110 ++++++++++++++++++++++++++
 4 files changed, 192 insertions(+)
 create mode 100644 filters/tracing/tagonresponse.go
 create mode 100644 filters/tracing/tagonresponse_test.go

diff --git a/docs/reference/filters.md b/docs/reference/filters.md
index 3ef8cfa4a8..f38935526f 100644
--- a/docs/reference/filters.md
+++ b/docs/reference/filters.md
@@ -3013,6 +3013,28 @@ Example: Set tag from request header
 tracingTag("http.flow_id", "${request.header.X-Flow-Id}")
 ```
 
+### tracingTagOnResponse
+
+This filter adds an opentracing tag upon completing the operation.
+
+Syntax:
+```
+tracingTagOnResponse("<tag_name>", "<tag_value>")
+```
+
+Tag value may contain [template placeholders](#template-placeholders).
+If a template placeholder can't be resolved then filter does not set the tag.
+
+Example: Adding the below filter will add a tag named `foo` with the value `bar`.
+```
+tracingTagOnResponse("foo", "bar")
+```
+
+Example: Set tag from response header
+```
+tracingTagOnResponse("error", "${response.header.X-Functional-Error}")
+```
+
 ### tracingSpanName
 
 This filter sets the name of the outgoing (client span) in opentracing. The default name is "proxy". Example:
diff --git a/filters/filters.go b/filters/filters.go
index 33d89ad731..de84116112 100644
--- a/filters/filters.go
+++ b/filters/filters.go
@@ -335,6 +335,7 @@ const (
 	TracingBaggageToTagName                    = "tracingBaggageToTag"
 	StateBagToTagName                          = "stateBagToTag"
 	TracingTagName                             = "tracingTag"
+	TracingTagOnResponseName                   = "tracingTagOnResponse"
 	TracingSpanNameName                        = "tracingSpanName"
 	OriginMarkerName                           = "originMarker"
 	FadeInName                                 = "fadeIn"
diff --git a/filters/tracing/tagonresponse.go b/filters/tracing/tagonresponse.go
new file mode 100644
index 0000000000..33c2109371
--- /dev/null
+++ b/filters/tracing/tagonresponse.go
@@ -0,0 +1,59 @@
+package tracing
+
+import (
+	"github.com/opentracing/opentracing-go"
+	"github.com/zalando/skipper/eskip"
+	"github.com/zalando/skipper/filters"
+)
+
+type tagOnResponseSpec struct {
+}
+
+type tagOnResponseFilter struct {
+	tagName  string
+	tagValue *eskip.Template
+}
+
+// NewTagOnResponse creates a filter specification for the tracingTagOnResponse filter.
+func NewTagOnResponse() filters.Spec {
+	return tagOnResponseSpec{}
+}
+
+func (s tagOnResponseSpec) Name() string {
+	return filters.TracingTagOnResponseName
+}
+
+func (s tagOnResponseSpec) CreateFilter(args []interface{}) (filters.Filter, error) {
+	if len(args) != 2 {
+		return nil, filters.ErrInvalidFilterParameters
+	}
+
+	tagName, ok := args[0].(string)
+	if !ok {
+		return nil, filters.ErrInvalidFilterParameters
+	}
+
+	tagValue, ok := args[1].(string)
+	if !ok {
+		return nil, filters.ErrInvalidFilterParameters
+	}
+
+	return tagOnResponseFilter{
+		tagName:  tagName,
+		tagValue: eskip.NewTemplate(tagValue),
+	}, nil
+}
+
+func (f tagOnResponseFilter) Request(ctx filters.FilterContext) {}
+
+func (f tagOnResponseFilter) Response(ctx filters.FilterContext) {
+	req := ctx.Request()
+	span := opentracing.SpanFromContext(req.Context())
+	if span == nil {
+		return
+	}
+
+	if v, ok := f.tagValue.ApplyContext(ctx); ok {
+		span.SetTag(f.tagName, v)
+	}
+}
diff --git a/filters/tracing/tagonresponse_test.go b/filters/tracing/tagonresponse_test.go
new file mode 100644
index 0000000000..d108d13fa3
--- /dev/null
+++ b/filters/tracing/tagonresponse_test.go
@@ -0,0 +1,110 @@
+package tracing
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/opentracing/opentracing-go"
+	"github.com/opentracing/opentracing-go/mocktracer"
+	"github.com/zalando/skipper/filters"
+	"github.com/zalando/skipper/filters/filtertest"
+)
+
+func TestTracingTagOnResponseNil(t *testing.T) {
+	context := &filtertest.Context{
+		FRequest: &http.Request{},
+	}
+	context.FRequest = context.FRequest.WithContext(opentracing.ContextWithSpan(context.FRequest.Context(), nil))
+
+	s := NewTagOnResponse()
+	f, err := s.CreateFilter([]interface{}{"test_tag", "foo"})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	f.Request(context)
+
+	span := opentracing.SpanFromContext(context.Request().Context())
+	if span != nil {
+		t.Errorf("span should be nil, but is '%v'", span)
+	}
+}
+
+func TestTracingTagOnResponseTagName(t *testing.T) {
+	if (tagOnResponseSpec{}).Name() != filters.TracingTagOnResponseName {
+		t.Error("Wrong tag spec name")
+	}
+}
+func TestTracingTagOnResponseCreateFilter(t *testing.T) {
+	spec := tagOnResponseSpec{}
+	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")
+	}
+
+	if _, err := spec.CreateFilter([]interface{}{"foo", 3}); err != filters.ErrInvalidFilterParameters {
+		t.Errorf("Create filter without second arg is string should return error")
+	}
+}
+
+func TestTracingTagOnResponseTag(t *testing.T) {
+	tracer := mocktracer.New()
+
+	for _, ti := range []struct {
+		name     string
+		value    string
+		context  *filtertest.Context
+		expected interface{}
+	}{{
+		"plain key value",
+		"test_value",
+		&filtertest.Context{
+			FResponse: &http.Response{},
+		},
+		"test_value",
+	}, {
+		"tag from header",
+		"${response.header.X-Fallback}",
+		&filtertest.Context{
+			FRequest: &http.Request{},
+			FResponse: &http.Response{
+				Header: http.Header{
+					"X-Fallback": []string{"true"},
+				},
+			},
+		},
+		"true",
+	}, {
+		"tag from missing header",
+		"${response.header.missing}",
+		&filtertest.Context{
+			FResponse: &http.Response{},
+		},
+		nil,
+	},
+	} {
+		t.Run(ti.name, func(t *testing.T) {
+			span := tracer.StartSpan("proxy").(*mocktracer.MockSpan)
+			defer span.Finish()
+
+			ti.context.FRequest = ti.context.FRequest.WithContext(opentracing.ContextWithSpan(ti.context.FRequest.Context(), span))
+
+			s := NewTagOnResponse()
+			f, err := s.CreateFilter([]interface{}{"test_tag", ti.value})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			f.Request(ti.context)
+
+			if got := span.Tag("test_tag"); got != ti.expected {
+				t.Errorf("unexpected tag value '%v' != '%v'", got, ti.expected)
+			}
+
+			f.Response(ti.context)
+		})
+	}
+}

From 40a2214c67cd6f7d8ae4a0caf387ace1f1606ba8 Mon Sep 17 00:00:00 2001
From: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
Date: Wed, 20 Sep 2023 14:02:06 +0200
Subject: [PATCH 2/8] fix tests in tagonresponse_test.go

In the test, the span tag was obtained before the response filter method was executed, resulting in no tags being found. This commit moves obtaining the tags to after the incovaction of the response filter method.

Signed-off-by: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
---
 filters/tracing/tagonresponse.go      | 4 ++--
 filters/tracing/tagonresponse_test.go | 6 ++++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/filters/tracing/tagonresponse.go b/filters/tracing/tagonresponse.go
index 33c2109371..ce467a420a 100644
--- a/filters/tracing/tagonresponse.go
+++ b/filters/tracing/tagonresponse.go
@@ -1,7 +1,7 @@
 package tracing
 
 import (
-	"github.com/opentracing/opentracing-go"
+	opentracing "github.com/opentracing/opentracing-go"
 	"github.com/zalando/skipper/eskip"
 	"github.com/zalando/skipper/filters"
 )
@@ -44,7 +44,7 @@ func (s tagOnResponseSpec) CreateFilter(args []interface{}) (filters.Filter, err
 	}, nil
 }
 
-func (f tagOnResponseFilter) Request(ctx filters.FilterContext) {}
+func (f tagOnResponseFilter) Request(filters.FilterContext) {}
 
 func (f tagOnResponseFilter) Response(ctx filters.FilterContext) {
 	req := ctx.Request()
diff --git a/filters/tracing/tagonresponse_test.go b/filters/tracing/tagonresponse_test.go
index d108d13fa3..9757a720c1 100644
--- a/filters/tracing/tagonresponse_test.go
+++ b/filters/tracing/tagonresponse_test.go
@@ -62,6 +62,7 @@ func TestTracingTagOnResponseTag(t *testing.T) {
 		"plain key value",
 		"test_value",
 		&filtertest.Context{
+			FRequest:  &http.Request{},
 			FResponse: &http.Response{},
 		},
 		"test_value",
@@ -81,6 +82,7 @@ func TestTracingTagOnResponseTag(t *testing.T) {
 		"tag from missing header",
 		"${response.header.missing}",
 		&filtertest.Context{
+			FRequest:  &http.Request{},
 			FResponse: &http.Response{},
 		},
 		nil,
@@ -100,11 +102,11 @@ func TestTracingTagOnResponseTag(t *testing.T) {
 
 			f.Request(ti.context)
 
+			f.Response(ti.context)
+
 			if got := span.Tag("test_tag"); got != ti.expected {
 				t.Errorf("unexpected tag value '%v' != '%v'", got, ti.expected)
 			}
-
-			f.Response(ti.context)
 		})
 	}
 }

From 10ba4b6b2348ed1c03e0274c5c29e6b53c38eeaf Mon Sep 17 00:00:00 2001
From: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
Date: Thu, 21 Sep 2023 08:26:15 +0200
Subject: [PATCH 3/8] review comments: add tracingTagOnResponse to Filters();
 use pointers

Signed-off-by: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>

response
---
 filters/builtin/builtin.go            |  1 +
 filters/tracing/tagonresponse.go      | 12 ++++++------
 filters/tracing/tagonresponse_test.go |  2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/filters/builtin/builtin.go b/filters/builtin/builtin.go
index a314ab6ce5..ba651aeacb 100644
--- a/filters/builtin/builtin.go
+++ b/filters/builtin/builtin.go
@@ -208,6 +208,7 @@ func Filters() []filters.Spec {
 		tracing.NewBaggageToTagFilter(),
 		tracing.NewTag(),
 		tracing.NewStateBagToTag(),
+		tracing.NewTagOnResponse(),
 		//lint:ignore SA1019 due to backward compatibility
 		accesslog.NewAccessLogDisabled(),
 		accesslog.NewDisableAccessLog(),
diff --git a/filters/tracing/tagonresponse.go b/filters/tracing/tagonresponse.go
index ce467a420a..13ab28f1ce 100644
--- a/filters/tracing/tagonresponse.go
+++ b/filters/tracing/tagonresponse.go
@@ -16,14 +16,14 @@ type tagOnResponseFilter struct {
 
 // NewTagOnResponse creates a filter specification for the tracingTagOnResponse filter.
 func NewTagOnResponse() filters.Spec {
-	return tagOnResponseSpec{}
+	return &tagOnResponseSpec{}
 }
 
-func (s tagOnResponseSpec) Name() string {
+func (s *tagOnResponseSpec) Name() string {
 	return filters.TracingTagOnResponseName
 }
 
-func (s tagOnResponseSpec) CreateFilter(args []interface{}) (filters.Filter, error) {
+func (s *tagOnResponseSpec) CreateFilter(args []interface{}) (filters.Filter, error) {
 	if len(args) != 2 {
 		return nil, filters.ErrInvalidFilterParameters
 	}
@@ -38,15 +38,15 @@ func (s tagOnResponseSpec) CreateFilter(args []interface{}) (filters.Filter, err
 		return nil, filters.ErrInvalidFilterParameters
 	}
 
-	return tagOnResponseFilter{
+	return &tagOnResponseFilter{
 		tagName:  tagName,
 		tagValue: eskip.NewTemplate(tagValue),
 	}, nil
 }
 
-func (f tagOnResponseFilter) Request(filters.FilterContext) {}
+func (f *tagOnResponseFilter) Request(filters.FilterContext) {}
 
-func (f tagOnResponseFilter) Response(ctx filters.FilterContext) {
+func (f *tagOnResponseFilter) Response(ctx filters.FilterContext) {
 	req := ctx.Request()
 	span := opentracing.SpanFromContext(req.Context())
 	if span == nil {
diff --git a/filters/tracing/tagonresponse_test.go b/filters/tracing/tagonresponse_test.go
index 9757a720c1..84a65c3d4c 100644
--- a/filters/tracing/tagonresponse_test.go
+++ b/filters/tracing/tagonresponse_test.go
@@ -31,7 +31,7 @@ func TestTracingTagOnResponseNil(t *testing.T) {
 }
 
 func TestTracingTagOnResponseTagName(t *testing.T) {
-	if (tagOnResponseSpec{}).Name() != filters.TracingTagOnResponseName {
+	if (&tagOnResponseSpec{}).Name() != filters.TracingTagOnResponseName {
 		t.Error("Wrong tag spec name")
 	}
 }

From dd36cec0a84e2540209ec2926c54db5fce4e3440 Mon Sep 17 00:00:00 2001
From: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
Date: Thu, 21 Sep 2023 10:55:06 +0200
Subject: [PATCH 4/8] review comments: code cleanup; use pointers also in
 tag.go

Signed-off-by: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
---
 filters/tracing/tag.go           | 13 ++++++-------
 filters/tracing/tag_test.go      |  2 +-
 filters/tracing/tagonresponse.go |  3 +--
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/filters/tracing/tag.go b/filters/tracing/tag.go
index 72672c682c..038b23298e 100644
--- a/filters/tracing/tag.go
+++ b/filters/tracing/tag.go
@@ -16,10 +16,10 @@ type tagFilter struct {
 
 // NewTag creates a filter specification for the tracingTag filter.
 func NewTag() filters.Spec {
-	return tagSpec{}
+	return &tagSpec{}
 }
 
-func (s tagSpec) Name() string {
+func (s *tagSpec) Name() string {
 	return filters.TracingTagName
 }
 
@@ -38,15 +38,14 @@ func (s tagSpec) CreateFilter(args []interface{}) (filters.Filter, error) {
 		return nil, filters.ErrInvalidFilterParameters
 	}
 
-	return tagFilter{
+	return &tagFilter{
 		tagName:  tagName,
 		tagValue: eskip.NewTemplate(tagValue),
 	}, nil
 }
 
-func (f tagFilter) Request(ctx filters.FilterContext) {
-	req := ctx.Request()
-	span := opentracing.SpanFromContext(req.Context())
+func (f *tagFilter) Request(ctx filters.FilterContext) {
+	span := opentracing.SpanFromContext(ctx.Request().Context())
 	if span == nil {
 		return
 	}
@@ -56,4 +55,4 @@ func (f tagFilter) Request(ctx filters.FilterContext) {
 	}
 }
 
-func (f tagFilter) Response(filters.FilterContext) {}
+func (f *tagFilter) Response(filters.FilterContext) {}
diff --git a/filters/tracing/tag_test.go b/filters/tracing/tag_test.go
index e2743717a4..5b8eef2781 100644
--- a/filters/tracing/tag_test.go
+++ b/filters/tracing/tag_test.go
@@ -31,7 +31,7 @@ func TestTracingTagNil(t *testing.T) {
 }
 
 func TestTagName(t *testing.T) {
-	if (tagSpec{}).Name() != filters.TracingTagName {
+	if (&tagSpec{}).Name() != filters.TracingTagName {
 		t.Error("Wrong tag spec name")
 	}
 }
diff --git a/filters/tracing/tagonresponse.go b/filters/tracing/tagonresponse.go
index 13ab28f1ce..9b2397f458 100644
--- a/filters/tracing/tagonresponse.go
+++ b/filters/tracing/tagonresponse.go
@@ -47,8 +47,7 @@ func (s *tagOnResponseSpec) CreateFilter(args []interface{}) (filters.Filter, er
 func (f *tagOnResponseFilter) Request(filters.FilterContext) {}
 
 func (f *tagOnResponseFilter) Response(ctx filters.FilterContext) {
-	req := ctx.Request()
-	span := opentracing.SpanFromContext(req.Context())
+	span := opentracing.SpanFromContext(ctx.Request().Context())
 	if span == nil {
 		return
 	}

From 3b9f672bf4ecd3d53b72baab7e75ed6c98f894f0 Mon Sep 17 00:00:00 2001
From: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
Date: Thu, 21 Sep 2023 15:33:48 +0200
Subject: [PATCH 5/8] refactoring of tracingTagOnResponse filter

This commit refactors the tracingTagOnResponse filter to use the same class as the tracingTag filter. It follows the same pattern as histogram.go.

Also, the filter is renamed to tracingTagFromResponse and the documentation simplified.

Signed-off-by: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
---
 docs/reference/filters.md             |  26 +-----
 filters/builtin/builtin.go            |   2 +-
 filters/filters.go                    |   2 +-
 filters/tracing/tag.go                |  43 +++++++---
 filters/tracing/tag_test.go           |  62 ++++++++++++--
 filters/tracing/tagonresponse.go      |  58 -------------
 filters/tracing/tagonresponse_test.go | 112 --------------------------
 7 files changed, 96 insertions(+), 209 deletions(-)
 delete mode 100644 filters/tracing/tagonresponse.go
 delete mode 100644 filters/tracing/tagonresponse_test.go

diff --git a/docs/reference/filters.md b/docs/reference/filters.md
index f38935526f..1fde77fe41 100644
--- a/docs/reference/filters.md
+++ b/docs/reference/filters.md
@@ -1696,8 +1696,8 @@ Given following example ID token:
   "email": "someone@example.org",
   "groups": [
     "CD-xyz",
-    "appX-Test-Users"
-    "Purchasing-Department",
+    "appX-Test-Users",
+    "Purchasing-Department"
   ],
   "name": "Some One"
 }
@@ -3013,27 +3013,9 @@ Example: Set tag from request header
 tracingTag("http.flow_id", "${request.header.X-Flow-Id}")
 ```
 
-### tracingTagOnResponse
+### tracingTagFromResponse
 
-This filter adds an opentracing tag upon completing the operation.
-
-Syntax:
-```
-tracingTagOnResponse("<tag_name>", "<tag_value>")
-```
-
-Tag value may contain [template placeholders](#template-placeholders).
-If a template placeholder can't be resolved then filter does not set the tag.
-
-Example: Adding the below filter will add a tag named `foo` with the value `bar`.
-```
-tracingTagOnResponse("foo", "bar")
-```
-
-Example: Set tag from response header
-```
-tracingTagOnResponse("error", "${response.header.X-Functional-Error}")
-```
+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. 
 
 ### tracingSpanName
 
diff --git a/filters/builtin/builtin.go b/filters/builtin/builtin.go
index ba651aeacb..066de2574a 100644
--- a/filters/builtin/builtin.go
+++ b/filters/builtin/builtin.go
@@ -208,7 +208,7 @@ func Filters() []filters.Spec {
 		tracing.NewBaggageToTagFilter(),
 		tracing.NewTag(),
 		tracing.NewStateBagToTag(),
-		tracing.NewTagOnResponse(),
+		tracing.NewTagFromResponse(),
 		//lint:ignore SA1019 due to backward compatibility
 		accesslog.NewAccessLogDisabled(),
 		accesslog.NewDisableAccessLog(),
diff --git a/filters/filters.go b/filters/filters.go
index de84116112..6caaaa7a41 100644
--- a/filters/filters.go
+++ b/filters/filters.go
@@ -335,7 +335,7 @@ const (
 	TracingBaggageToTagName                    = "tracingBaggageToTag"
 	StateBagToTagName                          = "stateBagToTag"
 	TracingTagName                             = "tracingTag"
-	TracingTagOnResponseName                   = "tracingTagOnResponse"
+	TracingTagFromResponseName                 = "tracingTagFromResponse"
 	TracingSpanNameName                        = "tracingSpanName"
 	OriginMarkerName                           = "originMarker"
 	FadeInName                                 = "fadeIn"
diff --git a/filters/tracing/tag.go b/filters/tracing/tag.go
index 038b23298e..f01b1641ba 100644
--- a/filters/tracing/tag.go
+++ b/filters/tracing/tag.go
@@ -7,23 +7,31 @@ import (
 )
 
 type tagSpec struct {
+	typ string
 }
 
 type tagFilter struct {
+	tagFromResponse bool
+
 	tagName  string
 	tagValue *eskip.Template
 }
 
 // NewTag creates a filter specification for the tracingTag filter.
 func NewTag() filters.Spec {
-	return &tagSpec{}
+	return &tagSpec{typ: filters.TracingTagName}
+}
+
+// NewTagFromResponse creates a filter similar to NewTag, but applies tags after the request has been processed.
+func NewTagFromResponse() filters.Spec {
+	return &tagSpec{typ: filters.TracingTagFromResponseName}
 }
 
 func (s *tagSpec) Name() string {
-	return filters.TracingTagName
+	return s.typ
 }
 
-func (s tagSpec) CreateFilter(args []interface{}) (filters.Filter, error) {
+func (s *tagSpec) CreateFilter(args []interface{}) (filters.Filter, error) {
 	if len(args) != 2 {
 		return nil, filters.ErrInvalidFilterParameters
 	}
@@ -39,20 +47,35 @@ func (s tagSpec) CreateFilter(args []interface{}) (filters.Filter, error) {
 	}
 
 	return &tagFilter{
+		tagFromResponse: s.typ == filters.TracingTagFromResponseName,
+
 		tagName:  tagName,
 		tagValue: eskip.NewTemplate(tagValue),
 	}, nil
 }
 
 func (f *tagFilter) Request(ctx filters.FilterContext) {
-	span := opentracing.SpanFromContext(ctx.Request().Context())
-	if span == nil {
-		return
-	}
+	if !f.tagFromResponse {
+		span := opentracing.SpanFromContext(ctx.Request().Context())
+		if span == nil {
+			return
+		}
 
-	if v, ok := f.tagValue.ApplyContext(ctx); ok {
-		span.SetTag(f.tagName, v)
+		if v, ok := f.tagValue.ApplyContext(ctx); ok {
+			span.SetTag(f.tagName, v)
+		}
 	}
 }
 
-func (f *tagFilter) Response(filters.FilterContext) {}
+func (f *tagFilter) Response(ctx filters.FilterContext) {
+	if f.tagFromResponse {
+		span := opentracing.SpanFromContext(ctx.Request().Context())
+		if span == nil {
+			return
+		}
+
+		if v, ok := f.tagValue.ApplyContext(ctx); ok {
+			span.SetTag(f.tagName, v)
+		}
+	}
+}
diff --git a/filters/tracing/tag_test.go b/filters/tracing/tag_test.go
index 5b8eef2781..fc1686478e 100644
--- a/filters/tracing/tag_test.go
+++ b/filters/tracing/tag_test.go
@@ -31,7 +31,14 @@ func TestTracingTagNil(t *testing.T) {
 }
 
 func TestTagName(t *testing.T) {
-	if (&tagSpec{}).Name() != filters.TracingTagName {
+	if (&tagSpec{
+		typ: filters.TracingTagName,
+	}).Name() != filters.TracingTagName {
+		t.Error("Wrong tag spec name")
+	}
+	if (&tagSpec{
+		typ: filters.TracingTagFromResponseName,
+	}).Name() != filters.TracingTagFromResponseName {
 		t.Error("Wrong tag spec name")
 	}
 }
@@ -55,11 +62,13 @@ func TestTracingTag(t *testing.T) {
 
 	for _, ti := range []struct {
 		name     string
+		filter   filters.Spec
 		value    string
 		context  *filtertest.Context
 		expected interface{}
 	}{{
 		"plain key value",
+		NewTag(),
 		"test_value",
 		&filtertest.Context{
 			FRequest: &http.Request{},
@@ -67,6 +76,7 @@ func TestTracingTag(t *testing.T) {
 		"test_value",
 	}, {
 		"tag from header",
+		NewTag(),
 		"${request.header.X-Flow-Id}",
 		&filtertest.Context{
 			FRequest: &http.Request{
@@ -76,8 +86,22 @@ func TestTracingTag(t *testing.T) {
 			},
 		},
 		"foo",
+	}, {
+		"tag from response",
+		NewTagFromResponse(),
+		"${response.header.X-Fallback}",
+		&filtertest.Context{
+			FRequest: &http.Request{},
+			FResponse: &http.Response{
+				Header: http.Header{
+					"X-Fallback": []string{"true"},
+				},
+			},
+		},
+		"true",
 	}, {
 		"tag from missing header",
+		NewTag(),
 		"${request.header.missing}",
 		&filtertest.Context{
 			FRequest: &http.Request{},
@@ -91,19 +115,47 @@ func TestTracingTag(t *testing.T) {
 
 			ti.context.FRequest = ti.context.FRequest.WithContext(opentracing.ContextWithSpan(ti.context.FRequest.Context(), span))
 
-			s := NewTag()
-			f, err := s.CreateFilter([]interface{}{"test_tag", ti.value})
+			f, err := ti.filter.CreateFilter([]interface{}{"test_tag", ti.value})
 			if err != nil {
 				t.Fatal(err)
 			}
 
 			f.Request(ti.context)
+			f.Response(ti.context)
 
 			if got := span.Tag("test_tag"); got != ti.expected {
 				t.Errorf("unexpected tag value '%v' != '%v'", got, ti.expected)
 			}
-
-			f.Response(ti.context)
 		})
 	}
 }
+
+func TestTagFilterIgnoresResponse(t *testing.T) {
+	tracer := mocktracer.New()
+	span := tracer.StartSpan("proxy").(*mocktracer.MockSpan)
+	defer span.Finish()
+
+	requestContext := &filtertest.Context{
+		FRequest: &http.Request{},
+	}
+	requestContext.FRequest = requestContext.FRequest.WithContext(opentracing.ContextWithSpan(requestContext.FRequest.Context(), span))
+
+	f, err := NewTag().CreateFilter([]interface{}{"test_tag", "${response.header.X-Fallback}"})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	f.Request(requestContext)
+
+	requestContext.FResponse = &http.Response{
+		Header: http.Header{
+			"X-Fallback": []string{"true"},
+		},
+	}
+
+	f.Response(requestContext)
+
+	if got := span.Tag("test_tag"); got != nil {
+		t.Errorf("unexpected tag value '%v' != '%v'", got, nil)
+	}
+}
diff --git a/filters/tracing/tagonresponse.go b/filters/tracing/tagonresponse.go
deleted file mode 100644
index 9b2397f458..0000000000
--- a/filters/tracing/tagonresponse.go
+++ /dev/null
@@ -1,58 +0,0 @@
-package tracing
-
-import (
-	opentracing "github.com/opentracing/opentracing-go"
-	"github.com/zalando/skipper/eskip"
-	"github.com/zalando/skipper/filters"
-)
-
-type tagOnResponseSpec struct {
-}
-
-type tagOnResponseFilter struct {
-	tagName  string
-	tagValue *eskip.Template
-}
-
-// NewTagOnResponse creates a filter specification for the tracingTagOnResponse filter.
-func NewTagOnResponse() filters.Spec {
-	return &tagOnResponseSpec{}
-}
-
-func (s *tagOnResponseSpec) Name() string {
-	return filters.TracingTagOnResponseName
-}
-
-func (s *tagOnResponseSpec) CreateFilter(args []interface{}) (filters.Filter, error) {
-	if len(args) != 2 {
-		return nil, filters.ErrInvalidFilterParameters
-	}
-
-	tagName, ok := args[0].(string)
-	if !ok {
-		return nil, filters.ErrInvalidFilterParameters
-	}
-
-	tagValue, ok := args[1].(string)
-	if !ok {
-		return nil, filters.ErrInvalidFilterParameters
-	}
-
-	return &tagOnResponseFilter{
-		tagName:  tagName,
-		tagValue: eskip.NewTemplate(tagValue),
-	}, nil
-}
-
-func (f *tagOnResponseFilter) Request(filters.FilterContext) {}
-
-func (f *tagOnResponseFilter) Response(ctx filters.FilterContext) {
-	span := opentracing.SpanFromContext(ctx.Request().Context())
-	if span == nil {
-		return
-	}
-
-	if v, ok := f.tagValue.ApplyContext(ctx); ok {
-		span.SetTag(f.tagName, v)
-	}
-}
diff --git a/filters/tracing/tagonresponse_test.go b/filters/tracing/tagonresponse_test.go
deleted file mode 100644
index 84a65c3d4c..0000000000
--- a/filters/tracing/tagonresponse_test.go
+++ /dev/null
@@ -1,112 +0,0 @@
-package tracing
-
-import (
-	"net/http"
-	"testing"
-
-	"github.com/opentracing/opentracing-go"
-	"github.com/opentracing/opentracing-go/mocktracer"
-	"github.com/zalando/skipper/filters"
-	"github.com/zalando/skipper/filters/filtertest"
-)
-
-func TestTracingTagOnResponseNil(t *testing.T) {
-	context := &filtertest.Context{
-		FRequest: &http.Request{},
-	}
-	context.FRequest = context.FRequest.WithContext(opentracing.ContextWithSpan(context.FRequest.Context(), nil))
-
-	s := NewTagOnResponse()
-	f, err := s.CreateFilter([]interface{}{"test_tag", "foo"})
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	f.Request(context)
-
-	span := opentracing.SpanFromContext(context.Request().Context())
-	if span != nil {
-		t.Errorf("span should be nil, but is '%v'", span)
-	}
-}
-
-func TestTracingTagOnResponseTagName(t *testing.T) {
-	if (&tagOnResponseSpec{}).Name() != filters.TracingTagOnResponseName {
-		t.Error("Wrong tag spec name")
-	}
-}
-func TestTracingTagOnResponseCreateFilter(t *testing.T) {
-	spec := tagOnResponseSpec{}
-	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")
-	}
-
-	if _, err := spec.CreateFilter([]interface{}{"foo", 3}); err != filters.ErrInvalidFilterParameters {
-		t.Errorf("Create filter without second arg is string should return error")
-	}
-}
-
-func TestTracingTagOnResponseTag(t *testing.T) {
-	tracer := mocktracer.New()
-
-	for _, ti := range []struct {
-		name     string
-		value    string
-		context  *filtertest.Context
-		expected interface{}
-	}{{
-		"plain key value",
-		"test_value",
-		&filtertest.Context{
-			FRequest:  &http.Request{},
-			FResponse: &http.Response{},
-		},
-		"test_value",
-	}, {
-		"tag from header",
-		"${response.header.X-Fallback}",
-		&filtertest.Context{
-			FRequest: &http.Request{},
-			FResponse: &http.Response{
-				Header: http.Header{
-					"X-Fallback": []string{"true"},
-				},
-			},
-		},
-		"true",
-	}, {
-		"tag from missing header",
-		"${response.header.missing}",
-		&filtertest.Context{
-			FRequest:  &http.Request{},
-			FResponse: &http.Response{},
-		},
-		nil,
-	},
-	} {
-		t.Run(ti.name, func(t *testing.T) {
-			span := tracer.StartSpan("proxy").(*mocktracer.MockSpan)
-			defer span.Finish()
-
-			ti.context.FRequest = ti.context.FRequest.WithContext(opentracing.ContextWithSpan(ti.context.FRequest.Context(), span))
-
-			s := NewTagOnResponse()
-			f, err := s.CreateFilter([]interface{}{"test_tag", ti.value})
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			f.Request(ti.context)
-
-			f.Response(ti.context)
-
-			if got := span.Tag("test_tag"); got != ti.expected {
-				t.Errorf("unexpected tag value '%v' != '%v'", got, ti.expected)
-			}
-		})
-	}
-}

From 6527fce4c418735f758d176cf734b28913c171b7 Mon Sep 17 00:00:00 2001
From: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
Date: Thu, 21 Sep 2023 16:16:05 +0200
Subject: [PATCH 6/8] review comments: some cleanup; one more test

Signed-off-by: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
---
 filters/builtin/builtin.go  |  2 +-
 filters/tracing/tag.go      | 29 ++++++++++-----------
 filters/tracing/tag_test.go | 51 +++++++++++++++++++++++++++++++------
 3 files changed, 57 insertions(+), 25 deletions(-)

diff --git a/filters/builtin/builtin.go b/filters/builtin/builtin.go
index 066de2574a..0609d87412 100644
--- a/filters/builtin/builtin.go
+++ b/filters/builtin/builtin.go
@@ -207,8 +207,8 @@ func Filters() []filters.Spec {
 		tracing.NewSpanName(),
 		tracing.NewBaggageToTagFilter(),
 		tracing.NewTag(),
-		tracing.NewStateBagToTag(),
 		tracing.NewTagFromResponse(),
+		tracing.NewStateBagToTag(),
 		//lint:ignore SA1019 due to backward compatibility
 		accesslog.NewAccessLogDisabled(),
 		accesslog.NewDisableAccessLog(),
diff --git a/filters/tracing/tag.go b/filters/tracing/tag.go
index f01b1641ba..b80de4bea0 100644
--- a/filters/tracing/tag.go
+++ b/filters/tracing/tag.go
@@ -56,26 +56,23 @@ func (s *tagSpec) CreateFilter(args []interface{}) (filters.Filter, error) {
 
 func (f *tagFilter) Request(ctx filters.FilterContext) {
 	if !f.tagFromResponse {
-		span := opentracing.SpanFromContext(ctx.Request().Context())
-		if span == nil {
-			return
-		}
-
-		if v, ok := f.tagValue.ApplyContext(ctx); ok {
-			span.SetTag(f.tagName, v)
-		}
+		f.setTag(ctx)
 	}
 }
 
 func (f *tagFilter) Response(ctx filters.FilterContext) {
 	if f.tagFromResponse {
-		span := opentracing.SpanFromContext(ctx.Request().Context())
-		if span == nil {
-			return
-		}
-
-		if v, ok := f.tagValue.ApplyContext(ctx); ok {
-			span.SetTag(f.tagName, v)
-		}
+		f.setTag(ctx)
+	}
+}
+
+func (f *tagFilter) setTag(ctx filters.FilterContext) {
+	span := opentracing.SpanFromContext(ctx.Request().Context())
+	if span == nil {
+		return
+	}
+
+	if v, ok := f.tagValue.ApplyContext(ctx); ok {
+		span.SetTag(f.tagName, v)
 	}
 }
diff --git a/filters/tracing/tag_test.go b/filters/tracing/tag_test.go
index fc1686478e..3ba27842b8 100644
--- a/filters/tracing/tag_test.go
+++ b/filters/tracing/tag_test.go
@@ -31,17 +31,14 @@ func TestTracingTagNil(t *testing.T) {
 }
 
 func TestTagName(t *testing.T) {
-	if (&tagSpec{
-		typ: filters.TracingTagName,
-	}).Name() != filters.TracingTagName {
+	if NewTag().Name() != filters.TracingTagName {
 		t.Error("Wrong tag spec name")
 	}
-	if (&tagSpec{
-		typ: filters.TracingTagFromResponseName,
-	}).Name() != filters.TracingTagFromResponseName {
+	if NewTagFromResponse().Name() != filters.TracingTagFromResponseName {
 		t.Error("Wrong tag spec name")
 	}
 }
+
 func TestTagCreateFilter(t *testing.T) {
 	spec := tagSpec{}
 	if _, err := spec.CreateFilter(nil); err != filters.ErrInvalidFilterParameters {
@@ -62,7 +59,7 @@ func TestTracingTag(t *testing.T) {
 
 	for _, ti := range []struct {
 		name     string
-		filter   filters.Spec
+		spec     filters.Spec
 		value    string
 		context  *filtertest.Context
 		expected interface{}
@@ -115,7 +112,7 @@ func TestTracingTag(t *testing.T) {
 
 			ti.context.FRequest = ti.context.FRequest.WithContext(opentracing.ContextWithSpan(ti.context.FRequest.Context(), span))
 
-			f, err := ti.filter.CreateFilter([]interface{}{"test_tag", ti.value})
+			f, err := ti.spec.CreateFilter([]interface{}{"test_tag", ti.value})
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -159,3 +156,41 @@ func TestTagFilterIgnoresResponse(t *testing.T) {
 		t.Errorf("unexpected tag value '%v' != '%v'", got, nil)
 	}
 }
+
+func TestTagFromResponseFilterIgnoresRequest(t *testing.T) {
+	tracer := mocktracer.New()
+	span := tracer.StartSpan("proxy").(*mocktracer.MockSpan)
+	defer span.Finish()
+
+	requestContext := &filtertest.Context{
+		FRequest: &http.Request{
+			Header: http.Header{
+				"X-Flow-Id": []string{"a-flow-id"},
+			},
+		},
+	}
+	requestContext.FRequest = requestContext.FRequest.WithContext(opentracing.ContextWithSpan(requestContext.FRequest.Context(), span))
+
+	f, err := NewTagFromResponse().CreateFilter([]interface{}{"test_tag", "${request.header.X-Flow-Id}"})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	f.Request(requestContext)
+
+	responseContext := &filtertest.Context{
+		FRequest: &http.Request{},
+		FResponse: &http.Response{
+			Header: http.Header{
+				"X-Flow-Id": []string{"a-different-flow-id"},
+			},
+		},
+	}
+	responseContext.FRequest.WithContext(opentracing.ContextWithSpan(responseContext.FRequest.Context(), span))
+
+	f.Response(responseContext)
+
+	if got := span.Tag("test_tag"); got != nil {
+		t.Errorf("unexpected tag value '%v' != '%v'", got, nil)
+	}
+}

From 0d0d60fc074d03e30945f0a25693689179de34ad Mon Sep 17 00:00:00 2001
From: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
Date: Thu, 21 Sep 2023 16:35:48 +0200
Subject: [PATCH 7/8] review comments: some cleanup; one more test

Signed-off-by: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
---
 filters/tracing/tag_test.go | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/filters/tracing/tag_test.go b/filters/tracing/tag_test.go
index 3ba27842b8..f7823a6471 100644
--- a/filters/tracing/tag_test.go
+++ b/filters/tracing/tag_test.go
@@ -178,15 +178,8 @@ func TestTagFromResponseFilterIgnoresRequest(t *testing.T) {
 
 	f.Request(requestContext)
 
-	responseContext := &filtertest.Context{
-		FRequest: &http.Request{},
-		FResponse: &http.Response{
-			Header: http.Header{
-				"X-Flow-Id": []string{"a-different-flow-id"},
-			},
-		},
-	}
-	responseContext.FRequest.WithContext(opentracing.ContextWithSpan(responseContext.FRequest.Context(), span))
+	responseContext := &filtertest.Context{FRequest: &http.Request{}}
+	responseContext.FRequest = responseContext.FRequest.WithContext(opentracing.ContextWithSpan(responseContext.FRequest.Context(), span))
 
 	f.Response(responseContext)
 

From 5c91b840faf5782b569b32dd2868b44fcecb06a4 Mon Sep 17 00:00:00 2001
From: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
Date: Fri, 22 Sep 2023 07:31:31 +0200
Subject: [PATCH 8/8] review comments: refactor test

Signed-off-by: lukas-c-wilhelm <11819719+lukas-c-wilhelm@users.noreply.github.com>
---
 filters/tracing/tag_test.go | 85 +++++++++----------------------------
 1 file changed, 21 insertions(+), 64 deletions(-)

diff --git a/filters/tracing/tag_test.go b/filters/tracing/tag_test.go
index f7823a6471..4947e3f2fd 100644
--- a/filters/tracing/tag_test.go
+++ b/filters/tracing/tag_test.go
@@ -104,21 +104,39 @@ func TestTracingTag(t *testing.T) {
 			FRequest: &http.Request{},
 		},
 		nil,
+	}, {
+		"tracingTag is not processed on response",
+		NewTag(),
+		"${response.header.X-Fallback}",
+		&filtertest.Context{
+			FRequest: &http.Request{},
+			FResponse: &http.Response{
+				Header: http.Header{
+					"X-Fallback": []string{"true"},
+				},
+			},
+		},
+		nil,
 	},
 	} {
 		t.Run(ti.name, func(t *testing.T) {
 			span := tracer.StartSpan("proxy").(*mocktracer.MockSpan)
 			defer span.Finish()
 
-			ti.context.FRequest = ti.context.FRequest.WithContext(opentracing.ContextWithSpan(ti.context.FRequest.Context(), span))
+			requestContext := &filtertest.Context{
+				FRequest: ti.context.FRequest.WithContext(opentracing.ContextWithSpan(ti.context.FRequest.Context(), span)),
+			}
 
 			f, err := ti.spec.CreateFilter([]interface{}{"test_tag", ti.value})
 			if err != nil {
 				t.Fatal(err)
 			}
 
-			f.Request(ti.context)
-			f.Response(ti.context)
+			f.Request(requestContext)
+
+			requestContext.FResponse = ti.context.FResponse
+
+			f.Response(requestContext)
 
 			if got := span.Tag("test_tag"); got != ti.expected {
 				t.Errorf("unexpected tag value '%v' != '%v'", got, ti.expected)
@@ -126,64 +144,3 @@ func TestTracingTag(t *testing.T) {
 		})
 	}
 }
-
-func TestTagFilterIgnoresResponse(t *testing.T) {
-	tracer := mocktracer.New()
-	span := tracer.StartSpan("proxy").(*mocktracer.MockSpan)
-	defer span.Finish()
-
-	requestContext := &filtertest.Context{
-		FRequest: &http.Request{},
-	}
-	requestContext.FRequest = requestContext.FRequest.WithContext(opentracing.ContextWithSpan(requestContext.FRequest.Context(), span))
-
-	f, err := NewTag().CreateFilter([]interface{}{"test_tag", "${response.header.X-Fallback}"})
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	f.Request(requestContext)
-
-	requestContext.FResponse = &http.Response{
-		Header: http.Header{
-			"X-Fallback": []string{"true"},
-		},
-	}
-
-	f.Response(requestContext)
-
-	if got := span.Tag("test_tag"); got != nil {
-		t.Errorf("unexpected tag value '%v' != '%v'", got, nil)
-	}
-}
-
-func TestTagFromResponseFilterIgnoresRequest(t *testing.T) {
-	tracer := mocktracer.New()
-	span := tracer.StartSpan("proxy").(*mocktracer.MockSpan)
-	defer span.Finish()
-
-	requestContext := &filtertest.Context{
-		FRequest: &http.Request{
-			Header: http.Header{
-				"X-Flow-Id": []string{"a-flow-id"},
-			},
-		},
-	}
-	requestContext.FRequest = requestContext.FRequest.WithContext(opentracing.ContextWithSpan(requestContext.FRequest.Context(), span))
-
-	f, err := NewTagFromResponse().CreateFilter([]interface{}{"test_tag", "${request.header.X-Flow-Id}"})
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	f.Request(requestContext)
-
-	responseContext := &filtertest.Context{FRequest: &http.Request{}}
-	responseContext.FRequest = responseContext.FRequest.WithContext(opentracing.ContextWithSpan(responseContext.FRequest.Context(), span))
-
-	f.Response(responseContext)
-
-	if got := span.Tag("test_tag"); got != nil {
-		t.Errorf("unexpected tag value '%v' != '%v'", got, nil)
-	}
-}