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

Eyre #215

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Eyre #215

wants to merge 6 commits into from

Conversation

jbr
Copy link
Member

@jbr jbr commented Aug 6, 2020

this is on top of the changes in #212 and not mutually exclusive with that or #213. The eyre-specific changes are in 46d2a1c

Tests are failing because somehow the Once is getting poisoned in tests

The Once is necessary because there's no obvious "app start" hook in http-types. One possibility is to just provide the backtrace method and require tide or some consumer to register stable-eyre. Or, ideally, we could just enable it with a feature flag on eyre, but eyre wants to be configured at runtime.

PR-ing this anyway for review and discussion and maybe someone can help me figure out why the Once is getting poisoned in test? @yoshuawuyts @yaahc

The upside is that this enables backtraces with files and line numbers, which seems like a big usability advantage over just a module name, and we can do stuff like filtering the backtrace by path in tide

Closes #214

@jbr jbr added the semver-major This change requires a semver major change label Aug 6, 2020
@jbr jbr requested a review from yoshuawuyts August 6, 2020 05:43
@yaahc
Copy link

yaahc commented Aug 6, 2020

ooh, as a library you'll probably want to avoid hard coding a dependency on one hook or another. I think this means I really should add back the object provider API in Eyre so libraries can do hook agnostic context lookup. Let me make some PRs to Eyre, stable-eyre, and color-eyre. then you'll need to use a diff fn to extract the backtrace but it will work for both stable-eyre and color-eyre.

@goto-bus-stop goto-bus-stop changed the base branch from master to main August 9, 2020 12:58
@jbr jbr mentioned this pull request Aug 12, 2020
@Fishrock123
Copy link
Member

What if we just use eyre directly as the error/result type and let e.g. Tide or the end application author install the hook?

That still allows the benefits and makes more structural sense. It also means that downstream could choose which reporter (such as color-eyre).

Additionally, perhaps we could make this a build option (feature)? We could then play around with it - I'm willing to try it out in my employer's application. I'm probably going to make my own branch to check things anyways, so I don't mind picking this up.

@yaahc
Copy link

yaahc commented Oct 27, 2020

What if we just use eyre directly as the error/result type and let e.g. Tide or the end application author install the hook?

That still allows the benefits and makes more structural sense. It also means that downstream could choose which reporter (such as color-eyre).

Additionally, perhaps we could make this a build option (feature)? We could then play around with it - I'm willing to try it out in my employer's application. I'm probably going to make my own branch to check things anyways, so I don't mind picking this up.

If you don't mind carrying the StatusCode along side the Report instead of inside, which this PR seems to imply already, then that's definitely the best option. I'm still a little sad about it. I wrote two PRs of different designs I was considering for how we could allow composable error reporting, or even extra handlers that you could push into reports from libraries, but neither of them really seemed like the right solution so they've kinda stalled.

My hope is that we'd eventually be able to consistently store the status code inside of the reporter, and retrieve it, and have little risk of users accidentally discarding them. Not there yet :/

@Fishrock123
Copy link
Member

@yaahc I noted in #263 (comment) (my rebase of this PR) that changing the underlying type and then having a global hook doesn't really solve anything for Tide in particular, which is where this issue is most focused for me.

In Tide all runtime errors stay within Tide - any that end up at the response are converted into a 500 Internal Server Error. To deal with runtime errors a logger middleware must be installed (one comes installed by default but can be disabled or replaced). It would have to handle the interaction with eyre reporting. Maybe you have some suggestions on how to best do that?

@yaahc
Copy link

yaahc commented Oct 28, 2020

In Tide all runtime errors stay within Tide - any that end up at the response are converted into a 500 Internal Server Error. To deal with runtime errors a logger middleware must be installed (one comes installed by default but can be disabled or replaced). It would have to handle the interaction with eyre reporting. Maybe you have some suggestions on how to best do that?

Probably, but I'll need more details:

to deal with runtime errors ...

Deal with how, just by logging or discarding them?

It would have to handle the interaction with eyre reporting

might also need clarification here, depending on the answer to the previous question.

@Fishrock123
Copy link
Member

Deal with how, just by logging or discarding them?

Tide turns all returns of Result::Err(http_types::Error) in middleware and endpoints immediately into a tide::Response with the http_types::Error attached. (`http_types::Error is a wrapper around anyhow.)

A bare tide server does nothing with this information (on purpose, to avoid leaking internal error messages) other than use it's status code. The default log middleware, or a user log middleware, error body transform middleware, or some other mechanism, must grab an optional error off the request to check if there was one, and then can decide what to do with it (use something from it, downcast it, etc).

I think the default LogMiddleware with error_eyre enabled would probably be where something should happen. This is where we'd want to format it in some eyre custom way.

@yaahc
Copy link

yaahc commented Oct 30, 2020

I think the default LogMiddleware with error_eyre enabled would probably be where something should happen. This is where we'd want to format it in some eyre custom way.

Okay, I think I'm following so far. And what extra features did you need to get from the wrapped eyre::Reports that aren't currently covered by anyhow::Report? Is it that you need those errors to capture more context than anyhow can or is it that you want to customize the format used when logging errors with the default LogMiddleware or is it something else entirely?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major This change requires a semver major change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Switch http-types::error::Error.error from anyhow to eyre
3 participants