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

Add support for RUST_LIB_BACKTRACE variable #38

Merged
merged 5 commits into from May 6, 2020
Merged

Conversation

yaahc
Copy link
Contributor

@yaahc yaahc commented May 1, 2020

@athre0z
Copy link
Owner

athre0z commented May 2, 2020

If I understand the distinction between RUST_BACKTRACE and RUST_LIB_BACKTRACE correctly, the former is used when capturing panic traces using the default panic handler and the latter is employed when a user manually captures backtraces, e.g. as context for their own errors. Is that correct? Until you created this issue I didn't even realize the latter existed, so I want to make sure I understand it correctly.

If my assumptions above are correct, shouldn't we somehow mirror this by having two distinct verbosity levels -- one for when used as a panic handler and one for printing user provided backtraces?

@yaahc
Copy link
Contributor Author

yaahc commented May 2, 2020

Yes we should, I'll fix that

@yaahc
Copy link
Contributor Author

yaahc commented May 4, 2020

Hmm, it seems like there are a few options here for how to move forward but I'm not really sure which one you'd prefer.

The main issue is the BacktracePrinter has a single verbosity level and is shared between the panic hook printing and the direct backtrace printing. Here are the solutions I can think of:

  • carry around two verbosity levels, track if the printing logic is being invoked via print_panic_info and use the correct level
  • split the printer so that we have one for panic printing and one for backtrace printing (breaking change i assume)

Additionally, I didn't notice before but you already added the ability to override the verbosity, so technically this issue isn't really required, I can get the correct verbosity level set in my crate even without merging this, so I guess the 3rd option is just to close this, though I do think it would be idea to provide some level of RUST_LIB_BACKTRACE support in the defaults.

@athre0z
Copy link
Owner

athre0z commented May 5, 2020

I'd say we go with the first option.

We'll also need a second from_env func -- perhaps call it lib_from_env or something like this. Totally non-intuitive name, but I can't come up with anything better right now. Same goes for the prefix for the second verbosity level. Better suggestions more than welcome.

@yaahc
Copy link
Contributor Author

yaahc commented May 5, 2020

We'll also need a second from_env func -- perhaps call it lib_from_env or something like this. Totally non-intuitive name, but I can't come up with anything better right now. Same goes for the prefix for the second verbosity level. Better suggestions more than welcome.

I'm gonna pass on bikeshedding internal API names, lib_from_env sounds great to me, lol

@athre0z
Copy link
Owner

athre0z commented May 5, 2020

Hahaha, alright, lib_from_env it is. :D

Ok(ref x) if x == "full" => Verbosity::Full,
Ok(_) => Verbosity::Medium,
Err(_) => Verbosity::Minimal,
Self::convert_env(env::var("RUST_BACKTRACE").ok())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should these methods follow std's example and make some effort to cache these values?

Copy link
Owner

Choose a reason for hiding this comment

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

Nah, I think this is fine as-is. I'd argue that panic printers and thus verbosity instances are generally constructed only few times per app-run and it wouldn't be worth the effort.

Copy link
Owner

@athre0z athre0z left a comment

Choose a reason for hiding this comment

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

Thanks, excellent PR!

Ok(ref x) if x == "full" => Verbosity::Full,
Ok(_) => Verbosity::Medium,
Err(_) => Verbosity::Minimal,
Self::convert_env(env::var("RUST_BACKTRACE").ok())
Copy link
Owner

Choose a reason for hiding this comment

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

Nah, I think this is fine as-is. I'd argue that panic printers and thus verbosity instances are generally constructed only few times per app-run and it wouldn't be worth the effort.

@athre0z athre0z merged commit eb8ce74 into athre0z:master May 6, 2020
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.

Use RUST_LIB_BACKTRACE env variable for non panic backtrace verbosity
2 participants