Add otel tracing implementation support #2578

lucastt · 2023-09-07T13:57:52Z

The idea of this PR is to add a new tracing implementation to skipper run options that would support OpenTelemetry (Otel), instead of the deprecated OpenTracing (Ot).

The initial plan is just to support Otel tracer creation and wrap Ot tracer into a custom wrapper that would implement Otel tracer and span interface but would call Ot under the hood.

With next PRs we would start to have Otel tracers as first class citizens in our structs and interfaces enabling a skipper user to use full Otel support in all functionalities.

On changing the tracer interface many modules were affected including hot path changes.

proxy/proxy.go

proxy/tracing.go

tracing/spanWrapper.go

tracing/tracers/otel/otel.go

tracing/tracing.go

filters/filters.go

go.mod

proxy/tracing.go

tracing/tracing.go

tracing/tracers/otel/otel.go

tracing/wrappers.go

Signed-off-by: Lucas Thiesen <lucas.thiesen@zalando.de>

Signed-off-by: Lucas Thiesen <lucassthiesen@gmail.com> Signed-off-by: Lucas Thiesen <lucas.thiesen@zalando.de>

Signed-off-by: Lucas Thiesen <lucas.thiesen@zalando.de>

Signed-off-by: Lucas Thiesen <lucassthiesen@gmail.com>

Signed-off-by: Lucas Thiesen <lucas.thiesen@zalando.de>

lucastt · 2024-03-22T09:35:05Z

👍

MustafaSaber · 2024-03-22T13:55:06Z

filters/tee/tee.go

-	// Tracer is the opentracing tracer to use in the client
-	Tracer opentracing.Tracer
+	// Tracer is the open telemetry tracer to use in the client
+	Tracer trace.Tracer


sometimes I see Tracer and others OtelTracer, is this intended?

Yeah, in some places I used OtelTracer to differentiate from Tracer and OtTracer, the former is only OT if we need to be backward compatible and the later is kept in places we need access to both tracers and I wanted to make it clear that it is one or the other. Marjority of cases it should be just tracer or Tracer.

Also, when we discontinue OpenTracing, which will probably take a while, we can have all of them as just Tracer/tracer.

lucastt · 2024-03-22T14:11:02Z

Regarding how I tested these changes locally, the strategy I used was unity tests + stdouttrace, which is a tracer that implements otel.Tracer interface and drops the span in JSON form in stdout or any other file.

I didn't test it directly with specific collectors or vendors like lightstep, I was planning to run skipper on a custom cluster and test it with lightstep collectors there before merging it.

stdouttrace implements all behaviors supported by Otel SDK (OTLP) what is not well tested is the config part, the arguments we would pass in skipper parameters that would configure the communication between skipper tracer and a tracer collector or tracing commercial solution.

cc: @MustafaSaber

MustafaSaber · 2024-03-22T15:31:31Z

scheduler/proxy_test.go

@@ -83,7 +83,7 @@ func TestFifoChanges(t *testing.T) {
 		t.Helper()

 		client := net.NewClient(net.Options{
-			ResponseHeaderTimeout: 2 * time.Second,
+			ResponseHeaderTimeout: 3 * time.Second,


why was this changed?

It was flaky and was failing most of the time in my local environment, increasing the timeout solved the problem.

MustafaSaber · 2024-03-22T15:33:39Z

skipper.go

-		}),
-	)
+	// tee filters override if we have a tracer
+	if len(o.OpenTracing) > 0 {


does the override only apply for opentracing?

This option has a very unfortunate name, but it contains both OpenTracing and OpenTelemetry tracers. I could not change the name to something with more meaning than Options.OpenTracing to not break backward compatibility with library users, which is not great but we don't have much choice. :/

szuecs · 2024-03-22T16:36:33Z

tracing/tracers/otel/otel.go

+				}
+				globalTags[tag] = tagVal
+			}
+		case "otlp-exporter-endpoint":


From testing in the cluster: maybe an URL?

Yeah, I'll try to test with a local OTLP collector to make sure no parameter is missing. But one of the things we need to change is this parameter should not be split by port and address only because we need the URL path to route it to the proper OTLP endpoint in most collectors.

szuecs · 2024-03-22T16:37:46Z

Tested in the cluster: compatibility mode works great

otel does not work right now, because of otlp-exporter-endpoint (likely requires an URL).

AlexanderYastrebov

Thank you for this big effort. I did a first review pass and left my comments.

AlexanderYastrebov · 2024-03-25T12:30:55Z

net/httpclient.go

 		},
 		DNSDone: func(httptrace.DNSDoneInfo) {
-			span.LogKV("DNS", "end")
+			span.AddEvent("DNS", trace.WithAttributes(attribute.String("DNS", "stop")))


Why end changed to stop?

Oh, this is a mistake. Good catch!

AlexanderYastrebov · 2024-03-25T12:32:48Z

proxy/context.go

 	originalRequest.Body = body
 	cc.request = cr
 	return cc, nil
 }

 func (c *context) Loopback() {
-	loopSpan := c.Tracer().StartSpan(c.proxy.tracing.initialOperationName, opentracing.ChildOf(c.ParentSpan().Context()))
-	defer loopSpan.Finish()
+	_, loopSpan := c.Tracer().Start(c.request.Context(), c.proxy.tracing.initialOperationName)


What return value is ignored here and why?

This is an stdlib context. This context doesn't seem to be used in the function.

One thing that could be done is update the request context so it carries the span information forward if any other function or package wants to use it.

But in theory this was not done in the past either, Ot.ContextWithSpan(...) was never called here so the context was never updated, the only difference is that Otel.Tracer.Start(...) always returns a context, which was not true for OT implementation.

AlexanderYastrebov · 2024-03-25T12:34:34Z

proxy/proxy.go

+	// OtTracer holds the open telemetry tracer enabled for this proxy instance
+	// DEPRECATED, use TracerWrapper.
 	Tracer ot.Tracer

+	// OtelTracer holds the opentracing tracer enabled for this proxy instance
+	OtelTracer trace.Tracer


Here and elsewhere we keep old and new tracer fields.
Why not use only OTEL tracer and provide users an adapter for opentracing tracer?

Actually we do this from skipper.Run() point of view @lucastt , so we likely can delete proxy.Tracer

Yeah, it is possible, I just considered this as something that people using skipper as a library could be using so I wanted to keep backward compatibility. Under the hood the old tracer is converted into a tracerWrapper instance that convert calls from OTEL to OT.

So yeah, if this part does not need to be backward compatible it would be much better to have a single tracer instance.

AlexanderYastrebov · 2024-03-25T12:36:43Z

proxy/proxy.go

+type otelTransport struct {
+	tr     *http.Transport
+	tracer trace.Tracer
+}


Why do we need this? Somehow we created proxy span without it before.

This is an interesting question and I think it's how OTel is different from OT. Please confirm @lucastt

Yeah, maybe this is not necessary. Because the goal of this struct is to inject the span into the request, but we already do this in net/httpclient.go:406 so the only case this struct would be necessary is if we make requests using this Roundtripper directly, without using skipper http client.

AlexanderYastrebov · 2024-03-25T12:37:20Z

proxy/proxy.go

@@ -1173,14 +1211,14 @@ 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)
+		_, loopSpan := p.tracing.Start(ctx.request.Context(), "loopback")


What return value is ignored here and why?

Same reason as here: https://github.com/zalando/skipper/pull/2578/files/a25b1840dc0decb0dfda88d2ca0163452eca73cd#r1542959056

AlexanderYastrebov · 2024-03-25T12:39:17Z

proxy/proxy.go

@@ -1310,7 +1354,7 @@ func (p *Proxy) serveResponse(ctx *context) {
 		p.tracing.setTag(ctx.proxySpan, ClientRequestStateTag, ClientRequestCanceled)
 	}

-	p.tracing.setTag(ctx.initialSpan, HTTPStatusCodeTag, uint16(ctx.response.StatusCode))
+	p.tracing.setTag(ctx.initialSpan, HTTPStatusCodeTag, fmt.Sprint(uint16(ctx.response.StatusCode)))


We used attributes elsewhere but here it is "tag", why is it so?

In the case of proxy it implements a tracing package, this package was refactored to use Otel.Tracer but I didn't change the names of the functions on this package, in fact I changed the minimum necessary to make it work with Otel, but if you think is more valuable here to change the nomenclature I can do that.

AlexanderYastrebov · 2024-03-25T12:40:55Z

proxy/tracing.go

-		} else {
-			span.SetTag(key, value)
-		}
+		span.SetAttributes(attribute.String(key, value))


ensureUTF8 was lost with this change.

AlexanderYastrebov · 2024-03-25T12:45:31Z

tracing/tracingtest/oteltesttracer.go

+// OtelTracer is an implementation of opentelemetry.OtelTracer for testing. It records
+// the defined spans during a series of operations.
+type OtelTracer struct {


I am wondering if we should take a chance and get rid of tracing and tracing/tracingtest packages and just use otel directly (and https://pkg.go.dev/go.opentelemetry.io/otel/oteltest).

In any case why do we implement own test tracer instead of using https://pkg.go.dev/go.opentelemetry.io/otel/oteltest ?

Good catch. This is a mistake, I didn't know oteltest package at the time. Oteltest seems to be a better solution for tracing tests for now on. But maybe we could address this in a different PR? It seems to me this will bring a lot of new changes to an already very big PR. WDYT?

lucastt self-assigned this Sep 7, 2023

lucastt force-pushed the add-opentelemetry-support branch 4 times, most recently from fbf33d5 to 7b78485 Compare September 8, 2023 10:02

lucastt commented Nov 8, 2023

View reviewed changes

proxy/proxy.go Outdated Show resolved Hide resolved