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

aiohttp.wep.Application.make_handler access_log_class support #2316

Merged
merged 13 commits into from
Oct 13, 2017
24 changes: 24 additions & 0 deletions aiohttp/abc.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,3 +146,27 @@ def write_eof(self, chunk=b''):
@abstractmethod
def drain(self):
"""Flush the write buffer."""


class AbstractAccessLogger(ABC):

def __init__(self, logger, log_format):
self.logger = logger
Copy link
Member

Choose a reason for hiding this comment

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

log_format seems to be lost.

Copy link
Member Author

@hellysmile hellysmile Oct 12, 2017

Choose a reason for hiding this comment

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

Yes, but it is exactly how current AccessLogger works, as well default log_format is AccessLogger.LOG_FORMAT not sure is it needs to be stored as self.log_format. My idea was that, logger itself is only one required attribute, log_format is actually optional

Copy link
Member

Choose a reason for hiding this comment

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

So, if I'm making own AccessLogger I should keep in mind, that super requires two arguments, one of which it will set to attributes and one that it will ignore? A bit not friendly behavior. The AccessLogger may have overloaded __init__ to accept log_format, but if the AbstractAccessLogger doesn't uses it in anyway there is no point to keep it there.

self.log_format = log_format

@abstractmethod
def _log(self, request, response, time):
"""

:param request: Request object.
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use sphinx formatting in docstrings -- it is a text for readers.
I'd like to rip it out everywhere and use only hand written documentation.

:param response: Response object.
:param float time: Time taken to serve the request.
"""

def log(self, request, response, time):
Copy link
Member

Choose a reason for hiding this comment

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

Need to document which objects and types that method accepts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, it will be documented

"""Emit log to logger"""

try:
self._log(request, response, time)
Copy link
Member

Choose a reason for hiding this comment

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

Let's drop _log() and make log() abstract method.
On one hand formatting logic always should be wrapped by catching exception obviously.
On other -- let's not make any decision about implementation details.
Implementor might want to do other things than just registering caught exception.
Let's keep ABC very abstract, my guts feel that there are many contradictory might occur if we will bake a code in abstract class.

except Exception:
self.logger.exception("Error in logging")
38 changes: 14 additions & 24 deletions aiohttp/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from yarl import URL

from . import hdrs
from .abc import AbstractAccessLogger
from .log import client_logger


Expand Down Expand Up @@ -346,7 +347,7 @@ def content_disposition_header(disptype, quote_fields=True, **params):
return value


class AccessLogger:
class AccessLogger(AbstractAccessLogger):
"""Helper object to log access.

Usage:
Expand Down Expand Up @@ -400,7 +401,7 @@ def __init__(self, logger, log_format=LOG_FORMAT):
:param log_format: apache compatible log format

"""
self.logger = logger
super().__init__(logger, log_format=log_format)

_compiled_format = AccessLogger._FORMAT_CACHE.get(log_format)
if not _compiled_format:
Expand Down Expand Up @@ -512,31 +513,20 @@ def _format_line(self, request, response, time):
return ((key, method(request, response, time))
for key, method in self._methods)

def log(self, request, response, time):
"""Log access.
def _log(self, request, response, time):
fmt_info = self._format_line(request, response, time)

:param message: Request object. May be None.
:param environ: Environment dict. May be None.
:param response: Response object.
:param transport: Tansport object. May be None
:param float time: Time taken to serve the request.
"""
try:
fmt_info = self._format_line(request, response, time)
values = list()
extra = dict()
for key, value in fmt_info:
values.append(value)

values = list()
extra = dict()
for key, value in fmt_info:
values.append(value)

if key.__class__ is str:
extra[key] = value
else:
extra[key[0]] = {key[1]: value}
if key.__class__ is str:
extra[key] = value
else:
extra[key[0]] = {key[1]: value}

self.logger.info(self._log_format % tuple(values), extra=extra)
except Exception:
self.logger.exception("Error in logging")
self.logger.info(self._log_format % tuple(values), extra=extra)


class reify:
Expand Down
10 changes: 9 additions & 1 deletion aiohttp/web.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from . import (hdrs, web_exceptions, web_fileresponse, web_middlewares,
web_protocol, web_request, web_response, web_server,
web_urldispatcher, web_ws)
from .abc import AbstractMatchInfo, AbstractRouter
from .abc import AbstractAccessLogger, AbstractMatchInfo, AbstractRouter
from .frozenlist import FrozenList
from .http import HttpVersion # noqa
from .log import access_logger, web_logger
Expand Down Expand Up @@ -231,6 +231,14 @@ def middlewares(self):
return self._middlewares

def make_handler(self, *, loop=None, **kwargs):
if 'access_log_class' in kwargs:
Copy link
Member

Choose a reason for hiding this comment

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

Just add access_log_class=helpers.AccessLogger into method signature.

access_log_class = kwargs['access_log_class']

assert issubclass(access_log_class, AbstractAccessLogger), (
Copy link
Member

Choose a reason for hiding this comment

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

Should be TypeError, not AssertionError.

'{} must be subclass of '
'aiohttp.abc.AbstractAccessLogger'.format(access_log_class)
)

self._set_loop(loop)
self.freeze()

Expand Down
6 changes: 5 additions & 1 deletion aiohttp/web_protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ class RequestHandler(asyncio.streams.FlowControlMixin, asyncio.Protocol):
:param logger: custom logger object
:type logger: aiohttp.log.server_logger

:param access_log_class: custom class for access_logger
:type access_log_class: aiohttp.abc.AbstractAccessLogger

:param access_log: custom logging object
:type access_log: aiohttp.log.server_logger

Expand All @@ -83,6 +86,7 @@ def __init__(self, manager, *, loop=None,
tcp_keepalive=True,
slow_request_timeout=None,
logger=server_logger,
access_log_class=helpers.AccessLogger,
access_log=access_logger,
access_log_format=helpers.AccessLogger.LOG_FORMAT,
debug=False,
Expand Down Expand Up @@ -138,7 +142,7 @@ def __init__(self, manager, *, loop=None,
self.debug = debug
self.access_log = access_log
if access_log:
self.access_logger = helpers.AccessLogger(
self.access_logger = access_log_class(
access_log, access_log_format)
else:
self.access_logger = None
Expand Down
1 change: 1 addition & 0 deletions changes/2315.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
aiohttp.wep.Application.make_handler support access_log_class
24 changes: 21 additions & 3 deletions docs/abc.rst
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ Not Allowed*. :meth:`AbstractMatchInfo.handler` raises
:attr:`~AbstractMatchInfo.http_exception` on call.


.. class:: AbstractRouter
.. class:: aiohttp.abc.AbstractRouter

Abstract router, :class:`aiohttp.web.Application` accepts it as
*router* parameter and returns as
Expand All @@ -54,7 +54,7 @@ Not Allowed*. :meth:`AbstractMatchInfo.handler` raises
:return: :class:`AbstractMatchInfo` instance.


.. class:: AbstractMatchInfo
.. class:: aiohttp.abc.AbstractMatchInfo

Abstract *match info*, returned by :meth:`AbstractRouter.resolve` call.

Expand Down Expand Up @@ -102,7 +102,7 @@ attribute.
Abstract Cookie Jar
-------------------

.. class:: AbstractCookieJar
.. class:: aiohttp.abc.AbstractCookieJar

The cookie jar instance is available as :attr:`ClientSession.cookie_jar`.

Expand Down Expand Up @@ -146,3 +146,21 @@ Abstract Cookie Jar

:return: :class:`http.cookies.SimpleCookie` with filtered
cookies for given URL.

Abstract Abstract Access Logger
-------------------------------

.. class:: aiohttp.abc.AbstractAccessLogger

An abstract class, base for all :class:`RequestHandler`
``access_logger`` implementations

Method ``_log`` should be overridden.

.. method:: _log(request, response, time)

:param request: :class:`aiohttp.web.Request` object.

:param response: :class:`aiohttp.web.Response` object.

:param float time: Time taken to serve the request.
14 changes: 14 additions & 0 deletions docs/logging.rst
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,20 @@ Default access log format is::

'%a %l %u %t "%r" %s %b "%{Referrer}i" "%{User-Agent}i"'

.. versionadded:: 2.3.0

*access_log_class* introduced.

Example of drop-in replacement for :class:`aiohttp.helpers.AccessLogger`::

from aiohttp.abc import AbstractAccessLogger

class AccessLogger(AbstractAccessLogger):

def _log(self, request, response, time):
self.logger.info(f'{request.remote} '
f'"{request.method} {request.path} '
f'done in {time}s: {response.status}')

.. note::

Expand Down
3 changes: 3 additions & 0 deletions docs/web_reference.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1347,6 +1347,9 @@ duplicated like one using :meth:`Application.copy`.
:data:`aiohttp.log.server_logger`.
:param access_log: Custom logging object. Default:
:data:`aiohttp.log.access_logger`.
:param access_log_class: class for `access_logger`. Default:
:data:`aiohttp.helpers.AccessLogger`.
Must to be a subclass of :class:`aiohttp.abc.AbstractAccessLogger`.
:param str access_log_format: Access log format string. Default:
:attr:`helpers.AccessLogger.LOG_FORMAT`.
:param bool debug: Switches debug mode. Default: ``False``.
Expand Down
25 changes: 25 additions & 0 deletions tests/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from yarl import URL

from aiohttp import helpers
from aiohttp.abc import AbstractAccessLogger


# -------------------- coro guard --------------------------------
Expand Down Expand Up @@ -256,6 +257,30 @@ def test_logger_no_transport():
mock_logger.info.assert_called_with("-", extra={'remote_address': '-'})


def test_logger_abc():
class Logger(AbstractAccessLogger):
def _log(self, request, response, time):
1 / 0

mock_logger = mock.Mock()
access_logger = Logger(mock_logger, None)
access_logger.log(None, None, None)
mock_logger.exception.assert_called_with("Error in logging")

class Logger(AbstractAccessLogger):
def _log(self, request, response, time):
self.logger.info(self.log_format.format(
request=request,
response=response,
time=time
))

mock_logger = mock.Mock()
access_logger = Logger(mock_logger, '{request} {response} {time}')
access_logger.log('request', 'response', 1)
mock_logger.info.assert_called_with('request response 1')


class TestReify:

def test_reify(self):
Expand Down
25 changes: 24 additions & 1 deletion tests/test_web_application.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import pytest

from aiohttp import helpers, log, web
from aiohttp.abc import AbstractRouter
from aiohttp.abc import AbstractAccessLogger, AbstractRouter


def test_app_ctor(loop):
Expand Down Expand Up @@ -79,6 +79,29 @@ def test_app_make_handler_args(loop, mocker):
loop=loop, debug=mock.ANY, test=True)


def test_app_make_handler_access_log_class(loop, mocker):
class Logger:
pass

app = web.Application()

with pytest.raises(AssertionError):
app.make_handler(access_log_class=Logger, loop=loop)

class Logger(AbstractAccessLogger):

def _log(self, request, response, time):
self.logger.info('msg')

srv = mocker.patch('aiohttp.web.Server')

app.make_handler(access_log_class=Logger, loop=loop)
srv.assert_called_with(app._handle,
access_log_class=Logger,
request_factory=app._make_request,
loop=loop, debug=mock.ANY)


@asyncio.coroutine
def test_app_register_on_finish():
app = web.Application()
Expand Down