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

IGNORE_URL setting to opt-out for some URLs #37

Closed
Iftahh opened this issue Apr 3, 2020 · 5 comments
Closed

IGNORE_URL setting to opt-out for some URLs #37

Iftahh opened this issue Apr 3, 2020 · 5 comments
Labels
suggestion A feature request/suggestion

Comments

@Iftahh
Copy link
Contributor

Iftahh commented Apr 3, 2020

I have a few requests that I don't want to see in the logs

eg. a load balancer sends a "ping" once in a while to see the server is up
the logs are spammed with generated new GUID messages:

django_guid - Header `Correlation-ID` was not found in the incoming request. Generated new GUID: cf306e6e90bc42238f025d8d3fddc8ac

for this request I would prefer to opt out of the middleware - a decorator would work nicely

@no_guid
def ping(request):
    return HttpResponse("Ok\n")

Edit:
It turns out that to use a decorator for opt-out would require to use the process_view hook, which runs after all the middelwares __call__ methods have been run. If the GUID would only be added at that point, it wouldn't be useful for debugging of other middleware, which is not a good idea.
For that reason, instead of a decorator, the plan is to use a "black list" of URLs to ignore. I edited the Issue title to reflect this change.

@Iftahh Iftahh added the suggestion A feature request/suggestion label Apr 3, 2020
@JonasKs
Copy link
Member

JonasKs commented Apr 3, 2020

Hmm, I see. I do have the same in my setup, but I don't really look at the logs without filters so haven't really noticed it. I understand where you're coming from, though.

As a quickfix right now you could disable that log for all requests by adding django_guid to the loggers and set level to WARNING. See example in step 4 here. If I remember correctly that's the only INFO there is. (I would double check this though)

For your suggestion I'm pretty sure it's not possible to disable middlewares from a view. (However it is possible to append a middleware before the view by using
django.utils.decorators.decorator_from_middleware)
With that said, my suggestion is to add a setting to config.py called IGNORE_URLS, and then in the beginning of the middleware check if the request.path is in the list of ignored URLs and return response if it is.
I'm unable to implement this today, but PRs are welcome!

@Iftahh
Copy link
Contributor Author

Iftahh commented Apr 3, 2020

Thank you for the quick and thoughtful response

I guess a IGNORE_URLS list would work but I personally like the decorator one better - the list can collect stale URLs as developers will remove urls and forget to update the list.

The CSRF protection middleware has a @csrf_exempt decorator. I took a look at your code vs their code, and its annoyingly almost possible but not quite.
The CSRF protection middleware uses only the process_view hook which has access to the view_func - this can be decorated and marked as "exempt".
Your middleware uses (the more modern? more pretty?) approach to use chains of get_response - and unfortunately this doesn't have access to the view function at all.

I can change your middleware to use the process_view hook instead, I believe that would work and have no negative impact.

The only negative impact that maybe can happen is a change in the position of your middleware's logic (in relation to the other middlewares).
I don't think this position will change, but I would like to verify it because I'm not 100% certain.

And I would like your permission before I even invest time in checking this -

assuming no impact on the middlewares order, the only impact would be less elegant code - would it be ok to move the middleware handling (or some of it) from __call__ to the process_view hook?

@JonasKs
Copy link
Member

JonasKs commented Apr 3, 2020

I see, thanks for doing the research!

If your assumption turns out to be right and the only impact would be less elegant code I'm not against the idea at all, however it's important to me that there's either good comments or docs that explains it. I want people to understand what's happening and why, making it easy to extend, contribute to and most importantly learn from.

@Iftahh
Copy link
Contributor Author

Iftahh commented Apr 4, 2020

hi,

So I did a quick test now - I added a middleware before and after the Guid middleware with logs in their __call__ method and for completeness also logs in their process_view method.

Unfortunately, the order of the calls is:

1. BeforeMiddlware::__call__
2. GuidMiddlware::__call__ 
3. AfterMiddlware::__call__
4. BeforeMiddlware::process_view
5. GuidMiddlware::process_view
6. AfterMiddlware::process_view

So, if the GUID is added today at step 2 - and is visible in the logs of steps 3+

but if I move it to process_view (so that I can opt-out by checking the view-function), the GUID will be added at step 5.
This has negative impact, eg. the "AfterMiddleware" __call__ logs will not show the GUID for that request.

I sadly recommend to give up on an opt-out decorator, and maybe revisit this in Django 4.0 if they change the way middleware work.

I'll make a PR with an (optional) IGNORE_URLS setting, as you suggested.

@Iftahh Iftahh changed the title decorator to opt-out IGNORE_URL setting to opt-out for some URLs Apr 4, 2020
@JonasKs
Copy link
Member

JonasKs commented Apr 5, 2020

I understand. Thanks for testing it out and reporting back!

@JonasKs JonasKs closed this as completed Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion A feature request/suggestion
Projects
None yet
Development

No branches or pull requests

2 participants