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 feature for limit request to log in middleware logger using status code #3086

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tglman
Copy link
Contributor

@tglman tglman commented Jul 22, 2023

Hi,
This is yet a preliminary change to just collect feedback:

So this PR add a new feature on the logger middleware, to limit the logging to a range of status code, so that are logged only some specific status in the range, a simple example that will log only the 400+ request/responses is:

        Logger::default().statuses(StatusCode::BAD_REQUEST..);

Do you like this API is the naming right or you prefer a different one ?
Do you like this? shall I continue 😄 ?

PR Checklist

  • Tests for the changes have been added / updated.
    Could not really find a simple way to test this without an application, I didn't see any similar tests for other similar features where could I find/plug a test case for this case ?
  • Documentation comments have been added / updated.
    work in progress
  • A changelog entry has been made for the appropriate packages.
    will do
  • Format code with the latest stable rustfmt.
  • (Team) Label with affected crates and semver status.

@tglman
Copy link
Contributor Author

tglman commented Sep 15, 2023

Hi,

I did add a simple dev-dependency to capture logs and check them in the tests, so I could add some tests that check the full flow, I updated also some existing tests to check that the output is what is expected.

Now this PR should be complete of all the requirements.

Regards

@robjtede robjtede removed this from the actix-web v4.5 milestone Feb 4, 2024
@robjtede
Copy link
Member

robjtede commented Jun 11, 2024

The only problem I see with this is the issue of disjoint ranges. I can see it being reasonable to want everything except redirects being logged; in which case it's either:

  • 100..=299 ∪ 400..=999
  • 100..=999 ∖ 300..=399

It's tempting to go with the set subtraction technique since more common patterns can be expressed with just one builder method. Despite being terse, it might be confusing, too.

So, the API could be log_enabled: Vec<Range<StatusCode>>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-web project: actix-web B-semver-minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants