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

Fix default access_log_format to correct value #2650

Merged
merged 3 commits into from
Jan 10, 2018
Merged

Fix default access_log_format to correct value #2650

merged 3 commits into from
Jan 10, 2018

Conversation

azhpushkin
Copy link
Contributor

@azhpushkin azhpushkin commented Jan 10, 2018

What do these changes do?

Fixing wrong default value of access log format

Are there changes in behavior for the user?

Should not affect anything

Related issue number

#2649

Checklist

  • I think the code is well written
  • Documentation reflects the changes
  • Add a new news fragment into the CHANGES folder

@fafhrd91
Copy link
Member

aiohttp uses None for specific reason. enabled server logging significantly reduce performance even if logging it self is not enabled.

@azhpushkin
Copy link
Contributor Author

Problem of this PR is more about having bad parameters, I tried to describe everything in details in the issue.
Access logger is enabled while default argument of class is set (access_logger is not False). I could set it to None in this PR also, however this is another story so I would like to leave it the way it is .

@fafhrd91
Copy link
Member

drop performance by 20% is not good change, especially for default parameters.

@codecov-io
Copy link

codecov-io commented Jan 10, 2018

Codecov Report

Merging #2650 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2650   +/-   ##
=======================================
  Coverage   97.92%   97.92%           
=======================================
  Files          38       38           
  Lines        7267     7267           
  Branches     1262     1262           
=======================================
  Hits         7116     7116           
  Misses         47       47           
  Partials      104      104
Impacted Files Coverage Δ
aiohttp/web.py 98.76% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8173a0...23e175b. Read the comment docs.

@azhpushkin
Copy link
Contributor Author

@fafhrd91 Sorry, maybe I`m not following something but with current default params for me views are not working at all. Access logger is being initialized for each request in the following part of code:

if access_log:
self.access_logger = access_log_class(
access_log, access_log_format)
else:
self.access_logger = None

As access logger over here is True access_log_class(...) is called. Afterwards, default AccessLogger.init is called, than compile_format() is called and it fails in the following line:

for atom in self.FORMAT_RE.findall(log_format):

This is happening because log_format over here is None.
Also there is no exception or traceback being produced before loop is closed. Can you please try Steps to reproduce to see whether you getting this problem too?

@fafhrd91
Copy link
Member

you are right, sorry. somehow I thought that access log is disabled by default.

@asvetlov
Copy link
Member

Access log was enabled by default in aiohttp 2.x
The issue is a result of my refactoring for AppRunner, I forgot to set proper default value.

@asvetlov asvetlov merged commit a5335b8 into aio-libs:master Jan 10, 2018
@asvetlov
Copy link
Member

Thanks

@lock
Copy link

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
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bot:chronographer:provided There is a change note present in this PR outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants