Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add history trace command #2904

Merged
merged 5 commits into from
Feb 11, 2025
Merged

Conversation

tonistiigi
Copy link
Member

Includes wip trace command on top of #2891

The goal was for this command to show a trace of the build directly, for example, in Jaeger UI, but it looks like there isn't a simple API that would allow uploading a trace to an existing Jaeger image (their upload feature is an ephemeral client-side-only solution).

Options would be to upload the trace spans again via controller APIs or build our own UI-only image that we could pair with a simple server that can return the trace JSON. Separating from #2891 in order to not block that PR.

The Jaeger conversion is mostly reuse of some old code. Looks like there is also a github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/jaeger package now that probably has similar functionality.

@tonistiigi
Copy link
Member Author

Added support for viewing traces using Jaeger UI.

// open trace for a ref in UI
docker buildx history trace <ref>

// open trace for last build in UI
docker buildx history trace

// save trace to file (eg. manual upload)
docker buildx history trace > trace.json

// open comparison trace with another build
docker buildx history trace --compare=otherref ref

// compare last build with the one before it
docker buildx history trace --compare '^1'

I think the UX is pretty ok and better than manual uploads and running jaeger-all-in-one. Maintainence-wise, there may be some questions though.

The old otelutil conversion code may need some updates as well. Looks like some info, like process data, could be deduplicated. I quickly looked at github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/jaeger but I don't think we have the original format for that converter, so still some work needed to use that.

@tonistiigi tonistiigi marked this pull request as ready for review February 6, 2025 07:17
@tonistiigi tonistiigi force-pushed the history-command-trace branch 2 times, most recently from 7f84270 to 3fd4ec0 Compare February 6, 2025 07:31
@thompson-shaun thompson-shaun added this to the v0.21.0 milestone Feb 6, 2025
@crazy-max
Copy link
Member

https://github.com/docker/buildx/actions/runs/13173913374/job/36769042423?pr=2904#step:6:1301

 === RUN   TestJaegerData
    jaeger_test.go:26: 
        	Error Trace:	D:/a/buildx/buildx/util/otelutil/jaeger_test.go:26
        	Error:      	Not equal:

I can't repro this one on a windows machine 🤔
I'm taking a closer look.

go.mod Outdated
@@ -1,6 +1,6 @@
module github.com/docker/buildx

go 1.23.0
go 1.23.1
Copy link
Member

Choose a reason for hiding this comment

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

It should be 1.23.0. Seems related to https://github.com/tonistiigi/jaeger-ui-rest/blob/bb2801509720cbedaac29fbefeeef9ba24e54c45/go.mod#L3

Looking at code on this repo I don't think there is any requirement for this specific patch release.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@crazy-max

This comment was marked as resolved.

tonistiigi and others added 5 commits February 11, 2025 11:38
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
@tonistiigi tonistiigi force-pushed the history-command-trace branch from 10499d4 to dc27815 Compare February 11, 2025 19:40
@tonistiigi tonistiigi merged commit 350d3f0 into docker:master Feb 11, 2025
129 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants