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")