allow tagFilter to use response data

[lukas-c-wilhelm]

Fixes #2316

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 tracingTagOnResponse("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.

[AlexanderYastrebov]

Looks like it attempts to fix #2316

[szuecs]

You also need to register the spec into the builtins https://github.com/zalando/skipper/blob/master/filters/builtin/builtin.go#L210 to make it available for use in skipper.

[lukas-c-wilhelm]

@szuecs Thanks for your review. I addressed your comments.

[szuecs]

Only one tiny nit and it's ready to merge, thanks

[AlexanderYastrebov]

Looks ok, couple of ideas.

Since the filter matches 99% with existing you could also add a flag like here https://github.com/zalando/skipper/blob/master/filters/diag/histogram.go

$ grep -o -E '".*esponse.*"' filters/filters.go
"modResponseHeader"
"setResponseHeader"
"appendResponseHeader"
"dropResponseHeader"
"setContextResponseHeader"
"appendContextResponseHeader"
"copyResponseHeader"
"uniformResponseLatency"
"normalResponseLatency"
"histogramResponseLatency"
"dropResponseCookie"
"responseCookie"
"opaServeResponse"

$ grep -o -i -E '".*from.*"' filters/filters.go | sort
"setDynamicBackendHostFromHeader"
"setDynamicBackendSchemeFromHeader"
"setDynamicBackendUrlFromHeader"

Should we name it tracingTagFromResponse maybe?

[szuecs]

@AlexanderYastrebov yeah makes sense to have "typ" like in https://github.com/zalando/skipper/blob/master/filters/diag/histogram.go#L39 and check in Request/Response https://github.com/zalando/skipper/blob/master/filters/diag/histogram.go#L104.

[lukas-c-wilhelm]

@AlexanderYastrebov @szuecs I'll try to implement it like that.

[szuecs]

👍

[AlexanderYastrebov]

I think this could be a test case in the TestTracingTag

	{
		"tag ignores response",
		NewTag(),
		"${response.header.X-Fallback}",
		&filtertest.Context{
			FRequest: &http.Request{
				Header: http.Header{
					"X-Fallback": []string{"true"},
				},
			},
		},
		"",
	}

and likely we need the symmetric that NewTagFromResponse ignores request.

[lukas-c-wilhelm]

Not sure about this. Wouldn't the header in the request only match "${request.header.X-Fallback}", not the provided template for the response? So in any case the result would be nil, right?

My reasoning was that we need to have a case where a header is only present in the response, not in the request.

Will add a test for the symmetrical case, though.

[AlexanderYastrebov]

Sorry, I meant to use FResponse of course, i.e. filter is NewTag but template is "${response.header.X-Fallback}" and X-Fallback is a response header

[lukas-c-wilhelm]

This does not really work because the context is only created once in the test and is the same for the whole duration of the test. This means that the response header is already present at request time. To replicate the real behavior, we have to provide a different context for the f.Response call than for the f.Request call, where the header was added. This cannot be done within the structure of this test.

I could generalize the template to provide both a context at request time and one at response time. But this seems a bit overkill to me given that it is a global construct.

[AlexanderYastrebov]

We can have request and response fields in the test cases instead of context, create context at the start of the test and set response after f.Request(ctx) but before f.Response(ctx):

ctx := &filtertest.Context{
  FRequest: ti.request,
}
...

f.Request(ctx)

ctx.FResponse = ti.response

f.Response(ctx)

[lukas-c-wilhelm]

This works for a test that show tracingTag only runs on the request. It does not work for the symmetric case to test that tracingTagFromResponse only runs on the response. To prove this, the request in the context has to be changed (header removed) to prove that tracingTagFromResponse cannot make use of headers that are only present at request time. It is a bit artificial anyway.

Would you prefer to just have one test according to the method you described, or separate tests as they are now? Or a different change?

[lukas-c-wilhelm]

Alternatively, I could change the test code to reset the context to blank after the request call. But this would also be a bit strange, because in a real scenario the request context would be available also at response time, if I understand it correctly.

[AlexanderYastrebov]

Lets have a single additional test case then.

[szuecs]

In the additional test you can also have a real backend httptest.Server to create the response header to test it.

[AlexanderYastrebov]

👍

[AlexanderYastrebov]

@lukas-c-wilhelm Thank you for your contribution.

[szuecs]

👍