Add CompactUserMessage() and StackTraceReport() #110
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sclevine
approved these changes
Dec 2, 2024
nklaassen
reviewed
Jan 18, 2025
| @@ -111,6 +131,26 @@ func UserMessage(err error) string { | |||
| return err.Error() | |||
| } | |||
|
|
|||
| // CompactUserMessage formats the wraoped error message as a compact go-style | |||
There was a problem hiding this comment.
Suggested change
| // CompactUserMessage formats the wraoped error message as a compact go-style | |
| // CompactUserMessage formats the wrapped error message as a compact go-style |
| @@ -111,6 +131,26 @@ func UserMessage(err error) string { | |||
| return err.Error() | |||
| } | |||
|
|
|||
| // CompactUserMessage formats the wraoped error message as a compact go-style | |||
| // wrapped error message. This format is more user-friendly and contains no | |||
| // new line nor tab (except the ones already present in the wrapped error). | |||
There was a problem hiding this comment.
Suggested change
| // new line nor tab (except the ones already present in the wrapped error). | |
| // newlines or tabs (except the ones already present in the wrapped error). |
Comment on lines
+431
to
+437
| // For example: | ||
| // | ||
| // trace.Wrap(trace.Wrap(trace.Errorf("foo"), "bar"), "baz") | ||
| // | ||
| // Will produce the following compact error: | ||
| // | ||
| // foo: bar: baz |
There was a problem hiding this comment.
imo we should return baz: bar: foo in this example, going from the outermost message to the innermost
if i have
return trace.Wrap(login(), "logging in")i would want this to be formatted like logging in: incorrect username or password, not the other way
i'm pretty sure this is actually what the code does, based on the tests, and the comment is just wrong
Comment on lines
+126
to
+127
| assert.Equal(t, stackTrace[1].Func, "testing.tRunner") | ||
| assert.Equal(t, stackTrace[2].Func, "runtime.goexit") |
There was a problem hiding this comment.
can we avoid asserting on these implementation details? probably best to create a stack trace with a few levels we control and only assert on those
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a couple of new functions in an attempt to improve error logging in teleport projects.
The goal is not to make
trace/v2, I tried a year ago and was not convinced by the result. I have not given up on making trace more aligned with go's wrapping and will likely resurrect the RFD at some point.This PR introduces new functions instead of changing
TraceErr.Error(), this ensures the changes can be backported (this is a requirement for the teleport-updater). This also allows us to experiment with error formats before doing atrace/v2.The main motivation for this PR is to have user-friendly error logging in the teleport-updater. During the implementation, I found the great loggin RFD written by @programmerq . While I'm not trying to implement it completely, the changes this PR proposes are aligned with the RFD suggestions of making shorter single-line error messages and decluttering the stacktrace debug report.
The goal is to:
provide concise go-style error messages for wrapped error:
The standard go practice is to wrap with
fmt.Errorf("callsite info: %w", err). This creates error messages looking likecallsite info: my error message. The error message fits in a single line and is friendly both for JSON logging and Text logging. Multiline errors add\nin the first and clutters the log output in the latter.improved structured logging support for traced errors
Currently the trace is contained in the DebugReport, a large multiline string that was intended for CLI troubleshooting. Debug reports are currently not logged properly by the JSON logger, and clutter the server logs when used with the Text logger. Exposing the structured trace will allow the structured logger to log it properly, regardless of the logging output.