From f15fdbb8308a91b890353178fc7829dcefc1574c Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Tue, 17 Oct 2023 17:41:21 -0700 Subject: [PATCH 1/2] [telemetry] fix segfault --- internal/telemetry/sentry.go | 12 ++++++------ internal/telemetry/telemetry.go | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/telemetry/sentry.go b/internal/telemetry/sentry.go index 258fec0f544..5b1420a1866 100644 --- a/internal/telemetry/sentry.go +++ b/internal/telemetry/sentry.go @@ -51,8 +51,8 @@ func initSentryClient(appName string) bool { return err == nil } -func newSentryException(err error) []sentry.Exception { - errMsg := err.Error() +func newSentryException(errToLog error) []sentry.Exception { + errMsg := errToLog.Error() binPkg := "" modPath := "" if build, ok := debug.ReadBuildInfo(); ok { @@ -66,12 +66,12 @@ func newSentryException(err error) []sentry.Exception { var stFunc func() []runtime.Frame errType := "Generic Error" for { - if t := exportedErrType(err); t != "" { + if t := exportedErrType(errToLog); t != "" { errType = t } //nolint:errorlint - switch stackErr := err.(type) { + switch stackErr := errToLog.(type) { // If the error implements the StackTrace method in the redact package, then // prefer that. The Sentry SDK gets some things wrong when guessing how // to extract the stack trace. @@ -98,11 +98,11 @@ func newSentryException(err error) []sentry.Exception { return frames } } - uw := errors.Unwrap(err) + uw := errors.Unwrap(errToLog) if uw == nil { break } - err = uw + errToLog = uw } ex := []sentry.Exception{{Type: errType, Value: errMsg}} if stFunc != nil { diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index f2f1622ef3e..351ad2aa4f5 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -113,8 +113,8 @@ func commandEvent(meta Metadata) (id string, msg *segment.Track) { } // Error reports an error to the telemetry server. -func Error(err error, meta Metadata) { - if !started || err == nil { +func Error(errToLog error, meta Metadata) { + if !started || errToLog == nil { return } @@ -127,7 +127,7 @@ func Error(err error, meta Metadata) { EventID: sentry.EventID(ExecutionID), Level: sentry.LevelError, User: sentry.User{ID: deviceID}, - Exception: newSentryException(redact.Error(err)), + Exception: newSentryException(redact.Error(errToLog)), Contexts: map[string]map[string]any{ "os": { "name": build.OS(), From 3fd854f93b56b9ea90705cf50c65c87b2495bce1 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Wed, 18 Oct 2023 10:09:25 -0700 Subject: [PATCH 2/2] add very basic test, and use err in package api function --- internal/telemetry/telemetry.go | 3 ++- internal/telemetry/telemetry_test.go | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 internal/telemetry/telemetry_test.go diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 351ad2aa4f5..aebda518101 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -113,7 +113,8 @@ func commandEvent(meta Metadata) (id string, msg *segment.Track) { } // Error reports an error to the telemetry server. -func Error(errToLog error, meta Metadata) { +func Error(err error, meta Metadata) { + errToLog := err // use errToLog to avoid shadowing err later. Use err to keep API clean. if !started || errToLog == nil { return } diff --git a/internal/telemetry/telemetry_test.go b/internal/telemetry/telemetry_test.go new file mode 100644 index 00000000000..97524376a10 --- /dev/null +++ b/internal/telemetry/telemetry_test.go @@ -0,0 +1,19 @@ +package telemetry + +import ( + "errors" + "testing" +) + +// TestErrorBasic does a very simple sanity check to ensure the error can be sent +// to the sentry and segment buffers +func TestErrorBasic(t *testing.T) { + segmentBufferDir = t.TempDir() + sentryBufferDir = t.TempDir() + started = true + + fakeErr := errors.New("fake error") + meta := Metadata{} + + Error(fakeErr, meta) +}