From feaf0213b926a3f6093cbec0217bb2239b2b0f25 Mon Sep 17 00:00:00 2001 From: Pau Freixes Date: Tue, 5 Dec 2017 20:39:08 +0100 Subject: [PATCH] Client signals get rid of trace_request_ctx as signal kw (#2560) * Client signals get rid of trace_request_ctx as signal kw * Fix typo in documentation * Renamed trace_config_ctx_class to trace_config_ctx_factory --- aiohttp/client.py | 6 ++- aiohttp/tracing.py | 27 ++++-------- docs/client_advanced.rst | 25 +++++++----- docs/client_reference.rst | 6 +-- docs/tracing_reference.rst | 9 ++-- tests/test_client_session.py | 11 ++--- tests/test_connector.py | 79 +++++++++++++----------------------- tests/test_tracing.py | 18 +++++--- 8 files changed, 79 insertions(+), 102 deletions(-) diff --git a/aiohttp/client.py b/aiohttp/client.py index 724941d228d..8dd5fd90e16 100644 --- a/aiohttp/client.py +++ b/aiohttp/client.py @@ -226,9 +226,11 @@ async def _request(self, method, url, *, traces = [ Trace( - trace_config, self, - trace_request_ctx=trace_request_ctx) + trace_config, + trace_config.trace_config_ctx( + trace_request_ctx=trace_request_ctx) + ) for trace_config in self._trace_configs ] diff --git a/aiohttp/tracing.py b/aiohttp/tracing.py index 06eecc2a863..cbfd1597352 100644 --- a/aiohttp/tracing.py +++ b/aiohttp/tracing.py @@ -10,7 +10,7 @@ class TraceConfig: """First-class used to trace requests launched via ClientSession objects.""" - def __init__(self, trace_config_ctx_class=SimpleNamespace): + def __init__(self, trace_config_ctx_factory=SimpleNamespace): self._on_request_start = Signal(self) self._on_request_end = Signal(self) self._on_request_exception = Signal(self) @@ -25,11 +25,12 @@ def __init__(self, trace_config_ctx_class=SimpleNamespace): self._on_dns_cache_hit = Signal(self) self._on_dns_cache_miss = Signal(self) - self._trace_config_ctx_class = trace_config_ctx_class + self._trace_config_ctx_factory = trace_config_ctx_factory - def trace_config_ctx(self): + def trace_config_ctx(self, trace_request_ctx=None): """ Return a new trace_config_ctx instance """ - return self._trace_config_ctx_class() + return self._trace_config_ctx_factory( + trace_request_ctx=trace_request_ctx) def freeze(self): self._on_request_start.freeze() @@ -103,10 +104,9 @@ class Trace: """ Internal class used to keep together the main dependencies used at the moment of send a signal.""" - def __init__(self, trace_config, session, trace_request_ctx=None): + def __init__(self, session, trace_config, trace_config_ctx): self._trace_config = trace_config - self._trace_config_ctx = self._trace_config.trace_config_ctx() - self._trace_request_ctx = trace_request_ctx + self._trace_config_ctx = trace_config_ctx self._session = session async def send_request_start(self, *args, **kwargs): @@ -114,7 +114,6 @@ async def send_request_start(self, *args, **kwargs): self._session, self._trace_config_ctx, *args, - trace_request_ctx=self._trace_request_ctx, **kwargs ) @@ -123,7 +122,6 @@ async def send_request_end(self, *args, **kwargs): self._session, self._trace_config_ctx, *args, - trace_request_ctx=self._trace_request_ctx, **kwargs ) @@ -132,7 +130,6 @@ async def send_request_exception(self, *args, **kwargs): self._session, self._trace_config_ctx, *args, - trace_request_ctx=self._trace_request_ctx, **kwargs ) @@ -141,7 +138,6 @@ async def send_request_redirect(self, *args, **kwargs): self._session, self._trace_config_ctx, *args, - trace_request_ctx=self._trace_request_ctx, **kwargs ) @@ -150,7 +146,6 @@ async def send_connection_queued_start(self, *args, **kwargs): self._session, self._trace_config_ctx, *args, - trace_request_ctx=self._trace_request_ctx, **kwargs ) @@ -159,7 +154,6 @@ async def send_connection_queued_end(self, *args, **kwargs): self._session, self._trace_config_ctx, *args, - trace_request_ctx=self._trace_request_ctx, **kwargs ) @@ -168,7 +162,6 @@ async def send_connection_create_start(self, *args, **kwargs): self._session, self._trace_config_ctx, *args, - trace_request_ctx=self._trace_request_ctx, **kwargs ) @@ -177,7 +170,6 @@ async def send_connection_create_end(self, *args, **kwargs): self._session, self._trace_config_ctx, *args, - trace_request_ctx=self._trace_request_ctx, **kwargs ) @@ -186,7 +178,6 @@ async def send_connection_reuseconn(self, *args, **kwargs): self._session, self._trace_config_ctx, *args, - trace_request_ctx=self._trace_request_ctx, **kwargs ) @@ -195,7 +186,6 @@ async def send_dns_resolvehost_start(self, *args, **kwargs): self._session, self._trace_config_ctx, *args, - trace_request_ctx=self._trace_request_ctx, **kwargs ) @@ -204,7 +194,6 @@ async def send_dns_resolvehost_end(self, *args, **kwargs): self._session, self._trace_config_ctx, *args, - trace_request_ctx=self._trace_request_ctx, **kwargs ) @@ -213,7 +202,6 @@ async def send_dns_cache_hit(self, *args, **kwargs): self._session, self._trace_config_ctx, *args, - trace_request_ctx=self._trace_request_ctx, **kwargs ) @@ -222,6 +210,5 @@ async def send_dns_cache_miss(self, *args, **kwargs): self._session, self._trace_config_ctx, *args, - trace_request_ctx=self._trace_request_ctx, **kwargs ) diff --git a/docs/client_advanced.rst b/docs/client_advanced.rst index de1c77cf7b7..02bed9995b4 100644 --- a/docs/client_advanced.rst +++ b/docs/client_advanced.rst @@ -217,12 +217,10 @@ disabled. The following snippet shows how the start and the end signals of a request flow can be followed:: async def on_request_start( - session, trace_config_ctx, method, - host, port, headers, request_trace_config_ctx=None): + session, trace_config_ctx, method, host, port, headers): print("Starting request") - async def on_request_end(session, trace_config_ctx, resp, - request_trace_config_ctx=None): + async def on_request_end(session, trace_config_ctx, resp): print("Ending request") trace_config = aiohttp.TraceConfig() @@ -253,12 +251,10 @@ share the state through to the different signals that belong to the same request and to the same :class:`TraceConfig` class, perhaps:: async def on_request_start( - session, trace_config_ctx, method, host, port, headers, - trace_request_ctx=None): + session, trace_config_ctx, method, host, port, headers): trace_config_ctx.start = session.loop.time() - async def on_request_end( - session, trace_config_ctx, resp, trace_request_ctx=None): + async def on_request_end(session, trace_config_ctx, resp): elapsed = session.loop.time() - trace_config_ctx.start print("Request took {}".format(elapsed)) @@ -266,12 +262,19 @@ same request and to the same :class:`TraceConfig` class, perhaps:: The ``trace_config_ctx`` param is by default a :class:`SimpleNampespace` that is initialized at the beginning of the request flow. However, the factory used to create this object can be -overwritten using the ``trace_config_ctx_class`` constructor param of +overwritten using the ``trace_config_ctx_factory`` constructor param of the :class:`TraceConfig` class. The ``trace_request_ctx`` param can given at the beginning of the -request execution and will be passed as a keyword argument for all of -the signals, as the following snippet shows:: +request execution, accepted by all of the HTTP verbs, and will be +passed as a keyword argument for the ``trace_config_ctx_factory`` +factory. This param is useful to pass data that is only available at +request time, perhaps:: + + async def on_request_start( + session, trace_config_ctx, method, host, port, headers): + print(trace_config_ctx.trace_request_ctx) + session.get('http://example.com/some/redirect/', trace_request_ctx={'foo': 'bar'}) diff --git a/docs/client_reference.rst b/docs/client_reference.rst index cb4d36660a9..fe8292329fd 100644 --- a/docs/client_reference.rst +++ b/docs/client_reference.rst @@ -294,9 +294,9 @@ The client session supports the context manager protocol for self closing. .. versionadded:: 2.3 - :param trace_request_ctx: Object used to give as a kw param for the all - signals triggered by the ongoing request and for all :class:`TraceConfig` - configured. Default passes a None value. + :param trace_request_ctx: Object used to give as a kw param for each new + :class:`TraceConfig` object instantiated, used to give information to the + tracers that is only available at request time. .. versionadded:: 3.0 diff --git a/docs/tracing_reference.rst b/docs/tracing_reference.rst index 6a90c963a44..71689bde02f 100644 --- a/docs/tracing_reference.rst +++ b/docs/tracing_reference.rst @@ -14,12 +14,15 @@ Trace config is the configuration object used to trace requests launched by a Client session object using different events related to different parts of the request flow. -.. class:: TraceConfig(trace_config_ctx_class=SimpleNamespace) +.. class:: TraceConfig(trace_config_ctx_factory=SimpleNamespace) - :param trace_config_ctx_class: factory used to create trace contexts, + :param trace_config_ctx_factory: factory used to create trace contexts, default class used :class:`SimpleNamespace` - .. method:: trace_config_ctx() + .. method:: trace_config_ctx(trace_request_ctx=None) + + :param trace_request_ctx: Will be used to pass as a kw for the + ``trace_config_ctx_factory``. Return a new trace context. diff --git a/tests/test_client_session.py b/tests/test_client_session.py index a2896681d87..4fa5c29ea89 100644 --- a/tests/test_client_session.py +++ b/tests/test_client_session.py @@ -462,7 +462,7 @@ async def test_request_tracing(loop): on_request_end = mock.Mock(side_effect=asyncio.coroutine(mock.Mock())) trace_config = aiohttp.TraceConfig( - trace_config_ctx_class=mock.Mock(return_value=trace_config_ctx) + trace_config_ctx_factory=mock.Mock(return_value=trace_config_ctx) ) trace_config.on_request_start.append(on_request_start) trace_config.on_request_end.append(on_request_end) @@ -480,8 +480,7 @@ async def test_request_tracing(loop): trace_config_ctx, hdrs.METH_GET, URL("http://example.com"), - CIMultiDict(), - trace_request_ctx=trace_request_ctx + CIMultiDict() ) on_request_end.assert_called_once_with( @@ -490,8 +489,7 @@ async def test_request_tracing(loop): hdrs.METH_GET, URL("http://example.com"), CIMultiDict(), - resp, - trace_request_ctx=trace_request_ctx + resp ) assert not on_request_redirect.called @@ -528,8 +526,7 @@ async def test_request_tracing_exception(loop): hdrs.METH_GET, URL("http://example.com"), CIMultiDict(), - error, - trace_request_ctx=mock.ANY + error ) assert not on_request_end.called diff --git a/tests/test_connector.py b/tests/test_connector.py index f41192f3e78..9c39bd46490 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -617,7 +617,6 @@ async def test_tcp_connector_dns_throttle_requests_cancelled_when_close( async def test_tcp_connector_dns_tracing(loop, dns_response): session = mock.Mock() trace_config_ctx = mock.Mock() - trace_request_ctx = mock.Mock() on_dns_resolvehost_start = mock.Mock( side_effect=asyncio.coroutine(mock.Mock()) ) @@ -632,7 +631,7 @@ async def test_tcp_connector_dns_tracing(loop, dns_response): ) trace_config = aiohttp.TraceConfig( - trace_config_ctx_class=mock.Mock(return_value=trace_config_ctx) + trace_config_ctx_factory=mock.Mock(return_value=trace_config_ctx) ) trace_config.on_dns_resolvehost_start.append(on_dns_resolvehost_start) trace_config.on_dns_resolvehost_end.append(on_dns_resolvehost_end) @@ -641,9 +640,9 @@ async def test_tcp_connector_dns_tracing(loop, dns_response): trace_config.freeze() traces = [ Trace( - trace_config, session, - trace_request_ctx=trace_request_ctx + trace_config, + trace_config.trace_config_ctx() ) ] @@ -664,17 +663,14 @@ async def test_tcp_connector_dns_tracing(loop, dns_response): on_dns_resolvehost_start.assert_called_once_with( session, trace_config_ctx, - trace_request_ctx=trace_request_ctx ) on_dns_resolvehost_start.assert_called_once_with( session, trace_config_ctx, - trace_request_ctx=trace_request_ctx ) on_dns_cache_miss.assert_called_once_with( session, trace_config_ctx, - trace_request_ctx=trace_request_ctx ) assert not on_dns_cache_hit.called @@ -686,14 +682,12 @@ async def test_tcp_connector_dns_tracing(loop, dns_response): on_dns_cache_hit.assert_called_once_with( session, trace_config_ctx, - trace_request_ctx=trace_request_ctx ) async def test_tcp_connector_dns_tracing_cache_disabled(loop, dns_response): session = mock.Mock() trace_config_ctx = mock.Mock() - trace_request_ctx = mock.Mock() on_dns_resolvehost_start = mock.Mock( side_effect=asyncio.coroutine(mock.Mock()) ) @@ -702,16 +696,16 @@ async def test_tcp_connector_dns_tracing_cache_disabled(loop, dns_response): ) trace_config = aiohttp.TraceConfig( - trace_config_ctx_class=mock.Mock(return_value=trace_config_ctx) + trace_config_ctx_factory=mock.Mock(return_value=trace_config_ctx) ) trace_config.on_dns_resolvehost_start.append(on_dns_resolvehost_start) trace_config.on_dns_resolvehost_end.append(on_dns_resolvehost_end) trace_config.freeze() traces = [ Trace( - trace_config, session, - trace_request_ctx=trace_request_ctx + trace_config, + trace_config.trace_config_ctx() ) ] @@ -741,25 +735,21 @@ async def test_tcp_connector_dns_tracing_cache_disabled(loop, dns_response): on_dns_resolvehost_start.assert_has_calls([ mock.call( session, - trace_config_ctx, - trace_request_ctx=trace_request_ctx + trace_config_ctx ), mock.call( session, - trace_config_ctx, - trace_request_ctx=trace_request_ctx + trace_config_ctx ) ]) on_dns_resolvehost_end.assert_has_calls([ mock.call( session, - trace_config_ctx, - trace_request_ctx=trace_request_ctx + trace_config_ctx ), mock.call( session, - trace_config_ctx, - trace_request_ctx=trace_request_ctx + trace_config_ctx ) ]) @@ -767,7 +757,6 @@ async def test_tcp_connector_dns_tracing_cache_disabled(loop, dns_response): async def test_tcp_connector_dns_tracing_throttle_requests(loop, dns_response): session = mock.Mock() trace_config_ctx = mock.Mock() - trace_request_ctx = mock.Mock() on_dns_cache_hit = mock.Mock( side_effect=asyncio.coroutine(mock.Mock()) ) @@ -776,16 +765,16 @@ async def test_tcp_connector_dns_tracing_throttle_requests(loop, dns_response): ) trace_config = aiohttp.TraceConfig( - trace_config_ctx_class=mock.Mock(return_value=trace_config_ctx) + trace_config_ctx_factory=mock.Mock(return_value=trace_config_ctx) ) trace_config.on_dns_cache_hit.append(on_dns_cache_hit) trace_config.on_dns_cache_miss.append(on_dns_cache_miss) trace_config.freeze() traces = [ Trace( - trace_config, session, - trace_request_ctx=trace_request_ctx + trace_config, + trace_config.trace_config_ctx() ) ] @@ -801,13 +790,11 @@ async def test_tcp_connector_dns_tracing_throttle_requests(loop, dns_response): await asyncio.sleep(0, loop=loop) on_dns_cache_hit.assert_called_once_with( session, - trace_config_ctx, - trace_request_ctx=trace_request_ctx + trace_config_ctx ) on_dns_cache_miss.assert_called_once_with( session, - trace_config_ctx, - trace_request_ctx=trace_request_ctx + trace_config_ctx ) @@ -909,7 +896,6 @@ async def test_connect(loop): async def test_connect_tracing(loop): session = mock.Mock() trace_config_ctx = mock.Mock() - trace_request_ctx = mock.Mock() on_connection_create_start = mock.Mock( side_effect=asyncio.coroutine(mock.Mock()) ) @@ -918,16 +904,16 @@ async def test_connect_tracing(loop): ) trace_config = aiohttp.TraceConfig( - trace_config_ctx_class=mock.Mock(return_value=trace_config_ctx) + trace_config_ctx_factory=mock.Mock(return_value=trace_config_ctx) ) trace_config.on_connection_create_start.append(on_connection_create_start) trace_config.on_connection_create_end.append(on_connection_create_end) trace_config.freeze() traces = [ Trace( - trace_config, session, - trace_request_ctx=trace_request_ctx + trace_config, + trace_config.trace_config_ctx() ) ] @@ -944,13 +930,11 @@ async def test_connect_tracing(loop): await conn.connect(req, traces=traces) on_connection_create_start.assert_called_with( session, - trace_config_ctx, - trace_request_ctx=trace_request_ctx + trace_config_ctx ) on_connection_create_end.assert_called_with( session, - trace_config_ctx, - trace_request_ctx=trace_request_ctx + trace_config_ctx ) @@ -1258,7 +1242,6 @@ async def f(): async def test_connect_queued_operation_tracing(loop, key): session = mock.Mock() trace_config_ctx = mock.Mock() - trace_request_ctx = mock.Mock() on_connection_queued_start = mock.Mock( side_effect=asyncio.coroutine(mock.Mock()) ) @@ -1267,16 +1250,16 @@ async def test_connect_queued_operation_tracing(loop, key): ) trace_config = aiohttp.TraceConfig( - trace_config_ctx_class=mock.Mock(return_value=trace_config_ctx) + trace_config_ctx_factory=mock.Mock(return_value=trace_config_ctx) ) trace_config.on_connection_queued_start.append(on_connection_queued_start) trace_config.on_connection_queued_end.append(on_connection_queued_end) trace_config.freeze() traces = [ Trace( - trace_config, session, - trace_request_ctx=trace_request_ctx + trace_config, + trace_config.trace_config_ctx() ) ] @@ -1302,13 +1285,11 @@ async def f(): ) on_connection_queued_start.assert_called_with( session, - trace_config_ctx, - trace_request_ctx=trace_request_ctx + trace_config_ctx ) on_connection_queued_end.assert_called_with( session, - trace_config_ctx, - trace_request_ctx=trace_request_ctx + trace_config_ctx ) connection2.release() @@ -1322,21 +1303,20 @@ async def f(): async def test_connect_reuseconn_tracing(loop, key): session = mock.Mock() trace_config_ctx = mock.Mock() - trace_request_ctx = mock.Mock() on_connection_reuseconn = mock.Mock( side_effect=asyncio.coroutine(mock.Mock()) ) trace_config = aiohttp.TraceConfig( - trace_config_ctx_class=mock.Mock(return_value=trace_config_ctx) + trace_config_ctx_factory=mock.Mock(return_value=trace_config_ctx) ) trace_config.on_connection_reuseconn.append(on_connection_reuseconn) trace_config.freeze() traces = [ Trace( - trace_config, session, - trace_request_ctx=trace_request_ctx + trace_config, + trace_config.trace_config_ctx() ) ] @@ -1353,8 +1333,7 @@ async def test_connect_reuseconn_tracing(loop, key): on_connection_reuseconn.assert_called_with( session, - trace_config_ctx, - trace_request_ctx=trace_request_ctx + trace_config_ctx ) conn.close() diff --git a/tests/test_tracing.py b/tests/test_tracing.py index 6235bd59bef..10ea6bbcf11 100644 --- a/tests/test_tracing.py +++ b/tests/test_tracing.py @@ -13,10 +13,17 @@ def test_trace_config_ctx_default(self): trace_config = TraceConfig() assert isinstance(trace_config.trace_config_ctx(), SimpleNamespace) - def test_trace_config_ctx_class(self): - trace_config = TraceConfig(trace_config_ctx_class=dict) + def test_trace_config_ctx_factory(self): + trace_config = TraceConfig(trace_config_ctx_factory=dict) assert isinstance(trace_config.trace_config_ctx(), dict) + def test_trace_config_ctx_request_ctx(self): + trace_request_ctx = Mock() + trace_config = TraceConfig() + trace_config_ctx = trace_config.trace_config_ctx( + trace_request_ctx=trace_request_ctx) + assert trace_config_ctx.trace_request_ctx is trace_request_ctx + def test_freeze(self): trace_config = TraceConfig() trace_config.freeze() @@ -63,15 +70,14 @@ async def test_send(self, loop, signal): getattr(trace_config, "on_%s" % signal).append(callback) trace_config.freeze() trace = Trace( - trace_config, session, - trace_request_ctx=trace_request_ctx + trace_config, + trace_config.trace_config_ctx(trace_request_ctx=trace_request_ctx) ) await getattr(trace, "send_%s" % signal)(param) callback.assert_called_once_with( session, - SimpleNamespace(), + SimpleNamespace(trace_request_ctx=trace_request_ctx), param, - trace_request_ctx=trace_request_ctx )