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

Log built-in rejections #1890

Merged
merged 13 commits into from
Apr 11, 2023
Merged

Log built-in rejections #1890

merged 13 commits into from
Apr 11, 2023

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Mar 27, 2023

This enables logging of axum's built-in rejections, to make things easier to debug.

I also opted to merge the define_rejection and composite_rejection macros that we had in axum-core and axum. That way we only have to update once place when changing the built-in rejections. Done in a separate commit.

Example output:

2023-04-09T11:43:00.432638Z TRACE http_request{method=GET matched_path="/"}: axum::rejection: rejecting request status=415 body=Expected request with `Content-Type: application/json` rejection_type="axum::extract::rejection::MissingJsonContentType"

TODO

  • Implement it for all built-in rejections. Grep for fn body_text.
  • Docs, including documenting the feature flags.
  • Update tracing example to show filtering for for the axum::rejection target.
  • Changelog
  • When merged make the tracing feature default in axum, probably not in axum-core.

@davidpdrsn davidpdrsn marked this pull request as ready for review April 9, 2023 11:51
@davidpdrsn davidpdrsn requested a review from jplatte April 9, 2023 11:52
@davidpdrsn davidpdrsn enabled auto-merge (squash) April 9, 2023 11:52
axum-core/Cargo.toml Outdated Show resolved Hide resolved
target: "axum::rejection",
tracing::Level::TRACE,
status = $status.as_u16(),
body = %$body_text,
Copy link
Member

Choose a reason for hiding this comment

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

Why %?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question 🤔 it's not needed. I've removed it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Wut? Docs show that the impl exists: https://docs.rs/tracing/latest/tracing/trait.Value.html#impl-Value-for-String

Also, I think the default formatting for str is equivalent to the ?debug formatting rather than %display formatting.

axum/src/docs/extract.md Outdated Show resolved Hide resolved
davidpdrsn and others added 2 commits April 11, 2023 16:16
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
@davidpdrsn davidpdrsn requested a review from jplatte April 11, 2023 14:17
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Let's get more clarity on the %display thing in the tracing event before merging.

@davidpdrsn
Copy link
Member Author

Let's get more clarity on the %display thing in the tracing event before merging.

It's how you tell the tracing macro to use the values Display implementation of an event field for formatting (see https://docs.rs/tracing/latest/tracing/#recording-fields). But actually I notice now it's the MSRV test that fails, which uses minimal dependency versions. So its probably pulling in an old version of tracing which didn't have impl Value for String.

@davidpdrsn
Copy link
Member Author

Yep that seems to have been it. The % is gone!

@davidpdrsn davidpdrsn requested a review from jplatte April 11, 2023 14:56
@davidpdrsn davidpdrsn merged commit 6b106f4 into main Apr 11, 2023
@davidpdrsn davidpdrsn deleted the david/log-rejections branch April 11, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants