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

make this module work nicely with wsgi-request-id #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

schlitzered
Copy link
Contributor

add the request uuid to the access log, if wsgi-request-id is used, otherwise ignore it.

some pep8 fixes in setup.py

@schlitzered
Copy link
Contributor Author

hey, did you had a chance to review this?

@pklaus
Copy link
Owner

pklaus commented May 10, 2016

Sorry, it took me so long to respond!

The problem with this pull request is that it changes what users expect from the Apache NCSA log format and it possibly breaks their log file analysis. That's not something that users expect.

Why not create a custom named "formatter" that includes this piece of information by adding it as a @staticmethod just as it's the case with format_with_response_time? In addition it should employ - as a placeholder, in case the REQUEST_ID is not found in the environ dictionary. This could make it a bit more compatible with the Apache conventions and the %L statement of Apache's mod_log_config custom logs format:

class ApacheFormatters(object):

    [...]

    @staticmethod
    def format_with_request_id(status_code, environ, content_length, **kwargs):
        """
        Log the WSGI REQUEST_ID too.
        The request id can be inserted with wsgi-request-id:
        https://pypi.python.org/pypi/wsgi-request-id/
        """
        log_line = ApacheFormatters.format_NCSA_log(status_code, environ, content_length)
        log_line += ' ' + environ.get('REQUEST_ID', '-')
        return  log_line

This formatter could then be used by initializing your logging in the appropriate way:

handlers = [ TimedRotatingFileHandler('access.log', 'd', 7) , ]
loggingapp = WSGILogger(application, handlers, ApacheFormatters.format_with_request_id)

This code is untested yet, but I think this is the way to go without breaking others' setups.

The open question to me is: Do you want the response time to be included in this new output format or not?

What do you think?

@schlitzered
Copy link
Contributor Author

@pklaus yes, is was not thinking at others logfiles analyzers, you are right, the NSCA format should not be changed.

implementing this as an additional formatter sounds reasonable, so yes, sounds great. It would also be nice to have the response time, at least i am using it.

Thank You.

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.

2 participants