Skip to content

Conversation

@nicks
Copy link
Contributor

@nicks nicks commented May 26, 2023

before this change, the threadsafe wrapper would hide the delegate interface, which breaks the ability
to send traces.

after this change, tracing works again!

fixes docker/buildx#1847

before this change, the threadsafe wrapper would hide
the delegate interface, which breaks the ability
to send traces.

after this change, tracing works again!

fixes docker/buildx#1847

Signed-off-by: Nick Santos <[email protected]>
}

func makeThreadSafe(exp sdktrace.SpanExporter) sdktrace.SpanExporter {
d, isDelegate := exp.(tracerDelegate)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should detect the *jaeger.Exporter instead and only make that thread-safe. Delegated exporter is already thread-safe.

Copy link
Member

@tonistiigi tonistiigi May 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or actually just wrap it in jaegerExporter(), not in the detect pkg.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya, maybe the real solution is to complain upstream about the jaeger exporter not being thread-safe. in any case, i moved this code into the jaeger package.

@tonistiigi
Copy link
Member

@jedevc We need to take this into v0.11 buildx

Signed-off-by: Nick Santos <[email protected]>
@nicks nicks requested a review from tonistiigi May 26, 2023 21:13
@tonistiigi tonistiigi merged commit e468373 into moby:master May 26, 2023
@nicks nicks deleted the nicks/tracing branch May 26, 2023 21:19
@thaJeztah
Copy link
Member

@jedevc We need to take this into v0.11 buildx

do we need this for the 0.11 branch as well, for the docker builder?

@tonistiigi
Copy link
Member

@thaJeztah dockerd does not support configuring telemetry endpoints. For the usage over History API I opened moby/moby#45579 . That one will trace only dockerd if buildx side has this issue. So, no this is not needed in Moby, but if you want buildx traces to be captured in Moby History API, then 45579 is needed.

@thaJeztah
Copy link
Member

@tonistiigi ah, gotcha.

Disclaimer: haven't fully followed all things, but I knew @milas was working on OTEL support (which I expect would also be for the "docker" builder), hence wondering if this would be related (and needed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

opentelemetry support does not send buildx spans

3 participants