warp icon indicating copy to clipboard operation
warp copied to clipboard

Feature request: More customization points in tracing

Open cbeck88 opened this issue 11 months ago • 2 comments

Is your feature request related to a problem? Please describe.

I have been greatly enjoying the use of warp::trace::request with my web service. It is really great that this can be done with so little boilerplate thanks to warp. However, I am frustrated by being unable to configure it as much as I would like.

The code that I would like to be able to configure is here in the finished_logger function: https://github.com/seanmonstar/warp/blob/376c80528fbf783dfb2825f8686698c3a51ac6d4/src/filters/trace.rs#L240

The issue for me is that, sometimes I get logs like this:

2023-08-01T20:13:54.040831Z  WARN request{method=POST path=/login version=HTTP/1.1 remote.addr=127.0.0.1:53508}: warp::filters::trace: unable to serve request (client error) status=423 error=None

What has actually happened is, my handler has reported an error and returned using something like warp::reply::with_status("Helpful error message", StatusCode::LOCKED). However, instead of seeing error=Helpful error message in the tracing logs, I see error = None.

That's in contrast with what happens when the handler actually returns a warp Rejection rather than a Reply. In that scenario it shows a list of rejection reasons at error = in the logs.

(Another approach for my situation might be to try to replace all these with_status results into warp rejections, but at least right now, that's not how I'm doing things. The thinking was that, in these cases, I don't want to go back into the routing system and try other routes and handlers, because at this point I really know what the response should be. It's also not clear to me how I can control the status code when using a custom rejection.)

I think the reason for the difference in these scenarios (reply with an error status code, vs. a rejection) is because at these lines in the finished_logger function:

        let (status, error) = match reply {
            Ok((Traced(resp),)) => (resp.status(), None),
            Err(error) => (error.status(), Some(error)),
        };

When we have a reply, "error" is always set to None, even if the status actually indicates an error. And then when we are in the client_error() scenario later:

        } else if status.is_client_error() {
            tracing::warn!(
                target: "warp::filters::trace",
                status = status.as_u16(),
                error = ?error,
                "unable to serve request (client error)"
            );

we will always report None for the error, even if the reply body contained an error message.

The other frustration I have is that, at least in some situations, I don't want to have client errors reported at warn level, because my logs can be massively spammed if someone is misusing my API. I might rather to set it to debug, run the servers at info level normally, and then change the filtering dynamically to debug for a short time if someone reaches out to me and asks me to investigate something.

In any case, as far as I can tell, I can't customize any of these decisions.

  • finished_logger is in the mod internal and baked into the WithTrace filter type
  • I can't make my own WithTrace because that requires me to have this bit: impl<FN, F> FilterBase for WithTrace<FN, F> but FilterBase is sealed.

(If there's a way to customize this that I'm missing, I would be interested to know)

Describe the solution you'd like

The simplest thing from my point of view would be if I can stick in my own fn(&Result<T, E>) or similar, which would be a member of WithTrace, to be used in place of finished_logger at this point: https://github.com/seanmonstar/warp/blob/376c80528fbf783dfb2825f8686698c3a51ac6d4/src/filters/trace.rs#L305, but could default to finished_logger perhaps.

Describe alternatives you've considered

I spent a while trying to figure out if there's some way to use this wrap_fn stuff to go around the WrapSealed trait: https://github.com/seanmonstar/warp/pull/693/files and do this all without modifying the library, but I haven't quite figured it out.

Additional context

Thanks

cbeck88 avatar Aug 01 '23 21:08 cbeck88