Specify tracing span kind on creation (#3039)

OpenTelemetry-OpenTracing bridge span kind can not be changed after creation, see open-telemetry/opentelemetry-go#3953 The workaround is to specify span kind on creation which works for both Open Tracing and Open Telemetry bridge spans. Note that this change removes non-standard "shedder" kind from spans created by `admissionControl` filter. Use operation name "admission_control" to query its spans instead if needed. For #2104 Signed-off-by: Alexander Yastrebov <[email protected]>
zalando · Apr 26, 2024 · b02d681 · b02d681
1 parent 429e8b5
commit b02d681
Show file tree

Hide file tree

Showing 7 changed files with 71 additions and 62 deletions.
diff --git a/filters/openpolicyagent/tracing.go b/filters/openpolicyagent/tracing.go
@@ -71,25 +71,27 @@ func (*tracingFactory) NewHandler(f http.Handler, label string, opts opatracing.
 
 func (tr *transport) RoundTrip(req *http.Request) (*http.Response, error) {
 	ctx := req.Context()
-	parentSpan := opentracing.SpanFromContext(ctx)
-	var span opentracing.Span
 
-	if parentSpan != nil {
-		span = parentSpan.Tracer().StartSpan(spanNameHttpOut, opentracing.ChildOf(parentSpan.Context()))
+	spanOpts := []opentracing.StartSpanOption{opentracing.Tags{
+		proxy.HTTPMethodTag: req.Method,
+		proxy.HTTPUrlTag:    req.URL.String(),
+		proxy.HostnameTag:   req.Host,
+		proxy.HTTPPathTag:   req.URL.Path,
+		proxy.ComponentTag:  "skipper",
+		proxy.SpanKindTag:   proxy.SpanKindClient,
+	}}
+
+	var span opentracing.Span
+	if parentSpan := opentracing.SpanFromContext(ctx); parentSpan != nil {
+		spanOpts = append(spanOpts, opentracing.ChildOf(parentSpan.Context()))
+		span = parentSpan.Tracer().StartSpan(spanNameHttpOut, spanOpts...)
 	} else if tr.tracer != nil {
-		span = tr.tracer.StartSpan(spanNameHttpOut)
+		span = tr.tracer.StartSpan(spanNameHttpOut, spanOpts...)
 	}
 
 	if span != nil {
 		defer span.Finish()
 
-		span.SetTag(proxy.HTTPMethodTag, req.Method)
-		span.SetTag(proxy.HTTPUrlTag, req.URL.String())
-		span.SetTag(proxy.HostnameTag, req.Host)
-		span.SetTag(proxy.HTTPPathTag, req.URL.Path)
-		span.SetTag(proxy.ComponentTag, "skipper")
-		span.SetTag(proxy.SpanKindTag, proxy.SpanKindClient)
-
 		setSpanTags(span, tr.bundleName, tr.manager)
 		req = req.WithContext(opentracing.ContextWithSpan(ctx, span))
 

diff --git a/filters/shedder/admission.go b/filters/shedder/admission.go
@@ -456,7 +456,6 @@ func (ac *admissionControl) startSpan(ctx context.Context) (span opentracing.Spa
 	if parent != nil {
 		span = ac.tracer.StartSpan(admissionControlSpanName, opentracing.ChildOf(parent.Context()))
 		ext.Component.Set(span, "skipper")
-		ext.SpanKind.Set(span, "shedder")
 		span.SetTag("mode", ac.mode.String())
 	}
 	return

diff --git a/net/httpclient.go b/net/httpclient.go
@@ -381,20 +381,17 @@ func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {
 }
 
 func (t *Transport) injectSpan(req *http.Request) (*http.Request, opentracing.Span) {
-	parentSpan := opentracing.SpanFromContext(req.Context())
-	var span opentracing.Span
-	if parentSpan != nil {
+	spanOpts := []opentracing.StartSpanOption{opentracing.Tags{
+		string(ext.Component):  t.componentName,
+		string(ext.SpanKind):   "client",
+		string(ext.HTTPMethod): req.Method,
+		string(ext.HTTPUrl):    req.URL.String(),
+	}}
+	if parentSpan := opentracing.SpanFromContext(req.Context()); parentSpan != nil {
 		req = req.WithContext(opentracing.ContextWithSpan(req.Context(), parentSpan))
-		span = t.tracer.StartSpan(t.spanName, opentracing.ChildOf(parentSpan.Context()))
-	} else {
-		span = t.tracer.StartSpan(t.spanName)
+		spanOpts = append(spanOpts, opentracing.ChildOf(parentSpan.Context()))
 	}
-
-	// add Tags
-	ext.Component.Set(span, t.componentName)
-	ext.HTTPUrl.Set(span, req.URL.String())
-	ext.HTTPMethod.Set(span, req.Method)
-	ext.SpanKind.Set(span, "client")
+	span := t.tracer.StartSpan(t.spanName, spanOpts...)
 
 	_ = t.tracer.Inject(span.Context(), opentracing.HTTPHeaders, opentracing.HTTPHeadersCarrier(req.Header))
 

diff --git a/proxy/proxy.go b/proxy/proxy.go
@@ -958,14 +958,19 @@ func (p *Proxy) makeBackendRequest(ctx *context, requestContext stdlibcontext.Co
 	if !ok {
 		spanName = "proxy"
 	}
-	ctx.proxySpan = tracing.CreateSpan(spanName, req.Context(), p.tracing.tracer)
+
+	proxySpanOpts := []ot.StartSpanOption{ot.Tags{
+		SpanKindTag:       SpanKindClient,
+		SkipperRouteIDTag: ctx.route.Id,
+	}}
+	if parentSpan := ot.SpanFromContext(req.Context()); parentSpan != nil {
+		proxySpanOpts = append(proxySpanOpts, ot.ChildOf(parentSpan.Context()))
+	}
+	ctx.proxySpan = p.tracing.tracer.StartSpan(spanName, proxySpanOpts...)
 
 	u := cloneURL(req.URL)
 	u.RawQuery = ""
-	p.tracing.
-		setTag(ctx.proxySpan, SpanKindTag, SpanKindClient).
-		setTag(ctx.proxySpan, SkipperRouteIDTag, ctx.route.Id).
-		setTag(ctx.proxySpan, HTTPUrlTag, u.String())
+	p.tracing.setTag(ctx.proxySpan, HTTPUrlTag, u.String())
 	p.setCommonSpanInfo(u, req, ctx.proxySpan)
 
 	carrier := ot.HTTPHeadersCarrier(req.Header)
@@ -1181,10 +1186,16 @@ func (p *Proxy) do(ctx *context, parentSpan ot.Span) (err error) {
 		ctx.ensureDefaultResponse()
 	} else if ctx.route.BackendType == eskip.LoopBackend {
 		loopCTX := ctx.clone()
-		loopSpan := tracing.CreateSpan("loopback", ctx.request.Context(), p.tracing.tracer)
-		p.tracing.
-			setTag(loopSpan, SpanKindTag, SpanKindServer).
-			setTag(loopSpan, SkipperRouteIDTag, ctx.route.Id)
+
+		loopSpanOpts := []ot.StartSpanOption{ot.Tags{
+			SpanKindTag:       SpanKindServer,
+			SkipperRouteIDTag: ctx.route.Id,
+		}}
+		if parentSpan := ot.SpanFromContext(ctx.request.Context()); parentSpan != nil {
+			loopSpanOpts = append(loopSpanOpts, ot.ChildOf(parentSpan.Context()))
+		}
+		loopSpan := p.tracing.tracer.StartSpan("loopback", loopSpanOpts...)
+
 		p.setCommonSpanInfo(ctx.Request().URL, ctx.Request(), loopSpan)
 		ctx.parentSpan = loopSpan
 
@@ -1481,12 +1492,15 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	p.metrics.IncCounter("incoming." + r.Proto)
 	var ctx *context
 
-	var span ot.Span
-	if wireContext, err := p.tracing.tracer.Extract(ot.HTTPHeaders, ot.HTTPHeadersCarrier(r.Header)); err != nil {
-		span = p.tracing.tracer.StartSpan(p.tracing.initialOperationName)
-	} else {
-		span = p.tracing.tracer.StartSpan(p.tracing.initialOperationName, ext.RPCServerOption(wireContext))
+	spanOpts := []ot.StartSpanOption{ot.Tags{
+		SpanKindTag:     SpanKindServer,
+		HTTPRemoteIPTag: stripPort(r.RemoteAddr),
+	}}
+	if wireContext, err := p.tracing.tracer.Extract(ot.HTTPHeaders, ot.HTTPHeadersCarrier(r.Header)); err == nil {
+		spanOpts = append(spanOpts, ext.RPCServerOption(wireContext))
 	}
+	span := p.tracing.tracer.StartSpan(p.tracing.initialOperationName, spanOpts...)
+
 	defer func() {
 		if ctx != nil && ctx.proxySpan != nil {
 			ctx.proxySpan.Finish()
@@ -1533,9 +1547,6 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		r.URL.Path = rfc.PatchPath(r.URL.Path, r.URL.RawPath)
 	}
 
-	p.tracing.
-		setTag(span, SpanKindTag, SpanKindServer).
-		setTag(span, HTTPRemoteIPTag, stripPort(r.RemoteAddr))
 	p.setCommonSpanInfo(r.URL, r, span)
 	r = r.WithContext(ot.ContextWithSpan(r.Context(), span))
 	r = r.WithContext(routing.NewContext(r.Context()))

diff --git a/ratelimit/leakybucket.go b/ratelimit/leakybucket.go
@@ -112,13 +112,12 @@ func (b *ClusterLeakyBucket) getBucketId(label string) string {
 }
 
 func (b *ClusterLeakyBucket) startSpan(ctx context.Context) (span opentracing.Span) {
-	parent := opentracing.SpanFromContext(ctx)
-	if parent != nil {
-		span = b.ringClient.StartSpan(leakyBucketSpanName, opentracing.ChildOf(parent.Context()))
-	} else {
-		span = opentracing.NoopTracer{}.StartSpan("")
+	spanOpts := []opentracing.StartSpanOption{opentracing.Tags{
+		string(ext.Component): "skipper",
+		string(ext.SpanKind):  "client",
+	}}
+	if parent := opentracing.SpanFromContext(ctx); parent != nil {
+		spanOpts = append(spanOpts, opentracing.ChildOf(parent.Context()))
 	}
-	ext.Component.Set(span, "skipper")
-	ext.SpanKind.Set(span, "client")
-	return
+	return b.ringClient.StartSpan(leakyBucketSpanName, spanOpts...)
 }
diff --git a/ratelimit/redis.go b/ratelimit/redis.go
@@ -84,14 +84,14 @@ func parentSpan(ctx context.Context) opentracing.Span {
 	return opentracing.SpanFromContext(ctx)
 }
 
-func (c *clusterLimitRedis) setCommonTags(span opentracing.Span) {
-	if span != nil {
-		ext.Component.Set(span, "skipper")
-		ext.SpanKind.Set(span, "client")
-		span.SetTag("ratelimit_type", c.typ)
-		span.SetTag("group", c.group)
-		span.SetTag("max_hits", c.maxHits)
-		span.SetTag("window", c.window.String())
+func (c *clusterLimitRedis) commonTags() opentracing.Tags {
+	return opentracing.Tags{
+		string(ext.Component): "skipper",
+		string(ext.SpanKind):  "client",
+		"ratelimit_type":      c.typ,
+		"group":               c.group,
+		"max_hits":            c.maxHits,
+		"window":              c.window.String(),
 	}
 }
 
@@ -114,10 +114,9 @@ func (c *clusterLimitRedis) Allow(ctx context.Context, clearText string) bool {
 
 	var span opentracing.Span
 	if parentSpan := parentSpan(ctx); parentSpan != nil {
-		span = c.ringClient.StartSpan(allowSpanName, opentracing.ChildOf(parentSpan.Context()))
+		span = c.ringClient.StartSpan(allowSpanName, opentracing.ChildOf(parentSpan.Context()), c.commonTags())
 		defer span.Finish()
 	}
-	c.setCommonTags(span)
 
 	allow, err := c.allow(ctx, clearText)
 	failed := err != nil
@@ -227,10 +226,9 @@ func (c *clusterLimitRedis) oldest(ctx context.Context, clearText string) (time.
 
 	var span opentracing.Span
 	if parentSpan := parentSpan(ctx); parentSpan != nil {
-		span = c.ringClient.StartSpan(oldestScoreSpanName, opentracing.ChildOf(parentSpan.Context()))
+		span = c.ringClient.StartSpan(oldestScoreSpanName, opentracing.ChildOf(parentSpan.Context()), c.commonTags())
 		defer span.Finish()
 	}
-	c.setCommonTags(span)
 
 	res, err := c.ringClient.ZRangeByScoreWithScoresFirst(ctx, key, 0.0, float64(now.UnixNano()), 0, 1)
 

diff --git a/tracing/tracingtest/testtracer.go b/tracing/tracingtest/testtracer.go
@@ -102,6 +102,9 @@ func (t *Tracer) StartSpan(operationName string, opts ...tracing.StartSpanOption
 	s := t.createSpanBase()
 	s.operationName = operationName
 	s.Refs = sso.References
+	for k, v := range sso.Tags {
+		s.Tags[k] = v
+	}
 	return s
 }