Merge pull request #630 from docker/cli_metrics_failures

gtardif · web-flow · commit 01ea2488a235 · 2020-09-18T18:26:54.000+02:00
Cli metrics failures
diff --git a/aci/context.go b/aci/context.go
@@ -28,6 +28,7 @@ import (
 	"github.com/pkg/errors"
 
 	"github.com/docker/compose-cli/context/store"
+	"github.com/docker/compose-cli/errdefs"
 	"github.com/docker/compose-cli/prompt"
 )
 
@@ -40,7 +41,7 @@ type ContextParams struct {
 }
 
 // ErrSubscriptionNotFound is returned when a required subscription is not found
-var ErrSubscriptionNotFound = errors.New("subscription not found")
+var ErrSubscriptionNotFound = errors.Wrapf(errdefs.ErrNotFound, "subscription")
 
 // IsSubscriptionNotFoundError returns true if the unwrapped error is IsSubscriptionNotFoundError
 func IsSubscriptionNotFoundError(err error) bool {
@@ -138,7 +139,7 @@ func (helper contextCreateACIHelper) chooseGroup(ctx context.Context, subscripti
 	group, err := helper.selector.Select("Select a resource group", groupNames)
 	if err != nil {
 		if err == terminal.InterruptErr {
-			os.Exit(0)
+			return resources.Group{}, errdefs.ErrCanceled
 		}
 
 		return resources.Group{}, err
diff --git a/cli/cmd/context/create.go b/cli/cmd/context/create.go
@@ -70,7 +70,7 @@ $ docker context create my-context --description "some description" --docker "ho
 		Use:   "create CONTEXT",
 		Short: "Create new context",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			mobycli.Exec()
+			mobycli.Exec(cmd.Root())
 			return nil
 		},
 		Long: longHelp,
diff --git a/cli/cmd/context/inspect.go b/cli/cmd/context/inspect.go
@@ -27,7 +27,7 @@ func inspectCommand() *cobra.Command {
 		Use:   "inspect",
 		Short: "Display detailed information on one or more contexts",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			mobycli.Exec()
+			mobycli.Exec(cmd.Root())
 			return nil
 		},
 	}
diff --git a/cli/cmd/context/ls.go b/cli/cmd/context/ls.go
@@ -69,7 +69,7 @@ func runList(cmd *cobra.Command, opts lsOpts) error {
 		return err
 	}
 	if opts.format != "" {
-		mobycli.Exec()
+		mobycli.Exec(cmd.Root())
 		return nil
 	}
 
diff --git a/cli/cmd/login/login.go b/cli/cmd/login/login.go
@@ -56,7 +56,7 @@ func runLogin(cmd *cobra.Command, args []string) error {
 		backend := args[0]
 		return errors.New("unknown backend type for cloud login: " + backend)
 	}
-	mobycli.Exec()
+	mobycli.Exec(cmd.Root())
 	return nil
 }
 
diff --git a/cli/cmd/logout/logout.go b/cli/cmd/logout/logout.go
@@ -37,6 +37,6 @@ func Command() *cobra.Command {
 }
 
 func runLogout(cmd *cobra.Command, args []string) error {
-	mobycli.Exec()
+	mobycli.Exec(cmd.Root())
 	return nil
 }
diff --git a/cli/cmd/version.go b/cli/cmd/version.go
@@ -51,7 +51,7 @@ func runVersion(cmd *cobra.Command, version string) error {
 	// we don't want to fail on error, there is an error if the engine is not available but it displays client version info
 	// Still, technically the [] byte versionResult could be nil, just let the original command display what it has to display
 	if versionResult == nil {
-		mobycli.Exec()
+		mobycli.Exec(cmd.Root())
 		return nil
 	}
 	var s string = string(versionResult)
diff --git a/cli/main.go b/cli/main.go
@@ -102,7 +102,7 @@ func main() {
 		SilenceUsage:  true,
 		PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
 			if !isContextAgnosticCommand(cmd) {
-				mobycli.ExecIfDefaultCtxType(cmd.Context())
+				mobycli.ExecIfDefaultCtxType(cmd.Context(), cmd.Root())
 			}
 			return nil
 		},
@@ -136,7 +136,7 @@ func main() {
 	helpFunc := root.HelpFunc()
 	root.SetHelpFunc(func(cmd *cobra.Command, args []string) {
 		if !isContextAgnosticCommand(cmd) {
-			mobycli.ExecIfDefaultCtxType(cmd.Context())
+			mobycli.ExecIfDefaultCtxType(cmd.Context(), cmd.Root())
 		}
 		helpFunc(cmd, args)
 	})
@@ -158,7 +158,7 @@ func main() {
 
 	// --host and --version should immediately be forwarded to the original cli
 	if opts.Host != "" || opts.Version {
-		mobycli.Exec()
+		mobycli.Exec(root)
 	}
 
 	if opts.Config == "" {
@@ -171,7 +171,7 @@ func main() {
 
 	s, err := store.New(configDir)
 	if err != nil {
-		mobycli.Exec()
+		mobycli.Exec(root)
 	}
 
 	ctype := store.DefaultContextType
@@ -185,41 +185,43 @@ func main() {
 		root.AddCommand(volume.ACICommand())
 	}
 
-	metrics.Track(ctype, os.Args[1:], root.PersistentFlags())
-
 	ctx = apicontext.WithCurrentContext(ctx, currentContext)
 	ctx = store.WithContextStore(ctx, s)
 
 	if err = root.ExecuteContext(ctx); err != nil {
 		// if user canceled request, simply exit without any error message
-		if errors.Is(ctx.Err(), context.Canceled) {
+		if errdefs.IsErrCanceled(err) || errors.Is(ctx.Err(), context.Canceled) {
+			metrics.Track(ctype, os.Args[1:], root.PersistentFlags(), 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: 
-$ docker context create %s <name>`, cc.Type(), store.EcsContextType))
+$ 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)
+			exit(root, currentContext, err, ctype)
 		}
-		mobycli.ExecIfDefaultCtxType(ctx)
+		mobycli.ExecIfDefaultCtxType(ctx, root)
 
-		checkIfUnknownCommandExistInDefaultContext(err, currentContext)
+		checkIfUnknownCommandExistInDefaultContext(err, currentContext, root)
 
-		exit(root, currentContext, err)
+		exit(root, currentContext, err, ctype)
 	}
+	metrics.Track(ctype, os.Args[1:], root.PersistentFlags(), metrics.SuccessStatus)
 }
 
-func exit(cmd *cobra.Command, ctx string, err error) {
+func exit(root *cobra.Command, ctx string, err error, ctype string) {
+	metrics.Track(ctype, os.Args[1:], root.PersistentFlags(), metrics.FailureStatus)
+
 	if errors.Is(err, errdefs.ErrLoginRequired) {
 		fmt.Fprintln(os.Stderr, err)
 		os.Exit(errdefs.ExitCodeLoginRequired)
 	}
 	if errors.Is(err, errdefs.ErrNotImplemented) {
-		cmd, _, _ := cmd.Traverse(os.Args[1:])
+		cmd, _, _ := root.Traverse(os.Args[1:])
 		name := cmd.Name()
 		parent := cmd.Parent()
 		if parent != nil && parent.Parent() != nil {
@@ -237,13 +239,14 @@ func fatal(err error) {
 	os.Exit(1)
 }
 
-func checkIfUnknownCommandExistInDefaultContext(err error, currentContext string) {
+func checkIfUnknownCommandExistInDefaultContext(err error, currentContext string, root *cobra.Command) {
 	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(currentContext, os.Args[1:], root.PersistentFlags(), metrics.FailureStatus)
 			os.Exit(1)
 		}
 	}
diff --git a/cli/mobycli/exec.go b/cli/mobycli/exec.go
@@ -24,25 +24,28 @@ import (
 	"os/signal"
 	"strings"
 
+	"github.com/spf13/cobra"
+
 	apicontext "github.com/docker/compose-cli/context"
 	"github.com/docker/compose-cli/context/store"
+	"github.com/docker/compose-cli/metrics"
 )
 
 var delegatedContextTypes = []string{store.DefaultContextType}
 
 // ComDockerCli name of the classic cli binary
 const ComDockerCli = "com.docker.cli"
 
-// ExecIfDefaultCtxType delegates to com.docker.cli if on moby or AWS context (until there is an AWS backend)
-func ExecIfDefaultCtxType(ctx context.Context) {
+// ExecIfDefaultCtxType delegates to com.docker.cli if on moby context
+func ExecIfDefaultCtxType(ctx context.Context, root *cobra.Command) {
 	currentContext := apicontext.CurrentContext(ctx)
 
 	s := store.ContextStore(ctx)
 
 	currentCtx, err := s.Get(currentContext)
 	// Only run original docker command if the current context is not ours.
 	if err != nil || mustDelegateToMoby(currentCtx.Type()) {
-		Exec()
+		Exec(root)
 	}
 }
 
@@ -56,7 +59,7 @@ func mustDelegateToMoby(ctxType string) bool {
 }
 
 // Exec delegates to com.docker.cli if on moby context
-func Exec() {
+func Exec(root *cobra.Command) {
 	cmd := exec.Command(ComDockerCli, os.Args[1:]...)
 	cmd.Stdin = os.Stdin
 	cmd.Stdout = os.Stdout
@@ -83,12 +86,16 @@ func Exec() {
 	err := cmd.Run()
 	childExit <- true
 	if err != nil {
+		metrics.Track(store.DefaultContextName, os.Args[1:], root.PersistentFlags(), metrics.FailureStatus)
+
 		if exiterr, ok := err.(*exec.ExitError); ok {
 			os.Exit(exiterr.ExitCode())
 		}
 		fmt.Fprintln(os.Stderr, err)
 		os.Exit(1)
 	}
+	metrics.Track(store.DefaultContextName, os.Args[1:], root.PersistentFlags(), metrics.SuccessStatus)
+
 	os.Exit(0)
 }
 
diff --git a/ecs/context.go b/ecs/context.go
@@ -30,6 +30,7 @@ import (
 	"gopkg.in/ini.v1"
 
 	"github.com/docker/compose-cli/context/store"
+	"github.com/docker/compose-cli/errdefs"
 	"github.com/docker/compose-cli/prompt"
 )
 
@@ -155,7 +156,7 @@ func (h contextCreateAWSHelper) chooseProfile(section map[string]ini.Section) (s
 	selected, err := h.user.Select("Select AWS Profile", profiles)
 	if err != nil {
 		if err == terminal.InterruptErr {
-			os.Exit(-1)
+			return "", errdefs.ErrCanceled
 		}
 		return "", err
 	}
diff --git a/errdefs/errors.go b/errdefs/errors.go
@@ -42,6 +42,8 @@ var (
 	// ErrNotImplemented is returned when a backend doesn't implement
 	// an action
 	ErrNotImplemented = errors.New("not implemented")
+	// ErrCanceled is returned when the command was canceled by user
+	ErrCanceled = errors.New("canceled")
 	// ErrParsingFailed is returned when a string cannot be parsed
 	ErrParsingFailed = errors.New("parsing failed")
 	// ErrWrongContextType is returned when the caller tries to get a context
@@ -78,3 +80,8 @@ func IsErrNotImplemented(err error) bool {
 func IsErrParsingFailed(err error) bool {
 	return errors.Is(err, ErrParsingFailed)
 }
+
+// IsErrCanceled returns true if the unwrapped error is ErrCanceled
+func IsErrCanceled(err error) bool {
+	return errors.Is(err, ErrCanceled)
+}
diff --git a/metrics/client.go b/metrics/client.go
@@ -22,6 +22,7 @@ import (
 	"encoding/json"
 	"net"
 	"net/http"
+	"time"
 )
 
 type client struct {
@@ -33,13 +34,20 @@ type Command struct {
 	Command string `json:"command"`
 	Context string `json:"context"`
 	Source  string `json:"source"`
+	Status  string `json:"status"`
 }
 
 const (
 	// CLISource is sent for cli metrics
 	CLISource = "cli"
 	// APISource is sent for API metrics
 	APISource = "api"
+	// SuccessStatus is sent for API metrics
+	SuccessStatus = "success"
+	// FailureStatus is sent for API metrics
+	FailureStatus = "failure"
+	// CanceledStatus is sent for API metrics
+	CanceledStatus = "canceled"
 )
 
 // Client sends metrics to Docker Desktopn
@@ -64,24 +72,23 @@ func NewClient() Client {
 }
 
 func (c *client) Send(command Command) {
-	wasIn := make(chan bool)
-
-	// Fire and forget, we don't want to slow down the user waiting for DD
-	// metrics endpoint to respond. We could lose some events but that's ok.
+	result := make(chan bool, 1)
 	go func() {
-		defer func() {
-			_ = recover()
-		}()
-
-		wasIn <- true
+		postMetrics(command, c)
+		result <- true
+	}()
 
-		req, err := json.Marshal(command)
-		if err != nil {
-			return
-		}
+	// wait for the post finished, or timeout in case anything freezes.
+	// Posting metrics without Desktop listening returns in less than a ms, and a handful of ms (often <2ms) when Desktop is listening
+	select {
+	case <-result:
+	case <-time.After(50 * time.Millisecond):
+	}
+}
 
+func postMetrics(command Command, c *client) {
+	req, err := json.Marshal(command)
+	if err == nil {
 		_, _ = c.httpClient.Post("http://localhost/usage", "application/json", bytes.NewBuffer(req))
-	}()
-	<-wasIn
-
+	}
 }
diff --git a/metrics/metrics.go b/metrics/metrics.go
@@ -77,14 +77,15 @@ const (
 )
 
 // Track sends the tracking analytics to Docker Desktop
-func Track(context string, args []string, flags *flag.FlagSet) {
+func Track(context string, args []string, flags *flag.FlagSet, status string) {
 	command := getCommand(args, flags)
 	if command != "" {
 		c := NewClient()
 		c.Send(Command{
 			Command: command,
 			Context: context,
 			Source:  CLISource,
+			Status:  status,
 		})
 	}
 }
diff --git a/server/metrics.go b/server/metrics.go
@@ -41,9 +41,7 @@ var (
 	}
 )
 
-func metricsServerInterceptor(clictx context.Context) grpc.UnaryServerInterceptor {
-	client := metrics.NewClient()
-
+func metricsServerInterceptor(clictx context.Context, client metrics.Client) grpc.UnaryServerInterceptor {
 	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
 		currentContext, err := getIncomingContext(ctx)
 		if err != nil {
@@ -53,15 +51,21 @@ func metricsServerInterceptor(clictx context.Context) grpc.UnaryServerIntercepto
 			}
 		}
 
+		data, err := handler(ctx, req)
+
+		status := metrics.SuccessStatus
+		if err != nil {
+			status = metrics.FailureStatus
+		}
 		command := methodMapping[info.FullMethod]
 		if command != "" {
 			client.Send(metrics.Command{
 				Command: command,
 				Context: currentContext,
 				Source:  metrics.APISource,
+				Status:  status,
 			})
 		}
-
-		return handler(ctx, req)
+		return data, err
 	}
 }
diff --git a/server/metrics_test.go b/server/metrics_test.go
diff --git a/server/server.go b/server/server.go

Original file line number	Diff line number	Diff line change
`@@ -28,6 +28,7 @@ import (`
`28`	`28`	`"github.com/pkg/errors"`
`29`	`29`
`30`	`30`	`"github.com/docker/compose-cli/context/store"`
	`31`	`+ "github.com/docker/compose-cli/errdefs"`
`31`	`32`	`"github.com/docker/compose-cli/prompt"`
`32`	`33`	`)`
`33`	`34`
`@@ -40,7 +41,7 @@ type ContextParams struct {`
`40`	`41`	`}`
`41`	`42`
`42`	`43`	`// ErrSubscriptionNotFound is returned when a required subscription is not found`
`43`		`-var ErrSubscriptionNotFound = errors.New("subscription not found")`
	`44`	`+var ErrSubscriptionNotFound = errors.Wrapf(errdefs.ErrNotFound, "subscription")`
`44`	`45`
`45`	`46`	`// IsSubscriptionNotFoundError returns true if the unwrapped error is IsSubscriptionNotFoundError`
`46`	`47`	`func IsSubscriptionNotFoundError(err error) bool {`
`@@ -138,7 +139,7 @@ func (helper contextCreateACIHelper) chooseGroup(ctx context.Context, subscripti`
`138`	`139`	`group, err := helper.selector.Select("Select a resource group", groupNames)`
`139`	`140`	`if err != nil {`
`140`	`141`	`if err == terminal.InterruptErr {`
`141`		`- os.Exit(0)`
	`142`	`+ return resources.Group{}, errdefs.ErrCanceled`
`142`	`143`	`}`
`143`	`144`
`144`	`145`	`return resources.Group{}, err`
Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,7 @@ func inspectCommand() *cobra.Command {`
`27`	`27`	`Use: "inspect",`
`28`	`28`	`Short: "Display detailed information on one or more contexts",`
`29`	`29`	`RunE: func(cmd *cobra.Command, args []string) error {`
`30`		`- mobycli.Exec()`
	`30`	`+ mobycli.Exec(cmd.Root())`
`31`	`31`	`return nil`
`32`	`32`	`},`
`33`	`33`	`}`
Original file line number	Diff line number	Diff line change
`@@ -69,7 +69,7 @@ func runList(cmd *cobra.Command, opts lsOpts) error {`
`69`	`69`	`return err`
`70`	`70`	`}`
`71`	`71`	`if opts.format != "" {`
`72`		`- mobycli.Exec()`
	`72`	`+ mobycli.Exec(cmd.Root())`
`73`	`73`	`return nil`
`74`	`74`	`}`
`75`	`75`
Original file line number	Diff line number	Diff line change
`@@ -56,7 +56,7 @@ func runLogin(cmd *cobra.Command, args []string) error {`
`56`	`56`	`backend := args[0]`
`57`	`57`	`return errors.New("unknown backend type for cloud login: " + backend)`
`58`	`58`	`}`
`59`		`- mobycli.Exec()`
	`59`	`+ mobycli.Exec(cmd.Root())`
`60`	`60`	`return nil`
`61`	`61`	`}`
`62`	`62`
Original file line number	Diff line number	Diff line change
`@@ -37,6 +37,6 @@ func Command() *cobra.Command {`
`37`	`37`	`}`
`38`	`38`
`39`	`39`	`func runLogout(cmd *cobra.Command, args []string) error {`
`40`		`- mobycli.Exec()`
	`40`	`+ mobycli.Exec(cmd.Root())`
`41`	`41`	`return nil`
`42`	`42`	`}`
Original file line number	Diff line number	Diff line change
`@@ -51,7 +51,7 @@ func runVersion(cmd *cobra.Command, version string) error {`
`51`	`51`	`// we don't want to fail on error, there is an error if the engine is not available but it displays client version info`
`52`	`52`	`// Still, technically the [] byte versionResult could be nil, just let the original command display what it has to display`
`53`	`53`	`if versionResult == nil {`
`54`		`- mobycli.Exec()`
	`54`	`+ mobycli.Exec(cmd.Root())`
`55`	`55`	`return nil`
`56`	`56`	`}`
`57`	`57`	`var s string = string(versionResult)`
Original file line number	Diff line number	Diff line change
`@@ -30,6 +30,7 @@ import (`
`30`	`30`	`"gopkg.in/ini.v1"`
`31`	`31`
`32`	`32`	`"github.com/docker/compose-cli/context/store"`
	`33`	`+ "github.com/docker/compose-cli/errdefs"`
`33`	`34`	`"github.com/docker/compose-cli/prompt"`
`34`	`35`	`)`
`35`	`36`
`@@ -155,7 +156,7 @@ func (h contextCreateAWSHelper) chooseProfile(section map[string]ini.Section) (s`
`155`	`156`	`selected, err := h.user.Select("Select AWS Profile", profiles)`
`156`	`157`	`if err != nil {`
`157`	`158`	`if err == terminal.InterruptErr {`
`158`		`- os.Exit(-1)`
	`159`	`+ return "", errdefs.ErrCanceled`
`159`	`160`	`}`
`160`	`161`	`return "", err`
`161`	`162`	`}`