telemetry: improve stack traces, error names, contexts (#844)

Clean up and fix telemetry so that it can be actually useful. This
commit is rather large because telemetry happens to touch a lot of
different things.

Example of what it looks like in Sentry:
https://jetpack-io.sentry.io/issues/4047374127

Client
------

- Remove unnecessary telemetry configuration, such as `InitOpts`. Make
the `build` package the source of truth for version information and
link-time variables. Generate a global execution ID once at startup.
- Standardize the contexts to use names and values recognized by Sentry.
- Remove arbitrary CLI args and DEVBOX_ environment variables from the
context, as these were leaking information.

Errors
------

- In combination with the `redact` package, errors can now include data
that has been marked as safe for telemetry.
- Errors that haven't been marked as safe have a more helpful
placeholder containing the chain of error types up until the first
exported error.

Stack Traces
------------

- Use the correct file and function names. This allows Sentry to group
errors correctly.
- Don't record every wrapped error. This doesn't make sense in Go since
an error is a value containing the entire message. Otherwise you end up
with a bunch of error messages that repeat themselves.

Author: gcurtis

internal/boxcli/featureflag/feature.go

@@ -50,3 +50,12 @@ func (f *feature) Enabled() bool {
 func (f *feature) Disabled() bool {
 	return !f.Enabled()
 }
+
+// All returns a map of all known features flags and whether they're enabled.
+func All() map[string]bool {
+	m := make(map[string]bool, len(features))
+	for name, feat := range features {
+		m[name] = feat.Enabled()
+	}
+	return m
+}

internal/boxcli/midcobra/debug.go

@@ -14,12 +14,12 @@ import (
 	"github.com/spf13/pflag"
 	"go.jetpack.io/devbox/internal/boxcli/usererr"
 	"go.jetpack.io/devbox/internal/debug"
+	"go.jetpack.io/devbox/internal/telemetry"
 	"go.jetpack.io/devbox/internal/ux"
 )
 
 type DebugMiddleware struct {
-	executionID string // uuid
-	flag        *pflag.Flag
+	flag *pflag.Flag
 }
 
 var _ Middleware = (*DebugMiddleware)(nil)
@@ -69,10 +69,5 @@ func (d *DebugMiddleware) postRun(cmd *cobra.Command, args []string, runErr erro
 	if errors.As(runErr, &exitErr) {
 		debug.Log("Command stderr: %s\n", exitErr.Stderr)
 	}
-	debug.Log("\nExecutionID:%s\n%+v\n", d.executionID, st)
-}
-
-func (d *DebugMiddleware) withExecutionID(execID string) Middleware {
-	d.executionID = execID
-	return d
+	debug.Log("\nExecutionID:%s\n%+v\n", telemetry.ExecutionID, st)
 }

internal/boxcli/midcobra/midcobra.go

@@ -5,11 +5,9 @@ package midcobra
 
 import (
 	"context"
-	"encoding/hex"
 	"errors"
 	"os/exec"
 
-	"github.com/google/uuid"
 	"github.com/spf13/cobra"
 	"go.jetpack.io/devbox/internal/boxcli/usererr"
 	"go.jetpack.io/devbox/internal/debug"
@@ -24,32 +22,24 @@ type Executable interface {
 type Middleware interface {
 	preRun(cmd *cobra.Command, args []string)
 	postRun(cmd *cobra.Command, args []string, runErr error)
-	withExecutionID(execID string) Middleware
 }
 
 func New(cmd *cobra.Command) Executable {
 	return &midcobraExecutable{
 		cmd:         cmd,
-		executionID: ExecutionID(),
 		middlewares: []Middleware{},
 	}
 }
 
 type midcobraExecutable struct {
 	cmd *cobra.Command
 
-	// executionID identifies a unique execution of the devbox CLI
-	executionID string // uuid
-
 	middlewares []Middleware
 }
 
 var _ Executable = (*midcobraExecutable)(nil)
 
 func (ex *midcobraExecutable) AddMiddleware(mids ...Middleware) {
-	for index, m := range mids {
-		mids[index] = m.withExecutionID(ex.executionID)
-	}
 	ex.middlewares = append(ex.middlewares, mids...)
 }
 
@@ -101,17 +91,3 @@ func (ex *midcobraExecutable) Execute(ctx context.Context, args []string) int {
 	}
 	return 0
 }
-
-func ExecutionID() string {
-	// google/uuid package's String() returns a value of the form:
-	// xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
-	//
-	// but sentry's EventID specifies:
-	//
-	// > EventID is a hexadecimal string representing a unique uuid4 for an Event.
-	// An EventID must be 32 characters long, lowercase and not have any dashes.
-	//
-	// so we pre-process to match sentry's requirements:
-	id := uuid.New()
-	return hex.EncodeToString(id[:])
-}

internal/boxcli/midcobra/telemetry.go

@@ -8,14 +8,17 @@ import (
 	"io"
 	"os"
 	"runtime/trace"
+	"sort"
+	"strconv"
 	"strings"
 	"time"
 
-	"github.com/getsentry/sentry-go"
 	segment "github.com/segmentio/analytics-go"
 	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
 	"go.jetpack.io/devbox"
-	"go.jetpack.io/devbox/internal/boxcli/usererr"
+	"go.jetpack.io/devbox/internal/boxcli/featureflag"
+	"go.jetpack.io/devbox/internal/build"
 	"go.jetpack.io/devbox/internal/cloud/envir"
 	"go.jetpack.io/devbox/internal/telemetry"
 )
@@ -27,60 +30,56 @@ import (
 // 2. Data is only stored in SOC 2 compliant systems, and we are SOC 2 compliant ourselves.
 // 3. Users should always have the ability to opt-out.
 func Telemetry() Middleware {
-
-	opts := telemetry.InitOpts()
-
-	return &telemetryMiddleware{
-		opts:     *opts,
-		disabled: telemetry.IsDisabled(opts),
-	}
+	return &telemetryMiddleware{}
 }
 
 type telemetryMiddleware struct {
-	// Setup:
-	opts     telemetry.Opts
-	disabled bool
-
 	// Used during execution:
 	startTime time.Time
-
-	executionID string
 }
 
 // telemetryMiddleware implements interface Middleware (compile-time check)
 var _ Middleware = (*telemetryMiddleware)(nil)
 
-func (m *telemetryMiddleware) withExecutionID(execID string) Middleware {
-	m.executionID = execID
-	return m
-}
-
 func (m *telemetryMiddleware) preRun(cmd *cobra.Command, args []string) {
 	m.startTime = telemetry.CommandStartTime()
 
+	telemetry.Start(telemetry.AppDevbox)
 	ctx := cmd.Context()
 	defer trace.StartRegion(ctx, "telemetryPreRun").End()
-	if m.disabled {
+	if !telemetry.Enabled() {
 		trace.Log(ctx, "telemetry", "telemetry is disabled")
 		return
 	}
-	sentry := telemetry.NewSentry(m.opts.SentryDSN)
-	sentry.Init(m.opts.AppName, m.opts.AppVersion, m.executionID)
 }
 
 func (m *telemetryMiddleware) postRun(cmd *cobra.Command, args []string, runErr error) {
 	defer trace.StartRegion(cmd.Context(), "telemetryPostRun").End()
-	if m.disabled {
+	defer telemetry.Stop()
+
+	meta := telemetry.Metadata{
+		FeatureFlags: featureflag.All(),
+		CloudRegion:  envir.GetRegion(),
+		CloudCache:   os.Getenv("DEVBOX_CACHE"),
+	}
+
+	subcmd, flags, err := getSubcommand(cmd, args)
+	if err != nil {
+		// Ignore invalid commands/flags.
 		return
 	}
+	meta.Command = subcmd.CommandPath()
+	meta.CommandFlags = flags
+	meta.Packages, meta.NixpkgsHash = getPackagesAndCommitHash(cmd)
+	meta.InShell, _ = strconv.ParseBool(os.Getenv("DEVBOX_SHELL_ENABLED"))
+	meta.InBrowser, _ = strconv.ParseBool(os.Getenv("START_WEB_TERMINAL"))
+	meta.InCloud = envir.IsDevboxCloud()
+	telemetry.Error(runErr, meta)
 
 	evt := m.newEventIfValid(cmd, args, runErr)
 	if evt == nil {
 		return
 	}
-
-	m.trackError(evt) // Sentry
-
 	m.trackEvent(evt) // Segment
 }
 
@@ -105,7 +104,7 @@ type event struct {
 // a valid event.
 func (m *telemetryMiddleware) newEventIfValid(cmd *cobra.Command, args []string, runErr error) *event {
 
-	subcmd, subargs, parseErr := getSubcommand(cmd, args)
+	subcmd, flags, parseErr := getSubcommand(cmd, args)
 	if parseErr != nil {
 		// Ignore invalid commands
 		return nil
@@ -126,16 +125,16 @@ func (m *telemetryMiddleware) newEventIfValid(cmd *cobra.Command, args []string,
 
 	return &event{
 		Event: telemetry.Event{
-			AnonymousID: telemetry.DeviceID(),
-			AppName:     m.opts.AppName,
-			AppVersion:  m.opts.AppVersion,
+			AnonymousID: telemetry.DeviceID,
+			AppName:     telemetry.AppDevbox,
+			AppVersion:  build.Version,
 			CloudRegion: envir.GetRegion(),
 			Duration:    time.Since(m.startTime),
-			OsName:      telemetry.OS(),
+			OsName:      build.OS(),
 			UserID:      userID,
 		},
 		Command:      subcmd.CommandPath(),
-		CommandArgs:  subargs,
+		CommandArgs:  flags,
 		CommandError: runErr,
 		// The command is hidden if either the top-level command is hidden or
 		// the specific sub-command that was executed is hidden.
@@ -149,38 +148,15 @@ func (m *telemetryMiddleware) newEventIfValid(cmd *cobra.Command, args []string,
 	}
 }
 
-func (m *telemetryMiddleware) trackError(evt *event) {
-	// Ensure error is not nil and not a non-loggable user error
-	if evt == nil || !usererr.ShouldLogError(evt.CommandError) {
-		return
-	}
-
-	sentry.ConfigureScope(func(scope *sentry.Scope) {
-		scope.SetTag("command", evt.Command)
-		scope.SetContext("command", map[string]interface{}{
-			"command":      evt.Command,
-			"command args": evt.CommandArgs,
-			"packages":     evt.Packages,
-			"nixpkgs hash": evt.CommitHash,
-			"in shell":     evt.InDevboxShell,
-		})
-		scope.SetContext("devbox env", evt.DevboxEnv)
-	})
-	sentry.CaptureException(evt.CommandError)
-}
-
 func (m *telemetryMiddleware) trackEvent(evt *event) {
 	if evt == nil || evt.CommandHidden {
 		return
 	}
 
 	if evt.CommandError != nil {
-		// verified with manual testing that the sentryID returned by CaptureException
-		// is the same as m.ExecutionID, since we set EventID = m.ExecutionID in sentry.Init
-		evt.SentryEventID = m.executionID
+		evt.SentryEventID = telemetry.ExecutionID
 	}
-
-	segmentClient := telemetry.NewSegmentClient(m.opts.TelemetryKey)
+	segmentClient := telemetry.NewSegmentClient(build.TelemetryKey)
 	defer func() {
 		_ = segmentClient.Close()
 	}()
@@ -219,13 +195,18 @@ func (m *telemetryMiddleware) trackEvent(evt *event) {
 	})
 }
 
-func getSubcommand(c *cobra.Command, args []string) (subcmd *cobra.Command, subargs []string, err error) {
-	if c.TraverseChildren {
-		subcmd, subargs, err = c.Traverse(args)
+func getSubcommand(cmd *cobra.Command, args []string) (subcmd *cobra.Command, flags []string, err error) {
+	if cmd.TraverseChildren {
+		subcmd, _, err = cmd.Traverse(args)
 	} else {
-		subcmd, subargs, err = c.Find(args)
+		subcmd, _, err = cmd.Find(args)
 	}
-	return subcmd, subargs, err
+
+	subcmd.Flags().Visit(func(f *pflag.Flag) {
+		flags = append(flags, "--"+f.Name)
+	})
+	sort.Strings(flags)
+	return subcmd, flags, err
 }
 
 func getPackagesAndCommitHash(c *cobra.Command) ([]string, string) {

internal/boxcli/midcobra/trace.go

@@ -13,10 +13,9 @@ import (
 )
 
 type TraceMiddleware struct {
-	executionID string // uuid
-	tracef      *os.File
-	flag        *pflag.Flag
-	task        *trace.Task
+	tracef *os.File
+	flag   *pflag.Flag
+	task   *trace.Task
 }
 
 var _ Middleware = (*DebugMiddleware)(nil)
@@ -28,7 +27,7 @@ func (t *TraceMiddleware) AttachToFlag(flags *pflag.FlagSet, flagName string) {
 	t.flag.NoOptDefVal = "trace.out"
 }
 
-func (t *TraceMiddleware) preRun(cmd *cobra.Command, args []string) {
+func (t *TraceMiddleware) preRun(cmd *cobra.Command, _ []string) {
 	if t == nil {
 		return
 	}
@@ -50,7 +49,7 @@ func (t *TraceMiddleware) preRun(cmd *cobra.Command, args []string) {
 	cmd.SetContext(ctx)
 }
 
-func (t *TraceMiddleware) postRun(cmd *cobra.Command, args []string, runErr error) {
+func (t *TraceMiddleware) postRun(*cobra.Command, []string, error) {
 	if t.tracef == nil {
 		return
 	}
@@ -60,8 +59,3 @@ func (t *TraceMiddleware) postRun(cmd *cobra.Command, args []string, runErr erro
 		panic("error closing trace file: " + err.Error())
 	}
 }
-
-func (t *TraceMiddleware) withExecutionID(execID string) Middleware {
-	t.executionID = execID
-	return t
-}

internal/boxcli/root.go

@@ -81,7 +81,7 @@ func Execute(ctx context.Context, args []string) int {
 func Main() {
 	if strings.HasSuffix(os.Args[0], "ssh") ||
 		strings.HasSuffix(os.Args[0], "scp") {
-		code := sshshim.Execute(context.Background(), os.Args)
+		code := sshshim.Execute(os.Args)
 		os.Exit(code)
 	}
 	code := Execute(context.Background(), os.Args[1:])

internal/build/build.go

@@ -1,11 +1,48 @@
 package build
 
+import (
+	"runtime"
+	"sync"
+
+	"go.jetpack.io/devbox/internal/fileutil"
+)
+
 // Variables in this file are set via ldflags.
 var (
+	IsDev      = Version == "0.0.0-dev"
 	Version    = "0.0.0-dev"
 	Commit     = "none"
 	CommitDate = "unknown"
 
 	SentryDSN    = "" // Disabled by default
 	TelemetryKey = "" // Disabled by default
 )
+
+// User-presentable names of operating systems supported by Devbox.
+const (
+	OSLinux  = "Linux"
+	OSDarwin = "macOS"
+	OSWSL    = "WSL"
+)
+
+var (
+	osName string
+	osOnce sync.Once
+)
+
+func OS() string {
+	osOnce.Do(func() {
+		switch runtime.GOOS {
+		case "linux":
+			if fileutil.Exists("/proc/sys/fs/binfmt_misc/WSLInterop") || fileutil.Exists("/run/WSL") {
+				osName = OSWSL
+			}
+			osName = OSLinux
+		case "darwin":
+			osName = OSDarwin
+		default:
+			osName = runtime.GOOS
+		}
+	})
+	return osName
+}