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

Variety of access logging issues #2641

Closed
thehesiod opened this issue Jan 6, 2018 · 5 comments
Closed

Variety of access logging issues #2641

thehesiod opened this issue Jan 6, 2018 · 5 comments
Labels

Comments

@thehesiod
Copy link
Contributor

@thehesiod thehesiod commented Jan 6, 2018

Issue 1

Looks like duplicate log format names was accidentally introduced: both %t and %T resolve to request_time see https://github.com/aio-libs/aiohttp/blob/master/aiohttp/helpers.py#L293-L299

each one should have a unique name, thoughts? Perhaps %T[f] + %D should be request_elapsed[_frac]

another idea is perhaps we should rename this to _LOG_FORMAT_MAP for 3.x?

Issue 2

%t is not the start time of the request, it's just the time the log entry was created (not meaningful). I suggest fixing this to be the time the request was received.

Issue 3

the base AccessLogger is not using self in __init__ so it's impossible for subclasses to replace methods like _format_t

Issue 4

logging of multiple header values doesn't serialize to extras, last one overwrites the others.

thehesiod added a commit to thehesiod-forks/aiohttp that referenced this issue Jan 6, 2018
@thehesiod thehesiod mentioned this issue Jan 6, 2018
2 of 5 tasks complete
@thehesiod
Copy link
Contributor Author

@thehesiod thehesiod commented Jan 6, 2018

I've created a PR with a rough idea of a fix

@thehesiod thehesiod changed the title Duplicate log format names Variety of access logging issues Jan 6, 2018
@asvetlov
Copy link
Member

@asvetlov asvetlov commented Jan 9, 2018

Let's discuss issues one by one.

  1. Log names was modeled after Apache log format: http://httpd.apache.org/docs/current/mod/mod_log_config.html

  2. Access Logger was not intended for inheritance, partially for historical reasons. That's why we don't use self in init but use class-level caching. The proper solution is moving a logger instantiation from web.RequestHandler to web.Server -- logger format parsing should be done once on server startup, not on every connection establishment.
    We can even rewrite the AccessLogger completely from scratch keeping AbstractAccessLogger API. It may speed up log generation as well (the best solution is replacing logger with f-strings as shown in https://docs.aiohttp.org/en/stable/logging.html#format-specification).

@thehesiod
Copy link
Contributor Author

@thehesiod thehesiod commented Jan 9, 2018

@asvetlov

  1. The issue isn't with the log specifiers, it's with the python logging extras key-name, they need to be unique in the dictionary.
  2. (actually 3) I'm not ready to re-write/architect the logger, I can revert this change for now

how about issues 2 + 4 ?

@thehesiod
Copy link
Contributor Author

@thehesiod thehesiod commented Jan 10, 2018

sorry, forgot to sync change for 4

@lock
Copy link

@lock lock bot commented Oct 28, 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].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.