From b62b182d43e85ba7cfb42c74ceb6c27d14947292 Mon Sep 17 00:00:00 2001
From: Magnus Jungsbluth <magnus.jungsbluth@zalando.de>
Date: Wed, 19 Jun 2024 08:38:30 +0200
Subject: [PATCH] OPA: Add response status to control plane traces

Signed-off-by: Magnus Jungsbluth <magnus.jungsbluth@zalando.de>
---
 filters/openpolicyagent/tracing.go      | 29 +++++++++++++++++--------
 filters/openpolicyagent/tracing_test.go | 20 +++++++++++++++++
 2 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/filters/openpolicyagent/tracing.go b/filters/openpolicyagent/tracing.go
index 531e563913..420deb231f 100644
--- a/filters/openpolicyagent/tracing.go
+++ b/filters/openpolicyagent/tracing.go
@@ -89,21 +89,32 @@ func (tr *transport) RoundTrip(req *http.Request) (*http.Response, error) {
 		span = tr.tracer.StartSpan(spanNameHttpOut, spanOpts...)
 	}
 
-	if span != nil {
-		defer span.Finish()
+	if span == nil {
+		return tr.wrapped.RoundTrip(req)
+	}
 
-		setSpanTags(span, tr.bundleName, tr.manager)
-		req = req.WithContext(opentracing.ContextWithSpan(ctx, span))
+	defer span.Finish()
 
-		carrier := opentracing.HTTPHeadersCarrier(req.Header)
-		span.Tracer().Inject(span.Context(), opentracing.HTTPHeaders, carrier)
-	}
+	setSpanTags(span, tr.bundleName, tr.manager)
+	req = req.WithContext(opentracing.ContextWithSpan(ctx, span))
+
+	carrier := opentracing.HTTPHeadersCarrier(req.Header)
+	span.Tracer().Inject(span.Context(), opentracing.HTTPHeaders, carrier)
 
 	resp, err := tr.wrapped.RoundTrip(req)
-	if err != nil && span != nil {
+
+	if err != nil {
 		span.SetTag("error", true)
 		span.LogKV("event", "error", "message", err.Error())
+		return resp, err
+	}
+
+	span.SetTag(proxy.HTTPStatusCodeTag, resp.StatusCode)
+
+	if resp.StatusCode > 399 {
+		span.SetTag("error", true)
+		span.LogKV("event", "error", "message", resp.Status)
 	}
 
-	return resp, err
+	return resp, nil
 }
diff --git a/filters/openpolicyagent/tracing_test.go b/filters/openpolicyagent/tracing_test.go
index dc5bd33370..92f295eea3 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/zalando/skipper/proxy"
 	"github.com/zalando/skipper/tracing/tracingtest"
 )
 
@@ -43,6 +44,7 @@ func TestTracingFactory(t *testing.T) {
 			},
 			tracer:     nil,
 			parentSpan: tracer.StartSpan("open-policy-agent"),
+			resp:       &http.Response{StatusCode: http.StatusOK},
 		},
 		{
 			name: "Sub-span created with parent span without tracer set",
@@ -54,6 +56,7 @@ func TestTracingFactory(t *testing.T) {
 			},
 			tracer:     tracer,
 			parentSpan: tracer.StartSpan("open-policy-agent"),
+			resp:       &http.Response{StatusCode: http.StatusOK},
 		},
 		{
 			name: "Sub-span created without parent span",
@@ -64,6 +67,7 @@ func TestTracingFactory(t *testing.T) {
 				URL:    &url.URL{Path: "/test", Scheme: "http", Host: "example.com"},
 			},
 			tracer: tracer,
+			resp:   &http.Response{StatusCode: http.StatusOK},
 		},
 		{
 			name: "Span contains error information",
@@ -76,6 +80,17 @@ func TestTracingFactory(t *testing.T) {
 			tracer:  tracer,
 			resperr: assert.AnError,
 		},
+		{
+			name: "Response has a 4xx response",
+			req: &http.Request{
+				Header: map[string][]string{},
+				Method: "GET",
+				Host:   "example.com",
+				URL:    &url.URL{Path: "/test", Scheme: "http", Host: "example.com"},
+			},
+			tracer: tracer,
+			resp:   &http.Response{StatusCode: http.StatusUnauthorized},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -105,6 +120,11 @@ func TestTracingFactory(t *testing.T) {
 
 			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, tc.resp.StatusCode, createdSpan.Tags[proxy.HTTPStatusCodeTag], "http status tag was not set")
 			} else {
 				assert.Equal(t, true, createdSpan.Tags["error"], "Error tag was not set")
 				assert.Equal(t, tc.resperr, err, "Error was not propagated")