util/tracing/detect: separate from bklog package

[thaJeztah]

• relates to Add OTEL support (carry 4556) docker/cli#4698 (comment)
• relates to vendor: OTEL v1.19.0 / v0.45.0, containerd v1.7.11 moby#46830

util/tracing/detect: rename package-level "err" var

This error is used to persist errors that occurred during detection
(ran in a sync.Once). Using err as variable name for this error
made it more likely to be inadvertently shadowed.

util/tracing/detect: don't use init() to register default otlp detector

The otlp detector was registered unconditionally; looks like no init()
function is needed for that.

util/tracing/detect: separate from bklog package

Separate detection from BuildKit's logger, with the potential to
separate this package from the BuildKit module.

This patch introduces a new "tracelogger" package, which allows
decorating an existing logger with trace- and span-ID's.

[thaJeztah]

Hmf!

637.8 util/tracing/detect/detect.go:37:5: the variable name `detectErr` should conform to the `errXxx` format (errname)
637.8 var detectErr error
637.8     ^

[tonistiigi]

What's the use-case for the 2nd/3rd commit? This behavior is already opt-in with user needing to define that tracing is enabled. I don't think we need double opt-in.

I can't imagine a configuration when tracing is enabled but otlp is not a valid implementation. When tracing is enabled then all buildkit logs should have tracing IDs, not sure why there would need to be a way to disable it.

[thaJeztah]

The intent here was to see if we could separate the detect package from BuildKit; this was with the assumption that we need to use the same detection logic in BuildKit, Engine, and the CLI (possibly elsewhere), and that the "bklog" package is deeply engrained in BuildKit, so cannot be moved out.

Currently; the PR in docker/cli also pulls in that package, and as a result we have a circular dependency on BuildKit again (undesirable).

Having the package separate could POSSIBLY also mean that different versions of the package / module could exist with different "semconv" versions, so that moby could move to a version that's compatible with containerd (if BuildKit 0.12 stays on the old version)

All that said, @milas may have found an alternative to all of this, that would mean the detect package is no longer needed at all (see the thread in the maintainers channel)

[tonistiigi]

detect package is a hard requirement for tracing to work in buildkit. It is also how moby/moby tracing works (although there it is not a hard requirement).

[thaJeztah]

Yes, we need some form of a detect package, but for tracing to work across moby, buildkit (vendored), and containerd, the semconv versions must match. That's currently the problem, because they don't, and they cannot be updated by updating a module (the version is in a "versioned" package name, not a module 😫)

IIUC alternative that @milas found is to use the detect package that's now provided by OTEL itself (in some recent release); that would keep versions out of both BuildKit and Moby (other than picking a version of the modules)

(or at least; that's my understanding)

[thaJeztah]

@tonistiigi looks like work in progress on that in containerd/containerd#9480

(on my phone right now, but PTAL if that makes sense; also /cc @dmcgowan )

[tonistiigi]

BuildKit uses the detect package also for recorder capability https://github.com/moby/buildkit/blob/master/util/tracing/detect/recorder.go and needs to capture the exporter.

[jsternberg]

Closing this because it is quite old and in draft. If this is still important, please contact me and I can probably try to pick this up or someone else can pick it up and create a new PR.