Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

Build metrics #2104

Merged
merged 4 commits into from
Nov 23, 2021
Merged

Build metrics #2104

merged 4 commits into from
Nov 23, 2021

Conversation

crazy-max
Copy link
Contributor

This PR handles metrics for build commands. It also adds a new field Metadata to the Command struct for extensibility. It could be used for other commands in the future.

Detecting build backend used by buildx is a bit tedious in the current release because no formatter is available to handle a proper output with buildx ls. The ls command locks the buildx store and that can take some time (top 20s) to complete because it needs to check the liveliness of the nodes so user cannot use buildx during this time (docker/buildx#764). That will cause some bad user experience if we rely on this command for metrics. I opened a pull request to start make things easier for us in the future (docker/buildx#830).

So for our purpose we directly read from the store without using an actual command which is more effective:

$ cat ~/.docker/buildx/current
{"Key":"unix:///var/run/docker.sock","Name":"builder","Global":false}
# "builder" is the current instance used. read the instance info and retrieve the current driver:
$ cat ~/.docker/buildx/instances/builder
{"Name":"builder","Driver":"docker-container","Nodes":[{"Name":"builder0","Endpoint":"unix:///var/run/docker.sock","Platforms":null,"Flags":["--allow-insecure-entitlement","security.insecure","--allow-insecure-entitlement","network.host"],"DriverOpts":{"env.JAEGER_TRACE":"localhost:6831","image":"moby/buildkit:latest","network":"host"},"Files":null}],"Dynamic":false}
# "docker-container" is the current driver

The step above is valid only if user doesn't provide the --builder flag to the build command (or BUILDX_BUILDER env var). If the builder is provided, we only have to read from the store in ~/.docker/buildx/instances/<builder>.

For the docker build command it's easier. We just need to read the cli config and check DOCKER_BUILDKIT env var as well as daemon config:

{
  "features": {
    "buildkit": true
  }
}

There is also a special case if a builder alias is being used:

$ cat ~/.docker/config.json
{
        "credsStore": "desktop.exe",
        "aliases": {
                "builder": "buildx"
        }
}

The following metadata will be send along the command:

{
        "build": {
                "cli": "docker | buildx | <alias>",
                "builder": "legacy | buildkit | <driver>"
        }
}
  • build.cli.docker: user type docker build ....
  • build.cli.buildx: user type docker buildx build ... or docker buildx bake ....
  • build.cli.<alias> : user type docker build ... but an alias is used in ~/.docker/config.json so set this one instead.
  • build.builder.legacy: legacy builder is used with docker build command.
  • build.server.buildkit: backend buildkit builder is used with docker build command.
  • build.server.<driver>: a driver is used (can be docker, docker-container or kubernetes atm). Only for buildx.

Metrics commands have also been regenerated and sorted in separate commits.

cc @chris-crone @tonistiigi @mat007 @gtardif

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max crazy-max force-pushed the build-metrics branch 6 times, most recently from af82c55 to a33a718 Compare November 16, 2021 12:17
@crazy-max crazy-max marked this pull request as ready for review November 16, 2021 13:28
cli/metrics/client.go Outdated Show resolved Hide resolved
cli/metrics/metrics.go Outdated Show resolved Hide resolved
cli/metrics/metrics.go Outdated Show resolved Hide resolved
cli/metrics/metrics.go Outdated Show resolved Hide resolved
cli/metrics/metrics_build.go Outdated Show resolved Hide resolved
@crazy-max crazy-max force-pushed the build-metrics branch 3 times, most recently from 0039c73 to 8758a65 Compare November 17, 2021 13:44
@crazy-max
Copy link
Contributor Author

crazy-max commented Nov 17, 2021

Putting in draft. I will add additional tests in the local/e2e/cli-only pkg.

cli/metrics/metadata/build.go Show resolved Hide resolved
cli/metrics/metadata/metadata.go Outdated Show resolved Hide resolved
cli/metrics/client.go Outdated Show resolved Hide resolved
cli/metrics/metrics.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the local Local context (moby) label Nov 19, 2021
@crazy-max crazy-max force-pushed the build-metrics branch 3 times, most recently from a92acaf to 4517b2a Compare November 19, 2021 15:14
@gtardif
Copy link
Contributor

gtardif commented Nov 19, 2021

Sorry I didn't comment in earlier on this ; adding what we just discussed on zoom with @crazy-max
Adding the metadata as a new field in metrics might be a bit trick, because of the aggregation that is done in pinata. Desktop aggregates counts of commands, and metrics are "bucketted" by status, context, command.
If we add metadata in the struct, I don't know how this will affect how we aggregate metrics in Desktop, and it might not be easy to find your information on the other end, in Looker.
An easier way to go with this is adding a few values (well defined, short list) as "source", like cli-build-container, etc. and use these values when the buildx command is invoked.
This will automatically translate in things we can query in looker (no need to change code in pinata).

Or we need to set another specific metrics field and also separate metrics based on that field in pinata (more bucketting than just based on status, context, cmd)

@crazy-max
Copy link
Contributor Author

@gtardif Thanks for your feedback! Using the existing source field sgtm.

@crazy-max crazy-max marked this pull request as ready for review November 19, 2021 16:23
@crazy-max
Copy link
Contributor Author

PTAL @mat007 @gtardif

Removed metadata field and instead extends the source field as suggested. Will look like this:

{"command":"build","context":"moby","source":"cli-docker;buildkit","status":"success"}
{"command":"buildx build","context":"moby","source":"cli-buildx;docker-container","status":"success"}
{"command":"buildx --builder build","context":"moby","source":"cli-buildx;docker-container","status":"success"}

cli/metrics/generatecommands/main.go Show resolved Hide resolved
cli/metrics/metadata/build.go Outdated Show resolved Hide resolved
local/e2e/cli-only/e2e_test.go Show resolved Hide resolved
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@mat007 mat007 merged commit cde09e2 into docker-archive:main Nov 23, 2021
@crazy-max crazy-max deleted the build-metrics branch November 23, 2021 13:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cli cli local Local context (moby) metrics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants