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

Output errors on a single line if backtraces are disabled #1514

Merged
merged 8 commits into from
Nov 16, 2021

Conversation

romac
Copy link
Member

@romac romac commented Oct 27, 2021

Closes: #1529

Description

This uses our fork of simple-eyre, called oneline-eyre.


Output with RUST_BACKTRACE = "full":

2021-10-27T22:00:13.861230Z ERROR ThreadId(27) [ibc-0] failed to collect events:
   0: WebSocket driver failed
   1:
   1:    0: web socket error: failed to read from WebSocket connection
   1:    1: WebSocket protocol error: Connection reset without closing handshake

   1: Location:
   1:    /Users/coromac/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/flex-error-0.4.4/src/tracer_impl/eyre.rs:10

   1:   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
   1:                                 ⋮ 10 frames hidden ⋮

...

Output without RUST_BACKTRACE unset or set to "0" or "":

2021-10-27T22:01:37.228355Z ERROR ThreadId(27) [ibc-0] failed to collect events: WebSocket driver failed: web socket error: failed to read from WebSocket connection: WebSocket protocol error: Connection reset without closing handshake

For contributor use:

  • Added a changelog entry, using unclog.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@romac romac self-assigned this Oct 28, 2021
@romac romac force-pushed the romac/oneline-errors branch from 231f908 to 9d6ca31 Compare November 9, 2021 13:56
@romac romac marked this pull request as ready for review November 9, 2021 13:56
@romac romac requested review from soareschen and mircea-c and removed request for adizere and ancazamfir November 9, 2021 13:56
@romac romac marked this pull request as draft November 9, 2021 13:57
@soareschen
Copy link
Contributor

soareschen commented Nov 10, 2021

This feels a bit overly restrictive that it would be impossible to capture full back trace in log files if it is ever needed. Should we only use the one-line error reporter if RUST_BACKTRACE is "" or not set?

@romac romac force-pushed the romac/oneline-errors branch from 9d6ca31 to ca0cdba Compare November 10, 2021 09:50
@romac romac changed the title Output errors on a single line if ANSI output is disabled Output errors on a single line if backtraces are disabled Nov 10, 2021
@romac romac marked this pull request as ready for review November 10, 2021 09:58
color_eyre::install()?;
}

abscissa_core::boot(&APPLICATION);
}

fn compact_logs() -> bool {
matches!(std::env::var("RUST_BACKTRACE").as_deref(), Ok("" | "0"))
Copy link
Contributor

@soareschen soareschen Nov 10, 2021

Choose a reason for hiding this comment

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

I think in this case if RUST_BACKTRACE is not set, the error case Err(_) will return false when it should be true.

This should be clearer with comment explaining what should happen in different branches:

fn main() -> eyre::Result<()> {
    install_error_reporter()?;

    abscissa_core::boot(&APPLICATION);
}

fn install_error_reporter() -> eyre::Result<()> {
    if enable_ansi() {
        // If we are in a terminal supporting color, display
        // full error logs in color
        color_eyre::install()?;
    } else if !backtrace_enabled() {
        // Else if we are piping logs to file and backtrace is *not* enabled,
        // display errors in single line.
        oneline_eyre::install()?;
    } else {
        // Else backtrace is enabled and we are piping to logs, so use the
        // default error report handler, which displays multiline errors
        // without color.
    }

    Ok(())
}

fn backtrace_enabled() -> bool {
    match std::env::var("RUST_BACKTRACE").as_deref() {
        Ok("" | "0") | Err(_) => false,
        Ok(_) => true,
    }
}

Copy link
Member Author

@romac romac Nov 11, 2021

Choose a reason for hiding this comment

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

Good catch, thanks! Shouldn't we invert the first two branches? ie. if backtraces are not enabled, we use one-line logs, otherwise if there is a tty we use colors, else we use the default reporter.

fn install_error_reporter() -> eyre::Result<()> {
    if !backtrace_enabled() {
        // If backtraces are disabled, display errors in single line.
        oneline_eyre::install()
    } else if enable_ansi() {
        // Else, if backtraces are enabled and we are in a terminal
        // supporting color, display full error logs in color.
        color_eyre::install()
    } else {
        // Otherwise, backtraces are enabled and we are piping to logs, so use the
        // default error report handler, which displays multiline errors
        // without color.
        Ok(())
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it depends on what we want the default behavior to be. Most of the time the relayer will run without RUST_BACKTRACE set, even for terminal cases. Do we want those to display one-line errors as well?

Choose a reason for hiding this comment

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

Most of the time the relayer will be piping to logs. So one line logs should be the default case.

@romac romac requested review from mircea-c and soareschen November 11, 2021 16:30
@romac romac merged commit e8cb03a into master Nov 16, 2021
@romac romac deleted the romac/oneline-errors branch November 16, 2021 10:18
mzabaluev added a commit that referenced this pull request Nov 23, 2021
Had to remove the changes done in
#1514
The changelog entry also mistakenly referred to #1515.
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
…stems#1514)

* Output errors on a single line if ANSI output is disabled

* Add changelog entry

* Use published version of `oneline-eyre`

* Use oneline logs if RUST_BACKTRACE is unset, set to 0 or empty

* Use colored logs only if ANSI output is enabled

* Use value of RUST_BACKTRACE at runtime instead of compile time

* Simplify code

* Use single-line errors if backtraces are disabled even with a tty
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output errors on a single line if backtraces are disabled
3 participants