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

Add rejection tracing to all extractors (#2526) #2584

Merged
merged 1 commit into from
Mar 16, 2024

Conversation

mladedav
Copy link
Collaborator

@mladedav mladedav commented Feb 9, 2024

Motivation

Rejection tracing was missing at some points.
Closes #2526

Solution

This should add the tracing eveyrwhere, I searched by into_response implementations.

I've also changed the places where it already was to call body_text only once as it makes an unnecessary copy of the string.

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Do you wanna update the changelog as well?

@mladedav
Copy link
Collaborator Author

Sure. When I added it to the changelog, I realized I'm using the same macro that's used in axum itself, so the target is axum::rejection which could be potentially confusing since it originates in axum-extra.

Do you think it'd be better to change the macro to also (optinally?) take the target as an argument and use axum-extra::rejection instead or is this ok?

@mladedav mladedav marked this pull request as draft February 12, 2024 12:38
@mladedav
Copy link
Collaborator Author

When I looked at #2527, I realized that this would not work as currently implemented because the macro uses cfg(features = ...) but since it is expanded in axum-extra which does not define the tracing feature at all, this would never actually log anything.

I'll fix this hopefully during today.

@mladedav mladedav force-pushed the dm/rejection-tracing branch 2 times, most recently from a865a4c to 7f15001 Compare February 12, 2024 22:44
@mladedav
Copy link
Collaborator Author

mladedav commented Feb 12, 2024

I've added tracing feature to axum-extra, same as is in axum. It's on by default in axum, so I've also turned it on by default in axum-extra so that it is not surprising to the user that something is logged and something else is not. However, it may still be surprising that they have to turn it off in two places.

I ultimately went this way because tracing wasn't a dependency so it felt wrong just adding it without it being behind a feature and once this needed feature anyway, there was no reason not to reuse the macro from axum-core.


By the way, I just noticed that the tracing feature in axum-core doesn't seem to be used anywhere except for the macro that generates the rejection message. However, it seems from my experiments that this checks for the features of the crate that uses the macro, not the one that has defined it. So I think that feature can be removed. Unless it's there to force the tracing version?

One way out of this I can think of is using a procedural macro instead which can use cfg internally which I believe would work in the way that seems intended here. Then enabling the feature in either crate could turn on logging of rejections in both crates. But I'm not sure that's the way.

To summarize, the options I think are:

  • Two separate features, axum has it on by default, axum-extra has it opt-in (current)
  • Two separate features, on by default for both
  • Two features, enabling it for axum-extra can also enable it for axum, of course not the other way around because of cyclic dependencies.
  • Using a proc macro for generating the tracing event, then enabling the feature in either crate could enable it in both.

I think the first two option give the most options to the users (although I don't know why they'd ever decide to turn on logging for one crate and not the other), with the second option acting in a least surprising way.

So if it's ok with you I would also remove the tracing feature from axum-core, otherwise I can change the features if you think axum-extra should not enable any by default.

@mladedav mladedav marked this pull request as ready for review February 12, 2024 22:45
@jplatte
Copy link
Member

jplatte commented Feb 13, 2024

Removing a feature is a breaking change, even if it does nothing. I agree that we should eventually remove it unless we want to actually make it do something, but please don't outright remove it for now.

@jplatte jplatte merged commit 2ec68d6 into tokio-rs:main Mar 16, 2024
18 checks passed
@mladedav mladedav deleted the dm/rejection-tracing branch March 16, 2024 21:37
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.

Rejection tracing not implemented for axum_extra::extract::Query
3 participants