Merge pull request #741 from gtardif/fix_cli_metrics

Simplify metrics gathering, separate running commands from getting help

cli/main.go

@@ -198,37 +198,37 @@ func main() {
 	if err = root.ExecuteContext(ctx); err != nil {
 		// if user canceled request, simply exit without any error message
 		if errdefs.IsErrCanceled(err) || errors.Is(ctx.Err(), context.Canceled) {
-			metrics.Track(ctype, os.Args[1:], root.PersistentFlags(), metrics.CanceledStatus)
+			metrics.Track(ctype, os.Args[1:], metrics.CanceledStatus)
 			os.Exit(130)
 		}
 		if ctype == store.AwsContextType {
-			exit(root, currentContext, errors.Errorf(`%q context type has been renamed. Recreate the context by running:
+			exit(currentContext, errors.Errorf(`%q context type has been renamed. Recreate the context by running:
 $ docker context create %s <name>`, cc.Type(), store.EcsContextType), ctype)
 		}
 
 		// Context should always be handled by new CLI
 		requiredCmd, _, _ := root.Find(os.Args[1:])
 		if requiredCmd != nil && isContextAgnosticCommand(requiredCmd) {
-			exit(root, currentContext, err, ctype)
+			exit(currentContext, err, ctype)
 		}
 		mobycli.ExecIfDefaultCtxType(ctx, root)
 
-		checkIfUnknownCommandExistInDefaultContext(err, currentContext, root, ctype)
+		checkIfUnknownCommandExistInDefaultContext(err, currentContext, ctype)
 
-		exit(root, currentContext, err, ctype)
+		exit(currentContext, err, ctype)
 	}
-	metrics.Track(ctype, os.Args[1:], root.PersistentFlags(), metrics.SuccessStatus)
+	metrics.Track(ctype, os.Args[1:], metrics.SuccessStatus)
 }
 
-func exit(root *cobra.Command, ctx string, err error, ctype string) {
-	metrics.Track(ctype, os.Args[1:], root.PersistentFlags(), metrics.FailureStatus)
+func exit(ctx string, err error, ctype string) {
+	metrics.Track(ctype, os.Args[1:], metrics.FailureStatus)
 
 	if errors.Is(err, errdefs.ErrLoginRequired) {
 		fmt.Fprintln(os.Stderr, err)
 		os.Exit(errdefs.ExitCodeLoginRequired)
 	}
 	if errors.Is(err, errdefs.ErrNotImplemented) {
-		name := metrics.GetCommand(os.Args[1:], root.PersistentFlags())
+		name := metrics.GetCommand(os.Args[1:])
 		fmt.Fprintf(os.Stderr, "Command %q not available in current context (%s)\n", name, ctx)
 
 		os.Exit(1)
@@ -242,14 +242,14 @@ func fatal(err error) {
 	os.Exit(1)
 }
 
-func checkIfUnknownCommandExistInDefaultContext(err error, currentContext string, root *cobra.Command, contextType string) {
+func checkIfUnknownCommandExistInDefaultContext(err error, currentContext string, contextType string) {
 	submatch := unknownCommandRegexp.FindSubmatch([]byte(err.Error()))
 	if len(submatch) == 2 {
 		dockerCommand := string(submatch[1])
 
 		if mobycli.IsDefaultContextCommand(dockerCommand) {
 			fmt.Fprintf(os.Stderr, "Command %q not available in current context (%s), you can use the \"default\" context to run this command\n", dockerCommand, currentContext)
-			metrics.Track(contextType, os.Args[1:], root.PersistentFlags(), metrics.FailureStatus)
+			metrics.Track(contextType, os.Args[1:], metrics.FailureStatus)
 			os.Exit(1)
 		}
 	}

cli/mobycli/exec.go

@@ -86,7 +86,7 @@ func Exec(root *cobra.Command) {
 	err := cmd.Run()
 	childExit <- true
 	if err != nil {
-		metrics.Track(store.DefaultContextType, os.Args[1:], root.PersistentFlags(), metrics.FailureStatus)
+		metrics.Track(store.DefaultContextType, os.Args[1:], metrics.FailureStatus)
 
 		if exiterr, ok := err.(*exec.ExitError); ok {
 			os.Exit(exiterr.ExitCode())
@@ -94,7 +94,7 @@ func Exec(root *cobra.Command) {
 		fmt.Fprintln(os.Stderr, err)
 		os.Exit(1)
 	}
-	metrics.Track(store.DefaultContextType, os.Args[1:], root.PersistentFlags(), metrics.SuccessStatus)
+	metrics.Track(store.DefaultContextType, os.Args[1:], metrics.SuccessStatus)
 
 	os.Exit(0)
 }

metrics/commands.go

@@ -0,0 +1,147 @@
+/*
+   Copyright 2020 Docker Compose CLI authors
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package metrics
+
+var commandFlags = []string{
+	//added to catch scan details
+	"--version", "--login",
+}
+
+// Generated with generatecommands/main.go
+var managementCommands = []string{
+	"ecs",
+	"scan",
+	"app",
+	"builder",
+	"imagetools",
+	"buildx",
+	"checkpoint",
+	"config",
+	"container",
+	"context",
+	"create",
+	"image",
+	"manifest",
+	"network",
+	"node",
+	"plugin",
+	"secret",
+	"service",
+	"stack",
+	"swarm",
+	"system",
+	"key",
+	"signer",
+	"trust",
+	"volume",
+	"login",
+	"logout",
+	"compose",
+}
+
+var commands = []string{
+	"bundle",
+	"completion",
+	"init",
+	"inspect",
+	"install",
+	"deploy",
+	"list",
+	"ls",
+	"merge",
+	"pull",
+	"push",
+	"render",
+	"split",
+	"status",
+	"uninstall",
+	"upgrade",
+	"validate",
+	"version",
+	"build",
+	"prune",
+	"create",
+	"bake",
+	"f",
+	"b",
+	"du",
+	"rm",
+	"stop",
+	"use",
+	"remove",
+	"attach",
+	"commit",
+	"cp",
+	"diff",
+	"exec",
+	"export",
+	"kill",
+	"logs",
+	"ps",
+	"pause",
+	"port",
+	"rename",
+	"restart",
+	"run",
+	"start",
+	"stats",
+	"top",
+	"unpause",
+	"update",
+	"wait",
+	"aci",
+	"ecs",
+	"show",
+	"history",
+	"import",
+	"load",
+	"images",
+	"rmi",
+	"save",
+	"tag",
+	"annotate",
+	"connect",
+	"disconnect",
+	"demote",
+	"promote",
+	"disable",
+	"enable",
+	"set",
+	"rollback",
+	"scale",
+	"up",
+	"down",
+	"services",
+	"ca",
+	"join",
+	"join-token",
+	"leave",
+	"unlock",
+	"unlock-key",
+	"df",
+	"events",
+	"info",
+	"generate",
+	"add",
+	"revoke",
+	"sign",
+	"login",
+	"azure",
+	"logout",
+	"search",
+	"convert",
+}

metrics/generatecommands/main.go

@@ -0,0 +1,106 @@
+/*
+   Copyright 2020 Docker Compose CLI authors
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package main
+
+import (
+	"fmt"
+	"os/exec"
+	"strings"
+
+	"github.com/docker/compose-cli/utils"
+)
+
+var managementCommands = []string{"ecs", "scan"}
+
+var commands = []string{}
+
+func main() {
+	fmt.Println("Walking through docker help to list commands...")
+	getCommands()
+	getCommands("compose")
+
+	fmt.Printf(`
+var managementCommands = []string{
+	"%s",
+}
+
+var commands = []string{
+	"%s",
+}
+`, strings.Join(managementCommands, "\", \n\t\""), strings.Join(commands, "\", \n\t\""))
+}
+
+const (
+	mgtCommandsSection = "Management Commands:"
+	commandsSection    = "Commands:"
+	aliasesSection     = "Aliases:"
+)
+
+func getCommands(execCommands ...string) {
+	withHelp := append(execCommands, "--help")
+	cmd := exec.Command("docker", withHelp...)
+	output, err := cmd.Output()
+	if err != nil {
+		return
+	}
+	text := string(output)
+	lines := strings.Split(text, "\n")
+	section := ""
+	for _, line := range lines {
+		trimmedLine := strings.TrimSpace(line)
+		if strings.HasPrefix(trimmedLine, mgtCommandsSection) {
+			section = mgtCommandsSection
+			continue
+		}
+		if strings.HasPrefix(trimmedLine, commandsSection) || strings.HasPrefix(trimmedLine, "Available Commands:") {
+			section = commandsSection
+			if len(execCommands) > 0 {
+				command := execCommands[len(execCommands)-1]
+				managementCommands = append(managementCommands, command)
+			}
+			continue
+		}
+		if strings.HasPrefix(trimmedLine, aliasesSection) {
+			section = aliasesSection
+			continue
+		}
+		if trimmedLine == "" {
+			section = ""
+			continue
+		}
+
+		tokens := strings.Split(trimmedLine, " ")
+		command := strings.Replace(tokens[0], "*", "", 1)
+		switch section {
+		case mgtCommandsSection:
+			getCommands(append(execCommands, command)...)
+		case commandsSection:
+			if !utils.StringContains(commands, command) {
+				commands = append(commands, command)
+			}
+			getCommands(append(execCommands, command)...)
+		case aliasesSection:
+			aliases := strings.Split(trimmedLine, ",")
+			for _, alias := range aliases {
+				trimmedAlias := strings.TrimSpace(alias)
+				if !utils.StringContains(commands, trimmedAlias) {
+					commands = append(commands, trimmedAlias)
+				}
+			}
+		}
+	}
+}

metrics/metrics.go

@@ -17,68 +17,12 @@
 package metrics
 
 import (
-	"strings"
-
-	flag "github.com/spf13/pflag"
-
 	"github.com/docker/compose-cli/utils"
 )
 
-var managementCommands = []string{
-	"app",
-	"assemble",
-	"builder",
-	"buildx",
-	"ecs",
-	"ecs compose",
-	"cluster",
-	"compose",
-	"config",
-	"container",
-	"context",
-	// We add "context create" as a management command to be able to catch
-	// calls to "context create aci"
-	"context create",
-	"help",
-	"image",
-	// Adding "login" as a management command so that the system can catch
-	// commands like `docker login azure`
-	"login",
-	"manifest",
-	"network",
-	"node",
-	"plugin",
-	"registry",
-	"secret",
-	"service",
-	"stack",
-	"swarm",
-	"system",
-	"template",
-	"trust",
-	"volume",
-}
-
-// managementSubCommands holds a list of allowed subcommands of a management
-// command. For example we want to send an event for "docker login azure" but
-// we don't wat to send the name of the registry when the user does a
-// "docker login my-registry", we only want to send "login"
-var managementSubCommands = map[string][]string{
-	"login": {
-		"azure",
-	},
-	"context create": {
-		"aci",
-	},
-}
-
-const (
-	scanCommand = "scan"
-)
-
 // Track sends the tracking analytics to Docker Desktop
-func Track(context string, args []string, flags *flag.FlagSet, status string) {
-	command := GetCommand(args, flags)
+func Track(context string, args []string, status string) {
+	command := GetCommand(args)
 	if command != "" {
 		c := NewClient()
 		c.Send(Command{
@@ -90,115 +34,39 @@ func Track(context string, args []string, flags *flag.FlagSet, status string) {
 	}
 }
 
-// GetCommand get the invoked command
-func GetCommand(args []string, flags *flag.FlagSet) string {
-	command := ""
-	strippedArgs := stripFlags(args, flags)
-
-	if len(strippedArgs) != 0 {
-		command = strippedArgs[0]
-
-		if command == scanCommand {
-			return getScanCommand(args)
-		}
-
-		for {
-			if utils.StringContains(managementCommands, command) {
-				if sub := getSubCommand(command, strippedArgs[1:]); sub != "" {
-					command += " " + sub
-					strippedArgs = strippedArgs[1:]
-					continue
-				}
-			}
-			break
-		}
-	}
-
-	return command
+func isCommand(word string) bool {
+	return utils.StringContains(commands, word) || isManagementCommand(word)
 }
 
-func getScanCommand(args []string) string {
-	command := args[0]
-
-	if utils.StringContains(args, "--auth") {
-		return command + " auth"
-	}
-
-	if utils.StringContains(args, "--version") {
-		return command + " version"
-	}
-
-	return command
+func isManagementCommand(word string) bool {
+	return utils.StringContains(managementCommands, word)
 }
 
-func getSubCommand(command string, args []string) string {
-	if len(args) == 0 {
-		return ""
-	}
-
-	if val, ok := managementSubCommands[command]; ok {
-		if utils.StringContains(val, args[0]) {
-			return args[0]
-		}
-		return ""
-	}
-
-	if isArg(args[0]) {
-		return args[0]
-	}
-
-	return ""
+func isCommandFlag(word string) bool {
+	return utils.StringContains(commandFlags, word)
 }
 
-func stripFlags(args []string, flags *flag.FlagSet) []string {
-	commands := []string{}
-
-	for len(args) > 0 {
-		s := args[0]
-		args = args[1:]
-
-		if s == "--" {
-			return commands
+// GetCommand get the invoked command
+func GetCommand(args []string) string {
+	result := ""
+	onlyFlags := false
+	for _, arg := range args {
+		if arg == "--help" {
+			return ""
 		}
-
-		if flagArg(s, flags) {
-			if len(args) <= 1 {
-				return commands
-			}
-			args = args[1:]
+		if arg == "--" {
+			break
 		}
-
-		if isArg(s) {
-			commands = append(commands, s)
+		if isCommandFlag(arg) || (!onlyFlags && isCommand(arg)) {
+			if result == "" {
+				result = arg
+			} else {
+				result = result + " " + arg
+			}
+			if !isManagementCommand(arg) {
+				onlyFlags = true
+			}
 		}
 	}
-
-	return commands
-}
-
-func flagArg(s string, flags *flag.FlagSet) bool {
-	return strings.HasPrefix(s, "--") && !strings.Contains(s, "=") && !hasNoOptDefVal(s[2:], flags) ||
-		strings.HasPrefix(s, "-") && !strings.Contains(s, "=") && len(s) == 2 && !shortHasNoOptDefVal(s[1:], flags)
-}
-
-func isArg(s string) bool {
-	return s != "" && !strings.HasPrefix(s, "-")
-}
-
-func hasNoOptDefVal(name string, fs *flag.FlagSet) bool {
-	flag := fs.Lookup(name)
-	if flag == nil {
-		return false
-	}
-
-	return flag.NoOptDefVal != ""
-}
-
-func shortHasNoOptDefVal(name string, fs *flag.FlagSet) bool {
-	flag := fs.ShorthandLookup(name[:1])
-	if flag == nil {
-		return false
-	}
-
-	return flag.NoOptDefVal != ""
+	return result
 }

metrics/metrics_test.go

@@ -19,15 +19,10 @@ package metrics
 import (
 	"testing"
 
-	"github.com/spf13/cobra"
 	"gotest.tools/v3/assert"
 )
 
-func TestFlag(t *testing.T) {
-	root := &cobra.Command{}
-	root.PersistentFlags().BoolP("debug", "D", false, "debug")
-	root.PersistentFlags().String("str", "str", "str")
-
+func TestGetCommand(t *testing.T) {
 	testCases := []struct {
 		name     string
 		args     []string
@@ -58,16 +53,6 @@ func TestFlag(t *testing.T) {
 			args:     []string{"--debug", "--str", "str-value"},
 			expected: "",
 		},
-		{
-			name:     "with unknown short flag",
-			args:     []string{"-f", "run"},
-			expected: "",
-		},
-		{
-			name:     "with unknown long flag",
-			args:     []string{"--unknown-flag", "run"},
-			expected: "",
-		},
 		{
 			name:     "management command",
 			args:     []string{"image", "ls"},
@@ -76,7 +61,7 @@ func TestFlag(t *testing.T) {
 		{
 			name:     "management command with flag",
 			args:     []string{"image", "--test", "ls"},
-			expected: "image",
+			expected: "image ls",
 		},
 		{
 			name:     "management subcommand with flag",
@@ -88,26 +73,36 @@ func TestFlag(t *testing.T) {
 			args:     []string{"login", "azure"},
 			expected: "login azure",
 		},
+		{
+			name:     "azure logout",
+			args:     []string{"logout", "azure"},
+			expected: "logout azure",
+		},
 		{
 			name:     "azure login with flags",
 			args:     []string{"login", "-u", "test", "azure"},
 			expected: "login azure",
 		},
 		{
-			name:     "azure login with azure user",
-			args:     []string{"login", "-u", "azure"},
+			name:     "login to a registry",
+			args:     []string{"login", "myregistry"},
 			expected: "login",
 		},
 		{
-			name:     "login to a registry",
-			args:     []string{"login", "registry"},
-			expected: "login",
+			name:     "logout from a registry",
+			args:     []string{"logout", "myregistry"},
+			expected: "logout",
 		},
 		{
 			name:     "context create aci",
 			args:     []string{"context", "create", "aci"},
 			expected: "context create aci",
 		},
+		{
+			name:     "context create ecs",
+			args:     []string{"context", "create", "ecs"},
+			expected: "context create ecs",
+		},
 		{
 			name:     "create a context from another context",
 			args:     []string{"context", "create", "test-context", "--from=default"},
@@ -119,9 +114,9 @@ func TestFlag(t *testing.T) {
 			expected: "create",
 		},
 		{
-			name:     "create a container named aci",
-			args:     []string{"create", "aci"},
-			expected: "create",
+			name:     "start a container named aci",
+			args:     []string{"start", "aci"},
+			expected: "start",
 		},
 		{
 			name:     "create a container named test-container",
@@ -152,15 +147,44 @@ func TestFlag(t *testing.T) {
 
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			result := GetCommand(testCase.args, root.PersistentFlags())
+			result := GetCommand(testCase.args)
 			assert.Equal(t, testCase.expected, result)
 		})
 	}
 }
 
-func TestEcs(t *testing.T) {
-	root := &cobra.Command{}
+func TestIgnoreHelpCommands(t *testing.T) {
+	testCases := []struct {
+		name     string
+		args     []string
+		expected string
+	}{
+		{
+			name:     "help",
+			args:     []string{"--help"},
+			expected: "",
+		},
+		{
+			name:     "help on run",
+			args:     []string{"run", "--help"},
+			expected: "",
+		},
+		{
+			name:     "help on compose up",
+			args:     []string{"compose", "up", "--help"},
+			expected: "",
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			result := GetCommand(testCase.args)
+			assert.Equal(t, testCase.expected, result)
+		})
+	}
+}
 
+func TestEcs(t *testing.T) {
 	testCases := []struct {
 		name     string
 		args     []string
@@ -217,23 +241,21 @@ func TestEcs(t *testing.T) {
 			expected: "ecs compose logs",
 		},
 		{
-			name:     "setup",
-			args:     []string{"ecs", "setup"},
-			expected: "ecs setup",
+			name:     "ecs",
+			args:     []string{"ecs", "anything"},
+			expected: "ecs",
 		},
 	}
 
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			result := GetCommand(testCase.args, root.PersistentFlags())
+			result := GetCommand(testCase.args)
 			assert.Equal(t, testCase.expected, result)
 		})
 	}
 }
 
 func TestScan(t *testing.T) {
-	root := &cobra.Command{}
-
 	testCases := []struct {
 		name     string
 		args     []string
@@ -246,34 +268,34 @@ func TestScan(t *testing.T) {
 		},
 		{
 			name:     "scan image with long flags",
-			args:     []string{"scan", "--file", "file", "image"},
+			args:     []string{"scan", "--file", "file", "myimage"},
 			expected: "scan",
 		},
 		{
 			name:     "scan image with short flags",
-			args:     []string{"scan", "-f", "file", "image"},
+			args:     []string{"scan", "-f", "file", "myimage"},
 			expected: "scan",
 		},
 		{
 			name:     "scan with long flag",
-			args:     []string{"scan", "--dependency-tree", "image"},
+			args:     []string{"scan", "--dependency-tree", "myimage"},
 			expected: "scan",
 		},
 		{
 			name:     "auth",
-			args:     []string{"scan", "--auth"},
-			expected: "scan auth",
+			args:     []string{"scan", "--login"},
+			expected: "scan --login",
 		},
 		{
 			name:     "version",
 			args:     []string{"scan", "--version"},
-			expected: "scan version",
+			expected: "scan --version",
 		},
 	}
 
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			result := GetCommand(testCase.args, root.PersistentFlags())
+			result := GetCommand(testCase.args)
 			assert.Equal(t, testCase.expected, result)
 		})
 	}

tests/e2e/e2e_test.go

@@ -164,6 +164,17 @@ func TestContextMetrics(t *testing.T) {
 	s.Start()
 	defer s.Stop()
 
+	t.Run("do not send metrics on help commands", func(t *testing.T) {
+		s.ResetUsage()
+
+		c.RunDockerCmd("--help")
+		c.RunDockerCmd("ps", "--help")
+		c.RunDockerCmd("run", "--help")
+
+		usage := s.GetUsage()
+		assert.Equal(t, 0, len(usage))
+	})
+
 	t.Run("metrics on default context", func(t *testing.T) {
 		s.ResetUsage()
 
@@ -185,7 +196,7 @@ func TestContextMetrics(t *testing.T) {
 		c.RunDockerCmd("ps")
 		c.RunDockerCmd("context", "use", "test-example")
 		c.RunDockerCmd("ps")
-		c.RunDockerOrExitError("error")
+		c.RunDockerOrExitError("stop", "unknown")
 		c.RunDockerCmd("context", "use", "default")
 		c.RunDockerCmd("--context", "test-example", "ps")
 
@@ -195,7 +206,7 @@ func TestContextMetrics(t *testing.T) {
 		assert.Equal(t, `{"command":"ps","context":"moby","source":"cli","status":"success"}`, usage[1])
 		assert.Equal(t, `{"command":"context use","context":"moby","source":"cli","status":"success"}`, usage[2])
 		assert.Equal(t, `{"command":"ps","context":"example","source":"cli","status":"success"}`, usage[3])
-		assert.Equal(t, `{"command":"error","context":"example","source":"cli","status":"failure"}`, usage[4])
+		assert.Equal(t, `{"command":"stop","context":"example","source":"cli","status":"failure"}`, usage[4])
 		assert.Equal(t, `{"command":"context use","context":"example","source":"cli","status":"success"}`, usage[5])
 		assert.Equal(t, `{"command":"ps","context":"example","source":"cli","status":"success"}`, usage[6])
 	})