Merge pull request #630 from docker/cli_metrics_failures

Cli metrics failures

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

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,

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
 		},
 	}

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
 	}
 

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
 }
 

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
 }

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)

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)
 		}
 	}

cli/mobycli/exec.go

@@ -24,8 +24,11 @@ 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}
@@ -33,8 +36,8 @@ 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)
+// ExecIfDefaultCtxType delegates to com.docker.cli if on moby context
-func ExecIfDefaultCtxType(ctx context.Context) {
+func ExecIfDefaultCtxType(ctx context.Context, root *cobra.Command) {
 	currentContext := apicontext.CurrentContext(ctx)
 
 	s := store.ContextStore(ctx)
@@ -42,7 +45,7 @@ func ExecIfDefaultCtxType(ctx context.Context) {
 	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)
 }
 

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
 	}

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)
+}

metrics/client.go

@@ -22,6 +22,7 @@ import (
 	"encoding/json"
 	"net"
 	"net/http"
+	"time"
 )
 
 type client struct {
@@ -33,6 +34,7 @@ type Command struct {
 	Command string `json:"command"`
 	Context string `json:"context"`
 	Source  string `json:"source"`
+	Status  string `json:"status"`
 }
 
 const (
@@ -40,6 +42,12 @@ const (
 	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)
+	result := make(chan bool, 1)
-
-	// 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.
 	go func() {
-		defer func() {
+		postMetrics(command, c)
-			_ = recover()
+		result <- true
-		}()
-
-		wasIn <- true
-
-		req, err := json.Marshal(command)
-		if err != nil {
-			return
-		}
-
-		_, _ = c.httpClient.Post("http://localhost/usage", "application/json", bytes.NewBuffer(req))
 	}()
-	<-wasIn
 
+	// 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))
+	}
 }

metrics/metrics.go

@@ -77,7 +77,7 @@ 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()
@@ -85,6 +85,7 @@ func Track(context string, args []string, flags *flag.FlagSet) {
 			Command: command,
 			Context: context,
 			Source:  CLISource,
+			Status:  status,
 		})
 	}
 }

server/metrics.go

@@ -41,9 +41,7 @@ var (
 	}
 )
 
-func metricsServerInterceptor(clictx context.Context) grpc.UnaryServerInterceptor {
+func metricsServerInterceptor(clictx context.Context, client metrics.Client) grpc.UnaryServerInterceptor {
-	client := metrics.NewClient()
-
 	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 data, err
-		return handler(ctx, req)
 	}
 }

server/metrics_test.go

@@ -21,9 +21,13 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/mock"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/metadata"
 	"gotest.tools/v3/assert"
 
+	"github.com/docker/compose-cli/errdefs"
+	"github.com/docker/compose-cli/metrics"
 	containersv1 "github.com/docker/compose-cli/protos/containers/v1"
 	contextsv1 "github.com/docker/compose-cli/protos/contexts/v1"
 	streamsv1 "github.com/docker/compose-cli/protos/streams/v1"
@@ -48,6 +52,44 @@ func TestAllMethodsHaveCorrespondingCliCommand(t *testing.T) {
 	}
 }
 
+func TestTrackSuccess(t *testing.T) {
+	var mockMetrics = &mockMetricsClient{}
+	mockMetrics.On("Send", metrics.Command{Command: "ps", Context: "aci", Status: "success", Source: "api"}).Return()
+	interceptor := metricsServerInterceptor(context.TODO(), mockMetrics)
+
+	_, err := interceptor(incomingContext("aci"), nil, containerMethodRoute("List"), mockHandler(nil))
+	assert.NilError(t, err)
+}
+
+func TestTrackSFailures(t *testing.T) {
+	var mockMetrics = &mockMetricsClient{}
+	mockMetrics.On("Send", metrics.Command{Command: "ps", Context: "default", Status: "failure", Source: "api"}).Return()
+	interceptor := metricsServerInterceptor(context.TODO(), mockMetrics)
+
+	_, err := interceptor(incomingContext("default"), nil, containerMethodRoute("Create"), mockHandler(errdefs.ErrLoginRequired))
+	assert.Assert(t, err == errdefs.ErrLoginRequired)
+}
+
+func containerMethodRoute(action string) *grpc.UnaryServerInfo {
+	var info = &grpc.UnaryServerInfo{
+		FullMethod: "/com.docker.api.protos.containers.v1.Containers/" + action,
+	}
+	return info
+}
+
+func mockHandler(err error) func(ctx context.Context, req interface{}) (interface{}, error) {
+	return func(ctx context.Context, req interface{}) (interface{}, error) {
+		return nil, err
+	}
+}
+
+func incomingContext(status string) context.Context {
+	ctx := metadata.NewIncomingContext(context.TODO(), metadata.MD{
+		(key): []string{status},
+	})
+	return ctx
+}
+
 func setupServer() *grpc.Server {
 	ctx := context.TODO()
 	s := New(ctx)
@@ -57,3 +99,11 @@ func setupServer() *grpc.Server {
 	contextsv1.RegisterContextsServer(s, p.ContextsProxy())
 	return s
 }
+
+type mockMetricsClient struct {
+	mock.Mock
+}
+
+func (s *mockMetricsClient) Send(command metrics.Command) {
+	s.Called(command)
+}

server/server.go

@@ -24,6 +24,8 @@ import (
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/health"
 	"google.golang.org/grpc/health/grpc_health_v1"
+
+	"github.com/docker/compose-cli/metrics"
 )
 
 // New returns a new GRPC server.
@@ -31,7 +33,7 @@ func New(ctx context.Context) *grpc.Server {
 	s := grpc.NewServer(
 		grpc.ChainUnaryInterceptor(
 			unaryServerInterceptor(ctx),
-			metricsServerInterceptor(ctx),
+			metricsServerInterceptor(ctx, metrics.NewClient()),
 		),
 		grpc.StreamInterceptor(streamServerInterceptor(ctx)),
 	)