Address review comments

Signed-off-by: Magnus Jungsbluth <magnus.jungsbluth@zalando.de>

filters/openpolicyagent/evaluation.go

@@ -11,7 +11,6 @@ import (
 	"github.com/open-policy-agent/opa/ast"
 	"github.com/open-policy-agent/opa/rego"
 	"github.com/open-policy-agent/opa/server"
-	"github.com/open-policy-agent/opa/tracing"
 	"github.com/opentracing/opentracing-go"
 )
 
@@ -58,7 +57,7 @@ func (opa *OpenPolicyAgentInstance) Eval(ctx context.Context, req *ext_authz_v3.
 		return nil, err
 	}
 
-	err = envoyauth.Eval(ctx, opa, inputValue, result, rego.DistributedTracingOpts(tracing.Options{opa}))
+	err = envoyauth.Eval(ctx, opa, inputValue, result, rego.DistributedTracingOpts(buildTracingOptions(opa.registry.tracer, opa.bundleName, opa.manager)))
 	if err != nil {
 		return nil, err
 	}

filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/zalando/skipper/filters"
 	"github.com/zalando/skipper/metrics/metricstest"
 	"github.com/zalando/skipper/proxy/proxytest"
+	"github.com/zalando/skipper/tracing/tracingtest"
 
 	"github.com/zalando/skipper/filters/filtertest"
 	"github.com/zalando/skipper/filters/openpolicyagent"
@@ -359,7 +360,7 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 				}
 			}`, opaControlPlane.URL(), ti.regoQuery))
 
-			opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry()
+			opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry(openpolicyagent.WithTracer(&tracingtest.Tracer{}))
 			ftSpec := NewOpaAuthorizeRequestSpec(opaFactory, openpolicyagent.WithConfigTemplate(config))
 			fr.Register(ftSpec)
 			ftSpec = NewOpaAuthorizeRequestWithBodySpec(opaFactory, openpolicyagent.WithConfigTemplate(config))

filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/zalando/skipper/eskip"
 	"github.com/zalando/skipper/filters"
 	"github.com/zalando/skipper/proxy/proxytest"
+	"github.com/zalando/skipper/tracing/tracingtest"
 
 	"github.com/zalando/skipper/filters/openpolicyagent"
 )
@@ -239,7 +240,7 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 				}
 			}`, opaControlPlane.URL(), ti.regoQuery))
 
-			opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry()
+			opaFactory := openpolicyagent.NewOpenPolicyAgentRegistry(openpolicyagent.WithTracer(&tracingtest.Tracer{}))
 			ftSpec := NewOpaServeResponseSpec(opaFactory, openpolicyagent.WithConfigTemplate(config))
 			fr.Register(ftSpec)
 			ftSpec = NewOpaServeResponseWithReqBodySpec(opaFactory, openpolicyagent.WithConfigTemplate(config))

filters/openpolicyagent/openpolicyagent.go

@@ -47,6 +47,8 @@ const (
 	DefaultMaxRequestBodySize   = 1 << 20 // 1 MB
 	DefaultMaxMemoryBodyParsing = 100 * DefaultMaxRequestBodySize
 	defaultBodyBufferSize       = 8192 * 1024
+
+	spanNameEval = "open-policy-agent"
 )
 
 type OpenPolicyAgentRegistry struct {
@@ -404,9 +406,13 @@ func interpolateConfigTemplate(configTemplate []byte, bundleName string) ([]byte
 	return buf.Bytes(), nil
 }
 
+func buildTracingOptions(tracer opentracing.Tracer, bundleName string, manager *plugins.Manager) opatracing.Options {
+	return opatracing.NewOptions(WithTracingOptTracer(tracer), WithTracingOptBundleName(bundleName), WithTracingOptManager(manager))
+}
+
 func (registry *OpenPolicyAgentRegistry) withTracingOptions(bundleName string) func(*plugins.Manager) {
 	return func(m *plugins.Manager) {
-		options := opatracing.NewOptions(
+		options := buildTracingOptions(
 			registry.tracer,
 			bundleName,
 			m,
@@ -567,20 +573,24 @@ func (opa *OpenPolicyAgentInstance) EnvoyPluginConfig() envoy.PluginConfig {
 }
 
 func setSpanTags(span opentracing.Span, bundleName string, manager *plugins.Manager) {
-	span.SetTag("opa.bundle_name", bundleName)
+	if bundleName != "" {
+		span.SetTag("opa.bundle_name", bundleName)
+	}
 
-	for label, value := range manager.Labels() {
-		span.SetTag("opa.label."+label, value)
+	if manager != nil {
+		for label, value := range manager.Labels() {
+			span.SetTag("opa.label."+label, value)
+		}
 	}
 }
 
 func (opa *OpenPolicyAgentInstance) startSpanFromContextWithTracer(tr opentracing.Tracer, parent opentracing.Span, ctx context.Context) (opentracing.Span, context.Context) {
 
 	var span opentracing.Span
 	if parent != nil {
-		span = tr.StartSpan("open-policy-agent", opentracing.ChildOf(parent.Context()))
+		span = tr.StartSpan(spanNameEval, opentracing.ChildOf(parent.Context()))
 	} else {
-		span = tracing.CreateSpan("open-policy-agent", ctx, tr)
+		span = tracing.CreateSpan(spanNameEval, ctx, tr)
 	}
 
 	setSpanTags(span, opa.bundleName, opa.manager)

filters/openpolicyagent/tracing.go

@@ -6,11 +6,12 @@ import (
 	"github.com/open-policy-agent/opa/plugins"
 	opatracing "github.com/open-policy-agent/opa/tracing"
 	"github.com/opentracing/opentracing-go"
+	"github.com/zalando/skipper/logging"
 	"github.com/zalando/skipper/proxy"
 )
 
 const (
-	spanName = "open-policy-agent.http"
+	spanNameHttpOut = "open-policy-agent.http"
 )
 
 func init() {
@@ -27,37 +28,41 @@ type transport struct {
 	wrapped http.RoundTripper
 }
 
-func (*tracingFactory) NewTransport(tr http.RoundTripper, opts opatracing.Options) http.RoundTripper {
-	var tracer opentracing.Tracer
-
-	if len(opts) < 3 {
-		panic("insufficient tracing options provided for outbound http calls from Open Policy Agent")
+func WithTracingOptTracer(tracer opentracing.Tracer) func(*transport) {
+	return func(t *transport) {
+		t.tracer = tracer
 	}
+}
 
-	if opts[0] != nil {
-		var ok bool
-		tracer, ok = opts[0].(opentracing.Tracer)
-		if !ok {
-			panic("invalid type for first argument of tracing options, expected opentracing.Tracer")
-		}
+func WithTracingOptBundleName(bundleName string) func(*transport) {
+	return func(t *transport) {
+		t.bundleName = bundleName
 	}
+}
 
-	bundleName, ok := opts[1].(string)
-	if !ok {
-		panic("invalid type for second argument of tracing options, expected string")
+func WithTracingOptManager(manager *plugins.Manager) func(*transport) {
+	return func(t *transport) {
+		t.manager = manager
 	}
+}
+
+func (*tracingFactory) NewTransport(tr http.RoundTripper, opts opatracing.Options) http.RoundTripper {
+	log := &logging.DefaultLog{}
 
-	manager, ok := opts[2].(*plugins.Manager)
-	if !ok {
-		panic("invalid type third argument of tracing options, expected *plugins.Manager")
+	wrapper := &transport{
+		wrapped: tr,
 	}
 
-	return &transport{
-		tracer:     tracer,
-		bundleName: bundleName,
-		manager:    manager,
-		wrapped:    tr,
+	for _, o := range opts {
+		opt, ok := o.(func(*transport))
+		if !ok {
+			log.Warn("invalid type for OPA tracing option, expected func(*transport), tracing informatation might be incomplete")
+		} else {
+			opt(wrapper)
+		}
 	}
+
+	return wrapper
 }
 
 func (*tracingFactory) NewHandler(f http.Handler, label string, opts opatracing.Options) http.Handler {
@@ -70,9 +75,9 @@ func (tr *transport) RoundTrip(req *http.Request) (*http.Response, error) {
 	var span opentracing.Span
 
 	if parentSpan != nil {
-		span = parentSpan.Tracer().StartSpan(spanName, opentracing.ChildOf(parentSpan.Context()))
+		span = parentSpan.Tracer().StartSpan(spanNameHttpOut, opentracing.ChildOf(parentSpan.Context()))
 	} else if tr.tracer != nil {
-		span = tr.tracer.StartSpan(spanName)
+		span = tr.tracer.StartSpan(spanNameHttpOut)
 	}
 
 	if span != nil {

filters/openpolicyagent/tracing_test.go

@@ -8,7 +8,6 @@ import (
 
 	"github.com/open-policy-agent/opa/config"
 	"github.com/open-policy-agent/opa/plugins"
-	opatracing "github.com/open-policy-agent/opa/tracing"
 	"github.com/opentracing/opentracing-go"
 	"github.com/stretchr/testify/assert"
 	"github.com/zalando/skipper/tracing/tracingtest"
@@ -84,12 +83,12 @@ func TestTracingFactory(t *testing.T) {
 			f := &tracingFactory{}
 			tracer.Reset("")
 
-			tr := f.NewTransport(&MockTransport{tc.resp, tc.resperr}, opatracing.Options{tc.tracer, "bundle", &plugins.Manager{
+			tr := f.NewTransport(&MockTransport{tc.resp, tc.resperr}, buildTracingOptions(tc.tracer, "bundle", &plugins.Manager{
 				ID: "manager-id",
 				Config: &config.Config{
 					Labels: map[string]string{"label": "value"},
 				},
-			}})
+			}))
 
 			if tc.parentSpan != nil {
 				ctx := opentracing.ContextWithSpan(context.Background(), tc.parentSpan)