How to make Logger extension to utilize Debug implementation for my errors instead of Display?

[gyzerok]

Hello!

Recently I've started using async-graphql and it feels really great so far. Thank you for working on it!

When I use logger extension in my logs I see strings which come from Display trait implementation for my errors. From my perspective Display represents what consumers/users should see and Debug represents what developers should see when they look at the error.

There are cases where I want to hide some details from users/consumers and just output "unexpected error", but I would like to have full details in the logs for debugging purposes.

How can I achieve that?

[gyzerok]

I looked a bit inside Logger extension implementation and it seems that logger doesn't get the raw error which I am throwing inside my resolver.

So even if I am to implement my own custom logger, it seems like there is no way to use debug output of my errors for logging.

Help me :)

[gyzerok]

So far the only way to achieve the behavior I want is to extract resolvers into their own functions and to do logging manually.

#[derive(thiserror::Error)]
pub enum Failure {
    // errors
}

impl Debug for Failure {
  // implementation
}

#[Object]
impl UserQuery {
    pub async fn viewer(&self, ctx: &async_graphql::Context<'_>) -> Result<User, Failure> {
        viewer(ctx).await.map_err(|e| {
            tracing::error!("{:?}", e);
            e
        })
    }
}

async fn viewer(ctx: &async_graphql::Context<'_>) -> Result<User, Failure> {
    // some logic here
}

However this means that I have to add tracing::error!() call to every single resolver. It would be perfect if I could achieve that with a custom written extension. Probably now this goes more towards the feature request space, rather then the question.

[Miaxos]

Well, in fact when you use the #[Object] macro, there is a bunch of stuff going on, and one of those is to create an impl for the ContainerType actually used to process the queries, inside this implementation, there is a convert to a server_error

async-graphql/derive/src/object.rs

Line 198 in 878a1d4

map_err(|err| err.into_server_error(ctx.item.pos))?;

Probably now this goes more towards the feature request space, rather then the question.
Yes

I though a little about it and this is what could be done to allow more flexibility to error management (but without doing it in an extension):

Right now, the into_server_error is a private function, what could be done is to work around Error to create a public trait like

trait ResolverError {
  pub fn into_server_error(self, pos) -> ServerError;
  
  ...
}

Which would expose the main behavior for an Error and allow you to implement it as you like, so you'll be able to create a TracingError which would be able to error! it before converting it to a client side error.

I'll be able to look into it this week.

EDIT: no so easy to implement, an issue little linked to this one #652.
There is an implicit From implemented for every Error type inside a resolver to transform it to a async_graphql::Error before converting it to an ServerError, but I may have an idea to propose a first solution.

[gyzerok]

Hello @Miaxos and thank you for the detailed response!

I think I might have a slightly different idea. Let me outline it a bit later today/tomorrow so we can compare them. I am pretty new to Rust, but maybe it's my opportunity to dig a bit deeper into async-graphql and make a useful contribution 🤷

[gyzerok]

Your proposal

The downside I see in your proposal is boilerplate. Let's imagine that each my resolver has it's own specific error type enum Failure {}. Then for each such type I need to implement ResolverError trait.

pub enum ViewerResolverFailure {}

impl ResoverError for ViewerResolverFailure {}

pub enum SomeOtherResolverFailure {}

impl ResoverError for SomeOtherResolverFailure {}

Which in a way will be almost exactly (but with traits) what I am currently doing with functions

#[Object]
impl UserQuery {
    pub async fn viewer(&self, ctx: &async_graphql::Context<'_>) -> Result<User, ViewerResolverFailure> {
        viewer(ctx).await.map_err(|e| {
            tracing::error!("{:?}", e);
            e
        })
    }

   pub async fn some_other(&self, ctx: &async_graphql::Context<'_>) -> Result<User, SomeOtherResolverFailure> {
        some_other(ctx).await.map_err(|e| {
            tracing::error!("{:?}", e);
            e
        })
   }
}

And what I actually want is to get rid of this repetition + make sure that you can't forget to log errors correctly while writing new code.

My proposal

My suggestion is to focus around saving the initial error so one can use it later in the pipeline. For example I would be able to easily achieve what I want by writing custom Logger implementation if only I could get raw error.

What if instead of converting errors from async_graphql::Error to async_graphql::ServerError before passing response to schema extension we would do it only after? This way developers would be able to write some custom logic for operating on errors before they get converted and sent to the client.

To do this I would change async_graphql::Error like so:

pub struct Error {
    // not sure about the exact syntax here, not so proficient in Rust yet
    pub source: std::error::Error,
    #[serde(skip_serializing_if = "error_extensions_is_empty")]
    pub extensions: Option<ErrorExtensionValues>,
}

impl Error {
    /// Create an error from the given error message.
    pub fn new(message: impl Into<String>) -> Self {
        Self {
            source: Err(message.into()),
            extensions: None,
        }
    }

    /// Convert the error to a server error.
    #[must_use]
    pub fn into_server_error(self, pos: Pos) -> ServerError {
        ServerError {
            message: self.source.to_string(),
            locations: vec![pos],
            path: Vec::new(),
            extensions: self.extensions,
        }
    }
}

impl From<std::error::Error> for Error {
    fn from(e: T) -> Self {
        Self {
            source: e,
            extensions: None,
        }
    }
}

And then later in the logger I could use source to do whatever I like.

[gyzerok]

@sunli829 do you have opinion on the matter?

What I am proposing is looking somewhat similar to how actix does it. But I am a Rust newbie, so can't really grasp all the tricky detail here. And probably contributing this change goes beyond my skillset (unless you provide some guidance).

[sunli829]

This problem is not as easy to solve as it seems, and I need to think about it. 😁

[sunli829]

I believe I have solved this problem, here are some tests.

async-graphql/tests/error_ext.rs

Line 81 in e898998

pub async fn test_failure() {

Hope to get your feedback.😁 @gyzerok

[gyzerok]

@sunli829 do I understand correctly that your solution is to pass debug message alongside regular message?

It will probably solve my problem, that's true. Otherwise I believe I don't have much experience in Rust to review.

Personally I would try to go for a solution which delays converting errors to ServerError until very last step. Basically do it at schema.execute(request).await.into().

Now I started wondering if configuring schema with my own type of error would be a possible solution. Imagine something like the following. Bare with me, I do not know enough Rust to write actual code, so it's more like a pseudocode 😅

impl From<T: Display> for ServerError {
    fn from(e: T) -> ServerError {
        format!("{}", e)
    }
}

pub trait MyCustomError: fmt::Debug + fmt::Display {}

// Not sure how to write it here.
// The idea is somehow force all the resolvers to resolve to MyCustomError.
pub type MySchema = Schema<Query<MyCustomError>, Mutation<MyCustomError>, Subscription<MyCustomError>>;

And then make it so that the extension is also parametrized with the error type. So in the end I can get my own errors inside extensions. This way I can define multiple arbitrary behaviors on my own trait and then do whatever I want.

With your solution I would be able only to get the debug string. Which does solve my problem while being not generic to support any other use cases.

Like what if I want to log different errors from different resolvers with various rates (not 100%). Some errors are more important than others, and those are 100% logged. But some are very frequent and would quickly overwhelm log database. And it's not such an imaginary case. In my company we are working with a lot of traffic, so we have to deal with this :)

With my suggestion you could do something like this:

pub trait MyCustomError: fmt::Debug + fmt::Display {
    pub fn log_rate(&self) -> u8 { 100 };
}

struct MyActualError {
  AuthError,
  ValidationError
}

impl MyCustomError for MyActualError {
  pub fn log_rate(&self) -> u8 {
    match self {
      MyActualError::AuthError => 100, // very important
      MyActualError::ValidationError => 10, // not that important
    }
  }
}

// now somewhere in my custom logger extension implementation
if random(0, 100) <= err.log_rate() { println("{:?}" err) }

[sunli829]

pub type MySchema = Schema<Query<MyCustomError>, Mutation<MyCustomError>, Subscription<MyCustomError>>;

This is a breaking change, so I cannot use this solution.

With your solution I would be able only to get the debug string. Which does solve my problem while being not generic to support any other use cases.

You are right, I thought of a better way, wait for my news. 🙂

[sunli829]

I'm done, now the only trouble is that you need to manually convert your error type to Failure, like the following:

let s = String::from_utf8(bytes).map_err(Failure)?;

Get concrete error from ServerError:

server_err.concrete_error::<FromUtf8Error>()

Here is the test:

async-graphql/tests/error_ext.rs

Line 80 in d62aca8

pub async fn test_failure() {

Because Rust does not support generic specialization, this Failure type is required.

[gyzerok]

This is a breaking change, so I cannot use this solution.

Is there any way to provide "default" behavior, so it won't be a breaking change? Like you can define default type parameters. You do it when you define your own Result in the library by providing your Error type as a default. Is there some black magic which can be used here to avoid breaking changes?

I'm done, now the only trouble is that you need to manually convert your error type to Failure, like the following

You second iteration does address broader use-cases and is job well done! I understand that Failure seems unavoidable which makes for a less ergonomic API. And I am wondering if there is a hidden opportunity here to make one more iteration and twist the solution somehow else to provide more ergonomics? Maybe we can sit a bit more on it and think? Or there is literally no way around?

I have great time using your library so far and believe others feel the same. So in my opinion it's worth making extra effort to maintain that great feeling in terms of APIs.

[sunli829]

I improved again, and this time I think you should be satisfied. 🙂 @gyzerok

async-graphql/tests/error_ext.rs

Line 183 in 490cfec

Err(MyError::Error1)?

[gyzerok]

Yeah, if I understand everything correctly it looks great now! Thank you a lot for all the iterations and being so responsive 👏

May I suggest some subjective naming alterations?

1. Rename Failure to ResolverError so it's in like with ServerError and Error. I personally name errors as Failure just to avoid collision of my application errors with libs errors. But in the lib itself I think consistent naming should be made.
2. Rename concrete_error to cause. I think this way it the error conversion chain would be clear. Also personally I am not quite sure what "concrete" means here. Isn't ServerError concrete already? Feels a bit confusing.

Anyway I would be happy to try this out to see how it works in my app. Would you mind updating actix-web 4 branch after releasing this, so I can try it out?

[sunli829]

Yeah, if I understand everything correctly it looks great now! Thank you a lot for all the iterations and being so responsive 👏

May I suggest some subjective naming alterations?

1. Rename Failure to ResolverError so it's in like with ServerError and Error. I personally name errors as Failure just to avoid collision of my application errors with libs errors. But in the lib itself I think consistent naming should be made.
2. Rename concrete_error to cause. I think this way it the error conversion chain would be clear. Also personally I am not quite sure what "concrete" means here. Isn't ServerError concrete already? Feels a bit confusing.

Anyway I would be happy to try this out to see how it works in my app. Would you mind updating actix-web 4 branch after releasing this, so I can try it out?

Thank you for always helping to improve this feature, naming is very difficult for me. 🙂
BTW: Maybe you can try poem.

[sunli829]

Released in 2.11.1, and the branch actix-web-v4-beta has been updated.

[gyzerok]

Thank you, gonna try it now 😄 And I actually found a case why the order of fields in the response do matter. The version before that fix breaks my tests. So your update is right on time!

BTW: Maybe you can try poem.

Currently I am at my early stages of learning Rust. And most of the stuff I am doing is based on Zero to Production book.

Author is using actix-web there so I am using it as well. However I've decided to diverge a bit and use GraphQL instead of writing REST. While working with your library simple simple and fun, I believe that enough of additional learning complexity for me now.

Perhaps once I feel confident with all the things I have now on my place I can give a try to poem. Thank you for suggestion!

[gyzerok]

Ok, I've tried the code and it looks like I misunderstood how it was supposed to work. My apologies, Rust is still new to me.

Here is an example code from my project:

#[derive(Default)]
pub struct UserQuery;

#[Object]
impl UserQuery {
    pub async fn viewer(
        &self,
        ctx: &async_graphql::Context<'_>,
    ) -> Result<UserObject, ResolverError> {
        Ok(viewer(ctx).await?)
    }
}

async fn viewer(ctx: &async_graphql::Context<'_>) -> Result<UserObject, Problem> {
    // some code to get viewer from context

    let user = UserRepo::get_user_by_id(pool, &viewer.id)
        .await
        .context("failed to get user from the repo")?
        .ok_or_else(|| Problem::UnexpectedError(anyhow!("user not found")))?;

    Ok(UserObject { user })
}

The following problems appeared:

1. It works the way it is written in a code sample. However if I inline code from viewer function into resolver it stops working.
2. Also I was expecting to define resolver type as -> Result<UserObject, Problem> and not as -> Result<UserObject, ResolverError>.

Nonetheless I think it is better already, since I don't have to repeat tracting::error!() everywhere. And I was able to implement my custom logger which does exactly what I wanted it to.

However at least for me as a Rust newbee it wasn't really clear why the things are not working.

Also now it looks like we have regular error (which is part of libraries Result type alias) and ResolverError (which is not). Those 2 can probably bring some confusion for others.

Given all this I am not sure what to suggest. Personally I would really love this feature to work great for everybody. So I would rather struggle without this feature myself then do disservice to the library. Maybe you have some ideas?

[gyzerok]

It seems also that query function which helps to create cursor connections is forced to return async_graphql::Result. So I am not sure how to use it with my custom error types.

[sunli829]

It seems also that query function which helps to create cursor connections is forced to return async_graphql::Result. So I am not sure how to use it with my custom error types.

I changed the signature of the query function to allow the callback to use any type that implements Into<Error>.

[sunli829]

Given all this I am not sure what to suggest. Personally I would really love this feature to work great for everybody. So I would rather struggle without this feature myself then do disservice to the library. Maybe you have some ideas?

At the moment I think this ResolverError is very useful, but maybe we should explain why ResolverError is necessary in the book. Would you like to do this for this project? 🙂

[sunli829]

I just think ResolverError is not a good name. 🙂

[Miaxos]

For referencee, I proposed 'ResolverError' first thinking of a previous implementation we did at work:

Errors are implemented by implementing a trait which will tell:

• Should we stop the whole query execution and return a 500, or something or Should we just return a None for the upper level resolver
• Describe the error message retured
• Describe the error message logged

The ResolverError was the error which returned None for the upper optinal parent.

[sunli829]

The current implementation is more complicated, I decided to refactor in the v3.0 to provide a simpler API. 🙂

[sunli829]

Since Error no longer implements Display (I just found out), there will be no more type conflicts, so I deleted ResolverError. @Miaxos @gyzerok

async-graphql/tests/error_ext.rs

Line 80 in d33c9ce

pub async fn test_source() {

It looks pretty good now!

---

It is still a breaking change, so I support it in v3.0.

[sunli829]

I can't design a more convenient API without generic specialization, so I can only keep ResolverError. 😒

[gyzerok]

I will definitely be interested to contribute to the package in some way. Maybe improving docs is a possible option.

Currently I am working on a project and trying to collect my thoughts around the library APIs and the ways things are not working that well for me. Will try to wrap all the experiences up and share, once I am ready. Hopefully we can take things forward from that point :)

[Miaxos]

I can't design a more convenient API without generic specialization, so I can only keep ResolverError. 😒

Haha, I'm also looking forward to the specialization feature 😁!
I will try to take some time to look into what you did tonight, I have also some ideas to write for the v3!

[sunli829]

I added Error::new_with_source method in v3, so the ResolverError type is no longer needed.

[gyzerok]

Hello @sunli829!

I am a bit confused with Error::new_with_source and how I am supposed to use it. Does it mean that I need to wrap every error in every resolver with it, or is it possible to make this work generically somehow?

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[github-actions]

This issue was closed because it has been stalled for 5 days with no activity.