Solve panic due to concurrent access to ExportSpans

[gsaraf]

Background:
After enabling Open Telemetry export to Jaeger using the JAEGER_TRACE env var, started seeing occasional panics across our fleet of buildkitd containers. A helpful pointer from the people at the OpenTelemetry Go repo indicated a possible concurrency problem. Wrapping the ExportSpans with a mutex solved the problem completely.

Issue: #3004

Testing: Before this fix, would get several panics a day. After, have gone for several weeks without any panics.
I've also run the suggested tests: ./hack/test integration gateway dockerfile.

[tonistiigi]

I'm not sure if this is really correct.

If your claim that ExportSpans can only be called on a single thread is correct then this is not the only place where it gets called. It will get called by regular context traces from buildkitd (maybe container forwarding as well).

In that case the only way I would see is that all the exporter implementations https://github.com/moby/buildkit/blob/master/util/tracing/detect/detect.go#L78 would need to be wrapped with something that makes ExportSpans safe to call before this exporter is used to create a traceprovider.

Tbh, if the description is correct I don't really understand the logic of this API design. Exporter is supposed to be input to sdktrace.NewBatchSpanProcessor in the regular flow. But if everything breaks down if there are two spanprocessor/traceprovider instances with the same exporter I don't understand what is the point of defining an exporter interface at all.

[gsaraf]

I've mentioned this thread in the original issue in the opentelemetry SDK, hopefully they will respond.

Without knowing too much about the reason this architecture was chosen, there is a benefit to the exporter interface, as it allows for different implementations of the exporter. This is used even in buildkit, for creating a delegated exporter.

Going through all calls to ExportSpans in the codebase:

• in util/tracing/detect/delegated/delegated.go the export is guarded by a mutex, though this may have been intended to guard the exporters list. Still safe.
• in control/control.go I add the mutex in this PR.
• in cmd/buildkitd/main.go maybe used for uploading traces from the client? I'm not 100% sure. Would potentially need to be guarded as well. I'm happy to add that change.

I see what you mean about detectExporter returning a safe exporter - that sounds fine as well. Do you prefer that change?

[tonistiigi]

https://github.com/moby/buildkit/blob/master/util/tracing/detect/detect.go#L78 will return for example in this case the Jaeger exporter. Then this exporter is called for example in control where you added mutex. But it also goes to the https://github.com/moby/buildkit/blob/master/util/tracing/detect/detect.go#L99 and from there all the local traces will end up in the exporter. Even if the control path has a mutex and SpanProcessor implementation is single-threaded they still don't know about each other at all.

[gsaraf]

Sorry about the delay, I've changed the implementation to create a thread-safe exporter instance and use that instead. Let me know if you'd like me to change anything! :)

[gsaraf]

@tonistiigi - can you re-review? Thanks!

[gsaraf]

Hi @tonistiigi - can you re-review this please? I'd like to stop having to maintaining my fork :)

[tonistiigi]

@gsaraf Could you rebase. GH doesn't show conflict but this code has actually changed in latest version.

[gsaraf]

Thanks for the review! Rebased and made the requested changes.