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

enhancement to AccessLogger #1303

Merged
merged 10 commits into from
Oct 15, 2016
Merged

enhancement to AccessLogger #1303

merged 10 commits into from
Oct 15, 2016

Conversation

thehesiod
Copy link
Contributor

populates "extra" dict for clients

@thehesiod thehesiod mentioned this pull request Oct 11, 2016

extra = {name: value for name, value in fmt_info}
self.logger.info(self._log_format %
tuple([value for _, value in fmt_info]),
Copy link
Member

Choose a reason for hiding this comment

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

is it fmt_info.values()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not currently as fmt_info is a tuple of tuples to keep items in format order. Would you prefer it be a OrderedDict/namedtuple?

Copy link
Member

Choose a reason for hiding this comment

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

Aaah, got it.
But if you'll return just extra OrderedDict from _format_line the code may look better.

Also keys for headers/envvars from dicts like extra={'%{None}i': '-', '%{SPAM}e': 'EGGS', '%{Content-Length}o': 123, '%{User-Agent}i': 'Mock/1.0'} looks... Well, they looks non-intuitive.
I would expect something like input_header[User-Agent] instead of cryptic %{User-Agent}i.

Maybe constructing _log_format as format accepting params by name in new-styled .format form like "{status} {remote_address} {input_headers[User-Agent]}" makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually reading the docs for the new formatter it seems like the logging module in python 3.2+ supports the new '{}' style formatting, making this solution a win-win and removes any hesitations I had for this solution, yay! I'll code something up shortly.

@thehesiod
Copy link
Contributor Author

thehesiod commented Oct 12, 2016

ok I think this is what we want, extra will contain things like:

{"response_status": 200, 
"remote_address": "127.0.0.1", 
"request_time_frac": "0.317883", 
"request_header": {
    "User-Agent": "python-requests/2.11.1"
}, 
"first_request_line": "POST /v1/locations HTTP/1.1", 
"response_size": 83}

this way users can have a formatter as follows:

access_log = aiohttp.log.access_logger
fmtr = logging.Formatter('user_agent: {request_header[User-Agent]}', style='{')
lh2 = logging.StreamHandler()
lh2.setFormatter(fmtr)
access_log.addHandler(lh2)

we set the root log handler so if we have the correct format everything "just works" ™️ :)

@thehesiod
Copy link
Contributor Author

I'm not sure if it's doable but it may be worth thinking if it's possible to use a new-style formatter and having the logging system now compose the logging message based on this new extra param.

@asvetlov
Copy link
Member

Awesome!
Would you fix broken tests?

@codecov-io
Copy link

codecov-io commented Oct 13, 2016

Current coverage is 96.68% (diff: 100%)

Merging #1303 into master will decrease coverage by 1.82%

@@             master      #1303   diff @@
==========================================
  Files            29         29          
  Lines          6497       6515    +18   
  Methods           0          0          
  Messages          0          0          
  Branches       1090       1094     +4   
==========================================
- Hits           6400       6299   -101   
- Misses           47        148   +101   
- Partials         50         68    +18   

Powered by Codecov. Last update d9d4d12...fe5e5eb

@asvetlov asvetlov merged commit d54e41d into aio-libs:master Oct 15, 2016
@asvetlov
Copy link
Member

Thanks!

@thehesiod thehesiod deleted the thehesiod-logging branch December 6, 2016 03:35
@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants