metrics: add metrics for bake command by jsternberg · Pull Request #2610 · docker/buildx (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation9 Commits1 Checks106 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This adds metrics for the bake command using a different method of calculating the build identifier but with the same attributes otherwise.
Comment on lines 632 to 622
| 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) |
| } |
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.
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?
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:
and retrieved here
| rfiles, inp, err = bake.ReadRemoteFiles(ctx, nodes, url, rnames, pw) |
|---|
Stat of these definitions are not on the host but memory:
| 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.
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.
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 cpy = s is supposed to be s = cpy. Just going to change it to return cpy just to avoid the entire thing.
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 jonathan.sternberg@docker.com
Labelled as needs follow-up to not forget for itg tests