Skip to content

Telemetry should send the failure message #78

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Telemetry should send the failure message #78

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e9f47c
Add failed message
loreto Sep 7, 2022
9420977
Send failure message as part of telemetry
loreto Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions boxcli/midcobra/telemetry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,11 @@ func (m *telemetryMiddleware) postRun(cmd *cobra.Command, args []string, runErr
return
}

segmentClient, _ := segment.NewWithConfig(m.opts.TelemetryKey, segment.Config{Verbose: false})
segmentClient, _ := segment.NewWithConfig(m.opts.TelemetryKey,
segment.Config{
Verbose: false,
BatchSize: 1, // We're in a short lived program, so batching doesn't make sense
})

defer func() {
_ = segmentClient.Close()
Expand All @@ -70,15 +74,19 @@ func (m *telemetryMiddleware) postRun(cmd *cobra.Command, args []string, runErr
return // Ignore invalid commands
}

trackEvent(segmentClient, &event{
evt := &event{
AppName: m.opts.AppName,
AppVersion: m.opts.AppVersion,
Command: subcmd.CommandPath(),
CommandArgs: subargs,
DeviceID: deviceID(),
Duration: time.Since(m.startTime),
Failed: runErr != nil,
})
}
if runErr != nil {
evt.Failed = true
evt.FailedMsg = runErr.Error()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good idea to only track a distinct error type so that we can redact errors for analytics. Otherwise I worry that we might return an error with sensitive info somewhere else in the code and accidentally log it.

Maybe something like this?

type RedactedError interface {
	Redact() string
}

func (m *telemetryMiddleware) postRun(cmd *cobra.Command, args []string, runErr error) {
	// ...

	if runErr != nil {
		evt.Failed = true
		if redactErr, ok := runErr.(RedactedError); ok {
			evt.FailedMsg = redactErr.Redact()
		}
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea, but I'm also worried this means we will not capture a bunch of errors as a result. I'm trying to think if there's an in-between.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is to do redacted format strings.

// Not sure on the naming or if it should go in a new package.
type SafeError struct {
	format string
	args   []any
}

func (e *SafeError) Redacted() string {
	// This is ugly. You could probably do something fancier by substituting
	// the args with something else of the same type. Or use a template string
	// instead of fmt.
	return format
}

func (e *SafeError) Error() string {
	return fmt.Sprintf(e.format, e.args...)
}

}
trackEvent(segmentClient, evt)
}

func deviceID() string {
Expand All @@ -104,6 +112,7 @@ type event struct {
DeviceID string
Duration time.Duration
Failed bool
FailedMsg string
}

func trackEvent(client segment.Client, evt *event) {
Expand All @@ -126,6 +135,7 @@ func trackEvent(client segment.Client, evt *event) {
Set("command", evt.Command).
Set("command_args", evt.CommandArgs).
Set("failed", evt.Failed).
Set("failed_msg", evt.FailedMsg).
Set("duration", evt.Duration.Milliseconds()),
})
}