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

Implement axum_extra query rejection tracing #2527

Closed

Conversation

ghashy
Copy link

@ghashy ghashy commented Jan 19, 2024

This pull request aims to implement rejection tracing for axum_extra::extract::Query. Currently, the rejection tracing feature works well for other axum extractor types when using the env_filter feature, but it is not implemented for axum_extra::extract::Query.

The expected behavior is that rejections from axum_extra::extract::Query should be traced to the terminal when using the rejection tracing feature in Axum.

To address this issue, I have thoroughly investigated how rejection tracing is implemented for the axum::extract::Query type. I discovered that a macro from axum_core is used for this purpose. However, due to the presence of private fields in some of the generated types, it is not possible to simply import them. As a solution, I have created a new axum_extra::extract::rejection submodule and carefully copied the relevant macro calls from axum::extract::rejection.

Additionally, I have observed that proper tracing will only occur if the rejection tracing feature is enabled as an optional feature, as indicated by the #[cfg(feature = "tracing")] in axum_core crate (conditional compilation used):

/// Private API.
#[doc(hidden)]
#[macro_export]
macro_rules! __log_rejection {
    (
        rejection_type = $ty:ident,
        body_text = $body_text:expr,
        status = $status:expr,
    ) => {
        #[cfg(feature = "tracing")]
        {
            tracing::event!(
                target: "axum::rejection",
                tracing::Level::TRACE,
                status = $status.as_u16(),
                body = $body_text,
                rejection_type = std::any::type_name::<$ty>(),
                "rejecting request",
            );
        }
    };
}

With the changes that have been made, the tracing of rejections from axum_extra::extract::Query works now ("query" and "tracing" features should be enabled in axum_extra).
I have conducted thorough testing by running cargo test, and I am pleased to report that all tests have passed successfully.

Furthermore, I have noticed that rejection tracing is not implemented for some types within the axum_extra library. If you believe that my approach is on the right track, I would be more than happy to extend this implementation to cover all relevant types.

Thank you for your consideration.

@davidpdrsn
Copy link
Member

Is what #2584 not sufficient?

@ghashy
Copy link
Author

ghashy commented Feb 12, 2024

I reviewed @mladedav 's pull request, and his approach is definitely valid. There is only one thing to consider: axum-extra rejection types were initially implemented manually, without using macros from axum_core::macros, which were written to avoid code duplication when implementing rejection types necessary for extractors. The rejection types from axum-extra and their implementations (IntoResponse, fmt::Display, std::error::Error) looks similar to ones that axum_core::macros::__define_rejection and __composite_rejection generates, additionally macros also generates Debug implementation. With that being said, there are two approaches:

  1. Accept @mladedav 's implementation, if we need flexibility, for example we can have some unique features in (IntoResponse, fmt::Display, std::error::Error) implementations that will differ from original axum's rejection types.
  2. Do a little refactoring of axum-extra: replace all manual implementations of rejection types to axum_core::macros calls, and update FromRequestParts's associated types for extractors to generated ones. In that case we can achieve consistency and avoid code duplication.

I think I might had missed some details of axum architecture, so we need a more qualified opinion on this matter.

@jplatte
Copy link
Member

jplatte commented Apr 1, 2024

Not sure about reusing internal macros across crates, but the other PR is merged so I'll close this for now.

Thanks for your work anyways :)

@jplatte jplatte closed this Apr 1, 2024
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

Successfully merging this pull request may close these issues.

None yet

3 participants