tl-its-umich-edu / remote-office-hours-queue

Virtual queuing tool supporting Zoom video conferencing and/or in person meetings.
https://officehours.it.umich.edu/
Apache License 2.0
12 stars 28 forks source link

#477 removed django.middleware.common.BrokenLinkEmailsMiddleware #478

Closed zqian closed 5 months ago

jonespm commented 7 months ago

This is fine, I thought we'd configured this by changing the IGNORABLE_404_URLS and adding some patterns, but I don't see that done.

I'd like to verify that the errors that are sent in the emails are still logged. We might need to adjust the logging to pick those up if they aren't.

zqian commented 7 months ago

Based on the alert messages, there are a lot a URL patterns to be added into IGNORABLE_404_URLS .

I'd like to verify that the errors that are sent in the emails are still logged. We might need to adjust the logging to pick those up if they aren't. This is a better approach: log known errors if this is a short list.

jonespm commented 5 months ago

GPT suggested subclassing this. I think we can have a few patterns we care about and just log the rest.

Yes, you can log the ignored URLs by modifying the FilteredBrokenLinkEmailsMiddleware to use Django's logging system. Here's how you can update the process_response method to log ignored 404 URLs:

  1. First, make sure Django's logging is configured in your settings.py file. Here's an example configuration that logs messages to the console:
LOGGING = {
    'version': 1,
    'disable_existing_loggers': False,
    'handlers': {
        'console': {
            'level': 'INFO',
            'class': 'logging.StreamHandler',
        },
    },
    'loggers': {
        'django.request': {
            'handlers': ['console'],
            'level': 'INFO',
            'propagate': False,
        },
    },
}
  1. Update your subclass FilteredBrokenLinkEmailsMiddleware to include the logging logic:
import logging
from django.utils.deprecation import MiddlewareMixin
from django.middleware.common import BrokenLinkEmailsMiddleware

logger = logging.getLogger('django.request')

class FilteredBrokenLinkEmailsMiddleware(BrokenLinkEmailsMiddleware, MiddlewareMixin):
    def process_response(self, request, response):
        # Only process 404 responses.
        if response.status_code == 404:
            # Check if the requested URL path starts with '/queue/'
            if request.path.startswith('/queue/'):
                # If it does, call the superclass method to process this response.
                return super().process_response(request, response)
            else:
                # If the URL path is ignored, log the event.
                logger.info('Ignored 404 URL: %s', request.path)

        # For 404 responses of other paths or non-404 responses, no further action is taken.
        return response
  1. Make sure your new middleware is included in the MIDDLEWARE setting in your settings.py file (as explained in the previous messages).

With the updated middleware, whenever a URL that does not start with /queue/ results in a 404 response, it will be logged using the 'django.request' logger. The logs will go to the configured handlers—in this case, to the console.

Remember that you can also log to a file or any other handler supported by Python's logging module, depending on your logging configuration in settings.py. Adjust the logger settings to fit your application's requirements such as logging levels, file handlers, formatting, etc.

jonespm commented 5 months ago

This was a GPT suggestion from a prompt like

how could we just have BrokenLinkEmailsMiddleware report on urls like /queue/ and not anything else? Could you also log the other ones?

and seems good. Seems easier to whitelist rather than blacklist. Something like INCLUDABLE_404_URLS rather than IGNORABLE.

I'd make the paths configurable in this but otherwise seems fine. And you probably don't need the logging config. if request.path.startswith('/queue/'):

zqian commented 5 months ago

@jonespm The PR is updated: added FilteredBrokenLinkEmailsMiddleware with filtered 404 url patterns.

However, after the recent change in #504 , the /query/ url now returns status 200 instead of 404, hence no alert email will be generated for /query patterns.

jonespm commented 5 months ago

You mean the /queue/ queries? Yeah, I guess that probably doesn't need to be an exception anymore since we're giving an appropriate error now. So probably don't need to consider that. I'm not sure how many of those matched that pattern.

Looking to see what other patterns might be useful

Maybe /api/queues or /api/meetings'. Though we've never really followed up on those when they happen.

zqian commented 5 months ago

URL patterns updated to /api/queues or /api/meetings