Skip to content

Improve utils.FatalError error formatting#34417

Merged
strideynet merged 4 commits intomasterfrom
strideynet/improve-fatal-error-log-order
Nov 20, 2023
Merged

Improve utils.FatalError error formatting#34417
strideynet merged 4 commits intomasterfrom
strideynet/improve-fatal-error-log-order

Conversation

@strideynet
Copy link
Copy Markdown
Contributor

@strideynet strideynet commented Nov 9, 2023

I've noticed the past few months a number of cases where users reporting an issue with tbot were confused by the error message output by tbot.

A contributing factor to this is that they appear "scrambled". This scrambling appears to be caused by utils.formatErrorWriter which prints the Messages from deepest to shallowest, and then prints the underlying error. But - this makes no sense - the underlying error is the deepest element. You can see an example of this below.

I noticed that when -d was provided, a different implementation was used to print the error. This implementation is within our trace package, and correctly shows the messages from shallowest to deepest - and also indents each message. This helps distinguish each message and is especially important when printing a stack including an aggregate error. So - I figured it made sense to reuse that implementation.

Example Code:

package main

import (
	"github.com/gravitational/teleport/lib/utils"
	"github.com/gravitational/trace"
)

func chargeCapacitors() error {
	return trace.BadParameter("capacitor bank exploded")
}

func prepareEnergySubsystem() error {
	return trace.Wrap(chargeCapacitors(), "charging capacitors")
}

func jumpToLightspeed() error {
	return trace.Wrap(prepareEnergySubsystem(), "preparing energy subsystem")
}

func main() {
	err := trace.Wrap(jumpToLightspeed(), "jumping to lightspeed")
	utils.FatalError(err)
}

Prior to this PR:

noah@Noahs-MBP teleport % go run ./example   
ERROR: charging capacitors
preparing energy subsystem
jumping to lightspeed
capacitor exploded

exit status 1

After this PR:

noah@Noahs-MBP teleport % go run ./example 
ERROR: jumping to lightspeed
        preparing energy subsystem
                charging capacitors
                        capacitor exploded

exit status 1

If we feel the risk this change is too wide for utils.FatalError, I'm more than happy to isolate this fix to tbot.

nb: I'm also currently investigating an issue within trace when pretty printing aggregate errors. I hope to raise a PR for that soonish as part of a general effort to improve tbot's error output.

@strideynet strideynet changed the title Improve utils.FatalError Improve utils.FatalError error formatting Nov 9, 2023
@strideynet strideynet marked this pull request as ready for review November 9, 2023 19:54
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 9, 2023

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@github-actions github-actions Bot requested review from bl-nero and tigrato November 9, 2023 19:55
@strideynet strideynet added the no-changelog Indicates that a PR does not require a changelog entry label Nov 9, 2023
Comment thread lib/utils/cli.go
Comment thread lib/utils/cli.go
Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me, as is the change in error messages overall.

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from bl-nero November 13, 2023 20:28
@strideynet strideynet enabled auto-merge November 20, 2023 11:24
@strideynet strideynet added this pull request to the merge queue Nov 20, 2023
Merged via the queue into master with commit fd80ffa Nov 20, 2023
@strideynet strideynet deleted the strideynet/improve-fatal-error-log-order branch November 20, 2023 11:59
@public-teleport-github-review-bot
Copy link
Copy Markdown

@strideynet See the table below for backport results.

Branch Result
branch/v12 Create PR
branch/v13 Create PR
branch/v14 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants