moby / buildkit

concurrent, cache-efficient, and Dockerfile-agnostic builder toolkit
https://github.com/moby/moby/issues/34227
Apache License 2.0
8.05k stars 1.13k forks source link

Jaeger exporter no longer needed #3364

Open errordeveloper opened 1 year ago

errordeveloper commented 1 year ago

Jaeger already supports OTLP, so some of the code dedicated to handling Jaeger's own protocol can be remove in favour of OTLP.

errordeveloper commented 1 year ago

I think there are two feasible options:

errordeveloper commented 1 year ago

Doing this would allow us to remove significant portion of the code in utils/tracing.

Thе code that can be removed includes imported internal conversion code from the SDK

https://github.com/moby/buildkit/blob/4451e1be0e6889ffc56225e54f7e26bd6fdada54/util/tracing/transform/span.go#L22

which originates from go.opentelemetry.io/otel/exporters/otlp/otlptrace/internal/tracetransform" , and it is only needed due to the use of otlptrace.Exporter interface instead of otlptrace.Client, as that's the inteface Jaeger exporter implements.

(I wish the code had a comment saying where it was imported from, I only found this by chance after digging for a few hours).

Furthermore, there is complex code to do with provider detection and registration etc

https://github.com/moby/buildkit/blob/4451e1be0e6889ffc56225e54f7e26bd6fdada54/util/tracing/detect/detect.go#L35

Instead of all this, it should be possible to just use a few of the OpenTelemetry SDK functions.

tonistiigi commented 1 year ago

transform.Spans is not used by Jaeger but by the client-side traces proxy.

errordeveloper commented 1 year ago

transform.Spans is not used by Jaeger but by the client-side traces proxy.

Could you please provide a link?

I didn't say it was used by utils/tracing/detect/jaeger, I was saying that it's needed because of reliance on otlptrace.Exporter interface that has ExportSpans(ctx, []sdktrace.ReadOnlySpan) error, rather then otlptrace.Client that has UploadTraces(context.Context, []*tracepb.ResourceSpans) error and does the transform before calling underlying ExportSpans. And my reading of the code is that ExportSpans is the common denominator due to Jaeger exporter, which only implements otlptrace.Exporter, so doesn't have UploadTraces and only has ExportSpans.