metrics: add build command duration metric #2225

jsternberg · 2024-01-30T21:16:04Z

This adds a build duration metric for the build command with attributes
related to the buildx driver, the error type (if any), and which options
were used to perform the build from a subset of the options.

crazy-max · 2024-01-31T13:31:26Z

util/osutil/path_windows.go


-import "golang.org/x/sys/windows"


Looks missing

Good catch. I'll have to watch for that in the future. I didn't realize my IDE wasn't running goimports on files that didn't match my build constraints.

tonistiigi · 2024-02-02T05:45:05Z

commands/build.go

+
+// getVCSPath returns the vcs:source attribute for a context path.
+func getVCSPath(contextPath string) string {
+	var wd string


Name should remain contextPath. Otherwise it is confusing why wd is passed to git command.

tonistiigi · 2024-02-02T05:52:49Z

commands/build.go

+	return o.result
+}
+
+// getVCSPath returns the vcs:source attribute for a context path.


The comment is wrong as this does not share implementation with vcs:source. Atm it would be "returns git remote URL". Rename the function as well as this isn't "Path".

tonistiigi · 2024-02-02T05:53:39Z

commands/build.go

+func getVCSPath(contextPath string) string {
+	var wd string
+	if filepath.IsAbs(contextPath) {
+		wd = contextPath


contextPath in here can be - or a URL.

The getGitAttributes function in build/git.go doesn't seem to handle this case. Does that need to be fixed too? It seems to get passed these parameters directly.

This code will also just fail and ignore the error if the git commands don't succeed. Is that suitable or do we need to catch the URL?

cc @crazy-max

I've updated this to include a Stat call before the logic. That should catch any paths that don't make sense before we start to try running git commands.

contextPath in here can be - or a URL.

Hum right this was never checked when getting Git attributes.

tonistiigi · 2024-02-02T06:04:40Z

commands/build.go

@@ -279,6 +362,7 @@ func runBuild(ctx context.Context, dockerCli command.Cli, options buildOptions)
 		return err
 	}

+	done := timeBuildCommand(mp, options.Attributes(dockerCli))


Make this (build)metricsAttributes(b, dockerCli, options) attribute.Set.

Don't need options.newBuilder() or other methods on options.

Inside the function, read the b.Driver directly from input without the struct/once. If you want then for the hash you can leave the lazy evaluation, but for just reading the driver property, there is no need for this overhead.

The call could also be inside timeBuildCommand.

Made the change. This isn't called inside of timeBuildCommand as there's another section of code that's going to use these same attributes in another PR.

tonistiigi · 2024-02-02T06:09:57Z

commands/build.go

+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 buildx command."),


"build command" seems more appropriate as looks specific to that command atm.

tonistiigi · 2024-02-02T06:11:19Z

commands/build.go

+
+// errorType returns an attribute for the error type based on the error category.
+// If nil, this function returns an invalid attribute.
+func errorType(err error) string {


otelErrorType/metricsErrorType maybe to make it clearer what "type" this is.

tonistiigi · 2024-02-02T17:50:16Z

commands/build.go

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


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)

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.

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.

tonistiigi · 2024-02-02T17:53:02Z

commands/build.go

+func getGitRemoteURL(contextPath string) string {
+	// Check if this is a valid path to begin with before attempting
+	// to run git. It might be a non-filesystem URL.
+	if _, err := os.Stat(contextPath); err != nil {


I think we should do the URL/- detection to match LoadInputs and unless I'm missing something, same validation would need to be called for the vcs codepath.

If you think this is outside of scope, then create a follow-up issue for it and assign to yourself.

I took a look at LoadInputs and it checks if the path is a local directory before checking for a URL. It seems to use this same code for that same functionality so I just exposed it and used that function instead. I've also included an explicit check for - to avoid attempting to call stat on that path and to avoid performing this logic at all in those circumstances.

We'll only do git remote url detection when it's a local directory.

@jsternberg Looks good

crazy-max · 2024-02-06T09:21:16Z

build/utils.go

@@ -34,7 +34,7 @@ func IsRemoteURL(c string) bool {
 	return false
 }

-func isLocalDir(c string) bool {
+func IsLocalDir(c string) bool {


Maybe move this to osutil?

crazy-max · 2024-02-06T09:25:48Z

util/gitutil/gitutil.go

+func ToAbs(path string) string {
+	if !filepath.IsAbs(path) {
+		path, _ = filepath.Abs(filepath.Join(osutil.GetWd(), path))
+	}
+	return SanitizePath(path)
+}


Could be moved to osutil as well but needs to move SanitizePath there too, can be for a follow-up.

crazy-max · 2024-02-06T09:27:45Z

commands/build.go

+func getVCSPath(contextPath string) string {
+	var wd string
+	if filepath.IsAbs(contextPath) {
+		wd = contextPath


contextPath in here can be - or a URL.

Hum right this was never checked when getting Git attributes.

crazy-max · 2024-02-06T09:29:03Z

commands/build.go

+func getGitRemoteURL(contextPath string) string {
+	// Check if this is a valid path to begin with before attempting
+	// to run git. It might be a non-filesystem URL.
+	if _, err := os.Stat(contextPath); err != nil {


@jsternberg Looks good

jsternberg · 2024-02-13T22:22:49Z

This PR is now also semi-dependent on this PR here: docker/cli#4875

At the current moment, we don't have a way of obtaining the alias that was used to launch buildx. That PR sets an environment variable so the plugin can find the original command name so we can include it in the metric.

jsternberg · 2024-02-14T21:03:20Z

I've removed the section of this that's dependent on docker/cli. That PR seems like it's likely going to come to either a different method of passing the data or setting something like OTEL_SERVICE_NAME and including this information as part of the service name.

I don't want to prevent this PR from being merged while we hash that out so I've removed it.

tonistiigi

@jsternberg Check the windows CI

This adds a build duration metric for the build command with attributes related to the buildx driver, the error type (if any), and which options were used to perform the build from a subset of the options. This also refactors some of the utility methods used by the git tool to determine filepaths into its own separate package so they can be reused in another place. Also adds a test to ensure the resource is initialized correctly and doesn't error. The otel handler logging message is suppressed on buildx invocations so we never see the error if there's a problem with the schema url. It's so easy to mess up the schema url when upgrading OTEL that we need a proper test to make sure we haven't broken the functionality. Signed-off-by: Jonathan A. Sternberg <[email protected]>

jsternberg · 2024-02-14T22:00:27Z

Was missing an import on the Windows build because my setup doesn't auto perform imports for systems that aren't compiled on my platform (aka windows in this case). Fixed it by adding the correct import. I'll keep an eye out because some stuff got moved around and that commonly means imports get removed and readded in new places so hopefully there's no more missing areas.

jsternberg mentioned this pull request Jan 30, 2024

metrics: add build duration metric #2185

Closed

jsternberg force-pushed the command-duration-metric branch from 997c258 to 006b74d Compare January 30, 2024 22:29

crazy-max reviewed Jan 31, 2024

View reviewed changes

jsternberg force-pushed the command-duration-metric branch 4 times, most recently from 99bea90 to 43ffe95 Compare February 1, 2024 18:10

jsternberg requested review from crazy-max and tonistiigi February 1, 2024 18:10

tonistiigi reviewed Feb 2, 2024

View reviewed changes

tonistiigi mentioned this pull request Feb 2, 2024

otel: create utility for storing common attributes for build options #2234

Closed

jsternberg force-pushed the command-duration-metric branch 2 times, most recently from db17998 to af1350b Compare February 2, 2024 17:25

jsternberg requested a review from tonistiigi February 2, 2024 17:33

tonistiigi reviewed Feb 2, 2024

View reviewed changes

jsternberg force-pushed the command-duration-metric branch 3 times, most recently from 555c4eb to 3d93cd6 Compare February 2, 2024 19:50

crazy-max reviewed Feb 6, 2024

View reviewed changes

jsternberg mentioned this pull request Feb 13, 2024

plugins: set OTEL_RESOURCE_ATTRIBUTES when invoking a plugin docker/cli#4875

Merged

jsternberg force-pushed the command-duration-metric branch from 3d93cd6 to a60f069 Compare February 13, 2024 22:04

jsternberg requested review from tonistiigi and crazy-max February 13, 2024 22:22

jsternberg force-pushed the command-duration-metric branch from a60f069 to 8f88d0e Compare February 14, 2024 20:59

jsternberg force-pushed the command-duration-metric branch from 8f88d0e to c42265f Compare February 14, 2024 21:03

tonistiigi requested changes Feb 14, 2024

View reviewed changes

jsternberg force-pushed the command-duration-metric branch from c42265f to bda968a Compare February 14, 2024 21:58

jsternberg requested a review from tonistiigi February 14, 2024 22:29

tonistiigi approved these changes Feb 14, 2024

View reviewed changes

tonistiigi merged commit c9d1c41 into docker:master Feb 14, 2024
63 checks passed

jsternberg deleted the command-duration-metric branch February 14, 2024 23:58

crazy-max added the area/metrics label Jun 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

metrics: add build command duration metric #2225

metrics: add build command duration metric #2225

jsternberg commented Jan 30, 2024

crazy-max Jan 31, 2024

jsternberg Jan 31, 2024

tonistiigi Feb 2, 2024 •

edited

Loading

tonistiigi Feb 2, 2024

tonistiigi Feb 2, 2024

jsternberg Feb 2, 2024

tonistiigi Feb 2, 2024

jsternberg Feb 2, 2024

crazy-max Feb 6, 2024 •

edited

Loading

tonistiigi Feb 2, 2024

jsternberg Feb 2, 2024

tonistiigi Feb 2, 2024

tonistiigi Feb 2, 2024

tonistiigi Feb 2, 2024 •

edited

Loading

jsternberg Feb 2, 2024

jsternberg Feb 2, 2024

tonistiigi Feb 2, 2024

jsternberg Feb 2, 2024

crazy-max Feb 6, 2024

crazy-max Feb 6, 2024

crazy-max Feb 6, 2024

crazy-max Feb 6, 2024 •

edited

Loading

crazy-max Feb 6, 2024

jsternberg commented Feb 13, 2024

jsternberg commented Feb 14, 2024

tonistiigi left a comment

jsternberg commented Feb 14, 2024

metrics: add build command duration metric #2225

metrics: add build command duration metric #2225

Conversation

jsternberg commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazy-max Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazy-max Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsternberg commented Feb 13, 2024

jsternberg commented Feb 14, 2024

tonistiigi left a comment

Choose a reason for hiding this comment

jsternberg commented Feb 14, 2024

tonistiigi Feb 2, 2024 •

edited

Loading

crazy-max Feb 6, 2024 •

edited

Loading

tonistiigi Feb 2, 2024 •

edited

Loading

crazy-max Feb 6, 2024 •

edited

Loading