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

feat: log inner error in additon to the context #4128

Closed
wants to merge 1 commit into from
Closed

Conversation

hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Apr 1, 2024

Summary of changes

anyhow::Context provides a convenient way of wrapping the error with some context text, and the inner text is present in stack trace when the final error is unwrapped. However, In a long-running service like forest, most of the errors are logged with tracing instead of being unwrapped. In this case, unless the debug formatting("{error:?}") is specified, the inner errors which are quite helpful for troubleshooting in many cases are not present in the log text. To fix the issue, we could

  1. specify debug formatting
  2. make a simple wrapper of .map_err.

This PR tries to implement the 2nd solution above.

Changes introduced in this pull request:

  • Introduce crate::error::Context2 as a drop-in replacement of anyhow::Context

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@hanabi1224 hanabi1224 marked this pull request as ready for review April 1, 2024 10:08
@hanabi1224 hanabi1224 requested a review from a team as a code owner April 1, 2024 10:08
@hanabi1224 hanabi1224 requested review from LesnyRumcajs and sudo-shashank and removed request for a team April 1, 2024 10:08
@aatifsyed aatifsyed self-requested a review April 2, 2024 09:32
Copy link
Contributor

@aatifsyed aatifsyed left a comment

Choose a reason for hiding this comment

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

I don't think we should do this (or I don't understand the problem enough).

let e: anyhow::Result;
tracing::error(?e, "error doing foo");

This gives us the desired formatting:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b139670fd7096276d92c0da257608a86

If you really want to do it this way, I would have a shim around anyhow::Error that just changed the formatting params

@LesnyRumcajs
Copy link
Member

How it's done is a detail. The fact that we need to use the Debug to get the full trace is non-intuitive, making the current error messages difficult to reason about.

Either this change needs to be implemented, one way or another, or all error messages should use the ?e or a variant of that. If we do the latter, it should be enforced in CI. As discussed with @hanabi1224 in Slack, our expectation is that the Display of an error is a sum of its errors, not just the top-level context, which is rarely useful on its own.

@aatifsyed
Copy link
Contributor

The fact that we need to use the Debug to get the full trace is non-intuitive, making the current error messages difficult to reason about.
...
As discussed with @hanabi1224 in Slack, our expectation is that the Display of an error is a sum of its errors, not just the top-level context, which is rarely useful on its own.

Anyhow is correctly following the documentation on std::error::Error:

Error messages are typically concise lowercase sentences without trailing punctuation

A motivator for anyhow::Error's Debug implementation is that it gives the chain when used with std::process::Termination

I don't think the error is in any of the APIs we use, so I don't in-principle agree with (either)

  • duplicating anyhow::Context in this PR
  • (or) wrapping anyhow::Error to change its formatting

I can bear it as an interim measure, but the real fix is to use tracing's structured logging machinery #4143

Copy link
Contributor

@aatifsyed aatifsyed left a comment

Choose a reason for hiding this comment

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

Fine with this as an interim measure, but prefer #4143

@hanabi1224
Copy link
Contributor Author

Superseded by #4174

@hanabi1224 hanabi1224 closed this Apr 22, 2024
@hanabi1224 hanabi1224 deleted the hm/context2 branch April 22, 2024 01:25
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.

3 participants