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

metrics: add build command duration metric #2225

Merged
merged 1 commit into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 4 additions & 29 deletions build/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,8 @@ import (
"bufio"
"bytes"
"context"
"crypto/rand"
_ "crypto/sha256" // ensure digests can be computed
"encoding/base64"
"encoding/hex"
"encoding/json"
"fmt"
"io"
Expand All @@ -26,9 +24,11 @@ import (
"github.com/distribution/reference"
"github.com/docker/buildx/builder"
"github.com/docker/buildx/driver"
"github.com/docker/buildx/util/confutil"
"github.com/docker/buildx/util/desktop"
"github.com/docker/buildx/util/dockerutil"
"github.com/docker/buildx/util/imagetools"
"github.com/docker/buildx/util/osutil"
"github.com/docker/buildx/util/progress"
"github.com/docker/buildx/util/resolver"
"github.com/docker/buildx/util/waitmap"
Expand Down Expand Up @@ -389,7 +389,7 @@ func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt Op
if p, err := filepath.Abs(sharedKey); err == nil {
sharedKey = filepath.Base(p)
}
so.SharedKey = sharedKey + ":" + tryNodeIdentifier(configDir)
so.SharedKey = sharedKey + ":" + confutil.TryNodeIdentifier(configDir)
}

if opt.Pull {
Expand Down Expand Up @@ -1148,7 +1148,7 @@ func LoadInputs(ctx context.Context, d *driver.DriverHandle, inp Inputs, pw prog
target.LocalDirs["context"] = inp.ContextPath
}
}
case isLocalDir(inp.ContextPath):
case osutil.IsLocalDir(inp.ContextPath):
target.LocalDirs["context"] = inp.ContextPath
switch inp.DockerfilePath {
case "-":
Expand Down Expand Up @@ -1465,31 +1465,6 @@ func handleLowercaseDockerfile(dir, p string) string {
return p
}

var nodeIdentifierMu sync.Mutex

func tryNodeIdentifier(configDir string) (out string) {
nodeIdentifierMu.Lock()
defer nodeIdentifierMu.Unlock()
sessionFile := filepath.Join(configDir, ".buildNodeID")
if _, err := os.Lstat(sessionFile); err != nil {
if os.IsNotExist(err) { // create a new file with stored randomness
b := make([]byte, 8)
if _, err := rand.Read(b); err != nil {
return out
}
if err := os.WriteFile(sessionFile, []byte(hex.EncodeToString(b)), 0600); err != nil {
return out
}
}
}

dt, err := os.ReadFile(sessionFile)
if err == nil {
return string(dt)
}
return
}

func noPrintFunc(opt map[string]Options) bool {
for _, v := range opt {
if v.PrintFunc != nil {
Expand Down
19 changes: 6 additions & 13 deletions build/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"strings"

"github.com/docker/buildx/util/gitutil"
"github.com/docker/buildx/util/osutil"
"github.com/moby/buildkit/client"
specs "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/pkg/errors"
Expand Down Expand Up @@ -46,9 +47,9 @@ func getGitAttributes(ctx context.Context, contextPath string, dockerfilePath st
if filepath.IsAbs(contextPath) {
wd = contextPath
} else {
wd, _ = filepath.Abs(filepath.Join(getWd(), contextPath))
wd, _ = filepath.Abs(filepath.Join(osutil.GetWd(), contextPath))
}
wd = gitutil.SanitizePath(wd)
wd = osutil.SanitizePath(wd)

gitc, err := gitutil.New(gitutil.WithContext(ctx), gitutil.WithWorkingDir(wd))
if err != nil {
Expand Down Expand Up @@ -104,7 +105,7 @@ func getGitAttributes(ctx context.Context, contextPath string, dockerfilePath st
dockerfilePath = filepath.Join(wd, "Dockerfile")
}
if !filepath.IsAbs(dockerfilePath) {
dockerfilePath = filepath.Join(getWd(), dockerfilePath)
dockerfilePath = filepath.Join(osutil.GetWd(), dockerfilePath)
}
if r, err := filepath.Rel(root, dockerfilePath); err == nil && !strings.HasPrefix(r, "..") {
res["label:"+DockerfileLabel] = r
Expand All @@ -124,21 +125,13 @@ func getGitAttributes(ctx context.Context, contextPath string, dockerfilePath st
if err != nil {
continue
}
if lp, err := getLongPathName(dir); err == nil {
if lp, err := osutil.GetLongPathName(dir); err == nil {
dir = lp
}
dir = gitutil.SanitizePath(dir)
dir = osutil.SanitizePath(dir)
if r, err := filepath.Rel(root, dir); err == nil && !strings.HasPrefix(r, "..") {
so.FrontendAttrs["vcs:localdir:"+k] = r
}
}
}, nil
}

func getWd() string {
wd, _ := os.Getwd()
if lp, err := getLongPathName(wd); err == nil {
return lp
}
return wd
}
9 changes: 0 additions & 9 deletions build/git_unix.go

This file was deleted.

6 changes: 0 additions & 6 deletions build/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"bytes"
"context"
"net"
"os"
"strings"

"github.com/docker/buildx/driver"
Expand Down Expand Up @@ -34,11 +33,6 @@ func IsRemoteURL(c string) bool {
return false
}

func isLocalDir(c string) bool {
st, err := os.Stat(c)
return err == nil && st.IsDir()
}

func isArchive(header []byte) bool {
for _, m := range [][]byte{
{0x42, 0x5A, 0x68}, // bzip2
Expand Down
93 changes: 93 additions & 0 deletions commands/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ package commands
import (
"bytes"
"context"
"crypto/sha256"
"encoding/base64"
"encoding/csv"
"encoding/hex"
"encoding/json"
"fmt"
"io"
Expand All @@ -13,6 +15,8 @@ import (
"path/filepath"
"strconv"
"strings"
"sync"
"time"

"github.com/containerd/console"
"github.com/docker/buildx/build"
Expand All @@ -28,9 +32,11 @@ import (
"github.com/docker/buildx/store/storeutil"
"github.com/docker/buildx/util/buildflags"
"github.com/docker/buildx/util/cobrautil"
"github.com/docker/buildx/util/confutil"
"github.com/docker/buildx/util/desktop"
"github.com/docker/buildx/util/ioset"
"github.com/docker/buildx/util/metricutil"
"github.com/docker/buildx/util/osutil"
"github.com/docker/buildx/util/progress"
"github.com/docker/buildx/util/tracing"
"github.com/docker/cli-docs-tool/annotation"
Expand All @@ -52,6 +58,8 @@ import (
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/metric"
"google.golang.org/grpc/codes"
)

Expand Down Expand Up @@ -211,6 +219,52 @@ func (o *buildOptions) toDisplayMode() (progressui.DisplayMode, error) {
return progress, nil
}

func buildMetricAttributes(dockerCli command.Cli, b *builder.Builder, options *buildOptions) attribute.Set {
return attribute.NewSet(
attribute.String("command.name", "build"),
attribute.Stringer("command.options.hash", &buildOptionsHash{
buildOptions: options,
configDir: confutil.ConfigDir(dockerCli),
}),
attribute.String("driver.name", options.builder),
attribute.String("driver.type", b.Driver),
)
}

// buildOptionsHash computes a hash for the buildOptions when the String method is invoked.
// This is done so we can delay the computation of the hash until needed by OTEL using
// the fmt.Stringer interface.
type buildOptionsHash struct {
*buildOptions
configDir string
result string
resultOnce sync.Once
}

func (o *buildOptionsHash) String() string {
o.resultOnce.Do(func() {
target := o.target
contextPath := o.contextPath
Copy link
Member

@tonistiigi tonistiigi Feb 2, 2024

Choose a reason for hiding this comment

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

These paths need to be normalized so they always generate same hash(Eg. made absolute).

As this is leaking the system paths and Git info it still needs the salt like #2185 (comment) . The multi-machine case doesn't apply anyway as system paths are different, and I don't think there is a way to implement it safely with some other way.

If the path is a URL then the URL should go into hash. If it is - then maybe workdir is better than just const character? (but it needs salt)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since this code is in a new place, I don't have easy access to the node identifier anymore. I'd have to make that function public.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've also now changed this so it will attempt to transform context path into a relative path. If a git directory is present, the relative path will be dependent on the root directory of the git repository.

dockerfile := o.dockerfileName
if dockerfile == "" {
dockerfile = "Dockerfile"
}

if contextPath != "-" && osutil.IsLocalDir(contextPath) {
contextPath = osutil.ToAbs(contextPath)
}
salt := confutil.TryNodeIdentifier(o.configDir)

h := sha256.New()
for _, s := range []string{target, contextPath, dockerfile, salt} {
_, _ = io.WriteString(h, s)
h.Write([]byte{0})
}
o.result = hex.EncodeToString(h.Sum(nil))
})
return o.result
}

func runBuild(ctx context.Context, dockerCli command.Cli, options buildOptions) (err error) {
mp, err := metricutil.NewMeterProvider(ctx, dockerCli)
if err != nil {
Expand Down Expand Up @@ -279,6 +333,8 @@ func runBuild(ctx context.Context, dockerCli command.Cli, options buildOptions)
return err
}

attributes := buildMetricAttributes(dockerCli, b, &options)
done := timeBuildCommand(mp, attributes)
var resp *client.SolveResponse
var retErr error
if isExperimental() {
Expand All @@ -290,6 +346,8 @@ func runBuild(ctx context.Context, dockerCli command.Cli, options buildOptions)
if err := printer.Wait(); retErr == nil {
retErr = err
}

done(retErr)
if retErr != nil {
return retErr
}
Expand Down Expand Up @@ -933,3 +991,38 @@ func maybeJSONArray(v string) []string {
}
return []string{v}
}

// timeBuildCommand will start a timer for timing the build command. It records the time when the returned
// function is invoked into a metric.
func timeBuildCommand(mp metric.MeterProvider, attrs attribute.Set) func(err error) {
meter := metricutil.Meter(mp)
counter, _ := meter.Float64Counter("command.time",
metric.WithDescription("Measures the duration of the build command."),
metric.WithUnit("ms"),
)

start := time.Now()
return func(err error) {
dur := float64(time.Since(start)) / float64(time.Millisecond)
extraAttrs := attribute.NewSet()
if err != nil {
extraAttrs = attribute.NewSet(
attribute.String("error.type", otelErrorType(err)),
)
}
counter.Add(context.Background(), dur,
metric.WithAttributeSet(attrs),
metric.WithAttributeSet(extraAttrs),
)
}
}

// otelErrorType returns an attribute for the error type based on the error category.
// If nil, this function returns an invalid attribute.
func otelErrorType(err error) string {
name := "generic"
if errors.Is(err, context.Canceled) {
name = "canceled"
}
return name
}
34 changes: 34 additions & 0 deletions util/confutil/node.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package confutil

import (
"crypto/rand"
"encoding/hex"
"os"
"path/filepath"
"sync"
)

var nodeIdentifierMu sync.Mutex

func TryNodeIdentifier(configDir string) (out string) {
nodeIdentifierMu.Lock()
defer nodeIdentifierMu.Unlock()
sessionFile := filepath.Join(configDir, ".buildNodeID")
if _, err := os.Lstat(sessionFile); err != nil {
if os.IsNotExist(err) { // create a new file with stored randomness
b := make([]byte, 8)
if _, err := rand.Read(b); err != nil {
return out
}
if err := os.WriteFile(sessionFile, []byte(hex.EncodeToString(b)), 0600); err != nil {
return out
}
}
}

dt, err := os.ReadFile(sessionFile)
if err == nil {
return string(dt)
}
return
}
3 changes: 2 additions & 1 deletion util/gitutil/gitutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"path/filepath"
"strings"

"github.com/docker/buildx/util/osutil"
"github.com/pkg/errors"
)

Expand Down Expand Up @@ -70,7 +71,7 @@ func (c *Git) RootDir() (string, error) {
if err != nil {
return "", err
}
return SanitizePath(root), nil
return osutil.SanitizePath(root), nil
}

func (c *Git) GitDir() (string, error) {
Expand Down
17 changes: 0 additions & 17 deletions util/gitutil/path.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import (
"os"
"os/exec"
"path/filepath"
"regexp"
"strings"

"github.com/moby/sys/mountinfo"
)
Expand Down Expand Up @@ -42,18 +40,3 @@ func gitPath(wd string) (string, error) {
}
return exec.LookPath("git")
}

var windowsPathRegex = regexp.MustCompile(`^[A-Za-z]:[\\/].*$`)

func SanitizePath(path string) string {
// If we're running in WSL, we need to convert Windows paths to Unix paths.
// This is because the git binary can be invoked through `git.exe` and
// therefore returns Windows paths.
if os.Getenv("WSL_DISTRO_NAME") != "" && windowsPathRegex.MatchString(path) {
unixPath := strings.ReplaceAll(path, "\\", "/")
drive := strings.ToLower(string(unixPath[0]))
rest := filepath.Clean(unixPath[3:])
return filepath.Join("/mnt", drive, rest)
}
return filepath.Clean(path)
}
Loading
Loading