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

Make EyreHandlers composible #35

Closed
yaahc opened this issue Sep 27, 2020 · 5 comments
Closed

Make EyreHandlers composible #35

yaahc opened this issue Sep 27, 2020 · 5 comments

Comments

@yaahc
Copy link
Collaborator

yaahc commented Sep 27, 2020

Right now eyre's design essentially requires that there's a single "Handler" type that is globally used in the application. Every error Report that is created will use the globally installed constructor for this handler type to create a new handler to go with the report to store additional context and print the error report + context. This is then meant to be paired with handler().downcast::<MyExpectedHandlerType>() when attempting to insert additional context into a Report as it propagates up the stack.

The problem is, if you are a library author who wishes to leverage eyre to hook additional functionality into error reporting you end up mandating that your users use your expected error report handler type, and if they ever install a different one all your downcasts will fail.

For a while my main plan for solving this problem has been to leverage https://github.com/mystor/object-provider to allow data to flow both ways across the dyn trait boundary without needing to downcast. This at least lets additional context be stored, so long as your users install a handler that will carry and provide your context the way you want them too, but this seems onerous and easy to forget to do.

I was looking at https://github.com/kellpossible/sentry-eyre today and trying to see what problems they were trying to solve and might run into and it dawned on me that what they might want is the ability to capture and send error report to sentry whenever they're printed, regardless of where the printing happens. The act of reporting it to a log or to a user would notify the back end.

That plus another PR that was opened today, eyre-rs/color-eyre#67, which enables composition of panic handlers, made me realize that this same approach might be very useful for EyreHandler as well. If it was possible to take the Box<dyn EyreHandler> out of a report and replace it with another one we could effectively allow libraries to tap into the error handler behind a report to add additional context and functionality to reporters.

Problems

So now I need to figure out how or if this can be done. The first issue that comes to mind is losing the ability to downcast back to the original type when you wrap it, thus breaking existing integrations such as .suggestion as provided by color-eyre. It might be possible to work around this by doing some shenanigans in the downcast impls in eyre, similar to how it already supports downcasting to two alternate types for errors constructed with wrap_err.

Another alternative is to build the composition into the trait itself by adding an inner() method or something, essentially an equivalent to source() on the Error trait. Though I think I'd rather leverage the object-provider crate for this, which lets us solve more than one problem at a time.

Yet another alternative is to allow the Report type to store multiple handlers, which are called sequentially during reporting. This would then probably require some changes to the external interface of eyre so that it can downcast to the correct types but it also seems like it is probably the most intuitive approach to use. I'd likely opt to deprecate the existing handler interface which I doubt anyone uses, rather than making a breaking change, since I already know from experience that that causes immediate issues to be filed in this repo, lol. Report does not match Report is not a fun error.

@kellpossible
Copy link
Contributor

sentry-eyre is just an experiment fork of sentry-anyhow at this stage I was playing with 😅 I'm not sure it's actually needed/beneficial yet. Still figuring out the sentry stuff myself. What you're saying here makes sense though, and I'm looking forward to spending more time to understand it and make sentry work well together with color-eyre/eyre

@yaahc
Copy link
Collaborator Author

yaahc commented Sep 29, 2020

sentry-eyre is just an experiment fork of sentry-anyhow at this stage I was playing with sweat_smile I'm not sure it's actually needed/beneficial yet. Still figuring out the sentry stuff myself. What you're saying here makes sense though, and I'm looking forward to spending more time to understand it and make sentry work well together with color-eyre/eyre

Still looks really interesting and is definitely something I want to support going forward, so I'm happy I got to start thinking about it now and mulling over the problem space and the possible solutions.

@yaahc
Copy link
Collaborator Author

yaahc commented Nov 13, 2020

I'm leaning towards closing this issue for now for a couple reasons.

  • It's not clear that many library authors will or should be depending on the behavior of the specific installed reporting hook
  • I'm convinced it should be possible to build any EyreHandler composition that libraries might want as an abstraction ontop of the current eyre interfaces.

I think that this problem is mostly a non issue, and that the way to handle this in practice will be to build abstractions on top of the current design. Currently there's a single reporter defined and controlled by the end binary. Without changing this we could build up "reporter composition" as a way to create the single reporter controlled by the end binary. I'm imaging some library with a hook type that takes some set of components that implement a trait it defines. Libraries could implement the parts of reporting they care about as these components, then a user could take the hook type and the set of library components they want and combine them to get a single reporter that has all the features the libraries they're using need.

In practice though I have the impression that most libraries shouldn't end up depending on eyre or any eyre hooks at all, and should instead define their own error types that fit their specific needs and work within the Error trait.

@yaahc yaahc closed this as completed Nov 13, 2020
@kellpossible
Copy link
Contributor

Hi @yaahc , does this affect getting eyre-rs/color-eyre#67 merged?

@yaahc
Copy link
Collaborator Author

yaahc commented Nov 17, 2020

@kellpossible yea, I will do that today, I'm sorry about the incredibly long delay.

Edit: wait I meant to say no, it doesn't affect it getting merged, since your PR is focused on making color-eyre's panic handler composible. I've just decided not to add extra features to do the same for eyre since it should already be possible the same way panic handlers are composible if the authors of them expose their guts, such as in your PR. ^_^

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

No branches or pull requests

2 participants