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 access_log_format in GunicornWebWorker #1117

Merged
merged 1 commit into from
Aug 25, 2016

Conversation

f0t0n
Copy link
Contributor

@f0t0n f0t0n commented Aug 24, 2016

What do these changes do?

  • This is a workaround for a problem described in access log format is invalid when using gunicorn worker #705.
    It's intended to check GunicornWebWorker.cfg.access_log_format.
    If its value is default Gunicorn's format string, it will be replaced with default aiohttp's log format string to create request handler with.
    If it doesn't use default Gunicorn's format string but still sticks to Gunicorn's specification with format options in form of %(name)s, the ValueError will be thrown with an appropriate message.
  • Additionally a note about this added to Logging section of the documentation.

Are there changes in behavior for the user?

Should fix #705 and show correct output in the access log.

Related issue number

#705

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@f0t0n
Copy link
Contributor Author

f0t0n commented Aug 24, 2016

@asvetlov, does this change reflect the workaround you suggested?

@codecov-io
Copy link

codecov-io commented Aug 24, 2016

Current coverage is 98.15% (diff: 100%)

Merging #1117 into master will increase coverage by <.01%

@@             master      #1117   diff @@
==========================================
  Files            28         28          
  Lines          6371       6381    +10   
  Methods           0          0          
  Messages          0          0          
  Branches       1070       1072     +2   
==========================================
+ Hits           6253       6263    +10   
  Misses           63         63          
  Partials         55         55          

Powered by Codecov. Last update eb87b19...6111338

@@ -159,6 +161,11 @@ def _create_ssl_context(cfg):
ctx.set_ciphers(cfg.ciphers)
return ctx

def _fix_log_format(self, source_format, default_format):
if re.search(r'%\([^\)]+\)', source_format):
return default_format
Copy link
Member

Choose a reason for hiding this comment

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

Oh, no.
Silent format replacement should be performed only if source format is exactly default gunicorn log format, %(h)s %(l)s %(u)s %(t)s "%(r)s" %(s)s %(b)s "%(f)s" "%(a)s" according to the doc.
For all other gunicorn-styled but not default formats ValueError should be raised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, now I got you. The comment in issue was unclear to me. Going to fix this according to your input. Thanks!

Copy link
Contributor Author

@f0t0n f0t0n Aug 25, 2016

Choose a reason for hiding this comment

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

Seems this is done. Also added a note to docs "Logging" section.

image

@f0t0n f0t0n force-pushed the bug/gunicorn-access-log branch 5 times, most recently from 5112495 to 961dd21 Compare August 25, 2016 09:38
return self.DEFAULT_AIOHTTP_LOG_FORMAT
elif re.search(r'%\([^\)]+\)', source_format):
raise ValueError('Gunicorn style using `%(name)s` '
'is not supported for the log formatting.')
Copy link
Member

Choose a reason for hiding this comment

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

Please append http://aiohttp.readthedocs.io/en/stable/logging.html#format-specification link to exception text.
Now exception states that something is wrong but doesn't point on solution.

But it's most likely configuration problem, not programming error.
The exception will be caught by admin or devops, not software developer.

I suggest to provide very detailed error info in these cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Thanks for the point. Added a link and a text with correct directive.

- Replace default gunicorn's access log format with the default aiohttp's
    one.
- Use regular expression to check if the log format passed as a gunicorn's
option `access_logformat` uses Gunicorn's formatting style and in this case
raise a `ValueError`.
- Add a note describing this behavior to the `Logging` section of the
documentation.
@asvetlov asvetlov merged commit 21aae82 into aio-libs:master Aug 25, 2016
@asvetlov
Copy link
Member

Thanks!

@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.

access log format is invalid when using gunicorn worker
3 participants