http-rs / http-types

Common types for HTTP operations
https://docs.rs/http-types
Apache License 2.0
200 stars 83 forks source link

Eyre #215

Open jbr opened 4 years ago

jbr commented 4 years ago

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

yaahc commented 4 years ago

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.

Fishrock123 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@yaahc I noted in https://github.com/http-rs/http-types/pull/263#issuecomment-717487784 (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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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?