Skip to content
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.

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

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

Merged
merged 7 commits into from
Mar 31, 2023

Conversation

gcurtis
Copy link
Collaborator

@gcurtis gcurtis commented Mar 29, 2023

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

Depends on #841.

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.

Package redact implements functions to redact sensitive information from
errors. A basic example is:

	name := "Alex"
	id := 5
	err := redact.Errorf("error getting user %s with ID %d",
		name, Safe(id))

	fmt.Println(err)
	// error getting user Alex with ID 5

	fmt.Println(redact.Error(err))
	// error getting user <redacted string> with ID 5

See the package docs and tests for more examples.

This will allow us to get more information in Sentry without sending any
identifying information.
internal/boxcli/midcobra/telemetry.go Outdated Show resolved Hide resolved
internal/boxcli/midcobra/telemetry.go Outdated Show resolved Hide resolved
Make `safeError` satisfy `fmt.Format` so that it can print stack traces
the same way pkgs/errors does. Don't double redact errors so that
already-redacted output isn't replaced with a placeholder.
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.
Base automatically changed from gcurtis/redact to main March 30, 2023 23:03
@gcurtis gcurtis merged commit 0f51657 into main Mar 31, 2023
@gcurtis gcurtis deleted the gcurtis/sentry-errors branch March 31, 2023 02:47
mikeland73 added a commit that referenced this pull request Sep 27, 2024
## Summary

We're running out of Sentry events and it looks like the culprit is
devbox overlogging, including a lot of user errors that should not be
logged.

This PR #844 removed code that
avoided logging user errors. I think this was a bug? If programmer wants
a user error to be logged, they should use `WithLoggedUserMessage`
instead.

## How was it tested?

Untested
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants