-
Notifications
You must be signed in to change notification settings - Fork 600
metrics: add metrics for bake command #2610
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
Conversation
commands/bake.go
Outdated
| var files []string | ||
| if len(o.files) == 0 { | ||
| files = []string{osutil.GetWd()} | ||
| } else { | ||
| files = make([]string, len(o.files)) | ||
| for i, file := range o.files { | ||
| files[i] = osutil.ToAbs(file) | ||
| } | ||
| sort.Strings(files) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that would work for remote bake definitions, maybe using cmdContext would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand on what you mean by this? I'm not very familiar with remote bake definitions. I'm guessing this is referring to osutil.ToAbs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this is referring to
osutil.ToAbs?
Yes I mean osutil.* calls that would not be right for remote bake definition set here:
Line 68 in e2f6808
| url = targets[0] |
Line 474 in e2f6808
| rfiles, inp, err = bake.ReadRemoteFiles(ctx, nodes, url, rnames, pw) |
Stat of these definitions are not on the host but memory:
Lines 186 to 194 in e2f6808
| dt, err = ref.ReadFile(ctx, gwclient.ReadRequest{ | |
| Filename: filename, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| return []File{{Name: name, Data: dt}}, nil |
Hence why I think we should use cmdContext and the relative path of files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I think I understand now. Do you think that it would be better to just include the url and cmdContext attributes in the hash instead of trying to resolve the path of the files? The intention is just to differentiate different bake commands in different directories on the same machine. I don't think we need to resolve the absolute path to these files.
I'll mock something up and see how it looks.
55a2957 to
5207602
Compare
5207602 to
01ce010
Compare
crazy-max
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some lint issues otherwise looks pretty good! https://github.com/docker/buildx/actions/runs/10286248497/job/28466467965?pr=2610#step:5:368
#18 [lint 1/1] RUN --mount=target=/go/src/github.com/docker/buildx --mount=target=/root/.cache,type=cache,id=lint-cache-darwin/amd64 xx-go --wrap && golangci-lint run
#18 85.65 commands/bake.go:649:3: ineffectual assignment to cpy (ineffassign)
#18 85.65 cpy = s
#18 85.65 ^
#18 ERROR: process "/bin/sh -c xx-go --wrap && golangci-lint run" did not complete successfully: exit code: 1
|
Not for this PR but I wonder we could have integration tests pushing metrics to a local endpoint in the sandbox similar to minio integration for s3 remote cache moby/buildkit#3398? |
|
We don't have any integration tests like that but I think we really need that at this point. |
|
Ha I must have been really tired for this one. The |
This adds metrics for the bake command using a different method of calculating the build identifier but with the same attributes otherwise. Signed-off-by: Jonathan A. Sternberg <[email protected]>
01ce010 to
58571ff
Compare
|
Labelled as needs follow-up to not forget for itg tests |
This adds metrics for the bake command using a different method of calculating the build identifier but with the same attributes otherwise.