From 9febb7e7e840f25309afefd5822ebb58a4398644 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Sat, 6 Mar 2021 09:50:29 -0500 Subject: [PATCH 01/31] statsd.Processor that reconnects. --- sprockets_statsd/statsd.py | 71 +++++++++++++++++++++++ tests/helpers.py | 66 ++++++++++++++++++++++ tests/test_processor.py | 112 +++++++++++++++++++++++++++++++++++++ 3 files changed, 249 insertions(+) create mode 100644 sprockets_statsd/statsd.py create mode 100644 tests/helpers.py create mode 100644 tests/test_processor.py diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py new file mode 100644 index 0000000..2f450ca --- /dev/null +++ b/sprockets_statsd/statsd.py @@ -0,0 +1,71 @@ +import asyncio +import logging +import typing + + +class Processor(asyncio.Protocol): + def __init__(self, *, host, port: int = 8125, **kwargs): + super().__init__(**kwargs) + self.host = host + self.port = port + + self.closed = asyncio.Event() + self.connected = asyncio.Event() + self.logger = logging.getLogger(__package__).getChild('Processor') + self.running = False + self.transport = None + + async def run(self): + self.running = True + while self.running: + try: + await self._connect_if_necessary() + await asyncio.sleep(0.1) + except asyncio.CancelledError: + self.logger.info('task cancelled, exiting') + break + + self.running = False + if self.connected.is_set(): + self.logger.debug('closing transport') + self.transport.close() + + while self.connected.is_set(): + self.logger.debug('waiting on transport to close') + await asyncio.sleep(0.1) + + self.logger.info('processing is exiting') + self.closed.set() + + async def stop(self): + self.running = False + await self.closed.wait() + + def eof_received(self): + self.logger.warning('received EOF from statsd server') + self.connected.clear() + + def connection_made(self, transport: asyncio.Transport): + server, port = transport.get_extra_info('peername') + self.logger.info('connected to statsd %s:%s', server, port) + self.transport = transport + self.connected.set() + + def connection_lost(self, exc: typing.Optional[Exception]): + self.logger.warning('statsd server connection lost') + self.connected.clear() + + async def _connect_if_necessary(self, wait_time: float = 0.1): + try: + await asyncio.wait_for(self.connected.wait(), wait_time) + except asyncio.TimeoutError: + try: + self.logger.debug('starting connection to %s:%s', self.host, + self.port) + await asyncio.get_running_loop().create_connection( + protocol_factory=lambda: self, + host=self.host, + port=self.port) + except IOError as error: + self.logger.warning('connection to %s:%s failed: %s', + self.host, self.port, error) diff --git a/tests/helpers.py b/tests/helpers.py new file mode 100644 index 0000000..fad9a39 --- /dev/null +++ b/tests/helpers.py @@ -0,0 +1,66 @@ +import asyncio +import io + + +class StatsdServer(asyncio.Protocol): + def __init__(self): + self.service = None + self.host = '127.0.0.1' + self.port = 0 + self.connections_made = 0 + self.connections_lost = 0 + self.message_counter = 0 + + self.buffer = io.BytesIO() + self.running = asyncio.Event() + self.client_connected = asyncio.Semaphore(value=0) + self.message_received = asyncio.Semaphore(value=0) + self.transports: list[asyncio.Transport] = [] + + async def run(self): + loop = asyncio.get_running_loop() + self.service = await loop.create_server(lambda: self, + self.host, + self.port, + reuse_port=True) + listening_sock = self.service.sockets[0] + self.host, self.port = listening_sock.getsockname() + self.running.set() + try: + await self.service.serve_forever() + self.running.clear() + except asyncio.CancelledError: + self.close() + await self.service.wait_closed() + except Exception as error: + raise error + + def close(self): + self.running.clear() + self.service.close() + for connected_client in self.transports: + connected_client.close() + self.transports.clear() + + async def wait_running(self): + await self.running.wait() + + async def wait_closed(self): + if self.service.is_serving(): + self.close() + await self.service.wait_closed() + while self.running.is_set(): + await asyncio.sleep(0.1) + + def connection_made(self, transport: asyncio.Transport): + self.client_connected.release() + self.connections_made += 1 + self.transports.append(transport) + + def connection_lost(self, exc) -> None: + self.connections_lost += 1 + + def data_received(self, data: bytes): + self.buffer.write(data) + self.message_received.release() + self.message_counter += 1 diff --git a/tests/test_processor.py b/tests/test_processor.py new file mode 100644 index 0000000..e8e73d9 --- /dev/null +++ b/tests/test_processor.py @@ -0,0 +1,112 @@ +import asyncio +import time +import unittest + +from sprockets_statsd import statsd + +from tests import helpers + + +class ProcessorTests(unittest.IsolatedAsyncioTestCase): + def setUp(self): + super().setUp() + self.test_timeout = 5.0 + + async def asyncSetUp(self): + await super().asyncSetUp() + self.statsd_server = helpers.StatsdServer() + self.statsd_task = asyncio.create_task(self.statsd_server.run()) + await self.statsd_server.wait_running() + + async def asyncTearDown(self): + self.statsd_task.cancel() + await self.statsd_server.wait_closed() + await super().asyncTearDown() + + async def wait_for(self, fut): + try: + await asyncio.wait_for(fut, timeout=self.test_timeout) + except asyncio.TimeoutError: + self.fail('future took too long to resolve') + + async def test_that_processor_connects_and_disconnects(self): + processor = statsd.Processor(host=self.statsd_server.host, + port=self.statsd_server.port) + asyncio.create_task(processor.run()) + await self.wait_for(self.statsd_server.client_connected.acquire()) + await self.wait_for(processor.stop()) + + self.assertEqual(1, self.statsd_server.connections_made) + self.assertEqual(1, self.statsd_server.connections_lost) + + async def test_that_processor_reconnects(self): + processor = statsd.Processor(host=self.statsd_server.host, + port=self.statsd_server.port) + asyncio.create_task(processor.run()) + await self.wait_for(self.statsd_server.client_connected.acquire()) + + # Now that the server is running and the client has connected, + # cancel the server and let it die off. + self.statsd_server.close() + await self.statsd_server.wait_closed() + until = time.time() + self.test_timeout + while processor.connected.is_set(): + await asyncio.sleep(0.1) + if time.time() >= until: + self.fail('processor never disconnected') + + # Start the server on the same port and let the client reconnect. + self.statsd_task = asyncio.create_task(self.statsd_server.run()) + await self.wait_for(self.statsd_server.client_connected.acquire()) + self.assertTrue(processor.connected.is_set()) + + await self.wait_for(processor.stop()) + + async def test_that_processor_can_be_cancelled(self): + processor = statsd.Processor(host=self.statsd_server.host, + port=self.statsd_server.port) + task = asyncio.create_task(processor.run()) + await self.wait_for(self.statsd_server.client_connected.acquire()) + + task.cancel() + await self.wait_for(processor.closed.wait()) + + async def test_shutdown_when_disconnected(self): + processor = statsd.Processor(host=self.statsd_server.host, + port=self.statsd_server.port) + asyncio.create_task(processor.run()) + await self.wait_for(self.statsd_server.client_connected.acquire()) + + self.statsd_server.close() + await self.statsd_server.wait_closed() + + await self.wait_for(processor.stop()) + + async def test_socket_resets(self): + processor = statsd.Processor(host=self.statsd_server.host, + port=self.statsd_server.port) + asyncio.create_task(processor.run()) + await self.wait_for(self.statsd_server.client_connected.acquire()) + + self.statsd_server.transports[0].close() + await self.wait_for(self.statsd_server.client_connected.acquire()) + + async def test_connection_failures(self): + processor = statsd.Processor(host=self.statsd_server.host, + port=self.statsd_server.port) + asyncio.create_task(processor.run()) + await self.wait_for(self.statsd_server.client_connected.acquire()) + + # Change the port and close the transport, this will cause the + # processor to reconnect to the new port and fail. + processor.port = 1 + processor.transport.close() + + # Wait for the processor to be disconnected, then change the + # port back and let the processor reconnect. + while processor.connected.is_set(): + await asyncio.sleep(0.1) + await asyncio.sleep(0.2) + processor.port = self.statsd_server.port + + await self.wait_for(self.statsd_server.client_connected.acquire()) From ff6f13591c394b7082108b16bb080cc343d95734 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Sun, 7 Mar 2021 14:35:42 -0500 Subject: [PATCH 02/31] Send metrics asynchronously. --- sprockets_statsd/statsd.py | 26 +++++++++++- tests/helpers.py | 47 +++++++++++++++++++-- tests/test_processor.py | 83 +++++++++++++++++++++++++++++++++++--- 3 files changed, 144 insertions(+), 12 deletions(-) diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py index 2f450ca..ef821f1 100644 --- a/sprockets_statsd/statsd.py +++ b/sprockets_statsd/statsd.py @@ -15,18 +15,26 @@ class Processor(asyncio.Protocol): self.running = False self.transport = None + self._queue = asyncio.Queue() + async def run(self): self.running = True while self.running: try: await self._connect_if_necessary() - await asyncio.sleep(0.1) + await self._process_metric() except asyncio.CancelledError: self.logger.info('task cancelled, exiting') break self.running = False + self.logger.info('loop finished with %d metrics in the queue', + self._queue.qsize()) if self.connected.is_set(): + num_ready = self._queue.qsize() + self.logger.info('draining %d metrics', num_ready) + for _ in range(num_ready): + await self._process_metric() self.logger.debug('closing transport') self.transport.close() @@ -34,13 +42,18 @@ class Processor(asyncio.Protocol): self.logger.debug('waiting on transport to close') await asyncio.sleep(0.1) - self.logger.info('processing is exiting') + self.logger.info('processor is exiting') self.closed.set() async def stop(self): self.running = False await self.closed.wait() + def inject_metric(self, path: str, value: typing.Union[float, int, str], + type_code: str): + payload = f'{path}:{value}|{type_code}\n' + self._queue.put_nowait(payload.encode('utf-8')) + def eof_received(self): self.logger.warning('received EOF from statsd server') self.connected.clear() @@ -69,3 +82,12 @@ class Processor(asyncio.Protocol): except IOError as error: self.logger.warning('connection to %s:%s failed: %s', self.host, self.port, error) + + async def _process_metric(self): + try: + metric = await asyncio.wait_for(self._queue.get(), 0.1) + except asyncio.TimeoutError: + return # nothing to do + else: + self.transport.write(metric) + self._queue.task_done() diff --git a/tests/helpers.py b/tests/helpers.py index fad9a39..5e71207 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -11,13 +11,17 @@ class StatsdServer(asyncio.Protocol): self.connections_lost = 0 self.message_counter = 0 - self.buffer = io.BytesIO() + self.metrics = [] self.running = asyncio.Event() self.client_connected = asyncio.Semaphore(value=0) self.message_received = asyncio.Semaphore(value=0) self.transports: list[asyncio.Transport] = [] + self._buffer = io.BytesIO() + async def run(self): + await self._reset() + loop = asyncio.get_running_loop() self.service = await loop.create_server(lambda: self, self.host, @@ -61,6 +65,41 @@ class StatsdServer(asyncio.Protocol): self.connections_lost += 1 def data_received(self, data: bytes): - self.buffer.write(data) - self.message_received.release() - self.message_counter += 1 + self._buffer.write(data) + buf = self._buffer.getvalue() + if b'\n' in buf: + buf_complete = buf[-1] == ord('\n') + if not buf_complete: + offset = buf.rfind(b'\n') + self._buffer = io.BytesIO(buf[offset:]) + buf = buf[:offset] + else: + self._buffer = io.BytesIO() + buf = buf[:-1] + + for metric in buf.split(b'\n'): + self.metrics.append(metric) + self.message_received.release() + self.message_counter += 1 + + async def _reset(self): + self._buffer = io.BytesIO() + self.connections_made = 0 + self.connections_lost = 0 + self.message_counter = 0 + self.metrics.clear() + for transport in self.transports: + transport.close() + self.transports.clear() + + self.running.clear() + await self._drain_semaphore(self.client_connected) + await self._drain_semaphore(self.message_received) + + @staticmethod + async def _drain_semaphore(semaphore: asyncio.Semaphore): + while not semaphore.locked(): + try: + await asyncio.wait_for(semaphore.acquire(), 0.1) + except asyncio.TimeoutError: + break diff --git a/tests/test_processor.py b/tests/test_processor.py index e8e73d9..1e07ac2 100644 --- a/tests/test_processor.py +++ b/tests/test_processor.py @@ -7,11 +7,17 @@ from sprockets_statsd import statsd from tests import helpers -class ProcessorTests(unittest.IsolatedAsyncioTestCase): +class ProcessorTestCase(unittest.IsolatedAsyncioTestCase): def setUp(self): super().setUp() self.test_timeout = 5.0 + async def wait_for(self, fut): + try: + await asyncio.wait_for(fut, timeout=self.test_timeout) + except asyncio.TimeoutError: + self.fail('future took too long to resolve') + async def asyncSetUp(self): await super().asyncSetUp() self.statsd_server = helpers.StatsdServer() @@ -23,12 +29,8 @@ class ProcessorTests(unittest.IsolatedAsyncioTestCase): await self.statsd_server.wait_closed() await super().asyncTearDown() - async def wait_for(self, fut): - try: - await asyncio.wait_for(fut, timeout=self.test_timeout) - except asyncio.TimeoutError: - self.fail('future took too long to resolve') +class ProcessorTests(ProcessorTestCase): async def test_that_processor_connects_and_disconnects(self): processor = statsd.Processor(host=self.statsd_server.host, port=self.statsd_server.port) @@ -110,3 +112,72 @@ class ProcessorTests(unittest.IsolatedAsyncioTestCase): processor.port = self.statsd_server.port await self.wait_for(self.statsd_server.client_connected.acquire()) + + +class MetricProcessingTests(ProcessorTestCase): + async def asyncSetUp(self): + await super().asyncSetUp() + self.processor = statsd.Processor(host=self.statsd_server.host, + port=self.statsd_server.port) + asyncio.create_task(self.processor.run()) + await self.wait_for(self.statsd_server.client_connected.acquire()) + + async def asyncTearDown(self): + await self.wait_for(self.processor.stop()) + await super().asyncTearDown() + + def assert_metrics_equal(self, recvd: bytes, path, value, type_code): + recvd = recvd.decode('utf-8') + recvd_path, _, rest = recvd.partition(':') + recvd_value, _, recvd_code = rest.partition('|') + self.assertEqual(path, recvd_path, 'metric path mismatch') + self.assertEqual(recvd_value, str(value), 'metric value mismatch') + self.assertEqual(recvd_code, type_code, 'metric type mismatch') + + async def test_sending_simple_counter(self): + self.processor.inject_metric('simple.counter', 1000, 'c') + await self.wait_for(self.statsd_server.message_received.acquire()) + self.assert_metrics_equal(self.statsd_server.metrics[0], + 'simple.counter', 1000, 'c') + + async def test_adjusting_gauge(self): + self.processor.inject_metric('simple.gauge', 100, 'g') + self.processor.inject_metric('simple.gauge', -10, 'g') + self.processor.inject_metric('simple.gauge', '+10', 'g') + for _ in range(3): + await self.wait_for(self.statsd_server.message_received.acquire()) + + self.assert_metrics_equal(self.statsd_server.metrics[0], + 'simple.gauge', '100', 'g') + self.assert_metrics_equal(self.statsd_server.metrics[1], + 'simple.gauge', '-10', 'g') + self.assert_metrics_equal(self.statsd_server.metrics[2], + 'simple.gauge', '+10', 'g') + + async def test_sending_timer(self): + secs = 12.34 + self.processor.inject_metric('simple.timer', secs * 1000.0, 'ms') + await self.wait_for(self.statsd_server.message_received.acquire()) + self.assert_metrics_equal(self.statsd_server.metrics[0], + 'simple.timer', 12340.0, 'ms') + + async def test_that_queued_metrics_are_drained(self): + # The easiest way to test that the internal metrics queue + # is drained when the processor is stopped is to monkey + # patch the "process metric" method to enqueue a few + # metrics and then set running to false. It will exit + # the run loop and drain the queue. + real_process_metric = self.processor._process_metric + + async def fake_process_metric(): + if self.processor.running: + self.processor.inject_metric('counter', 1, 'c') + self.processor.inject_metric('counter', 2, 'c') + self.processor.inject_metric('counter', 3, 'c') + self.processor.running = False + return await real_process_metric() + + self.processor._process_metric = fake_process_metric + await self.wait_for(self.statsd_server.message_received.acquire()) + await self.wait_for(self.statsd_server.message_received.acquire()) + await self.wait_for(self.statsd_server.message_received.acquire()) From ed6e479e2a6fb5d0a1262b14e85df90e8293e673 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Sun, 7 Mar 2021 14:37:24 -0500 Subject: [PATCH 03/31] Make statsd metric sending resilient. --- sprockets_statsd/statsd.py | 38 ++++++++++++++++++++++++++++++++------ tests/test_processor.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py index ef821f1..5b62b1a 100644 --- a/sprockets_statsd/statsd.py +++ b/sprockets_statsd/statsd.py @@ -16,6 +16,7 @@ class Processor(asyncio.Protocol): self.transport = None self._queue = asyncio.Queue() + self._failed_sends = [] async def run(self): self.running = True @@ -84,10 +85,35 @@ class Processor(asyncio.Protocol): self.host, self.port, error) async def _process_metric(self): - try: - metric = await asyncio.wait_for(self._queue.get(), 0.1) - except asyncio.TimeoutError: - return # nothing to do + processing_failed_send = False + if self._failed_sends: + self.logger.debug('using previous send attempt') + metric = self._failed_sends[0] + processing_failed_send = True else: - self.transport.write(metric) - self._queue.task_done() + try: + metric = await asyncio.wait_for(self._queue.get(), 0.1) + self.logger.debug('received %r from queue', metric) + except asyncio.TimeoutError: + return + else: + # Since we `await`d the state of the transport may have + # changed. Sending on the closed transport won't return + # an error since the send is async. We can catch the + # problem here though. + if self.transport.is_closing(): + self.logger.debug('preventing send on closed transport') + self._failed_sends.append(metric) + return + + self.transport.write(metric) + if self.transport.is_closing(): + # Writing to a transport does not raise exceptions, it + # will close the transport if a low-level error occurs. + self.logger.debug('transport closed by writing') + else: + self.logger.debug('sent %r to statsd', metric) + if processing_failed_send: + self._failed_sends.pop(0) + else: + self._queue.task_done() diff --git a/tests/test_processor.py b/tests/test_processor.py index 1e07ac2..e094c5f 100644 --- a/tests/test_processor.py +++ b/tests/test_processor.py @@ -181,3 +181,32 @@ class MetricProcessingTests(ProcessorTestCase): await self.wait_for(self.statsd_server.message_received.acquire()) await self.wait_for(self.statsd_server.message_received.acquire()) await self.wait_for(self.statsd_server.message_received.acquire()) + + async def test_metrics_sent_while_disconnected_are_queued(self): + self.statsd_server.close() + await self.statsd_server.wait_closed() + + for value in range(50): + self.processor.inject_metric('counter', value, 'c') + + asyncio.create_task(self.statsd_server.run()) + await self.wait_for(self.statsd_server.client_connected.acquire()) + for value in range(50): + await self.wait_for(self.statsd_server.message_received.acquire()) + self.assertEqual(f'counter:{value}|c'.encode(), + self.statsd_server.metrics.pop(0)) + + async def test_socket_closure_while_processing_failed_event(self): + state = {'first_time': True} + real_process_metric = self.processor._process_metric + + async def fake_process_metric(): + if state['first_time']: + self.processor._failed_sends.append(b'counter:1|c\n') + self.processor.transport.close() + state['first_time'] = False + return await real_process_metric() + + self.processor._process_metric = fake_process_metric + + await self.wait_for(self.statsd_server.message_received.acquire()) From 0d5b212efc4d90571af29b5652891581a850ba93 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Mon, 8 Mar 2021 07:27:40 -0500 Subject: [PATCH 04/31] Add docs, rename a few things. --- docs/index.rst | 13 ++++ sprockets_statsd/statsd.py | 152 ++++++++++++++++++++++++++++++++----- tests/test_processor.py | 53 +++++++------ 3 files changed, 175 insertions(+), 43 deletions(-) diff --git a/docs/index.rst b/docs/index.rst index 95ec2fb..ccfec4b 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -4,6 +4,19 @@ sprockets-statsd .. include:: ../README.rst +Reference +========= + +Connector +--------- +.. autoclass:: sprockets_statsd.statsd.Connector + :members: + +Processor internals +------------------- +.. autoclass:: sprockets_statsd.statsd.Processor + :members: + Release history =============== diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py index 5b62b1a..92994cd 100644 --- a/sprockets_statsd/statsd.py +++ b/sprockets_statsd/statsd.py @@ -3,24 +3,136 @@ import logging import typing +class Connector: + """Sends metrics to a statsd server. + + :param host: statsd server to send metrics to + :param port: TCP port that the server is listening on + + This class maintains a TCP connection to a statsd server and + sends metric lines to it asynchronously. You must call the + :meth:`start` method when your application is starting. It + creates a :class:`~asyncio.Task` that manages the connection + to the statsd server. You must also call :meth:`.stop` before + terminating to ensure that all metrics are flushed to the + statsd server. + + When the connector is *should_terminate*, metric payloads are sent by + calling the :meth:`.inject_metric` method. The payloads are + stored in an internal queue that is consumed whenever the + connection to the server is active. + + .. attribute:: processor + :type: Processor + + The statsd processor that maintains the connection and + sends the metric payloads. + + """ + def __init__(self, host: str, port: int = 8125): + self.processor = Processor(host=host, port=port) + self._processor_task = None + + async def start(self): + """Start the processor in the background.""" + self._processor_task = asyncio.create_task(self.processor.run()) + + async def stop(self): + """Stop the background processor. + + Items that are currently in the queue will be flushed to + the statsd server if possible. This is a *blocking* method + and does not return until the background processor has + stopped. + + """ + await self.processor.stop() + + def inject_metric(self, path: str, value, type_code: str): + """Send a metric to the statsd server. + + :param path: formatted metric name + :param value: metric value as a number or a string. The + string form is required for relative gauges. + :param type_code: type of the metric to send + + This method formats the payload and inserts it on the + internal queue for future processing. + + """ + payload = f'{path}:{value}|{type_code}\n' + self.processor.queue.put_nowait(payload.encode('utf-8')) + + class Processor(asyncio.Protocol): - def __init__(self, *, host, port: int = 8125, **kwargs): - super().__init__(**kwargs) + """Maintains the statsd connection and sends metric payloads. + + :param host: statsd server to send metrics to + :param port: TCP port that the server is listening on + + This class implements :class:`~asyncio.Protocol` for the statsd + TCP connection. The :meth:`.run` method is run as a background + :class:`~asyncio.Task` that consumes payloads from an internal + queue, connects to the TCP server as required, and sends the + already formatted payloads. + + .. attribute:: host + :type: str + + IP address or DNS name for the statsd server to send metrics to + + .. attribute:: port + :type: int + + TCP port number that the statsd server is listening on + + .. attribute:: should_terminate + :type: bool + + Flag that controls whether the background task is active or + not. This flag is set to :data:`False` when the task is started. + Setting it to :data:`True` will cause the task to shutdown in + an orderly fashion. + + .. attribute:: queue + :type: asyncio.Queue + + Formatted metric payloads to send to the statsd server. Enqueue + payloads to send them to the server. + + .. attribute:: connected + :type: asyncio.Event + + Is the TCP connection currently connected? + + .. attribute:: stopped + :type: asyncio.Event + + Is the background task currently stopped? This is the event that + :meth:`.run` sets when it exits and that :meth:`.stop` blocks on + until the task stops. + + """ + def __init__(self, *, host, port: int = 8125): + super().__init__() self.host = host self.port = port - self.closed = asyncio.Event() + self.stopped = asyncio.Event() + self.stopped.set() self.connected = asyncio.Event() self.logger = logging.getLogger(__package__).getChild('Processor') - self.running = False + self.should_terminate = False self.transport = None + self.queue = asyncio.Queue() - self._queue = asyncio.Queue() self._failed_sends = [] async def run(self): - self.running = True - while self.running: + """Maintains the connection and processes metric payloads.""" + self.stopped.clear() + self.should_terminate = False + while not self.should_terminate: try: await self._connect_if_necessary() await self._process_metric() @@ -28,11 +140,11 @@ class Processor(asyncio.Protocol): self.logger.info('task cancelled, exiting') break - self.running = False + self.should_terminate = True self.logger.info('loop finished with %d metrics in the queue', - self._queue.qsize()) + self.queue.qsize()) if self.connected.is_set(): - num_ready = self._queue.qsize() + num_ready = self.queue.qsize() self.logger.info('draining %d metrics', num_ready) for _ in range(num_ready): await self._process_metric() @@ -44,16 +156,18 @@ class Processor(asyncio.Protocol): await asyncio.sleep(0.1) self.logger.info('processor is exiting') - self.closed.set() + self.stopped.set() async def stop(self): - self.running = False - await self.closed.wait() + """Stop the processor. - def inject_metric(self, path: str, value: typing.Union[float, int, str], - type_code: str): - payload = f'{path}:{value}|{type_code}\n' - self._queue.put_nowait(payload.encode('utf-8')) + This is an asynchronous but blocking method. It does not + return until enqueued metrics are flushed and the processor + connection is closed. + + """ + self.should_terminate = True + await self.stopped.wait() def eof_received(self): self.logger.warning('received EOF from statsd server') @@ -92,7 +206,7 @@ class Processor(asyncio.Protocol): processing_failed_send = True else: try: - metric = await asyncio.wait_for(self._queue.get(), 0.1) + metric = await asyncio.wait_for(self.queue.get(), 0.1) self.logger.debug('received %r from queue', metric) except asyncio.TimeoutError: return @@ -116,4 +230,4 @@ class Processor(asyncio.Protocol): if processing_failed_send: self._failed_sends.pop(0) else: - self._queue.task_done() + self.queue.task_done() diff --git a/tests/test_processor.py b/tests/test_processor.py index e094c5f..a882558 100644 --- a/tests/test_processor.py +++ b/tests/test_processor.py @@ -71,7 +71,7 @@ class ProcessorTests(ProcessorTestCase): await self.wait_for(self.statsd_server.client_connected.acquire()) task.cancel() - await self.wait_for(processor.closed.wait()) + await self.wait_for(processor.stopped.wait()) async def test_shutdown_when_disconnected(self): processor = statsd.Processor(host=self.statsd_server.host, @@ -113,17 +113,22 @@ class ProcessorTests(ProcessorTestCase): await self.wait_for(self.statsd_server.client_connected.acquire()) + async def test_that_stopping_when_not_running_is_safe(self): + processor = statsd.Processor(host=self.statsd_server.host, + port=self.statsd_server.port) + await self.wait_for(processor.stop()) -class MetricProcessingTests(ProcessorTestCase): + +class ConnectorTests(ProcessorTestCase): async def asyncSetUp(self): await super().asyncSetUp() - self.processor = statsd.Processor(host=self.statsd_server.host, - port=self.statsd_server.port) - asyncio.create_task(self.processor.run()) + self.connector = statsd.Connector(self.statsd_server.host, + self.statsd_server.port) + await self.connector.start() await self.wait_for(self.statsd_server.client_connected.acquire()) async def asyncTearDown(self): - await self.wait_for(self.processor.stop()) + await self.wait_for(self.connector.stop()) await super().asyncTearDown() def assert_metrics_equal(self, recvd: bytes, path, value, type_code): @@ -135,15 +140,15 @@ class MetricProcessingTests(ProcessorTestCase): self.assertEqual(recvd_code, type_code, 'metric type mismatch') async def test_sending_simple_counter(self): - self.processor.inject_metric('simple.counter', 1000, 'c') + self.connector.inject_metric('simple.counter', 1000, 'c') await self.wait_for(self.statsd_server.message_received.acquire()) self.assert_metrics_equal(self.statsd_server.metrics[0], 'simple.counter', 1000, 'c') async def test_adjusting_gauge(self): - self.processor.inject_metric('simple.gauge', 100, 'g') - self.processor.inject_metric('simple.gauge', -10, 'g') - self.processor.inject_metric('simple.gauge', '+10', 'g') + self.connector.inject_metric('simple.gauge', 100, 'g') + self.connector.inject_metric('simple.gauge', -10, 'g') + self.connector.inject_metric('simple.gauge', '+10', 'g') for _ in range(3): await self.wait_for(self.statsd_server.message_received.acquire()) @@ -156,7 +161,7 @@ class MetricProcessingTests(ProcessorTestCase): async def test_sending_timer(self): secs = 12.34 - self.processor.inject_metric('simple.timer', secs * 1000.0, 'ms') + self.connector.inject_metric('simple.timer', secs * 1000.0, 'ms') await self.wait_for(self.statsd_server.message_received.acquire()) self.assert_metrics_equal(self.statsd_server.metrics[0], 'simple.timer', 12340.0, 'ms') @@ -165,19 +170,19 @@ class MetricProcessingTests(ProcessorTestCase): # The easiest way to test that the internal metrics queue # is drained when the processor is stopped is to monkey # patch the "process metric" method to enqueue a few - # metrics and then set running to false. It will exit + # metrics and then terminate the processor. It will exit # the run loop and drain the queue. - real_process_metric = self.processor._process_metric + real_process_metric = self.connector.processor._process_metric async def fake_process_metric(): - if self.processor.running: - self.processor.inject_metric('counter', 1, 'c') - self.processor.inject_metric('counter', 2, 'c') - self.processor.inject_metric('counter', 3, 'c') - self.processor.running = False + if not self.connector.processor.should_terminate: + self.connector.inject_metric('counter', 1, 'c') + self.connector.inject_metric('counter', 2, 'c') + self.connector.inject_metric('counter', 3, 'c') + self.connector.processor.should_terminate = True return await real_process_metric() - self.processor._process_metric = fake_process_metric + self.connector.processor._process_metric = fake_process_metric await self.wait_for(self.statsd_server.message_received.acquire()) await self.wait_for(self.statsd_server.message_received.acquire()) await self.wait_for(self.statsd_server.message_received.acquire()) @@ -187,7 +192,7 @@ class MetricProcessingTests(ProcessorTestCase): await self.statsd_server.wait_closed() for value in range(50): - self.processor.inject_metric('counter', value, 'c') + self.connector.inject_metric('counter', value, 'c') asyncio.create_task(self.statsd_server.run()) await self.wait_for(self.statsd_server.client_connected.acquire()) @@ -198,15 +203,15 @@ class MetricProcessingTests(ProcessorTestCase): async def test_socket_closure_while_processing_failed_event(self): state = {'first_time': True} - real_process_metric = self.processor._process_metric + real_process_metric = self.connector.processor._process_metric async def fake_process_metric(): if state['first_time']: - self.processor._failed_sends.append(b'counter:1|c\n') - self.processor.transport.close() + self.connector.processor._failed_sends.append(b'counter:1|c\n') + self.connector.processor.transport.close() state['first_time'] = False return await real_process_metric() - self.processor._process_metric = fake_process_metric + self.connector.processor._process_metric = fake_process_metric await self.wait_for(self.statsd_server.message_received.acquire()) From 720dd79193b04dbeb3354faf5e45fd02b30eef2a Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Tue, 9 Mar 2021 15:06:23 -0500 Subject: [PATCH 05/31] Add Application & RequestHandler mixins. --- CHANGELOG.rst | 5 +- MANIFEST.in | 4 +- README.rst | 63 ++++++++++- docs/index.rst | 29 ++++- example.py | 42 ++++++++ setup.cfg | 2 +- sprockets_statsd/mixins.py | 144 +++++++++++++++++++++++++ sprockets_statsd/statsd.py | 23 +++- tests/helpers.py | 3 + tests/test_mixins.py | 213 +++++++++++++++++++++++++++++++++++++ tests/test_processor.py | 19 +++- 11 files changed, 536 insertions(+), 11 deletions(-) create mode 100644 example.py create mode 100644 sprockets_statsd/mixins.py create mode 100644 tests/test_mixins.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index c90af8f..ef9c576 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -1,2 +1,3 @@ -Next Release ------------- +Initial release +--------------- +- support for sending counters & timers to statsd over a TCP socket diff --git a/MANIFEST.in b/MANIFEST.in index ba8e519..ab53868 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -1,4 +1,6 @@ graft docs graft tests -include LICENSE + include CHANGELOG.rst +include example.py +include LICENSE diff --git a/README.rst b/README.rst index bbb7a36..e7dff89 100644 --- a/README.rst +++ b/README.rst @@ -1,5 +1,64 @@ -Report metrics from your tornado_ web application to a StatsD_ instance. +Report metrics from your tornado_ web application to a statsd_ instance. -.. _StatsD: https://github.com/statsd/statsd/ +.. code-block:: python + + import asyncio + import logging + + from tornado import ioloop, web + + import sprockets_statsd.mixins + + + class MyHandler(sprockets_statsd.mixins.RequestHandler, + web.RequestHandler): + async def get(self): + with self.execution_timer('some-operation'): + await self.do_something() + self.set_status(204) + + async def do_something(self): + await asyncio.sleep(1) + + + class Application(sprockets_statsd.mixins.Application, web.Application): + def __init__(self, **settings): + super().__init__([web.url('/', MyHandler)], **settings) + + async def on_start(self): + await self.start_statsd() + + async def on_stop(self): + await self.stop_statsd() + + + if __name__ == '__main__': + logging.basicConfig(level=logging.DEBUG) + app = Application() + app.listen(8888) + iol = ioloop.IOLoop.current() + try: + iol.add_callback(app.on_start) + iol.start() + except KeyboardInterrupt: + iol.add_future(asyncio.ensure_future(app.on_stop()), + lambda f: iol.stop()) + iol.start() + +This application will emit two timing metrics each time that the endpoint is invoked:: + + applications.timers.some-operation:1001.3449192047119|ms + applications.timers.MyHandler.GET.204:1002.4960041046143|ms + +You will need to set the ``$STATSD_HOST`` environment variable to enable the statsd processing inside of the +application. The ``RequestHandler`` class exposes methods that send counter and timing metrics to a statsd server. +The connection is managed by the ``Application`` provided that you call the ``start_statsd`` method during application +startup. + +Metrics are sent by a ``asyncio.Task`` that is started by ``start_statsd``. The request handler methods insert the +metric data onto a ``asyncio.Queue`` that the task reads from. Metric data remains on the queue when the task is +not connected to the server and will be sent in the order received when the task establishes the server connection. + +.. _statsd: https://github.com/statsd/statsd/ .. _tornado: https://tornadoweb.org/ diff --git a/docs/index.rst b/docs/index.rst index ccfec4b..461a22f 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -4,16 +4,39 @@ sprockets-statsd .. include:: ../README.rst +Configuration +============= +The statsd connection is configured by the ``statsd`` application settings key. The default values can be set by +the following environment variables. + +.. envvar:: STATSD_HOST + + The host or IP address of the StatsD server to send metrics to. + +.. envvar:: STATSD_PORT + + The TCP port number that the StatsD server is listening on. This defaults to 8125 if it is not configured. + +You can fine tune the metric payloads and the connector by setting additional values in the ``stats`` key of +:attr:`tornado.web.Application.settings`. See the :class:`sprockets_statsd.mixins.Application` class +documentation for a description of the supported settings. + Reference ========= -Connector +Mixin classes +------------- +.. autoclass:: sprockets_statsd.mixins.Application + :members: + +.. autoclass:: sprockets_statsd.mixins.RequestHandler + :members: + +Internals --------- .. autoclass:: sprockets_statsd.statsd.Connector :members: -Processor internals -------------------- .. autoclass:: sprockets_statsd.statsd.Processor :members: diff --git a/example.py b/example.py new file mode 100644 index 0000000..0a41df2 --- /dev/null +++ b/example.py @@ -0,0 +1,42 @@ +import asyncio +import logging + +from tornado import ioloop, web + +import sprockets_statsd.mixins + + +class MyHandler(sprockets_statsd.mixins.RequestHandler, + web.RequestHandler): + async def get(self): + with self.execution_timer('some-operation'): + await self.do_something() + self.set_status(204) + + async def do_something(self): + await asyncio.sleep(1) + + +class Application(sprockets_statsd.mixins.Application, web.Application): + def __init__(self, **settings): + super().__init__([web.url('/', MyHandler)], **settings) + + async def on_start(self): + await self.start_statsd() + + async def on_stop(self): + await self.stop_statsd() + + +if __name__ == '__main__': + logging.basicConfig(level=logging.DEBUG) + app = Application() + app.listen(8888) + iol = ioloop.IOLoop.current() + try: + iol.add_callback(app.on_start) + iol.start() + except KeyboardInterrupt: + iol.add_future(asyncio.ensure_future(app.on_stop()), + lambda f: iol.stop()) + iol.start() diff --git a/setup.cfg b/setup.cfg index 7eb0493..e3bc6f4 100644 --- a/setup.cfg +++ b/setup.cfg @@ -59,7 +59,7 @@ branch = 1 source = sprockets_statsd [flake8] -application_import_names = statsd +application_import_names = sprockets_statsd,tests exclude = build,env,dist import_order_style = pycharm diff --git a/sprockets_statsd/mixins.py b/sprockets_statsd/mixins.py new file mode 100644 index 0000000..8d3a11d --- /dev/null +++ b/sprockets_statsd/mixins.py @@ -0,0 +1,144 @@ +import contextlib +import os +import time + +from tornado import web + +from sprockets_statsd import statsd + + +class Application(web.Application): + """Mix this into your application to add a statsd connection. + + This mix-in is configured by the ``statsd`` settings key. The + value should be a dictionary with the following keys. + + +--------+---------------------------------------------+ + | host | the statsd host to send metrics to | + +--------+---------------------------------------------+ + | port | TCP port number that statsd is listening on | + +--------+---------------------------------------------+ + | prefix | segment to prefix to metrics | + +--------+---------------------------------------------+ + + *host* defaults to the :envvar:`STATSD_HOST` environment variable. + If this value is not set, then the statsd connector **WILL NOT** + be enabled. + + *port* defaults to the :envvar:`STATSD_PORT` environment variable + with a back up default of 8125 if the environment variable is not + set. + + *prefix* defaults to ``applications..`` where + ** and ** are replaced with the keys from + `settings` if they are present. + + """ + def __init__(self, *args, **settings): + statsd_settings = settings.setdefault('statsd', {}) + statsd_settings.setdefault('host', os.environ.get('STATSD_HOST')) + statsd_settings.setdefault('port', + os.environ.get('STATSD_PORT', '8125')) + + prefix = ['applications'] + if 'service' in settings: + prefix.append(settings['service']) + if 'environment' in settings: + prefix.append(settings['environment']) + statsd_settings.setdefault('prefix', '.'.join(prefix)) + + super().__init__(*args, **settings) + + self.settings['statsd']['port'] = int(self.settings['statsd']['port']) + self.__statsd_connector = None + + async def start_statsd(self): + """Start the connector during startup. + + Call this method during application startup to enable the statsd + connection. A new :class:`~sprockets_statsd.statsd.Connector` + instance will be created and started. This method does not return + until the connector is running. + + """ + statsd_settings = self.settings['statsd'] + if statsd_settings.get('_connector') is None: + connector = statsd.Connector(host=statsd_settings['host'], + port=statsd_settings['port']) + await connector.start() + self.settings['statsd']['_connector'] = connector + + async def stop_statsd(self): + """Stop the connector during shutdown. + + If the connector was started, then this method will gracefully + terminate it. The method does not return until after the + connector is stopped. + + """ + connector = self.settings['statsd'].pop('_connector', None) + if connector is not None: + await connector.stop() + + +class RequestHandler(web.RequestHandler): + """Mix this into your handler to send metrics to a statsd server.""" + __connector: statsd.Connector + + def initialize(self, **kwargs): + super().initialize(**kwargs) + self.__connector = self.settings.get('statsd', {}).get('_connector') + + def __build_path(self, *path): + full_path = '.'.join(str(c) for c in path) + if self.settings.get('statsd', {}).get('prefix', ''): + return f'{self.settings["statsd"]["prefix"]}.{full_path}' + return full_path + + def record_timing(self, secs: float, *path): + """Record the duration. + + :param secs: number of seconds to record + :param path: path to record the duration under + + """ + if self.__connector is not None: + self.__connector.inject_metric(self.__build_path('timers', *path), + secs * 1000.0, 'ms') + + def increase_counter(self, *path, amount: int = 1): + """Adjust a counter. + + :param path: path of the counter to adjust + :param amount: amount to adjust the counter by. Defaults to + 1 and can be negative + + """ + if self.__connector is not None: + self.__connector.inject_metric( + self.__build_path('counters', *path), amount, 'c') + + @contextlib.contextmanager + def execution_timer(self, *path): + """Record the execution duration of a block of code. + + :param path: path to record the duration as + + """ + start = time.time() + try: + yield + finally: + self.record_timing(time.time() - start, *path) + + def on_finish(self): + """Extended to record the request time as a duration. + + This method extends :meth:`tornado.web.RequestHandler.on_finish` + to record ``self.request.request_time`` as a timing metric. + + """ + super().on_finish() + self.record_timing(self.request.request_time(), + self.__class__.__name__, self.request.method, + self.get_status()) diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py index 92994cd..11e0223 100644 --- a/sprockets_statsd/statsd.py +++ b/sprockets_statsd/statsd.py @@ -34,8 +34,14 @@ class Connector: self._processor_task = None async def start(self): - """Start the processor in the background.""" + """Start the processor in the background. + + This is a *blocking* method and does not return until the + processor task is actually running. + + """ self._processor_task = asyncio.create_task(self.processor.run()) + await self.processor.running.wait() async def stop(self): """Stop the background processor. @@ -105,6 +111,13 @@ class Processor(asyncio.Protocol): Is the TCP connection currently connected? + .. attribute:: running + :type: asyncio.Event + + Is the background task currently running? This is the event that + :meth:`.run` sets when it starts and it remains set until the task + exits. + .. attribute:: stopped :type: asyncio.Event @@ -115,9 +128,15 @@ class Processor(asyncio.Protocol): """ def __init__(self, *, host, port: int = 8125): super().__init__() + if not host: + raise RuntimeError('host must be set') + if not port or port < 1: + raise RuntimeError('port must be a positive integer') + self.host = host self.port = port + self.running = asyncio.Event() self.stopped = asyncio.Event() self.stopped.set() self.connected = asyncio.Event() @@ -130,6 +149,7 @@ class Processor(asyncio.Protocol): async def run(self): """Maintains the connection and processes metric payloads.""" + self.running.set() self.stopped.clear() self.should_terminate = False while not self.should_terminate: @@ -156,6 +176,7 @@ class Processor(asyncio.Protocol): await asyncio.sleep(0.1) self.logger.info('processor is exiting') + self.running.clear() self.stopped.set() async def stop(self): diff --git a/tests/helpers.py b/tests/helpers.py index 5e71207..f32ed86 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -1,8 +1,11 @@ import asyncio import io +import typing class StatsdServer(asyncio.Protocol): + metrics: typing.List[bytes] + def __init__(self): self.service = None self.host = '127.0.0.1' diff --git a/tests/test_mixins.py b/tests/test_mixins.py new file mode 100644 index 0000000..dc7d89f --- /dev/null +++ b/tests/test_mixins.py @@ -0,0 +1,213 @@ +import asyncio +import os +import time +import typing + +from tornado import testing, web + +import sprockets_statsd.mixins +from tests import helpers + +ParsedMetric = typing.Tuple[str, float, str] + + +class Handler(sprockets_statsd.mixins.RequestHandler, web.RequestHandler): + async def get(self): + with self.execution_timer('execution-timer'): + await asyncio.sleep(0.1) + self.increase_counter('request-count') + self.write('true') + + +class Application(sprockets_statsd.mixins.Application, web.Application): + def __init__(self, **settings): + super().__init__([web.url('/', Handler)], **settings) + + +class ApplicationTests(testing.AsyncTestCase): + def setUp(self): + super().setUp() + self._environ = {} + + def setenv(self, name, value): + self._environ.setdefault(name, os.environ.pop(name, None)) + os.environ[name] = value + + def unsetenv(self, name): + self._environ.setdefault(name, os.environ.pop(name, None)) + + def test_statsd_setting_defaults(self): + self.unsetenv('STATSD_HOST') + self.unsetenv('STATSD_PORT') + + app = sprockets_statsd.mixins.Application() + self.assertIn('statsd', app.settings) + self.assertIsNone(app.settings['statsd']['host'], + 'default host value should be None') + self.assertEqual(8125, app.settings['statsd']['port']) + self.assertEqual('applications', app.settings['statsd']['prefix']) + + def test_that_statsd_settings_read_from_environment(self): + self.setenv('STATSD_HOST', 'statsd') + self.setenv('STATSD_PORT', '5218') + + app = sprockets_statsd.mixins.Application() + self.assertIn('statsd', app.settings) + self.assertEqual('statsd', app.settings['statsd']['host']) + self.assertEqual(5218, app.settings['statsd']['port']) + + def test_that_service_included_in_prefix_if_set(self): + app = sprockets_statsd.mixins.Application(service='blah') + self.assertIn('statsd', app.settings) + self.assertEqual('applications.blah', app.settings['statsd']['prefix']) + + def test_that_environment_included_in_prefix_if_set(self): + app = sprockets_statsd.mixins.Application(environment='whatever') + self.assertIn('statsd', app.settings) + self.assertEqual('applications.whatever', + app.settings['statsd']['prefix']) + + def test_fully_specified_prefix(self): + app = sprockets_statsd.mixins.Application(environment='whatever', + service='blah') + self.assertIn('statsd', app.settings) + self.assertEqual('applications.blah.whatever', + app.settings['statsd']['prefix']) + + def test_overridden_settings(self): + self.setenv('STATSD_HOST', 'statsd') + self.setenv('STATSD_PORT', '9999') + app = sprockets_statsd.mixins.Application(statsd={ + 'host': 'statsd.example.com', + 'port': 5218, + 'prefix': 'myapp', + }) + self.assertEqual('statsd.example.com', app.settings['statsd']['host']) + self.assertEqual(5218, app.settings['statsd']['port']) + self.assertEqual('myapp', app.settings['statsd']['prefix']) + + def test_that_starting_without_configuration_fails(self): + self.unsetenv('STATSD_HOST') + app = sprockets_statsd.mixins.Application() + with self.assertRaises(RuntimeError): + self.io_loop.run_sync(app.start_statsd) + + def test_starting_twice(self): + app = sprockets_statsd.mixins.Application(statsd={ + 'host': 'localhost', + 'port': '8125', + }) + try: + self.io_loop.run_sync(app.start_statsd) + connector = app.settings['statsd']['_connector'] + self.assertIsNotNone(connector, 'statsd.Connector not created') + + self.io_loop.run_sync(app.start_statsd) + self.assertIs(app.settings['statsd']['_connector'], connector, + 'statsd.Connector should not be recreated') + finally: + self.io_loop.run_sync(app.stop_statsd) + + def test_stopping_without_starting(self): + app = sprockets_statsd.mixins.Application(statsd={ + 'host': 'localhost', + 'port': '8125', + }) + self.io_loop.run_sync(app.stop_statsd) + + +class RequestHandlerTests(testing.AsyncHTTPTestCase): + def setUp(self): + super().setUp() + self.statsd_server = helpers.StatsdServer() + self.io_loop.spawn_callback(self.statsd_server.run) + self.io_loop.run_sync(self.statsd_server.wait_running) + + self.app.settings['statsd']['host'] = self.statsd_server.host + self.app.settings['statsd']['port'] = self.statsd_server.port + self.io_loop.run_sync(self.app.start_statsd) + + def tearDown(self): + self.io_loop.run_sync(self.app.stop_statsd) + self.statsd_server.close() + self.io_loop.run_sync(self.statsd_server.wait_closed) + super().tearDown() + + def get_app(self): + self.app = Application() + return self.app + + def wait_for_metrics(self, metric_count=3): + timeout_remaining = testing.get_async_test_timeout() + for _ in range(metric_count): + start = time.time() + self.io_loop.run_sync(self.statsd_server.message_received.acquire, + timeout=timeout_remaining) + timeout_remaining -= (time.time() - start) + + def parse_metric(self, metric_line: bytes) -> ParsedMetric: + metric_line = metric_line.decode() + path, _, rest = metric_line.partition(':') + value, _, type_code = rest.partition('|') + try: + value = float(value) + except ValueError: + self.fail(f'value of {path} is not a number: value={value!r}') + return path, value, type_code + + def find_metric(self, needle: str) -> ParsedMetric: + needle = needle.encode() + for line in self.statsd_server.metrics: + if needle in line: + return self.parse_metric(line) + self.fail(f'failed to find metric containing {needle!r}') + + def test_the_request_metric_is_sent_last(self): + rsp = self.fetch('/') + self.assertEqual(200, rsp.code) + self.wait_for_metrics() + + path, _, type_code = self.find_metric('Handler.GET.200') + self.assertEqual(path, 'applications.timers.Handler.GET.200') + self.assertEqual('ms', type_code) + + def test_execution_timer(self): + rsp = self.fetch('/') + self.assertEqual(200, rsp.code) + self.wait_for_metrics() + + path, _, type_code = self.find_metric('execution-timer') + self.assertEqual('applications.timers.execution-timer', path) + self.assertEqual('ms', type_code) + + def test_counter(self): + rsp = self.fetch('/') + self.assertEqual(200, rsp.code) + self.wait_for_metrics() + + path, value, type_code = self.find_metric('request-count') + self.assertEqual('applications.counters.request-count', path) + self.assertEqual(1.0, value) + self.assertEqual('c', type_code) + + def test_handling_request_without_statsd_configured(self): + self.io_loop.run_sync(self.app.stop_statsd) + + rsp = self.fetch('/') + self.assertEqual(200, rsp.code) + + def test_handling_request_without_prefix(self): + self.app.settings['statsd']['prefix'] = '' + + rsp = self.fetch('/') + self.assertEqual(200, rsp.code) + self.wait_for_metrics() + + path, _, _ = self.find_metric('Handler.GET.200') + self.assertEqual('timers.Handler.GET.200', path) + + path, _, _ = self.find_metric('execution-timer') + self.assertEqual('timers.execution-timer', path) + + path, _, _ = self.find_metric('request-count') + self.assertEqual('counters.request-count', path) diff --git a/tests/test_processor.py b/tests/test_processor.py index a882558..cde9cfd 100644 --- a/tests/test_processor.py +++ b/tests/test_processor.py @@ -3,7 +3,6 @@ import time import unittest from sprockets_statsd import statsd - from tests import helpers @@ -118,6 +117,24 @@ class ProcessorTests(ProcessorTestCase): port=self.statsd_server.port) await self.wait_for(processor.stop()) + def test_that_processor_fails_when_host_is_none(self): + with self.assertRaises(RuntimeError) as context: + statsd.Processor(host=None, port=12345) + self.assertIn('host', str(context.exception)) + + def test_that_processor_fails_when_port_is_invalid(self): + with self.assertRaises(RuntimeError) as context: + statsd.Processor(host='localhost', port=None) + self.assertIn('port', str(context.exception)) + + with self.assertRaises(RuntimeError) as context: + statsd.Processor(host='localhost', port=0) + self.assertIn('port', str(context.exception)) + + with self.assertRaises(RuntimeError) as context: + statsd.Processor(host='localhost', port=-1) + self.assertIn('port', str(context.exception)) + class ConnectorTests(ProcessorTestCase): async def asyncSetUp(self): From f8c63a55fccbc75779b89ef9a88bfb3547e67eaf Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Tue, 9 Mar 2021 15:41:18 -0500 Subject: [PATCH 06/31] Switch to asynctest for Py 3.7 support. Decided to switch the tests to using asynctest so that we can guarantee support on Python 3.7 and newer. Also added pins in setup.cfg and a tox.ini file if you want to use it. --- setup.cfg | 16 ++++++++-------- tests/test_processor.py | 16 ++++++++++------ tox.ini | 19 +++++++++++++++++++ 3 files changed, 37 insertions(+), 14 deletions(-) create mode 100644 tox.ini diff --git a/setup.cfg b/setup.cfg index e3bc6f4..5690df0 100644 --- a/setup.cfg +++ b/setup.cfg @@ -29,17 +29,17 @@ classifiers = [options] packages = find: install_requires = - tornado + tornado>=5 [options.extras_require] dev = - coverage - flake8 - flake8-import-order - sphinx - sphinx-autodoc-typehints - tox - yapf + asynctest==0.13.0 + coverage==5.5 + flake8==3.8.4 + flake8-import-order==0.18.1 + sphinx==3.5.2 + sphinx-autodoc-typehints==1.11.1 + yapf==0.30.0 [options.packages.find] exclude = diff --git a/tests/test_processor.py b/tests/test_processor.py index cde9cfd..67ded25 100644 --- a/tests/test_processor.py +++ b/tests/test_processor.py @@ -1,15 +1,21 @@ import asyncio import time -import unittest + +import asynctest from sprockets_statsd import statsd from tests import helpers -class ProcessorTestCase(unittest.IsolatedAsyncioTestCase): - def setUp(self): - super().setUp() +class ProcessorTestCase(asynctest.TestCase): + async def setUp(self): self.test_timeout = 5.0 + super().setUp() + await self.asyncSetUp() + + async def tearDown(self): + await self.asyncTearDown() + super().tearDown() async def wait_for(self, fut): try: @@ -18,7 +24,6 @@ class ProcessorTestCase(unittest.IsolatedAsyncioTestCase): self.fail('future took too long to resolve') async def asyncSetUp(self): - await super().asyncSetUp() self.statsd_server = helpers.StatsdServer() self.statsd_task = asyncio.create_task(self.statsd_server.run()) await self.statsd_server.wait_running() @@ -26,7 +31,6 @@ class ProcessorTestCase(unittest.IsolatedAsyncioTestCase): async def asyncTearDown(self): self.statsd_task.cancel() await self.statsd_server.wait_closed() - await super().asyncTearDown() class ProcessorTests(ProcessorTestCase): diff --git a/tox.ini b/tox.ini new file mode 100644 index 0000000..e1c0671 --- /dev/null +++ b/tox.ini @@ -0,0 +1,19 @@ +[tox] +envlist = lint,py37,py39,tornado5 +toxworkdir = ./build/tox + +[testenv] +deps = + .[dev] +commands = + python -m unittest + +[testenv:lint] +commands = + flake8 sprockets_statsd tests + yapf -dr sprockets_statsd tests + +[testenv:tornado5] +deps = + tornado>=5,<6 + .[dev] From 4360bc298adaf56227674f70a51364bc508a998c Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Wed, 10 Mar 2021 06:39:26 -0500 Subject: [PATCH 07/31] Run style, lint, and unittests using GH actions. There is some commented out code for code coverage reporting that can be enabled after this repo is publicised. --- .github/workflows/run-tests.yml | 45 +++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 .github/workflows/run-tests.yml diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml new file mode 100644 index 0000000..a58a08f --- /dev/null +++ b/.github/workflows/run-tests.yml @@ -0,0 +1,45 @@ +name: Testing +on: + push: + branches: ["*"] + tags-ignore: ["*"] + pull_request: + branches: [main] + +jobs: + test: + runs-on: ubuntu-latest + strategy: + matrix: + python-version: [3.7, 3.8, 3.9] + steps: + - uses: actions/checkout@v2 + - name: Install python ${{ matrix.python-version }} + uses: actions/setup-python@v2 + with: + python-version: ${{ matrix.python-version }} + - name: Install dependencies + run: | + python -m pip install --upgrade pip setuptools + python -m pip install '.[dev]' + python -m pip install -e . + - name: Lint + run: | + flake8 sprockets_statsd tests + - name: Check format + run: | + yapf -dr sprockets_statsd tests + - name: Run tests + run: | + coverage run -m unittest + coverage report + coverage xml +# TODO +# - name: Upload coverage +# uses: codecov/codecov-action@v1.0.2 +# if: github.event_name == 'push' && github.repository == 'sprockets/sprockets-statsd' && github.branch == 'main' +# with: +# token: ${{ secrets.CODECOV_TOKEN }} +# file: build/coverage.xml +# flags: unittests +# fail_ci_if_error: true From 00759ed20bd7872243dcd481e20473ecd89d03a3 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Wed, 10 Mar 2021 07:16:47 -0500 Subject: [PATCH 08/31] Correct some resource leaks in tests. --- tests/test_processor.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_processor.py b/tests/test_processor.py index 67ded25..a7186aa 100644 --- a/tests/test_processor.py +++ b/tests/test_processor.py @@ -95,6 +95,7 @@ class ProcessorTests(ProcessorTestCase): self.statsd_server.transports[0].close() await self.wait_for(self.statsd_server.client_connected.acquire()) + await self.wait_for(processor.stop()) async def test_connection_failures(self): processor = statsd.Processor(host=self.statsd_server.host, @@ -115,6 +116,7 @@ class ProcessorTests(ProcessorTestCase): processor.port = self.statsd_server.port await self.wait_for(self.statsd_server.client_connected.acquire()) + await self.wait_for(processor.stop()) async def test_that_stopping_when_not_running_is_safe(self): processor = statsd.Processor(host=self.statsd_server.host, From 65b5bacbee65d143906dd3aa45663144787ba293 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Thu, 11 Mar 2021 07:03:18 -0500 Subject: [PATCH 09/31] Reorder logic to emphasize common paths. I snuck in a call to verify that the connected event is set before the call to _process_metric. This closes a small loophole where the transport could be None inside of _process_metric. --- sprockets_statsd/statsd.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py index 11e0223..933f23b 100644 --- a/sprockets_statsd/statsd.py +++ b/sprockets_statsd/statsd.py @@ -155,7 +155,8 @@ class Processor(asyncio.Protocol): while not self.should_terminate: try: await self._connect_if_necessary() - await self._process_metric() + if self.connected.is_set(): + await self._process_metric() except asyncio.CancelledError: self.logger.info('task cancelled, exiting') break @@ -221,14 +222,11 @@ class Processor(asyncio.Protocol): async def _process_metric(self): processing_failed_send = False - if self._failed_sends: - self.logger.debug('using previous send attempt') - metric = self._failed_sends[0] - processing_failed_send = True - else: + if not self._failed_sends: try: metric = await asyncio.wait_for(self.queue.get(), 0.1) self.logger.debug('received %r from queue', metric) + self.queue.task_done() except asyncio.TimeoutError: return else: @@ -240,15 +238,17 @@ class Processor(asyncio.Protocol): self.logger.debug('preventing send on closed transport') self._failed_sends.append(metric) return + else: + self.logger.debug('using previous send attempt') + metric = self._failed_sends[0] + processing_failed_send = True self.transport.write(metric) - if self.transport.is_closing(): - # Writing to a transport does not raise exceptions, it - # will close the transport if a low-level error occurs. - self.logger.debug('transport closed by writing') - else: + if not self.transport.is_closing(): self.logger.debug('sent %r to statsd', metric) if processing_failed_send: self._failed_sends.pop(0) - else: - self.queue.task_done() + else: + # Writing to a transport does not raise exceptions, it + # will close the transport if a low-level error occurs. + self.logger.debug('transport closed by writing') From 28e369c1227a0aead8e2e2e33044fc0141fc52b6 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Thu, 11 Mar 2021 07:31:24 -0500 Subject: [PATCH 10/31] Make timeout parameters configurable. --- sprockets_statsd/mixins.py | 43 ++++++++++++++++++++++++++++++-------- sprockets_statsd/statsd.py | 27 ++++++++++++++++++------ tests/test_mixins.py | 15 +++++++++++++ 3 files changed, 70 insertions(+), 15 deletions(-) diff --git a/sprockets_statsd/mixins.py b/sprockets_statsd/mixins.py index 8d3a11d..76eb807 100644 --- a/sprockets_statsd/mixins.py +++ b/sprockets_statsd/mixins.py @@ -13,13 +13,11 @@ class Application(web.Application): This mix-in is configured by the ``statsd`` settings key. The value should be a dictionary with the following keys. - +--------+---------------------------------------------+ - | host | the statsd host to send metrics to | - +--------+---------------------------------------------+ - | port | TCP port number that statsd is listening on | - +--------+---------------------------------------------+ - | prefix | segment to prefix to metrics | - +--------+---------------------------------------------+ + +-------------------+---------------------------------------------+ + | host | the statsd host to send metrics to | + +-------------------+---------------------------------------------+ + | port | TCP port number that statsd is listening on | + +-------------------+---------------------------------------------+ *host* defaults to the :envvar:`STATSD_HOST` environment variable. If this value is not set, then the statsd connector **WILL NOT** @@ -29,10 +27,30 @@ class Application(web.Application): with a back up default of 8125 if the environment variable is not set. + The following keys MAY also be specified to fine tune the statsd + connection. + + +-------------------+---------------------------------------------+ + | prefix | segment to prefix to metrics. | + +-------------------+---------------------------------------------+ + | reconnect_timeout | number of seconds to sleep after a statsd | + | | connection attempt fails | + +-------------------+---------------------------------------------+ + | wait_timeout | number of seconds to wait for a metric to | + | | arrive on the queue before verifying the | + | | connection | + +-------------------+---------------------------------------------+ + *prefix* defaults to ``applications..`` where ** and ** are replaced with the keys from `settings` if they are present. + *reconnect_timeout* defaults to 1.0 seconds which limits the + aggressiveness of creating new TCP connections. + + *wait_timeout* defaults to 0.1 seconds which ensures that the + processor quickly responds to connection faults. + """ def __init__(self, *args, **settings): statsd_settings = settings.setdefault('statsd', {}) @@ -63,8 +81,15 @@ class Application(web.Application): """ statsd_settings = self.settings['statsd'] if statsd_settings.get('_connector') is None: - connector = statsd.Connector(host=statsd_settings['host'], - port=statsd_settings['port']) + kwargs = { + 'host': statsd_settings['host'], + 'port': statsd_settings['port'], + } + if 'reconnect_sleep' in statsd_settings: + kwargs['reconnect_sleep'] = statsd_settings['reconnect_sleep'] + if 'wait_timeout' in statsd_settings: + kwargs['wait_timeout'] = statsd_settings['wait_timeout'] + connector = statsd.Connector(**kwargs) await connector.start() self.settings['statsd']['_connector'] = connector diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py index 933f23b..e67ae1c 100644 --- a/sprockets_statsd/statsd.py +++ b/sprockets_statsd/statsd.py @@ -8,6 +8,8 @@ class Connector: :param host: statsd server to send metrics to :param port: TCP port that the server is listening on + :param kwargs: additional keyword parameters are passed + to the :class:`.Processor` initializer This class maintains a TCP connection to a statsd server and sends metric lines to it asynchronously. You must call the @@ -29,8 +31,8 @@ class Connector: sends the metric payloads. """ - def __init__(self, host: str, port: int = 8125): - self.processor = Processor(host=host, port=port) + def __init__(self, host: str, port: int = 8125, **kwargs): + self.processor = Processor(host=host, port=port, **kwargs) self._processor_task = None async def start(self): @@ -75,6 +77,10 @@ class Processor(asyncio.Protocol): :param host: statsd server to send metrics to :param port: TCP port that the server is listening on + :param reconnect_sleep: number of seconds to sleep after socket + error occurs when connecting + :param wait_timeout: number os seconds to wait for a message to + arrive on the queue This class implements :class:`~asyncio.Protocol` for the statsd TCP connection. The :meth:`.run` method is run as a background @@ -126,7 +132,12 @@ class Processor(asyncio.Protocol): until the task stops. """ - def __init__(self, *, host, port: int = 8125): + def __init__(self, + *, + host, + port: int = 8125, + reconnect_sleep: float = 1.0, + wait_timeout: float = 0.1): super().__init__() if not host: raise RuntimeError('host must be set') @@ -135,6 +146,8 @@ class Processor(asyncio.Protocol): self.host = host self.port = port + self._reconnect_sleep = reconnect_sleep + self._wait_timeout = wait_timeout self.running = asyncio.Event() self.stopped = asyncio.Event() @@ -205,9 +218,9 @@ class Processor(asyncio.Protocol): self.logger.warning('statsd server connection lost') self.connected.clear() - async def _connect_if_necessary(self, wait_time: float = 0.1): + async def _connect_if_necessary(self): try: - await asyncio.wait_for(self.connected.wait(), wait_time) + await asyncio.wait_for(self.connected.wait(), self._wait_timeout) except asyncio.TimeoutError: try: self.logger.debug('starting connection to %s:%s', self.host, @@ -219,12 +232,14 @@ class Processor(asyncio.Protocol): except IOError as error: self.logger.warning('connection to %s:%s failed: %s', self.host, self.port, error) + await asyncio.sleep(self._reconnect_sleep) async def _process_metric(self): processing_failed_send = False if not self._failed_sends: try: - metric = await asyncio.wait_for(self.queue.get(), 0.1) + metric = await asyncio.wait_for(self.queue.get(), + self._wait_timeout) self.logger.debug('received %r from queue', metric) self.queue.task_done() except asyncio.TimeoutError: diff --git a/tests/test_mixins.py b/tests/test_mixins.py index dc7d89f..c820c2e 100644 --- a/tests/test_mixins.py +++ b/tests/test_mixins.py @@ -115,6 +115,21 @@ class ApplicationTests(testing.AsyncTestCase): }) self.io_loop.run_sync(app.stop_statsd) + def test_optional_parameters(self): + app = sprockets_statsd.mixins.Application( + statsd={ + 'host': 'localhost', + 'port': '8125', + 'reconnect_sleep': 0.5, + 'wait_timeout': 0.25, + }) + self.io_loop.run_sync(app.start_statsd) + + processor = app.settings['statsd']['_connector'].processor + self.assertEqual(0.5, processor._reconnect_sleep) + self.assertEqual(0.25, processor._wait_timeout) + self.io_loop.run_sync(app.stop_statsd) + class RequestHandlerTests(testing.AsyncHTTPTestCase): def setUp(self): From 63dc44fd22b668d117f88e6d8c3e413dd1f83c7c Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Thu, 11 Mar 2021 07:43:17 -0500 Subject: [PATCH 11/31] Ensure that docs build as part of CI. --- .github/workflows/run-tests.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index a58a08f..c65650c 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -34,6 +34,9 @@ jobs: coverage run -m unittest coverage report coverage xml + - name: Generate documentation + run: | + sphinx-build -b html -W --no-color docs build/sphinx/html # TODO # - name: Upload coverage # uses: codecov/codecov-action@v1.0.2 From 69dfb51bf8720ab1c9a44ecc2acbb807d401567f Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Tue, 23 Mar 2021 07:08:34 -0400 Subject: [PATCH 12/31] Rework application prefix configuration. Use a default of `None` instead of `applications` if the neither service nor environment keys are set. --- sprockets_statsd/mixins.py | 55 ++++++++++++++++++++------------------ tests/test_mixins.py | 32 +++++++++++----------- 2 files changed, 46 insertions(+), 41 deletions(-) diff --git a/sprockets_statsd/mixins.py b/sprockets_statsd/mixins.py index 76eb807..c473f08 100644 --- a/sprockets_statsd/mixins.py +++ b/sprockets_statsd/mixins.py @@ -11,25 +11,12 @@ class Application(web.Application): """Mix this into your application to add a statsd connection. This mix-in is configured by the ``statsd`` settings key. The - value should be a dictionary with the following keys. + value is a dictionary with the following keys. +-------------------+---------------------------------------------+ | host | the statsd host to send metrics to | +-------------------+---------------------------------------------+ - | port | TCP port number that statsd is listening on | - +-------------------+---------------------------------------------+ - - *host* defaults to the :envvar:`STATSD_HOST` environment variable. - If this value is not set, then the statsd connector **WILL NOT** - be enabled. - - *port* defaults to the :envvar:`STATSD_PORT` environment variable - with a back up default of 8125 if the environment variable is not - set. - - The following keys MAY also be specified to fine tune the statsd - connection. - + | port | port number that statsd is listening on | +-------------------+---------------------------------------------+ | prefix | segment to prefix to metrics. | +-------------------+---------------------------------------------+ @@ -41,14 +28,27 @@ class Application(web.Application): | | connection | +-------------------+---------------------------------------------+ - *prefix* defaults to ``applications..`` where - ** and ** are replaced with the keys from - `settings` if they are present. + **host** defaults to the :envvar:`STATSD_HOST` environment variable. + If this value is not set, then the statsd connector *WILL NOT* be + enabled. - *reconnect_timeout* defaults to 1.0 seconds which limits the + **port** defaults to the :envvar:`STATSD_PORT` environment variable + with a back up default of 8125 if the environment variable is not + set. + + **prefix** is prefixed to all metric paths. This provides a + namespace for metrics so that each applications metrics are maintained + in separate buckets. + + If the *service* and *environment* keys are set in ``settings``, + then the default prefix is ``applications..``. + This is a convenient way to maintain consistent metric paths when + you are managing a larger number of services. + + **reconnect_timeout** defaults to 1.0 seconds which limits the aggressiveness of creating new TCP connections. - *wait_timeout* defaults to 0.1 seconds which ensures that the + **wait_timeout** defaults to 0.1 seconds which ensures that the processor quickly responds to connection faults. """ @@ -58,12 +58,15 @@ class Application(web.Application): statsd_settings.setdefault('port', os.environ.get('STATSD_PORT', '8125')) - prefix = ['applications'] - if 'service' in settings: - prefix.append(settings['service']) - if 'environment' in settings: - prefix.append(settings['environment']) - statsd_settings.setdefault('prefix', '.'.join(prefix)) + try: + prefix = '.'.join([ + 'applications', + settings['service'], + settings['environment'], + ]) + except KeyError: + prefix = None + statsd_settings.setdefault('prefix', prefix) super().__init__(*args, **settings) diff --git a/tests/test_mixins.py b/tests/test_mixins.py index c820c2e..06f35b5 100644 --- a/tests/test_mixins.py +++ b/tests/test_mixins.py @@ -45,7 +45,7 @@ class ApplicationTests(testing.AsyncTestCase): self.assertIsNone(app.settings['statsd']['host'], 'default host value should be None') self.assertEqual(8125, app.settings['statsd']['port']) - self.assertEqual('applications', app.settings['statsd']['prefix']) + self.assertEqual(None, app.settings['statsd']['prefix']) def test_that_statsd_settings_read_from_environment(self): self.setenv('STATSD_HOST', 'statsd') @@ -56,22 +56,21 @@ class ApplicationTests(testing.AsyncTestCase): self.assertEqual('statsd', app.settings['statsd']['host']) self.assertEqual(5218, app.settings['statsd']['port']) - def test_that_service_included_in_prefix_if_set(self): + def test_prefix_when_only_service_is_set(self): app = sprockets_statsd.mixins.Application(service='blah') self.assertIn('statsd', app.settings) - self.assertEqual('applications.blah', app.settings['statsd']['prefix']) + self.assertEqual(None, app.settings['statsd']['prefix']) - def test_that_environment_included_in_prefix_if_set(self): + def test_prefix_when_only_environment_is_set(self): app = sprockets_statsd.mixins.Application(environment='whatever') self.assertIn('statsd', app.settings) - self.assertEqual('applications.whatever', - app.settings['statsd']['prefix']) + self.assertEqual(None, app.settings['statsd']['prefix']) - def test_fully_specified_prefix(self): - app = sprockets_statsd.mixins.Application(environment='whatever', - service='blah') + def test_prefix_default_when_service_and_environment_are_set(self): + app = sprockets_statsd.mixins.Application(environment='development', + service='my-service') self.assertIn('statsd', app.settings) - self.assertEqual('applications.blah.whatever', + self.assertEqual('applications.my-service.development', app.settings['statsd']['prefix']) def test_overridden_settings(self): @@ -138,8 +137,11 @@ class RequestHandlerTests(testing.AsyncHTTPTestCase): self.io_loop.spawn_callback(self.statsd_server.run) self.io_loop.run_sync(self.statsd_server.wait_running) - self.app.settings['statsd']['host'] = self.statsd_server.host - self.app.settings['statsd']['port'] = self.statsd_server.port + self.app.settings['statsd'].update({ + 'host': self.statsd_server.host, + 'port': self.statsd_server.port, + 'prefix': 'applications.service', + }) self.io_loop.run_sync(self.app.start_statsd) def tearDown(self): @@ -183,7 +185,7 @@ class RequestHandlerTests(testing.AsyncHTTPTestCase): self.wait_for_metrics() path, _, type_code = self.find_metric('Handler.GET.200') - self.assertEqual(path, 'applications.timers.Handler.GET.200') + self.assertEqual(path, 'applications.service.timers.Handler.GET.200') self.assertEqual('ms', type_code) def test_execution_timer(self): @@ -192,7 +194,7 @@ class RequestHandlerTests(testing.AsyncHTTPTestCase): self.wait_for_metrics() path, _, type_code = self.find_metric('execution-timer') - self.assertEqual('applications.timers.execution-timer', path) + self.assertEqual('applications.service.timers.execution-timer', path) self.assertEqual('ms', type_code) def test_counter(self): @@ -201,7 +203,7 @@ class RequestHandlerTests(testing.AsyncHTTPTestCase): self.wait_for_metrics() path, value, type_code = self.find_metric('request-count') - self.assertEqual('applications.counters.request-count', path) + self.assertEqual('applications.service.counters.request-count', path) self.assertEqual(1.0, value) self.assertEqual('c', type_code) From 6d310db517a8433c45faa8da5075322e4aeb8b0e Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Tue, 23 Mar 2021 07:43:20 -0400 Subject: [PATCH 13/31] Move statsd connector to attribute. Since @aremm still believes that `Application.settings` should only be used for configuration and not for state ;) --- sprockets_statsd/mixins.py | 39 ++++++++++++++++++++++---------------- tests/test_mixins.py | 6 +++--- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/sprockets_statsd/mixins.py b/sprockets_statsd/mixins.py index c473f08..4a4c593 100644 --- a/sprockets_statsd/mixins.py +++ b/sprockets_statsd/mixins.py @@ -10,6 +10,12 @@ from sprockets_statsd import statsd class Application(web.Application): """Mix this into your application to add a statsd connection. + .. attribute:: statsd_connector + :type: sprockets_statsd.statsd.Connector + + Connection to the StatsD server that is set between calls + to :meth:`.start_statsd` and :meth:`.stop_statsd`. + This mix-in is configured by the ``statsd`` settings key. The value is a dictionary with the following keys. @@ -71,7 +77,7 @@ class Application(web.Application): super().__init__(*args, **settings) self.settings['statsd']['port'] = int(self.settings['statsd']['port']) - self.__statsd_connector = None + self.statsd_connector = None async def start_statsd(self): """Start the connector during startup. @@ -82,8 +88,8 @@ class Application(web.Application): until the connector is running. """ - statsd_settings = self.settings['statsd'] - if statsd_settings.get('_connector') is None: + if self.statsd_connector is None: + statsd_settings = self.settings['statsd'] kwargs = { 'host': statsd_settings['host'], 'port': statsd_settings['port'], @@ -92,9 +98,9 @@ class Application(web.Application): kwargs['reconnect_sleep'] = statsd_settings['reconnect_sleep'] if 'wait_timeout' in statsd_settings: kwargs['wait_timeout'] = statsd_settings['wait_timeout'] - connector = statsd.Connector(**kwargs) - await connector.start() - self.settings['statsd']['_connector'] = connector + + self.statsd_connector = statsd.Connector(**kwargs) + await self.statsd_connector.start() async def stop_statsd(self): """Stop the connector during shutdown. @@ -104,18 +110,19 @@ class Application(web.Application): connector is stopped. """ - connector = self.settings['statsd'].pop('_connector', None) - if connector is not None: - await connector.stop() + if self.statsd_connector is not None: + await self.statsd_connector.stop() + self.statsd_connector = None class RequestHandler(web.RequestHandler): """Mix this into your handler to send metrics to a statsd server.""" - __connector: statsd.Connector + statsd_connector: statsd.Connector def initialize(self, **kwargs): super().initialize(**kwargs) - self.__connector = self.settings.get('statsd', {}).get('_connector') + self.application: Application + self.statsd_connector = self.application.statsd_connector def __build_path(self, *path): full_path = '.'.join(str(c) for c in path) @@ -130,9 +137,9 @@ class RequestHandler(web.RequestHandler): :param path: path to record the duration under """ - if self.__connector is not None: - self.__connector.inject_metric(self.__build_path('timers', *path), - secs * 1000.0, 'ms') + if self.statsd_connector is not None: + self.statsd_connector.inject_metric( + self.__build_path('timers', *path), secs * 1000.0, 'ms') def increase_counter(self, *path, amount: int = 1): """Adjust a counter. @@ -142,8 +149,8 @@ class RequestHandler(web.RequestHandler): 1 and can be negative """ - if self.__connector is not None: - self.__connector.inject_metric( + if self.statsd_connector is not None: + self.statsd_connector.inject_metric( self.__build_path('counters', *path), amount, 'c') @contextlib.contextmanager diff --git a/tests/test_mixins.py b/tests/test_mixins.py index 06f35b5..20c4151 100644 --- a/tests/test_mixins.py +++ b/tests/test_mixins.py @@ -98,11 +98,11 @@ class ApplicationTests(testing.AsyncTestCase): }) try: self.io_loop.run_sync(app.start_statsd) - connector = app.settings['statsd']['_connector'] + connector = app.statsd_connector self.assertIsNotNone(connector, 'statsd.Connector not created') self.io_loop.run_sync(app.start_statsd) - self.assertIs(app.settings['statsd']['_connector'], connector, + self.assertIs(app.statsd_connector, connector, 'statsd.Connector should not be recreated') finally: self.io_loop.run_sync(app.stop_statsd) @@ -124,7 +124,7 @@ class ApplicationTests(testing.AsyncTestCase): }) self.io_loop.run_sync(app.start_statsd) - processor = app.settings['statsd']['_connector'].processor + processor = app.statsd_connector.processor self.assertEqual(0.5, processor._reconnect_sleep) self.assertEqual(0.25, processor._wait_timeout) self.io_loop.run_sync(app.stop_statsd) From 38a5e3f566b87e786332ac3c6b51e8c8c4037537 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Fri, 19 Mar 2021 18:19:03 -0400 Subject: [PATCH 14/31] Refactor to pull out TCP details. This will make it easier to add the UDP protocol. --- docs/index.rst | 10 +- setup.cfg | 3 + sprockets_statsd/statsd.py | 221 +++++++++++++++++++++++++------------ tests/test_mixins.py | 8 +- tests/test_processor.py | 127 ++++++++++++++------- 5 files changed, 256 insertions(+), 113 deletions(-) diff --git a/docs/index.rst b/docs/index.rst index 461a22f..b359fc3 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -24,6 +24,9 @@ documentation for a description of the supported settings. Reference ========= +.. autoclass:: sprockets_statsd.statsd.Connector + :members: + Mixin classes ------------- .. autoclass:: sprockets_statsd.mixins.Application @@ -34,10 +37,13 @@ Mixin classes Internals --------- -.. autoclass:: sprockets_statsd.statsd.Connector +.. autoclass:: sprockets_statsd.statsd.Processor :members: -.. autoclass:: sprockets_statsd.statsd.Processor +.. autoclass:: sprockets_statsd.statsd.StatsdProtocol + :members: + +.. autoclass:: sprockets_statsd.statsd.TCPProtocol :members: Release history diff --git a/setup.cfg b/setup.cfg index 5690df0..fb9fc61 100644 --- a/setup.cfg +++ b/setup.cfg @@ -51,6 +51,9 @@ nitpicky = 1 warning_is_error = 1 [coverage:report] +exclude_lines = + pragma: no cover + raise NotImplementedError fail_under = 100 show_missing = 1 diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py index e67ae1c..2547e45 100644 --- a/sprockets_statsd/statsd.py +++ b/sprockets_statsd/statsd.py @@ -68,11 +68,104 @@ class Connector: internal queue for future processing. """ - payload = f'{path}:{value}|{type_code}\n' + payload = f'{path}:{value}|{type_code}' self.processor.queue.put_nowait(payload.encode('utf-8')) -class Processor(asyncio.Protocol): +class StatsdProtocol: + """Common interface for backend protocols/transports. + + UDP and TCP transports have different interfaces (sendto vs write) + so this class adapts them to a common protocol that our code + can depend on. + + .. attribute:: buffered_data + :type: bytes + + Bytes that are buffered due to low-level transport failures. + Since protocols & transports are created anew with each connect + attempt, the :class:`.Processor` instance ensures that data + buffered on a transport is copied over to the new transport + when creating a connection. + + .. attribute:: connected + :type: asyncio.Event + + Is the protocol currently connected? + + """ + logger: logging.Logger + + def __init__(self): + self.buffered_data = b'' + self.connected = asyncio.Event() + self.logger = logging.getLogger(__package__).getChild( + self.__class__.__name__) + self.transport = None + + def send(self, metric: bytes) -> None: + """Send a metric payload over the transport.""" + raise NotImplementedError() + + async def shutdown(self) -> None: + """Shutdown the transport and wait for it to close.""" + raise NotImplementedError() + + def connection_made(self, transport: asyncio.Transport): + """Capture the new transport and set the connected event.""" + server, port = transport.get_extra_info('peername') + self.logger.info('connected to statsd %s:%s', server, port) + self.transport = transport + self.connected.set() + + def connection_lost(self, exc: typing.Optional[Exception]): + """Clear the connected event.""" + self.logger.warning('statsd server connection lost: %s', exc) + self.connected.clear() + + +class TCPProtocol(StatsdProtocol, asyncio.Protocol): + """StatsdProtocol implementation over a TCP/IP connection.""" + transport: asyncio.WriteTransport + + def eof_received(self): + self.logger.warning('received EOF from statsd server') + self.connected.clear() + + def send(self, metric: bytes) -> None: + """Send `metric` to the server. + + If sending the metric fails, it will be saved in + ``self.buffered_data``. The processor will save and + restore the buffered data if it needs to create a + new protocol object. + + """ + if not self.buffered_data and not metric: + return + + self.buffered_data = self.buffered_data + metric + b'\n' + while (self.transport is not None and self.connected.is_set() + and self.buffered_data): + line, maybe_nl, rest = self.buffered_data.partition(b'\n') + line += maybe_nl + self.transport.write(line) + if self.transport.is_closing(): + self.logger.warning('transport closed during write') + break + self.buffered_data = rest + + async def shutdown(self) -> None: + """Close the transport after flushing any outstanding data.""" + self.logger.info('shutting down') + if self.connected.is_set(): + self.send(b'') # flush buffered data + self.transport.close() + while self.connected.is_set(): + await asyncio.sleep(0.1) + + +class Processor: """Maintains the statsd connection and sends metric payloads. :param host: statsd server to send metrics to @@ -112,11 +205,6 @@ class Processor(asyncio.Protocol): Formatted metric payloads to send to the statsd server. Enqueue payloads to send them to the server. - .. attribute:: connected - :type: asyncio.Event - - Is the TCP connection currently connected? - .. attribute:: running :type: asyncio.Event @@ -132,6 +220,9 @@ class Processor(asyncio.Protocol): until the task stops. """ + + protocol: typing.Union[StatsdProtocol, None] + def __init__(self, *, host, @@ -141,8 +232,13 @@ class Processor(asyncio.Protocol): super().__init__() if not host: raise RuntimeError('host must be set') - if not port or port < 1: - raise RuntimeError('port must be a positive integer') + try: + port = int(port) + if not port or port < 1: + raise RuntimeError( + f'port must be a positive integer: {port!r}') + except TypeError: + raise RuntimeError(f'port must be a positive integer: {port!r}') self.host = host self.port = port @@ -152,14 +248,20 @@ class Processor(asyncio.Protocol): self.running = asyncio.Event() self.stopped = asyncio.Event() self.stopped.set() - self.connected = asyncio.Event() self.logger = logging.getLogger(__package__).getChild('Processor') self.should_terminate = False - self.transport = None + self.protocol = None self.queue = asyncio.Queue() self._failed_sends = [] + @property + def connected(self) -> bool: + """Is the processor connected?""" + if self.protocol is None: + return False + return self.protocol.connected.is_set() + async def run(self): """Maintains the connection and processes metric payloads.""" self.running.set() @@ -168,7 +270,7 @@ class Processor(asyncio.Protocol): while not self.should_terminate: try: await self._connect_if_necessary() - if self.connected.is_set(): + if self.connected: await self._process_metric() except asyncio.CancelledError: self.logger.info('task cancelled, exiting') @@ -177,17 +279,14 @@ class Processor(asyncio.Protocol): self.should_terminate = True self.logger.info('loop finished with %d metrics in the queue', self.queue.qsize()) - if self.connected.is_set(): - num_ready = self.queue.qsize() + if self.connected: + num_ready = max(self.queue.qsize(), 1) self.logger.info('draining %d metrics', num_ready) for _ in range(num_ready): await self._process_metric() self.logger.debug('closing transport') - self.transport.close() - - while self.connected.is_set(): - self.logger.debug('waiting on transport to close') - await asyncio.sleep(0.1) + if self.protocol is not None: + await self.protocol.shutdown() self.logger.info('processor is exiting') self.running.clear() @@ -204,66 +303,46 @@ class Processor(asyncio.Protocol): self.should_terminate = True await self.stopped.wait() - def eof_received(self): - self.logger.warning('received EOF from statsd server') - self.connected.clear() - - def connection_made(self, transport: asyncio.Transport): - server, port = transport.get_extra_info('peername') - self.logger.info('connected to statsd %s:%s', server, port) - self.transport = transport - self.connected.set() - - def connection_lost(self, exc: typing.Optional[Exception]): - self.logger.warning('statsd server connection lost') - self.connected.clear() - async def _connect_if_necessary(self): - try: - await asyncio.wait_for(self.connected.wait(), self._wait_timeout) - except asyncio.TimeoutError: + if self.protocol is not None: try: - self.logger.debug('starting connection to %s:%s', self.host, - self.port) - await asyncio.get_running_loop().create_connection( - protocol_factory=lambda: self, + await asyncio.wait_for(self.protocol.connected.wait(), + self._wait_timeout) + except asyncio.TimeoutError: + self.logger.debug('protocol is no longer connected') + + if not self.connected: + try: + buffered_data = b'' + if self.protocol is not None: + buffered_data = self.protocol.buffered_data + loop = asyncio.get_running_loop() + transport, protocol = await loop.create_connection( + protocol_factory=TCPProtocol, host=self.host, port=self.port) + self.protocol = typing.cast(TCPProtocol, protocol) + self.protocol.buffered_data = buffered_data + self.logger.info('connection established to %s', + transport.get_extra_info('peername')) except IOError as error: self.logger.warning('connection to %s:%s failed: %s', self.host, self.port, error) await asyncio.sleep(self._reconnect_sleep) async def _process_metric(self): - processing_failed_send = False - if not self._failed_sends: - try: - metric = await asyncio.wait_for(self.queue.get(), - self._wait_timeout) - self.logger.debug('received %r from queue', metric) - self.queue.task_done() - except asyncio.TimeoutError: - return - else: - # Since we `await`d the state of the transport may have - # changed. Sending on the closed transport won't return - # an error since the send is async. We can catch the - # problem here though. - if self.transport.is_closing(): - self.logger.debug('preventing send on closed transport') - self._failed_sends.append(metric) - return - else: - self.logger.debug('using previous send attempt') - metric = self._failed_sends[0] - processing_failed_send = True + try: + metric = await asyncio.wait_for(self.queue.get(), + self._wait_timeout) + self.logger.debug('received %r from queue', metric) + self.queue.task_done() + except asyncio.TimeoutError: + # we still want to invoke the protocol send in case + # it has queued metrics to send + metric = b'' - self.transport.write(metric) - if not self.transport.is_closing(): - self.logger.debug('sent %r to statsd', metric) - if processing_failed_send: - self._failed_sends.pop(0) - else: - # Writing to a transport does not raise exceptions, it - # will close the transport if a low-level error occurs. - self.logger.debug('transport closed by writing') + try: + self.protocol.send(metric) + except Exception as error: + self.logger.exception('exception occurred when sending metric: %s', + error) diff --git a/tests/test_mixins.py b/tests/test_mixins.py index 20c4151..d096470 100644 --- a/tests/test_mixins.py +++ b/tests/test_mixins.py @@ -158,8 +158,12 @@ class RequestHandlerTests(testing.AsyncHTTPTestCase): timeout_remaining = testing.get_async_test_timeout() for _ in range(metric_count): start = time.time() - self.io_loop.run_sync(self.statsd_server.message_received.acquire, - timeout=timeout_remaining) + try: + self.io_loop.run_sync( + self.statsd_server.message_received.acquire, + timeout=timeout_remaining) + except TimeoutError: + self.fail() timeout_remaining -= (time.time() - start) def parse_metric(self, metric_line: bytes) -> ParsedMetric: diff --git a/tests/test_processor.py b/tests/test_processor.py index a7186aa..fcbc515 100644 --- a/tests/test_processor.py +++ b/tests/test_processor.py @@ -1,4 +1,5 @@ import asyncio +import logging import time import asynctest @@ -55,7 +56,7 @@ class ProcessorTests(ProcessorTestCase): self.statsd_server.close() await self.statsd_server.wait_closed() until = time.time() + self.test_timeout - while processor.connected.is_set(): + while processor.connected: await asyncio.sleep(0.1) if time.time() >= until: self.fail('processor never disconnected') @@ -63,7 +64,7 @@ class ProcessorTests(ProcessorTestCase): # Start the server on the same port and let the client reconnect. self.statsd_task = asyncio.create_task(self.statsd_server.run()) await self.wait_for(self.statsd_server.client_connected.acquire()) - self.assertTrue(processor.connected.is_set()) + self.assertTrue(processor.connected) await self.wait_for(processor.stop()) @@ -97,27 +98,6 @@ class ProcessorTests(ProcessorTestCase): await self.wait_for(self.statsd_server.client_connected.acquire()) await self.wait_for(processor.stop()) - async def test_connection_failures(self): - processor = statsd.Processor(host=self.statsd_server.host, - port=self.statsd_server.port) - asyncio.create_task(processor.run()) - await self.wait_for(self.statsd_server.client_connected.acquire()) - - # Change the port and close the transport, this will cause the - # processor to reconnect to the new port and fail. - processor.port = 1 - processor.transport.close() - - # Wait for the processor to be disconnected, then change the - # port back and let the processor reconnect. - while processor.connected.is_set(): - await asyncio.sleep(0.1) - await asyncio.sleep(0.2) - processor.port = self.statsd_server.port - - await self.wait_for(self.statsd_server.client_connected.acquire()) - await self.wait_for(processor.stop()) - async def test_that_stopping_when_not_running_is_safe(self): processor = statsd.Processor(host=self.statsd_server.host, port=self.statsd_server.port) @@ -141,6 +121,92 @@ class ProcessorTests(ProcessorTestCase): statsd.Processor(host='localhost', port=-1) self.assertIn('port', str(context.exception)) + async def test_starting_and_stopping_without_connecting(self): + host, port = self.statsd_server.host, self.statsd_server.port + self.statsd_server.close() + await self.wait_for(self.statsd_server.wait_closed()) + processor = statsd.Processor(host=host, port=port) + asyncio.create_task(processor.run()) + await self.wait_for(processor.running.wait()) + await processor.stop() + + async def test_that_protocol_exceptions_are_logged(self): + processor = statsd.Processor(host=self.statsd_server.host, + port=self.statsd_server.port) + asyncio.create_task(processor.run()) + await self.wait_for(processor.running.wait()) + + with self.assertLogs(processor.logger, level=logging.ERROR) as cm: + processor.queue.put_nowait('not-bytes') + while processor.queue.qsize() > 0: + await asyncio.sleep(0.1) + + for record in cm.records: + if (record.exc_info is not None + and record.funcName == '_process_metric'): + break + else: + self.fail('Expected _process_metric to log exception') + + await processor.stop() + + +class TCPProcessingTests(ProcessorTestCase): + async def asyncSetUp(self): + await super().asyncSetUp() + self.processor = statsd.Processor(host=self.statsd_server.host, + port=self.statsd_server.port) + asyncio.create_task(self.processor.run()) + await self.wait_for(self.statsd_server.client_connected.acquire()) + + async def asyncTearDown(self): + await self.processor.stop() + await super().asyncTearDown() + + async def test_connection_failures(self): + # Change the port and close the transport, this will cause the + # processor to reconnect to the new port and fail. + self.processor.port = 1 + self.processor.protocol.transport.close() + + # Wait for the processor to be disconnected, then change the + # port back and let the processor reconnect. + while self.processor.connected: + await asyncio.sleep(0.1) + await asyncio.sleep(0.2) + self.processor.port = self.statsd_server.port + + await self.wait_for(self.statsd_server.client_connected.acquire()) + + async def test_socket_closure_while_processing_failed_event(self): + state = {'first_time': True} + real_process_metric = self.processor._process_metric + + async def fake_process_metric(): + if state['first_time']: + self.processor.protocol.buffered_data = b'counter:1|c\n' + self.processor.protocol.transport.close() + state['first_time'] = False + return await real_process_metric() + + self.processor._process_metric = fake_process_metric + + await self.wait_for(self.statsd_server.message_received.acquire()) + + async def test_socket_closure_while_sending(self): + state = {'first_time': True} + real_transport_write = self.processor.protocol.transport.write + + def fake_transport_write(buffer): + if state['first_time']: + self.processor.protocol.transport.close() + state['first_time'] = False + return real_transport_write(buffer) + + self.processor.protocol.transport.write = fake_transport_write + self.processor.queue.put_nowait(b'counter:1|c') + await self.wait_for(self.statsd_server.message_received.acquire()) + class ConnectorTests(ProcessorTestCase): async def asyncSetUp(self): @@ -223,18 +289,3 @@ class ConnectorTests(ProcessorTestCase): await self.wait_for(self.statsd_server.message_received.acquire()) self.assertEqual(f'counter:{value}|c'.encode(), self.statsd_server.metrics.pop(0)) - - async def test_socket_closure_while_processing_failed_event(self): - state = {'first_time': True} - real_process_metric = self.connector.processor._process_metric - - async def fake_process_metric(): - if state['first_time']: - self.connector.processor._failed_sends.append(b'counter:1|c\n') - self.connector.processor.transport.close() - state['first_time'] = False - return await real_process_metric() - - self.connector.processor._process_metric = fake_process_metric - - await self.wait_for(self.statsd_server.message_received.acquire()) From c4c44f98642e86232a8164877043ea39c26b4c72 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Sun, 21 Mar 2021 10:22:55 -0400 Subject: [PATCH 15/31] Add support for UDP sockets. --- CHANGELOG.rst | 2 +- README.rst | 39 +++++++++++++- setup.cfg | 2 +- sprockets_statsd/statsd.py | 69 ++++++++++++++++++------ tests/helpers.py | 76 +++++++++++++++++--------- tests/test_mixins.py | 3 +- tests/test_processor.py | 108 ++++++++++++++++++++++++++++++------- 7 files changed, 236 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ef9c576..25c7b1c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -1,3 +1,3 @@ Initial release --------------- -- support for sending counters & timers to statsd over a TCP socket +- support for sending counters & timers to statsd over a TCP or UDP socket diff --git a/README.rst b/README.rst index e7dff89..1033fd0 100644 --- a/README.rst +++ b/README.rst @@ -1,4 +1,40 @@ -Report metrics from your tornado_ web application to a statsd_ instance. +Asynchronously send metrics to a statsd_ instance. + +This library provides connectors to send metrics to a statsd_ instance using either TCP or UDP. + +.. code-block:: python + + import asyncio + import time + + import sprockets_statsd.statsd + + statsd = sprockets_statsd.statsd.Connector( + host=os.environ.get('STATSD_HOST', '127.0.0.1')) + + async def do_stuff(): + start = time.time() + response = make_some_http_call() + statsd.inject_metric(f'timers.http.something.{response.code}', + (time.time() - start) * 1000.0, 'ms') + + async def main(): + await statsd.start() + try: + do_stuff() + finally: + await statsd.stop() + +The ``Connector`` instance maintains a resilient connection to the target StatsD instance, formats the metric data +into payloads, and sends them to the StatsD target. It defaults to using TCP as the transport but will use UDP if +the ``ip_protocol`` keyword is set to ``socket.IPPROTO_UDP``. The ``Connector.start`` method starts a background +``asyncio.Task`` that is responsible for maintaining the connection. The ``inject_metric`` method enqueues metric +data to send and the task consumes the internal queue when it is connected. + +Tornado helpers +=============== +The ``sprockets_statsd.mixins`` module contains mix-in classes that make reporting metrics from your tornado_ web +application simple. .. code-block:: python @@ -61,4 +97,3 @@ not connected to the server and will be sent in the order received when the task .. _statsd: https://github.com/statsd/statsd/ .. _tornado: https://tornadoweb.org/ - diff --git a/setup.cfg b/setup.cfg index fb9fc61..75886e8 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,7 +1,7 @@ [metadata] name = sprockets-statsd version = attr: sprockets_statsd.version -description = Asynchronous Statsd connector. +description = Asynchronously send metrics to a statsd instance. long_description = file: README.rst license = BSD 3-Clause License url = https://sprockets-statsd.readthedocs.io/ diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py index 2547e45..43c1d34 100644 --- a/sprockets_statsd/statsd.py +++ b/sprockets_statsd/statsd.py @@ -1,5 +1,6 @@ import asyncio import logging +import socket import typing @@ -7,11 +8,14 @@ class Connector: """Sends metrics to a statsd server. :param host: statsd server to send metrics to - :param port: TCP port that the server is listening on + :param port: socket port that the server is listening on + :keyword ip_protocol: IP protocol to use for the underlying + socket -- either :data:`socket.IPPROTO_TCP` for TCP or + :data:`socket.IPPROTO_UDP` for UDP sockets. :param kwargs: additional keyword parameters are passed to the :class:`.Processor` initializer - This class maintains a TCP connection to a statsd server and + This class maintains a connection to a statsd server and sends metric lines to it asynchronously. You must call the :meth:`start` method when your application is starting. It creates a :class:`~asyncio.Task` that manages the connection @@ -165,6 +169,19 @@ class TCPProtocol(StatsdProtocol, asyncio.Protocol): await asyncio.sleep(0.1) +class UDPProtocol(StatsdProtocol, asyncio.DatagramProtocol): + """StatsdProtocol implementation over a UDP/IP connection.""" + transport: asyncio.DatagramTransport + + def send(self, metric: bytes) -> None: + if metric: + self.transport.sendto(metric) + + async def shutdown(self) -> None: + self.logger.info('shutting down') + self.transport.close() + + class Processor: """Maintains the statsd connection and sends metric payloads. @@ -222,12 +239,16 @@ class Processor: """ protocol: typing.Union[StatsdProtocol, None] + _create_transport: typing.Callable[[], typing.Coroutine[ + typing.Any, typing.Any, typing.Tuple[asyncio.BaseTransport, + StatsdProtocol]]] def __init__(self, *, host, port: int = 8125, reconnect_sleep: float = 1.0, + ip_protocol: int = socket.IPPROTO_TCP, wait_timeout: float = 0.1): super().__init__() if not host: @@ -237,11 +258,21 @@ class Processor: if not port or port < 1: raise RuntimeError( f'port must be a positive integer: {port!r}') - except TypeError: + except (TypeError, ValueError): raise RuntimeError(f'port must be a positive integer: {port!r}') + transport_creators = { + socket.IPPROTO_TCP: self._create_tcp_transport, + socket.IPPROTO_UDP: self._create_udp_transport, + } + try: + self._create_transport = transport_creators[ip_protocol] + except KeyError: + raise RuntimeError(f'ip_protocol {ip_protocol} is not supported') + self.host = host self.port = port + self._ip_protocol = ip_protocol self._reconnect_sleep = reconnect_sleep self._wait_timeout = wait_timeout @@ -274,7 +305,10 @@ class Processor: await self._process_metric() except asyncio.CancelledError: self.logger.info('task cancelled, exiting') - break + self.should_terminate = True + except Exception as error: + self.logger.exception('unexpected error occurred: %s', error) + self.should_terminate = True self.should_terminate = True self.logger.info('loop finished with %d metrics in the queue', @@ -303,6 +337,20 @@ class Processor: self.should_terminate = True await self.stopped.wait() + async def _create_tcp_transport( + self) -> typing.Tuple[asyncio.BaseTransport, StatsdProtocol]: + t, p = await asyncio.get_running_loop().create_connection( + protocol_factory=TCPProtocol, host=self.host, port=self.port) + return t, typing.cast(StatsdProtocol, p) + + async def _create_udp_transport( + self) -> typing.Tuple[asyncio.BaseTransport, StatsdProtocol]: + t, p = await asyncio.get_running_loop().create_datagram_endpoint( + protocol_factory=UDPProtocol, + remote_addr=(self.host, self.port), + reuse_port=True) + return t, typing.cast(StatsdProtocol, p) + async def _connect_if_necessary(self): if self.protocol is not None: try: @@ -316,12 +364,7 @@ class Processor: buffered_data = b'' if self.protocol is not None: buffered_data = self.protocol.buffered_data - loop = asyncio.get_running_loop() - transport, protocol = await loop.create_connection( - protocol_factory=TCPProtocol, - host=self.host, - port=self.port) - self.protocol = typing.cast(TCPProtocol, protocol) + transport, self.protocol = await self._create_transport() self.protocol.buffered_data = buffered_data self.logger.info('connection established to %s', transport.get_extra_info('peername')) @@ -341,8 +384,4 @@ class Processor: # it has queued metrics to send metric = b'' - try: - self.protocol.send(metric) - except Exception as error: - self.logger.exception('exception occurred when sending metric: %s', - error) + self.protocol.send(metric) diff --git a/tests/helpers.py b/tests/helpers.py index f32ed86..35e08a2 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -1,15 +1,23 @@ import asyncio import io +import socket import typing -class StatsdServer(asyncio.Protocol): - metrics: typing.List[bytes] +class SupportsClose(typing.Protocol): + def close(self) -> None: + ... - def __init__(self): - self.service = None + +class StatsdServer(asyncio.DatagramProtocol, asyncio.Protocol): + metrics: typing.List[bytes] + server: typing.Union[SupportsClose, None] + + def __init__(self, ip_protocol): + self.server = None self.host = '127.0.0.1' self.port = 0 + self.ip_protocol = ip_protocol self.connections_made = 0 self.connections_lost = 0 self.message_counter = 0 @@ -26,25 +34,41 @@ class StatsdServer(asyncio.Protocol): await self._reset() loop = asyncio.get_running_loop() - self.service = await loop.create_server(lambda: self, - self.host, - self.port, - reuse_port=True) - listening_sock = self.service.sockets[0] - self.host, self.port = listening_sock.getsockname() - self.running.set() - try: - await self.service.serve_forever() - self.running.clear() - except asyncio.CancelledError: - self.close() - await self.service.wait_closed() - except Exception as error: - raise error + if self.ip_protocol == socket.IPPROTO_TCP: + server = await loop.create_server(lambda: self, + self.host, + self.port, + reuse_port=True) + self.server = server + listening_sock = server.sockets[0] + self.host, self.port = listening_sock.getsockname() + self.running.set() + try: + await server.serve_forever() + except asyncio.CancelledError: + self.close() + await server.wait_closed() + except Exception as error: + raise error + finally: + self.running.clear() + + elif self.ip_protocol == socket.IPPROTO_UDP: + transport, protocol = await loop.create_datagram_endpoint( + lambda: self, + local_addr=(self.host, self.port), + reuse_port=True) + self.server = transport + self.host, self.port = transport.get_extra_info('sockname') + self.running.set() + try: + while not transport.is_closing(): + await asyncio.sleep(0.1) + finally: + self.running.clear() def close(self): - self.running.clear() - self.service.close() + self.server.close() for connected_client in self.transports: connected_client.close() self.transports.clear() @@ -53,9 +77,6 @@ class StatsdServer(asyncio.Protocol): await self.running.wait() async def wait_closed(self): - if self.service.is_serving(): - self.close() - await self.service.wait_closed() while self.running.is_set(): await asyncio.sleep(0.1) @@ -69,6 +90,13 @@ class StatsdServer(asyncio.Protocol): def data_received(self, data: bytes): self._buffer.write(data) + self._process_buffer() + + def datagram_received(self, data: bytes, _addr): + self._buffer.write(data + b'\n') + self._process_buffer() + + def _process_buffer(self): buf = self._buffer.getvalue() if b'\n' in buf: buf_complete = buf[-1] == ord('\n') diff --git a/tests/test_mixins.py b/tests/test_mixins.py index d096470..fa581ec 100644 --- a/tests/test_mixins.py +++ b/tests/test_mixins.py @@ -1,5 +1,6 @@ import asyncio import os +import socket import time import typing @@ -133,7 +134,7 @@ class ApplicationTests(testing.AsyncTestCase): class RequestHandlerTests(testing.AsyncHTTPTestCase): def setUp(self): super().setUp() - self.statsd_server = helpers.StatsdServer() + self.statsd_server = helpers.StatsdServer(socket.IPPROTO_TCP) self.io_loop.spawn_callback(self.statsd_server.run) self.io_loop.run_sync(self.statsd_server.wait_running) diff --git a/tests/test_processor.py b/tests/test_processor.py index fcbc515..fec1bb7 100644 --- a/tests/test_processor.py +++ b/tests/test_processor.py @@ -1,5 +1,6 @@ import asyncio import logging +import socket import time import asynctest @@ -9,6 +10,8 @@ from tests import helpers class ProcessorTestCase(asynctest.TestCase): + ip_protocol: int + async def setUp(self): self.test_timeout = 5.0 super().setUp() @@ -25,16 +28,18 @@ class ProcessorTestCase(asynctest.TestCase): self.fail('future took too long to resolve') async def asyncSetUp(self): - self.statsd_server = helpers.StatsdServer() + self.statsd_server = helpers.StatsdServer(self.ip_protocol) self.statsd_task = asyncio.create_task(self.statsd_server.run()) await self.statsd_server.wait_running() async def asyncTearDown(self): - self.statsd_task.cancel() + self.statsd_server.close() await self.statsd_server.wait_closed() class ProcessorTests(ProcessorTestCase): + ip_protocol = socket.IPPROTO_TCP + async def test_that_processor_connects_and_disconnects(self): processor = statsd.Processor(host=self.statsd_server.host, port=self.statsd_server.port) @@ -108,19 +113,6 @@ class ProcessorTests(ProcessorTestCase): statsd.Processor(host=None, port=12345) self.assertIn('host', str(context.exception)) - def test_that_processor_fails_when_port_is_invalid(self): - with self.assertRaises(RuntimeError) as context: - statsd.Processor(host='localhost', port=None) - self.assertIn('port', str(context.exception)) - - with self.assertRaises(RuntimeError) as context: - statsd.Processor(host='localhost', port=0) - self.assertIn('port', str(context.exception)) - - with self.assertRaises(RuntimeError) as context: - statsd.Processor(host='localhost', port=-1) - self.assertIn('port', str(context.exception)) - async def test_starting_and_stopping_without_connecting(self): host, port = self.statsd_server.host, self.statsd_server.port self.statsd_server.close() @@ -142,20 +134,22 @@ class ProcessorTests(ProcessorTestCase): await asyncio.sleep(0.1) for record in cm.records: - if (record.exc_info is not None - and record.funcName == '_process_metric'): + if record.exc_info is not None and record.funcName == 'run': break else: - self.fail('Expected _process_metric to log exception') + self.fail('Expected run to log exception') await processor.stop() class TCPProcessingTests(ProcessorTestCase): + ip_protocol = socket.IPPROTO_TCP + async def asyncSetUp(self): await super().asyncSetUp() self.processor = statsd.Processor(host=self.statsd_server.host, - port=self.statsd_server.port) + port=self.statsd_server.port, + reconnect_sleep=0.25) asyncio.create_task(self.processor.run()) await self.wait_for(self.statsd_server.client_connected.acquire()) @@ -208,7 +202,57 @@ class TCPProcessingTests(ProcessorTestCase): await self.wait_for(self.statsd_server.message_received.acquire()) +class UDPProcessingTests(ProcessorTestCase): + ip_protocol = socket.IPPROTO_UDP + + async def asyncSetUp(self): + await super().asyncSetUp() + self.connector = statsd.Connector(host=self.statsd_server.host, + port=self.statsd_server.port, + ip_protocol=self.ip_protocol, + reconnect_sleep=0.25) + await self.connector.start() + + async def asyncTearDown(self): + await self.connector.stop() + await super().asyncTearDown() + + async def test_sending_metrics(self): + self.connector.inject_metric('counter', 1, 'c') + self.connector.inject_metric('timer', 1.0, 'ms') + await self.wait_for(self.statsd_server.message_received.acquire()) + await self.wait_for(self.statsd_server.message_received.acquire()) + + self.assertEqual(self.statsd_server.metrics[0], b'counter:1|c') + self.assertEqual(self.statsd_server.metrics[1], b'timer:1.0|ms') + + async def test_that_client_sends_to_new_server(self): + self.statsd_server.close() + await self.statsd_server.wait_closed() + + self.connector.inject_metric('should.be.lost', 1, 'c') + await asyncio.sleep(self.connector.processor._wait_timeout * 2) + + self.statsd_task = asyncio.create_task(self.statsd_server.run()) + await self.statsd_server.wait_running() + + self.connector.inject_metric('should.be.recvd', 1, 'c') + await self.wait_for(self.statsd_server.message_received.acquire()) + self.assertEqual(self.statsd_server.metrics[0], b'should.be.recvd:1|c') + + async def test_that_client_handles_socket_closure(self): + self.connector.processor.protocol.transport.close() + await self.wait_for( + asyncio.sleep(self.connector.processor._reconnect_sleep)) + + self.connector.inject_metric('should.be.recvd', 1, 'c') + await self.wait_for(self.statsd_server.message_received.acquire()) + self.assertEqual(self.statsd_server.metrics[0], b'should.be.recvd:1|c') + + class ConnectorTests(ProcessorTestCase): + ip_protocol = socket.IPPROTO_TCP + async def asyncSetUp(self): await super().asyncSetUp() self.connector = statsd.Connector(self.statsd_server.host, @@ -289,3 +333,29 @@ class ConnectorTests(ProcessorTestCase): await self.wait_for(self.statsd_server.message_received.acquire()) self.assertEqual(f'counter:{value}|c'.encode(), self.statsd_server.metrics.pop(0)) + + +class ConnectorOptionTests(ProcessorTestCase): + ip_protocol = socket.IPPROTO_TCP + + def test_protocol_values(self): + connector = statsd.Connector(host=self.statsd_server.host, + port=self.statsd_server.port) + self.assertEqual(socket.IPPROTO_TCP, connector.processor._ip_protocol) + + connector = statsd.Connector(host=self.statsd_server.host, + port=self.statsd_server.port, + ip_protocol=socket.IPPROTO_UDP) + self.assertEqual(socket.IPPROTO_UDP, connector.processor._ip_protocol) + + with self.assertRaises(RuntimeError): + statsd.Connector(host=self.statsd_server.host, + port=self.statsd_server.port, + ip_protocol=socket.IPPROTO_GRE) + + def test_invalid_port_values(self): + for port in {None, 0, -1, 'not-a-number'}: + with self.assertRaises(RuntimeError) as context: + statsd.Connector(host=self.statsd_server.host, port=port) + self.assertIn('port', str(context.exception)) + self.assertIn(repr(port), str(context.exception)) From c634c2090641a9fee5fb23ee43e1bd5ed548e854 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Sun, 21 Mar 2021 17:45:23 -0400 Subject: [PATCH 16/31] Expose UDP support in the mixins. --- docs/index.rst | 7 +++- sprockets_statsd/mixins.py | 18 +++++++++ sprockets_statsd/statsd.py | 7 ++-- tests/test_mixins.py | 83 +++++++++++++++++++++++++++++--------- 4 files changed, 93 insertions(+), 22 deletions(-) diff --git a/docs/index.rst b/docs/index.rst index b359fc3..c78a2b8 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -17,7 +17,12 @@ the following environment variables. The TCP port number that the StatsD server is listening on. This defaults to 8125 if it is not configured. -You can fine tune the metric payloads and the connector by setting additional values in the ``stats`` key of +.. envvar:: STATSD_PROTOCOL + + The IP protocol to use when connecting to the StatsD server. You can specify either "tcp" or "udp". The + default is "tcp" if it not not configured. + +You can fine tune the metric payloads and the connector by setting additional values in the ``statsd`` key of :attr:`tornado.web.Application.settings`. See the :class:`sprockets_statsd.mixins.Application` class documentation for a description of the supported settings. diff --git a/sprockets_statsd/mixins.py b/sprockets_statsd/mixins.py index 4a4c593..e4c899e 100644 --- a/sprockets_statsd/mixins.py +++ b/sprockets_statsd/mixins.py @@ -1,5 +1,6 @@ import contextlib import os +import socket import time from tornado import web @@ -26,6 +27,8 @@ class Application(web.Application): +-------------------+---------------------------------------------+ | prefix | segment to prefix to metrics. | +-------------------+---------------------------------------------+ + | protocol | "tcp" or "udp" | + +-------------------+---------------------------------------------+ | reconnect_timeout | number of seconds to sleep after a statsd | | | connection attempt fails | +-------------------+---------------------------------------------+ @@ -51,6 +54,10 @@ class Application(web.Application): This is a convenient way to maintain consistent metric paths when you are managing a larger number of services. + **protocol** defaults to the :envvar:`STATSD_PROTOCOL` environment + variable with a back default of "tcp" if the environment variable + is not set. + **reconnect_timeout** defaults to 1.0 seconds which limits the aggressiveness of creating new TCP connections. @@ -63,6 +70,8 @@ class Application(web.Application): statsd_settings.setdefault('host', os.environ.get('STATSD_HOST')) statsd_settings.setdefault('port', os.environ.get('STATSD_PORT', '8125')) + statsd_settings.setdefault('protocol', + os.environ.get('STATSD_PROTOCOL', 'tcp')) try: prefix = '.'.join([ @@ -98,6 +107,15 @@ class Application(web.Application): kwargs['reconnect_sleep'] = statsd_settings['reconnect_sleep'] if 'wait_timeout' in statsd_settings: kwargs['wait_timeout'] = statsd_settings['wait_timeout'] + if statsd_settings['protocol'] == 'tcp': + kwargs['ip_protocol'] = socket.IPPROTO_TCP + elif statsd_settings['protocol'] == 'udp': + kwargs['ip_protocol'] = socket.IPPROTO_UDP + else: + raise RuntimeError( + f'statsd configuration error:' + f' {statsd_settings["protocol"]} is not a valid' + f' protocol') self.statsd_connector = statsd.Connector(**kwargs) await self.statsd_connector.start() diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py index 43c1d34..ac2eabe 100644 --- a/sprockets_statsd/statsd.py +++ b/sprockets_statsd/statsd.py @@ -10,8 +10,8 @@ class Connector: :param host: statsd server to send metrics to :param port: socket port that the server is listening on :keyword ip_protocol: IP protocol to use for the underlying - socket -- either :data:`socket.IPPROTO_TCP` for TCP or - :data:`socket.IPPROTO_UDP` for UDP sockets. + socket -- either ``socket.IPPROTO_TCP`` for TCP or + ``socket.IPPROTO_UDP`` for UDP sockets. :param kwargs: additional keyword parameters are passed to the :class:`.Processor` initializer @@ -117,7 +117,8 @@ class StatsdProtocol: def connection_made(self, transport: asyncio.Transport): """Capture the new transport and set the connected event.""" - server, port = transport.get_extra_info('peername') + # NB - this will return a 4-part tuple in some cases + server, port = transport.get_extra_info('peername')[:2] self.logger.info('connected to statsd %s:%s', server, port) self.transport = transport self.connected.set() diff --git a/tests/test_mixins.py b/tests/test_mixins.py index fa581ec..e472b76 100644 --- a/tests/test_mixins.py +++ b/tests/test_mixins.py @@ -25,7 +25,18 @@ class Application(sprockets_statsd.mixins.Application, web.Application): super().__init__([web.url('/', Handler)], **settings) -class ApplicationTests(testing.AsyncTestCase): +class AsyncTestCaseWithTimeout(testing.AsyncTestCase): + def run_coroutine(self, coro): + loop: asyncio.AbstractEventLoop = self.io_loop.asyncio_loop + try: + loop.run_until_complete( + asyncio.wait_for(coro, + timeout=testing.get_async_test_timeout())) + except asyncio.TimeoutError: + self.fail(f'coroutine {coro} took too long to complete') + + +class ApplicationTests(AsyncTestCaseWithTimeout): def setUp(self): super().setUp() self._environ = {} @@ -40,6 +51,7 @@ class ApplicationTests(testing.AsyncTestCase): def test_statsd_setting_defaults(self): self.unsetenv('STATSD_HOST') self.unsetenv('STATSD_PORT') + self.unsetenv('STATSD_PROTOCOL') app = sprockets_statsd.mixins.Application() self.assertIn('statsd', app.settings) @@ -47,15 +59,18 @@ class ApplicationTests(testing.AsyncTestCase): 'default host value should be None') self.assertEqual(8125, app.settings['statsd']['port']) self.assertEqual(None, app.settings['statsd']['prefix']) + self.assertEqual('tcp', app.settings['statsd']['protocol']) def test_that_statsd_settings_read_from_environment(self): self.setenv('STATSD_HOST', 'statsd') self.setenv('STATSD_PORT', '5218') + self.setenv('STATSD_PROTOCOL', 'udp') app = sprockets_statsd.mixins.Application() self.assertIn('statsd', app.settings) self.assertEqual('statsd', app.settings['statsd']['host']) self.assertEqual(5218, app.settings['statsd']['port']) + self.assertEqual('udp', app.settings['statsd']['protocol']) def test_prefix_when_only_service_is_set(self): app = sprockets_statsd.mixins.Application(service='blah') @@ -77,20 +92,24 @@ class ApplicationTests(testing.AsyncTestCase): def test_overridden_settings(self): self.setenv('STATSD_HOST', 'statsd') self.setenv('STATSD_PORT', '9999') - app = sprockets_statsd.mixins.Application(statsd={ - 'host': 'statsd.example.com', - 'port': 5218, - 'prefix': 'myapp', - }) + self.setenv('STATSD_PROTOCOL', 'tcp') + app = sprockets_statsd.mixins.Application( + statsd={ + 'host': 'statsd.example.com', + 'port': 5218, + 'prefix': 'myapp', + 'protocol': 'udp', + }) self.assertEqual('statsd.example.com', app.settings['statsd']['host']) self.assertEqual(5218, app.settings['statsd']['port']) self.assertEqual('myapp', app.settings['statsd']['prefix']) + self.assertEqual('udp', app.settings['statsd']['protocol']) def test_that_starting_without_configuration_fails(self): self.unsetenv('STATSD_HOST') app = sprockets_statsd.mixins.Application() with self.assertRaises(RuntimeError): - self.io_loop.run_sync(app.start_statsd) + self.run_coroutine(app.start_statsd()) def test_starting_twice(self): app = sprockets_statsd.mixins.Application(statsd={ @@ -98,22 +117,22 @@ class ApplicationTests(testing.AsyncTestCase): 'port': '8125', }) try: - self.io_loop.run_sync(app.start_statsd) + self.run_coroutine(app.start_statsd()) connector = app.statsd_connector self.assertIsNotNone(connector, 'statsd.Connector not created') - self.io_loop.run_sync(app.start_statsd) + self.run_coroutine(app.start_statsd()) self.assertIs(app.statsd_connector, connector, 'statsd.Connector should not be recreated') finally: - self.io_loop.run_sync(app.stop_statsd) + self.run_coroutine(app.stop_statsd()) def test_stopping_without_starting(self): app = sprockets_statsd.mixins.Application(statsd={ 'host': 'localhost', 'port': '8125', }) - self.io_loop.run_sync(app.stop_statsd) + self.run_coroutine(app.stop_statsd()) def test_optional_parameters(self): app = sprockets_statsd.mixins.Application( @@ -123,32 +142,60 @@ class ApplicationTests(testing.AsyncTestCase): 'reconnect_sleep': 0.5, 'wait_timeout': 0.25, }) - self.io_loop.run_sync(app.start_statsd) + self.run_coroutine(app.start_statsd()) processor = app.statsd_connector.processor self.assertEqual(0.5, processor._reconnect_sleep) self.assertEqual(0.25, processor._wait_timeout) - self.io_loop.run_sync(app.stop_statsd) + self.run_coroutine(app.stop_statsd()) + + def test_starting_with_invalid_protocol(self): + app = sprockets_statsd.mixins.Application(statsd={ + 'host': 'localhost', + 'protocol': 'unknown' + }) + with self.assertRaises(RuntimeError): + self.run_coroutine(app.start_statsd()) + + def test_that_protocol_strings_are_translated(self): + app = sprockets_statsd.mixins.Application(statsd={ + 'host': 'localhost', + 'protocol': 'tcp', + }) + self.run_coroutine(app.start_statsd()) + self.assertEqual(socket.IPPROTO_TCP, + app.statsd_connector.processor._ip_protocol) + self.run_coroutine(app.stop_statsd()) + + app = sprockets_statsd.mixins.Application(statsd={ + 'host': 'localhost', + 'protocol': 'udp', + }) + self.run_coroutine(app.start_statsd()) + self.assertEqual(socket.IPPROTO_UDP, + app.statsd_connector.processor._ip_protocol) + self.run_coroutine(app.stop_statsd()) -class RequestHandlerTests(testing.AsyncHTTPTestCase): +class RequestHandlerTests(AsyncTestCaseWithTimeout, testing.AsyncHTTPTestCase): def setUp(self): super().setUp() self.statsd_server = helpers.StatsdServer(socket.IPPROTO_TCP) self.io_loop.spawn_callback(self.statsd_server.run) - self.io_loop.run_sync(self.statsd_server.wait_running) + self.run_coroutine(self.statsd_server.wait_running()) self.app.settings['statsd'].update({ 'host': self.statsd_server.host, 'port': self.statsd_server.port, 'prefix': 'applications.service', + 'protocol': 'tcp', }) - self.io_loop.run_sync(self.app.start_statsd) + self.run_coroutine(self.app.start_statsd()) def tearDown(self): - self.io_loop.run_sync(self.app.stop_statsd) + self.run_coroutine(self.app.stop_statsd()) self.statsd_server.close() - self.io_loop.run_sync(self.statsd_server.wait_closed) + self.run_coroutine(self.statsd_server.wait_closed()) super().tearDown() def get_app(self): From 05650286da72324f69a4fcf7200325016549c51e Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Tue, 23 Mar 2021 21:21:16 -0400 Subject: [PATCH 17/31] Remove usage of typing.Protocol. It wasn't adding too much and it isn't available in Python 3.7. --- tests/helpers.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/helpers.py b/tests/helpers.py index 35e08a2..bab0078 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -4,14 +4,8 @@ import socket import typing -class SupportsClose(typing.Protocol): - def close(self) -> None: - ... - - class StatsdServer(asyncio.DatagramProtocol, asyncio.Protocol): metrics: typing.List[bytes] - server: typing.Union[SupportsClose, None] def __init__(self, ip_protocol): self.server = None From f6017d1a06428fe252065ef8c6328512b2373742 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Tue, 23 Mar 2021 22:01:02 -0400 Subject: [PATCH 18/31] Add $STATSD_PREFIX support. --- docs/index.rst | 5 +++++ sprockets_statsd/mixins.py | 21 ++++++++++++--------- tests/test_mixins.py | 4 ++++ 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/docs/index.rst b/docs/index.rst index c78a2b8..c85cc6c 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -17,6 +17,11 @@ the following environment variables. The TCP port number that the StatsD server is listening on. This defaults to 8125 if it is not configured. +.. envvar:: STATSD_PREFIX + + Optional prefix to use for metric paths. See the documentation for :class:`~sprockets_statsd.mixins.Application` + for addition notes on setting the path prefix. + .. envvar:: STATSD_PROTOCOL The IP protocol to use when connecting to the StatsD server. You can specify either "tcp" or "udp". The diff --git a/sprockets_statsd/mixins.py b/sprockets_statsd/mixins.py index e4c899e..2c0989a 100644 --- a/sprockets_statsd/mixins.py +++ b/sprockets_statsd/mixins.py @@ -73,15 +73,18 @@ class Application(web.Application): statsd_settings.setdefault('protocol', os.environ.get('STATSD_PROTOCOL', 'tcp')) - try: - prefix = '.'.join([ - 'applications', - settings['service'], - settings['environment'], - ]) - except KeyError: - prefix = None - statsd_settings.setdefault('prefix', prefix) + if os.environ.get('STATSD_PREFIX'): + statsd_settings.setdefault('prefix', os.environ['STATSD_PREFIX']) + else: + try: + prefix = '.'.join([ + 'applications', + settings['service'], + settings['environment'], + ]) + except KeyError: + prefix = None + statsd_settings.setdefault('prefix', prefix) super().__init__(*args, **settings) diff --git a/tests/test_mixins.py b/tests/test_mixins.py index e472b76..5155667 100644 --- a/tests/test_mixins.py +++ b/tests/test_mixins.py @@ -51,6 +51,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): def test_statsd_setting_defaults(self): self.unsetenv('STATSD_HOST') self.unsetenv('STATSD_PORT') + self.unsetenv('STATSD_PREFIX') self.unsetenv('STATSD_PROTOCOL') app = sprockets_statsd.mixins.Application() @@ -64,12 +65,14 @@ class ApplicationTests(AsyncTestCaseWithTimeout): def test_that_statsd_settings_read_from_environment(self): self.setenv('STATSD_HOST', 'statsd') self.setenv('STATSD_PORT', '5218') + self.setenv('STATSD_PREFIX', 'my-service') self.setenv('STATSD_PROTOCOL', 'udp') app = sprockets_statsd.mixins.Application() self.assertIn('statsd', app.settings) self.assertEqual('statsd', app.settings['statsd']['host']) self.assertEqual(5218, app.settings['statsd']['port']) + self.assertEqual('my-service', app.settings['statsd']['prefix']) self.assertEqual('udp', app.settings['statsd']['protocol']) def test_prefix_when_only_service_is_set(self): @@ -92,6 +95,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): def test_overridden_settings(self): self.setenv('STATSD_HOST', 'statsd') self.setenv('STATSD_PORT', '9999') + self.setenv('STATSD_PREFIX', 'service') self.setenv('STATSD_PROTOCOL', 'tcp') app = sprockets_statsd.mixins.Application( statsd={ From 9a8f23c6144ac034cb8cb42c9dd6252a6e20ffc4 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Tue, 23 Mar 2021 22:01:27 -0400 Subject: [PATCH 19/31] Require prefix unless allow_no_prefix is set. --- sprockets_statsd/mixins.py | 23 ++++++++++++++----- tests/test_mixins.py | 45 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 6 deletions(-) diff --git a/sprockets_statsd/mixins.py b/sprockets_statsd/mixins.py index 2c0989a..a21635a 100644 --- a/sprockets_statsd/mixins.py +++ b/sprockets_statsd/mixins.py @@ -25,7 +25,7 @@ class Application(web.Application): +-------------------+---------------------------------------------+ | port | port number that statsd is listening on | +-------------------+---------------------------------------------+ - | prefix | segment to prefix to metrics. | + | prefix | segment to prefix to metrics | +-------------------+---------------------------------------------+ | protocol | "tcp" or "udp" | +-------------------+---------------------------------------------+ @@ -47,12 +47,18 @@ class Application(web.Application): **prefix** is prefixed to all metric paths. This provides a namespace for metrics so that each applications metrics are maintained - in separate buckets. + in separate buckets. The default is to use the :envvar:`STATSD_PREFIX` + environment variable. If it is unset and the *service* and + *environment* keys are set in ``settings``, then the default is + ``applications..``. This is a convenient way to + maintain consistent metric paths when you are managing a larger number + of services. - If the *service* and *environment* keys are set in ``settings``, - then the default prefix is ``applications..``. - This is a convenient way to maintain consistent metric paths when - you are managing a larger number of services. + .. rubric:: Warning + + If you want to run without a prefix, then you are required to also + set the ``allow_no_prefix`` key with a *truthy* value. This prevents + accidentilly polluting the metric namespace with unqualified paths. **protocol** defaults to the :envvar:`STATSD_PROTOCOL` environment variable with a back default of "tcp" if the environment variable @@ -102,6 +108,11 @@ class Application(web.Application): """ if self.statsd_connector is None: statsd_settings = self.settings['statsd'] + if (statsd_settings.get('prefix', None) is None + and not statsd_settings.get('allow_no_prefix', False)): + raise RuntimeError( + 'statsd configuration error: prefix is not set') + kwargs = { 'host': statsd_settings['host'], 'port': statsd_settings['port'], diff --git a/tests/test_mixins.py b/tests/test_mixins.py index 5155667..f0955ca 100644 --- a/tests/test_mixins.py +++ b/tests/test_mixins.py @@ -115,10 +115,51 @@ class ApplicationTests(AsyncTestCaseWithTimeout): with self.assertRaises(RuntimeError): self.run_coroutine(app.start_statsd()) + def test_that_starting_without_prefix_fails_by_default(self): + self.unsetenv('STATSD_PREFIX') + app = sprockets_statsd.mixins.Application(statsd={ + 'host': 'statsd.example.com', + 'protocol': 'udp', + }) + with self.assertRaises(RuntimeError) as cm: + self.run_coroutine(app.start_statsd()) + self.assertTrue('prefix is not set' in str(cm.exception), + 'Expected "prefix is not set" in exception message') + + def test_starting_without_prefix_on_purpose(self): + self.unsetenv('STATSD_PREFIX') + app = sprockets_statsd.mixins.Application( + statsd={ + 'allow_no_prefix': True, + 'host': 'statsd.example.com', + 'protocol': 'udp', + }) + try: + self.run_coroutine(app.start_statsd()) + finally: + self.run_coroutine(app.stop_statsd()) + + def test_starting_with_calculated_prefix(self): + self.unsetenv('STATSD_PREFIX') + app = sprockets_statsd.mixins.Application( + environment='development', + service='my-service', + statsd={ + 'host': 'statsd.example.com', + 'protocol': 'udp', + }) + try: + self.run_coroutine(app.start_statsd()) + self.assertEqual('applications.my-service.development', + app.settings['statsd']['prefix']) + finally: + self.run_coroutine(app.stop_statsd()) + def test_starting_twice(self): app = sprockets_statsd.mixins.Application(statsd={ 'host': 'localhost', 'port': '8125', + 'prefix': 'my-service', }) try: self.run_coroutine(app.start_statsd()) @@ -143,6 +184,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): statsd={ 'host': 'localhost', 'port': '8125', + 'prefix': 'my-service', 'reconnect_sleep': 0.5, 'wait_timeout': 0.25, }) @@ -156,6 +198,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): def test_starting_with_invalid_protocol(self): app = sprockets_statsd.mixins.Application(statsd={ 'host': 'localhost', + 'prefix': 'my-service', 'protocol': 'unknown' }) with self.assertRaises(RuntimeError): @@ -164,6 +207,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): def test_that_protocol_strings_are_translated(self): app = sprockets_statsd.mixins.Application(statsd={ 'host': 'localhost', + 'prefix': 'my-service', 'protocol': 'tcp', }) self.run_coroutine(app.start_statsd()) @@ -173,6 +217,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): app = sprockets_statsd.mixins.Application(statsd={ 'host': 'localhost', + 'prefix': 'my-service', 'protocol': 'udp', }) self.run_coroutine(app.start_statsd()) From 7ee536e99fd9f8491a0185ddb6ddff71c6c4315c Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Wed, 24 Mar 2021 06:30:26 -0400 Subject: [PATCH 20/31] Fix test error. --- tests/test_mixins.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/test_mixins.py b/tests/test_mixins.py index f0955ca..6e86346 100644 --- a/tests/test_mixins.py +++ b/tests/test_mixins.py @@ -41,6 +41,14 @@ class ApplicationTests(AsyncTestCaseWithTimeout): super().setUp() self._environ = {} + def tearDown(self): + super().tearDown() + for name, value in self._environ.items(): + if value is not None: + os.environ[name] = value + else: + os.environ.pop(name, None) + def setenv(self, name, value): self._environ.setdefault(name, os.environ.pop(name, None)) os.environ[name] = value From 2aeddc8bc3b3503c6df2f4c7855a3d912d468905 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Wed, 24 Mar 2021 06:48:25 -0400 Subject: [PATCH 21/31] Rename mixins module to tornado. Tried to make it clear that tornado is an optional requirement. --- MANIFEST.in | 1 + README.rst | 19 ++++++---- docs/index.rst | 18 +++++----- example.py | 6 ++-- setup.cfg | 4 ++- sprockets_statsd/{mixins.py => tornado.py} | 0 tests/{test_mixins.py => test_tornado.py} | 40 +++++++++++----------- tox.ini | 2 +- 8 files changed, 49 insertions(+), 41 deletions(-) rename sprockets_statsd/{mixins.py => tornado.py} (100%) rename tests/{test_mixins.py => test_tornado.py} (90%) diff --git a/MANIFEST.in b/MANIFEST.in index ab53868..d2b04be 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -4,3 +4,4 @@ graft tests include CHANGELOG.rst include example.py include LICENSE +include tox.ini diff --git a/README.rst b/README.rst index 1033fd0..c5aa4a4 100644 --- a/README.rst +++ b/README.rst @@ -33,8 +33,9 @@ data to send and the task consumes the internal queue when it is connected. Tornado helpers =============== -The ``sprockets_statsd.mixins`` module contains mix-in classes that make reporting metrics from your tornado_ web -application simple. +The ``sprockets_statsd.tornado`` module contains mix-in classes that make reporting metrics from your tornado_ web +application simple. You will need to install the ``sprockets_statsd[tornado]`` mix-in to ensure that the Tornado +requirements for this library are met. .. code-block:: python @@ -43,10 +44,10 @@ application simple. from tornado import ioloop, web - import sprockets_statsd.mixins + import sprockets_statsd.tornado - class MyHandler(sprockets_statsd.mixins.RequestHandler, + class MyHandler(sprockets_statsd.tornado.RequestHandler, web.RequestHandler): async def get(self): with self.execution_timer('some-operation'): @@ -57,8 +58,12 @@ application simple. await asyncio.sleep(1) - class Application(sprockets_statsd.mixins.Application, web.Application): + class Application(sprockets_statsd.tornado.Application, web.Application): def __init__(self, **settings): + settings['statsd'] = { + 'host': os.environ['STATSD_HOST'], + 'prefix': 'applications.my-service', + } super().__init__([web.url('/', MyHandler)], **settings) async def on_start(self): @@ -83,8 +88,8 @@ application simple. This application will emit two timing metrics each time that the endpoint is invoked:: - applications.timers.some-operation:1001.3449192047119|ms - applications.timers.MyHandler.GET.204:1002.4960041046143|ms + applications.my-service.timers.some-operation:1001.3449192047119|ms + applications.my-service.timers.MyHandler.GET.204:1002.4960041046143|ms You will need to set the ``$STATSD_HOST`` environment variable to enable the statsd processing inside of the application. The ``RequestHandler`` class exposes methods that send counter and timing metrics to a statsd server. diff --git a/docs/index.rst b/docs/index.rst index c85cc6c..3e52b51 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -19,17 +19,17 @@ the following environment variables. .. envvar:: STATSD_PREFIX - Optional prefix to use for metric paths. See the documentation for :class:`~sprockets_statsd.mixins.Application` - for addition notes on setting the path prefix. + Optional prefix to use for metric paths. See the documentation for :class:`~sprockets_statsd.tornado.Application` + for addition notes on setting the path prefix when using the Tornado helpers. .. envvar:: STATSD_PROTOCOL The IP protocol to use when connecting to the StatsD server. You can specify either "tcp" or "udp". The default is "tcp" if it not not configured. -You can fine tune the metric payloads and the connector by setting additional values in the ``statsd`` key of -:attr:`tornado.web.Application.settings`. See the :class:`sprockets_statsd.mixins.Application` class -documentation for a description of the supported settings. +If you are using the Tornado helper clases, then you can fine tune the metric payloads and the connector by +setting additional values in the ``statsd`` key of :attr:`tornado.web.Application.settings`. See the +:class:`sprockets_statsd.tornado.Application` class documentation for a description of the supported settings. Reference ========= @@ -37,12 +37,12 @@ Reference .. autoclass:: sprockets_statsd.statsd.Connector :members: -Mixin classes -------------- -.. autoclass:: sprockets_statsd.mixins.Application +Tornado helpers +--------------- +.. autoclass:: sprockets_statsd.tornado.Application :members: -.. autoclass:: sprockets_statsd.mixins.RequestHandler +.. autoclass:: sprockets_statsd.tornado.RequestHandler :members: Internals diff --git a/example.py b/example.py index 0a41df2..c5c0cfb 100644 --- a/example.py +++ b/example.py @@ -3,10 +3,10 @@ import logging from tornado import ioloop, web -import sprockets_statsd.mixins +import sprockets_statsd.tornado -class MyHandler(sprockets_statsd.mixins.RequestHandler, +class MyHandler(sprockets_statsd.tornado.RequestHandler, web.RequestHandler): async def get(self): with self.execution_timer('some-operation'): @@ -17,7 +17,7 @@ class MyHandler(sprockets_statsd.mixins.RequestHandler, await asyncio.sleep(1) -class Application(sprockets_statsd.mixins.Application, web.Application): +class Application(sprockets_statsd.tornado.Application, web.Application): def __init__(self, **settings): super().__init__([web.url('/', MyHandler)], **settings) diff --git a/setup.cfg b/setup.cfg index 75886e8..aa5752c 100644 --- a/setup.cfg +++ b/setup.cfg @@ -29,9 +29,10 @@ classifiers = [options] packages = find: install_requires = - tornado>=5 [options.extras_require] +tornado = + tornado>=5 dev = asynctest==0.13.0 coverage==5.5 @@ -39,6 +40,7 @@ dev = flake8-import-order==0.18.1 sphinx==3.5.2 sphinx-autodoc-typehints==1.11.1 + tornado>=5 yapf==0.30.0 [options.packages.find] diff --git a/sprockets_statsd/mixins.py b/sprockets_statsd/tornado.py similarity index 100% rename from sprockets_statsd/mixins.py rename to sprockets_statsd/tornado.py diff --git a/tests/test_mixins.py b/tests/test_tornado.py similarity index 90% rename from tests/test_mixins.py rename to tests/test_tornado.py index 6e86346..ad0deae 100644 --- a/tests/test_mixins.py +++ b/tests/test_tornado.py @@ -6,13 +6,13 @@ import typing from tornado import testing, web -import sprockets_statsd.mixins +import sprockets_statsd.tornado from tests import helpers ParsedMetric = typing.Tuple[str, float, str] -class Handler(sprockets_statsd.mixins.RequestHandler, web.RequestHandler): +class Handler(sprockets_statsd.tornado.RequestHandler, web.RequestHandler): async def get(self): with self.execution_timer('execution-timer'): await asyncio.sleep(0.1) @@ -20,7 +20,7 @@ class Handler(sprockets_statsd.mixins.RequestHandler, web.RequestHandler): self.write('true') -class Application(sprockets_statsd.mixins.Application, web.Application): +class Application(sprockets_statsd.tornado.Application, web.Application): def __init__(self, **settings): super().__init__([web.url('/', Handler)], **settings) @@ -62,7 +62,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): self.unsetenv('STATSD_PREFIX') self.unsetenv('STATSD_PROTOCOL') - app = sprockets_statsd.mixins.Application() + app = sprockets_statsd.tornado.Application() self.assertIn('statsd', app.settings) self.assertIsNone(app.settings['statsd']['host'], 'default host value should be None') @@ -76,7 +76,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): self.setenv('STATSD_PREFIX', 'my-service') self.setenv('STATSD_PROTOCOL', 'udp') - app = sprockets_statsd.mixins.Application() + app = sprockets_statsd.tornado.Application() self.assertIn('statsd', app.settings) self.assertEqual('statsd', app.settings['statsd']['host']) self.assertEqual(5218, app.settings['statsd']['port']) @@ -84,18 +84,18 @@ class ApplicationTests(AsyncTestCaseWithTimeout): self.assertEqual('udp', app.settings['statsd']['protocol']) def test_prefix_when_only_service_is_set(self): - app = sprockets_statsd.mixins.Application(service='blah') + app = sprockets_statsd.tornado.Application(service='blah') self.assertIn('statsd', app.settings) self.assertEqual(None, app.settings['statsd']['prefix']) def test_prefix_when_only_environment_is_set(self): - app = sprockets_statsd.mixins.Application(environment='whatever') + app = sprockets_statsd.tornado.Application(environment='whatever') self.assertIn('statsd', app.settings) self.assertEqual(None, app.settings['statsd']['prefix']) def test_prefix_default_when_service_and_environment_are_set(self): - app = sprockets_statsd.mixins.Application(environment='development', - service='my-service') + app = sprockets_statsd.tornado.Application(environment='development', + service='my-service') self.assertIn('statsd', app.settings) self.assertEqual('applications.my-service.development', app.settings['statsd']['prefix']) @@ -105,7 +105,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): self.setenv('STATSD_PORT', '9999') self.setenv('STATSD_PREFIX', 'service') self.setenv('STATSD_PROTOCOL', 'tcp') - app = sprockets_statsd.mixins.Application( + app = sprockets_statsd.tornado.Application( statsd={ 'host': 'statsd.example.com', 'port': 5218, @@ -119,13 +119,13 @@ class ApplicationTests(AsyncTestCaseWithTimeout): def test_that_starting_without_configuration_fails(self): self.unsetenv('STATSD_HOST') - app = sprockets_statsd.mixins.Application() + app = sprockets_statsd.tornado.Application() with self.assertRaises(RuntimeError): self.run_coroutine(app.start_statsd()) def test_that_starting_without_prefix_fails_by_default(self): self.unsetenv('STATSD_PREFIX') - app = sprockets_statsd.mixins.Application(statsd={ + app = sprockets_statsd.tornado.Application(statsd={ 'host': 'statsd.example.com', 'protocol': 'udp', }) @@ -136,7 +136,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): def test_starting_without_prefix_on_purpose(self): self.unsetenv('STATSD_PREFIX') - app = sprockets_statsd.mixins.Application( + app = sprockets_statsd.tornado.Application( statsd={ 'allow_no_prefix': True, 'host': 'statsd.example.com', @@ -149,7 +149,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): def test_starting_with_calculated_prefix(self): self.unsetenv('STATSD_PREFIX') - app = sprockets_statsd.mixins.Application( + app = sprockets_statsd.tornado.Application( environment='development', service='my-service', statsd={ @@ -164,7 +164,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): self.run_coroutine(app.stop_statsd()) def test_starting_twice(self): - app = sprockets_statsd.mixins.Application(statsd={ + app = sprockets_statsd.tornado.Application(statsd={ 'host': 'localhost', 'port': '8125', 'prefix': 'my-service', @@ -181,14 +181,14 @@ class ApplicationTests(AsyncTestCaseWithTimeout): self.run_coroutine(app.stop_statsd()) def test_stopping_without_starting(self): - app = sprockets_statsd.mixins.Application(statsd={ + app = sprockets_statsd.tornado.Application(statsd={ 'host': 'localhost', 'port': '8125', }) self.run_coroutine(app.stop_statsd()) def test_optional_parameters(self): - app = sprockets_statsd.mixins.Application( + app = sprockets_statsd.tornado.Application( statsd={ 'host': 'localhost', 'port': '8125', @@ -204,7 +204,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): self.run_coroutine(app.stop_statsd()) def test_starting_with_invalid_protocol(self): - app = sprockets_statsd.mixins.Application(statsd={ + app = sprockets_statsd.tornado.Application(statsd={ 'host': 'localhost', 'prefix': 'my-service', 'protocol': 'unknown' @@ -213,7 +213,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): self.run_coroutine(app.start_statsd()) def test_that_protocol_strings_are_translated(self): - app = sprockets_statsd.mixins.Application(statsd={ + app = sprockets_statsd.tornado.Application(statsd={ 'host': 'localhost', 'prefix': 'my-service', 'protocol': 'tcp', @@ -223,7 +223,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): app.statsd_connector.processor._ip_protocol) self.run_coroutine(app.stop_statsd()) - app = sprockets_statsd.mixins.Application(statsd={ + app = sprockets_statsd.tornado.Application(statsd={ 'host': 'localhost', 'prefix': 'my-service', 'protocol': 'udp', diff --git a/tox.ini b/tox.ini index e1c0671..2939a2b 100644 --- a/tox.ini +++ b/tox.ini @@ -4,7 +4,7 @@ toxworkdir = ./build/tox [testenv] deps = - .[dev] + .[dev,tornado] commands = python -m unittest From e5c343e577cfc92d9e21ef475665ea8d6aaf4021 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Wed, 24 Mar 2021 07:35:49 -0400 Subject: [PATCH 22/31] Verify 100% coverage in local tox. --- tox.ini | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tox.ini b/tox.ini index 2939a2b..3b8ffcf 100644 --- a/tox.ini +++ b/tox.ini @@ -1,12 +1,13 @@ [tox] -envlist = lint,py37,py39,tornado5 +envlist = lint,py37,py38,py39,tornado5 toxworkdir = ./build/tox [testenv] deps = .[dev,tornado] commands = - python -m unittest + coverage run -m unittest + coverage report [testenv:lint] commands = From 65df48042158bd78bee3ec83b0d1f94ba07d931b Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Wed, 24 Mar 2021 07:36:16 -0400 Subject: [PATCH 23/31] Expose a more usable interface on the `Connector`. @nvllsvm likes the interface that https://statsd.readthedocs.io/ exposes and I agree. I mimicked this interface on the lower-level Connector. I left the sprockets.mixins.metrics style on the tornado helpers for compat reasons. You can always use `self.statsd_connector.incr()` in your request handlers if you prefer the other interface. --- README.rst | 21 +++++++++++++--- sprockets_statsd/statsd.py | 47 +++++++++++++++++++++++++++++++++++ sprockets_statsd/tornado.py | 8 +++--- tests/test_processor.py | 49 ++++++++++++++++++++++++------------- 4 files changed, 100 insertions(+), 25 deletions(-) diff --git a/README.rst b/README.rst index c5aa4a4..b072e31 100644 --- a/README.rst +++ b/README.rst @@ -15,8 +15,8 @@ This library provides connectors to send metrics to a statsd_ instance using eit async def do_stuff(): start = time.time() response = make_some_http_call() - statsd.inject_metric(f'timers.http.something.{response.code}', - (time.time() - start) * 1000.0, 'ms') + statsd.timing(f'timers.http.something.{response.code}', + (time.time() - start)) async def main(): await statsd.start() @@ -28,8 +28,21 @@ This library provides connectors to send metrics to a statsd_ instance using eit The ``Connector`` instance maintains a resilient connection to the target StatsD instance, formats the metric data into payloads, and sends them to the StatsD target. It defaults to using TCP as the transport but will use UDP if the ``ip_protocol`` keyword is set to ``socket.IPPROTO_UDP``. The ``Connector.start`` method starts a background -``asyncio.Task`` that is responsible for maintaining the connection. The ``inject_metric`` method enqueues metric -data to send and the task consumes the internal queue when it is connected. +``asyncio.Task`` that is responsible for maintaining the connection. The ``timing`` method enqueues a timing +metric to send and the task consumes the internal queue when it is connected. + +The following convenience methods are available. You can also call ``inject_metric`` for complete control over +the payload. + ++--------------+--------------------------------------+ +| ``incr`` | Increment a counter metric | ++--------------+--------------------------------------+ +| ``decr`` | Decrement a counter metric | ++--------------+--------------------------------------+ +| ``gauge`` | Adjust or set a gauge metric | ++--------------+--------------------------------------+ +| ``timing`` | Append a duration to a timer metric | ++--------------+--------------------------------------+ Tornado helpers =============== diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py index ac2eabe..1ef1957 100644 --- a/sprockets_statsd/statsd.py +++ b/sprockets_statsd/statsd.py @@ -60,6 +60,53 @@ class Connector: """ await self.processor.stop() + def incr(self, path: str, value: int = 1): + """Increment a counter metric. + + :param path: counter to increment + :param value: amount to increment the counter by + + """ + self.inject_metric(path, str(value), 'c') + + def decr(self, path: str, value: int = 1): + """Decrement a counter metric. + + :param path: counter to decrement + :param value: amount to decrement the counter by + + This is equivalent to ``self.incr(path, -value)``. + + """ + self.inject_metric(path, str(-value), 'c') + + def gauge(self, path: str, value: int, delta: bool = False): + """Manipulate a gauge metric. + + :param path: gauge to adjust + :param value: value to send + :param delta: is this an adjustment of the gauge? + + If the `delta` parameter is ``False`` (or omitted), then + `value` is the new value to set the gauge to. Otherwise, + `value` is an adjustment for the current gauge. + + """ + if delta: + value = f'{value:+d}' + else: + value = str(value) + self.inject_metric(path, value, 'g') + + def timing(self, path: str, seconds: float): + """Send a timer metric. + + :param path: timer to append a value to + :param seconds: number of **seconds** to record + + """ + self.inject_metric(path, seconds * 1000.0, 'ms') + def inject_metric(self, path: str, value, type_code: str): """Send a metric to the statsd server. diff --git a/sprockets_statsd/tornado.py b/sprockets_statsd/tornado.py index a21635a..56ab888 100644 --- a/sprockets_statsd/tornado.py +++ b/sprockets_statsd/tornado.py @@ -170,8 +170,8 @@ class RequestHandler(web.RequestHandler): """ if self.statsd_connector is not None: - self.statsd_connector.inject_metric( - self.__build_path('timers', *path), secs * 1000.0, 'ms') + self.statsd_connector.timing(self.__build_path('timers', *path), + secs) def increase_counter(self, *path, amount: int = 1): """Adjust a counter. @@ -182,8 +182,8 @@ class RequestHandler(web.RequestHandler): """ if self.statsd_connector is not None: - self.statsd_connector.inject_metric( - self.__build_path('counters', *path), amount, 'c') + self.statsd_connector.incr(self.__build_path('counters', *path), + amount) @contextlib.contextmanager def execution_timer(self, *path): diff --git a/tests/test_processor.py b/tests/test_processor.py index fec1bb7..59df78f 100644 --- a/tests/test_processor.py +++ b/tests/test_processor.py @@ -218,8 +218,8 @@ class UDPProcessingTests(ProcessorTestCase): await super().asyncTearDown() async def test_sending_metrics(self): - self.connector.inject_metric('counter', 1, 'c') - self.connector.inject_metric('timer', 1.0, 'ms') + self.connector.incr('counter') + self.connector.timing('timer', 0.001) await self.wait_for(self.statsd_server.message_received.acquire()) await self.wait_for(self.statsd_server.message_received.acquire()) @@ -230,13 +230,13 @@ class UDPProcessingTests(ProcessorTestCase): self.statsd_server.close() await self.statsd_server.wait_closed() - self.connector.inject_metric('should.be.lost', 1, 'c') + self.connector.incr('should.be.lost') await asyncio.sleep(self.connector.processor._wait_timeout * 2) self.statsd_task = asyncio.create_task(self.statsd_server.run()) await self.statsd_server.wait_running() - self.connector.inject_metric('should.be.recvd', 1, 'c') + self.connector.incr('should.be.recvd') await self.wait_for(self.statsd_server.message_received.acquire()) self.assertEqual(self.statsd_server.metrics[0], b'should.be.recvd:1|c') @@ -245,7 +245,7 @@ class UDPProcessingTests(ProcessorTestCase): await self.wait_for( asyncio.sleep(self.connector.processor._reconnect_sleep)) - self.connector.inject_metric('should.be.recvd', 1, 'c') + self.connector.incr('should.be.recvd') await self.wait_for(self.statsd_server.message_received.acquire()) self.assertEqual(self.statsd_server.metrics[0], b'should.be.recvd:1|c') @@ -272,16 +272,31 @@ class ConnectorTests(ProcessorTestCase): self.assertEqual(recvd_value, str(value), 'metric value mismatch') self.assertEqual(recvd_code, type_code, 'metric type mismatch') - async def test_sending_simple_counter(self): - self.connector.inject_metric('simple.counter', 1000, 'c') + async def test_adjusting_counter(self): + self.connector.incr('simple.counter') await self.wait_for(self.statsd_server.message_received.acquire()) - self.assert_metrics_equal(self.statsd_server.metrics[0], - 'simple.counter', 1000, 'c') + self.assert_metrics_equal(self.statsd_server.metrics[-1], + 'simple.counter', 1, 'c') + + self.connector.incr('simple.counter', 10) + await self.wait_for(self.statsd_server.message_received.acquire()) + self.assert_metrics_equal(self.statsd_server.metrics[-1], + 'simple.counter', 10, 'c') + + self.connector.decr('simple.counter') + await self.wait_for(self.statsd_server.message_received.acquire()) + self.assert_metrics_equal(self.statsd_server.metrics[-1], + 'simple.counter', -1, 'c') + + self.connector.decr('simple.counter', 10) + await self.wait_for(self.statsd_server.message_received.acquire()) + self.assert_metrics_equal(self.statsd_server.metrics[-1], + 'simple.counter', -10, 'c') async def test_adjusting_gauge(self): - self.connector.inject_metric('simple.gauge', 100, 'g') - self.connector.inject_metric('simple.gauge', -10, 'g') - self.connector.inject_metric('simple.gauge', '+10', 'g') + self.connector.gauge('simple.gauge', 100) + self.connector.gauge('simple.gauge', -10, delta=True) + self.connector.gauge('simple.gauge', 10, delta=True) for _ in range(3): await self.wait_for(self.statsd_server.message_received.acquire()) @@ -294,7 +309,7 @@ class ConnectorTests(ProcessorTestCase): async def test_sending_timer(self): secs = 12.34 - self.connector.inject_metric('simple.timer', secs * 1000.0, 'ms') + self.connector.timing('simple.timer', secs) await self.wait_for(self.statsd_server.message_received.acquire()) self.assert_metrics_equal(self.statsd_server.metrics[0], 'simple.timer', 12340.0, 'ms') @@ -309,9 +324,9 @@ class ConnectorTests(ProcessorTestCase): async def fake_process_metric(): if not self.connector.processor.should_terminate: - self.connector.inject_metric('counter', 1, 'c') - self.connector.inject_metric('counter', 2, 'c') - self.connector.inject_metric('counter', 3, 'c') + self.connector.incr('counter', 1) + self.connector.incr('counter', 2) + self.connector.incr('counter', 3) self.connector.processor.should_terminate = True return await real_process_metric() @@ -325,7 +340,7 @@ class ConnectorTests(ProcessorTestCase): await self.statsd_server.wait_closed() for value in range(50): - self.connector.inject_metric('counter', value, 'c') + self.connector.incr('counter', value) asyncio.create_task(self.statsd_server.run()) await self.wait_for(self.statsd_server.client_connected.acquire()) From b84e52592dfa902789249a0643ab20794bab0990 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Wed, 24 Mar 2021 08:10:07 -0400 Subject: [PATCH 24/31] Prefix management -- take two. Agree with @aremm here ... instead of requiring contortions to run without a prefix, let's just require that it is explicitly set to `None` if you want to skip the prefix. If you `prefix` isn't in the settings explicitly, then use `$STATSD_PREFIX` if it is set; otherwise, try to construct a prefix from service & environment, if that fails, then fail hard in the initializer. --- sprockets_statsd/tornado.py | 36 +++++++++++++---------------- tests/test_tornado.py | 45 +++++++++++++------------------------ 2 files changed, 31 insertions(+), 50 deletions(-) diff --git a/sprockets_statsd/tornado.py b/sprockets_statsd/tornado.py index 56ab888..c86e54d 100644 --- a/sprockets_statsd/tornado.py +++ b/sprockets_statsd/tornado.py @@ -56,9 +56,9 @@ class Application(web.Application): .. rubric:: Warning - If you want to run without a prefix, then you are required to also - set the ``allow_no_prefix`` key with a *truthy* value. This prevents - accidentilly polluting the metric namespace with unqualified paths. + If you want to run without a prefix, then you are required to explicitly + set ``statsd.prefix`` to ``None``. This prevents accidentally polluting + the metric namespace with unqualified paths. **protocol** defaults to the :envvar:`STATSD_PROTOCOL` environment variable with a back default of "tcp" if the environment variable @@ -79,18 +79,19 @@ class Application(web.Application): statsd_settings.setdefault('protocol', os.environ.get('STATSD_PROTOCOL', 'tcp')) - if os.environ.get('STATSD_PREFIX'): - statsd_settings.setdefault('prefix', os.environ['STATSD_PREFIX']) - else: - try: - prefix = '.'.join([ - 'applications', - settings['service'], - settings['environment'], - ]) - except KeyError: - prefix = None - statsd_settings.setdefault('prefix', prefix) + if 'prefix' not in statsd_settings: + statsd_settings['prefix'] = os.environ.get('STATSD_PREFIX') + if not statsd_settings['prefix']: + try: + statsd_settings['prefix'] = '.'.join([ + 'applications', + settings['service'], + settings['environment'], + ]) + except KeyError: + raise RuntimeError( + 'statsd configuration error: prefix is not set. Set' + ' $STATSD_PREFIX or configure settings.statsd.prefix') super().__init__(*args, **settings) @@ -108,11 +109,6 @@ class Application(web.Application): """ if self.statsd_connector is None: statsd_settings = self.settings['statsd'] - if (statsd_settings.get('prefix', None) is None - and not statsd_settings.get('allow_no_prefix', False)): - raise RuntimeError( - 'statsd configuration error: prefix is not set') - kwargs = { 'host': statsd_settings['host'], 'port': statsd_settings['port'], diff --git a/tests/test_tornado.py b/tests/test_tornado.py index ad0deae..0929caf 100644 --- a/tests/test_tornado.py +++ b/tests/test_tornado.py @@ -62,7 +62,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): self.unsetenv('STATSD_PREFIX') self.unsetenv('STATSD_PROTOCOL') - app = sprockets_statsd.tornado.Application() + app = sprockets_statsd.tornado.Application(statsd={'prefix': None}) self.assertIn('statsd', app.settings) self.assertIsNone(app.settings['statsd']['host'], 'default host value should be None') @@ -84,14 +84,12 @@ class ApplicationTests(AsyncTestCaseWithTimeout): self.assertEqual('udp', app.settings['statsd']['protocol']) def test_prefix_when_only_service_is_set(self): - app = sprockets_statsd.tornado.Application(service='blah') - self.assertIn('statsd', app.settings) - self.assertEqual(None, app.settings['statsd']['prefix']) + with self.assertRaises(RuntimeError): + sprockets_statsd.tornado.Application(service='blah') def test_prefix_when_only_environment_is_set(self): - app = sprockets_statsd.tornado.Application(environment='whatever') - self.assertIn('statsd', app.settings) - self.assertEqual(None, app.settings['statsd']['prefix']) + with self.assertRaises(RuntimeError): + sprockets_statsd.tornado.Application(environment='whatever') def test_prefix_default_when_service_and_environment_are_set(self): app = sprockets_statsd.tornado.Application(environment='development', @@ -117,35 +115,20 @@ class ApplicationTests(AsyncTestCaseWithTimeout): self.assertEqual('myapp', app.settings['statsd']['prefix']) self.assertEqual('udp', app.settings['statsd']['protocol']) - def test_that_starting_without_configuration_fails(self): + def test_that_starting_without_host_fails(self): self.unsetenv('STATSD_HOST') - app = sprockets_statsd.tornado.Application() + app = sprockets_statsd.tornado.Application(statsd={'prefix': 'app'}) with self.assertRaises(RuntimeError): self.run_coroutine(app.start_statsd()) - def test_that_starting_without_prefix_fails_by_default(self): + def test_creating_without_prefix_on_purpose(self): self.unsetenv('STATSD_PREFIX') app = sprockets_statsd.tornado.Application(statsd={ 'host': 'statsd.example.com', 'protocol': 'udp', + 'prefix': None, }) - with self.assertRaises(RuntimeError) as cm: - self.run_coroutine(app.start_statsd()) - self.assertTrue('prefix is not set' in str(cm.exception), - 'Expected "prefix is not set" in exception message') - - def test_starting_without_prefix_on_purpose(self): - self.unsetenv('STATSD_PREFIX') - app = sprockets_statsd.tornado.Application( - statsd={ - 'allow_no_prefix': True, - 'host': 'statsd.example.com', - 'protocol': 'udp', - }) - try: - self.run_coroutine(app.start_statsd()) - finally: - self.run_coroutine(app.stop_statsd()) + self.assertEqual(None, app.settings['statsd']['prefix']) def test_starting_with_calculated_prefix(self): self.unsetenv('STATSD_PREFIX') @@ -184,6 +167,7 @@ class ApplicationTests(AsyncTestCaseWithTimeout): app = sprockets_statsd.tornado.Application(statsd={ 'host': 'localhost', 'port': '8125', + 'prefix': 'my-service', }) self.run_coroutine(app.stop_statsd()) @@ -244,8 +228,6 @@ class RequestHandlerTests(AsyncTestCaseWithTimeout, testing.AsyncHTTPTestCase): self.app.settings['statsd'].update({ 'host': self.statsd_server.host, 'port': self.statsd_server.port, - 'prefix': 'applications.service', - 'protocol': 'tcp', }) self.run_coroutine(self.app.start_statsd()) @@ -256,7 +238,10 @@ class RequestHandlerTests(AsyncTestCaseWithTimeout, testing.AsyncHTTPTestCase): super().tearDown() def get_app(self): - self.app = Application() + self.app = Application(statsd={ + 'prefix': 'applications.service', + 'protocol': 'tcp', + }) return self.app def wait_for_metrics(self, metric_count=3): From 2ecdee61c4ea9fb513e84ebbd23e99f576535b01 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Fri, 26 Mar 2021 06:40:27 -0400 Subject: [PATCH 25/31] Clean up type annotations. This almost makes mypy happy. I did manage to step on a defect that there really isn't a great workaround for so I resorted to disabling typing there. https://github.com/python/mypy/issues/2427 --- setup.cfg | 15 +++++++ sprockets_statsd/statsd.py | 87 ++++++++++++++++++++++--------------- sprockets_statsd/tornado.py | 24 +++++----- tests/helpers.py | 10 +++-- tests/test_processor.py | 18 ++++---- tests/test_tornado.py | 12 ++--- 6 files changed, 102 insertions(+), 64 deletions(-) diff --git a/setup.cfg b/setup.cfg index aa5752c..ded2401 100644 --- a/setup.cfg +++ b/setup.cfg @@ -68,6 +68,21 @@ application_import_names = sprockets_statsd,tests exclude = build,env,dist import_order_style = pycharm +[mypy] +cache_dir = build/mypy-cache +check_untyped_defs = true +show_error_codes = true +warn_no_return = true +warn_redundant_casts = true +warn_unused_configs = true +warn_unused_ignores = true + +[mypy-sprockets_statsd] +disallow_incomplete_defs = true +disallow_untyped_defs = true +no_implicit_optional = true +strict = true + [yapf] allow_split_before_dict_value = false indent_dictionary_value = true diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py index 1ef1957..c2bf9b6 100644 --- a/sprockets_statsd/statsd.py +++ b/sprockets_statsd/statsd.py @@ -35,11 +35,16 @@ class Connector: sends the metric payloads. """ - def __init__(self, host: str, port: int = 8125, **kwargs): - self.processor = Processor(host=host, port=port, **kwargs) - self._processor_task = None + processor: 'Processor' - async def start(self): + def __init__(self, + host: str, + port: int = 8125, + **kwargs: typing.Any) -> None: + self.processor = Processor(host=host, port=port, **kwargs) + self._processor_task: typing.Optional[asyncio.Task[None]] = None + + async def start(self) -> None: """Start the processor in the background. This is a *blocking* method and does not return until the @@ -49,7 +54,7 @@ class Connector: self._processor_task = asyncio.create_task(self.processor.run()) await self.processor.running.wait() - async def stop(self): + async def stop(self) -> None: """Stop the background processor. Items that are currently in the queue will be flushed to @@ -60,7 +65,7 @@ class Connector: """ await self.processor.stop() - def incr(self, path: str, value: int = 1): + def incr(self, path: str, value: int = 1) -> None: """Increment a counter metric. :param path: counter to increment @@ -69,7 +74,7 @@ class Connector: """ self.inject_metric(path, str(value), 'c') - def decr(self, path: str, value: int = 1): + def decr(self, path: str, value: int = 1) -> None: """Decrement a counter metric. :param path: counter to decrement @@ -80,7 +85,7 @@ class Connector: """ self.inject_metric(path, str(-value), 'c') - def gauge(self, path: str, value: int, delta: bool = False): + def gauge(self, path: str, value: int, delta: bool = False) -> None: """Manipulate a gauge metric. :param path: gauge to adjust @@ -93,26 +98,25 @@ class Connector: """ if delta: - value = f'{value:+d}' + payload = f'{value:+d}' else: - value = str(value) - self.inject_metric(path, value, 'g') + payload = str(value) + self.inject_metric(path, payload, 'g') - def timing(self, path: str, seconds: float): + def timing(self, path: str, seconds: float) -> None: """Send a timer metric. :param path: timer to append a value to :param seconds: number of **seconds** to record """ - self.inject_metric(path, seconds * 1000.0, 'ms') + self.inject_metric(path, str(seconds * 1000.0), 'ms') - def inject_metric(self, path: str, value, type_code: str): + def inject_metric(self, path: str, value: str, type_code: str) -> None: """Send a metric to the statsd server. :param path: formatted metric name - :param value: metric value as a number or a string. The - string form is required for relative gauges. + :param value: formatted metric value :param type_code: type of the metric to send This method formats the payload and inserts it on the @@ -120,10 +124,10 @@ class Connector: """ payload = f'{path}:{value}|{type_code}' - self.processor.queue.put_nowait(payload.encode('utf-8')) + self.processor.enqueue(payload.encode('utf-8')) -class StatsdProtocol: +class StatsdProtocol(asyncio.BaseProtocol): """Common interface for backend protocols/transports. UDP and TCP transports have different interfaces (sendto vs write) @@ -145,9 +149,12 @@ class StatsdProtocol: Is the protocol currently connected? """ + buffered_data: bytes + ip_protocol: int = socket.IPPROTO_NONE logger: logging.Logger + transport: typing.Optional[asyncio.BaseTransport] - def __init__(self): + def __init__(self) -> None: self.buffered_data = b'' self.connected = asyncio.Event() self.logger = logging.getLogger(__package__).getChild( @@ -162,15 +169,16 @@ class StatsdProtocol: """Shutdown the transport and wait for it to close.""" raise NotImplementedError() - def connection_made(self, transport: asyncio.Transport): + def connection_made(self, transport: asyncio.BaseTransport) -> None: """Capture the new transport and set the connected event.""" # NB - this will return a 4-part tuple in some cases server, port = transport.get_extra_info('peername')[:2] self.logger.info('connected to statsd %s:%s', server, port) self.transport = transport + self.transport.set_protocol(self) self.connected.set() - def connection_lost(self, exc: typing.Optional[Exception]): + def connection_lost(self, exc: typing.Optional[Exception]) -> None: """Clear the connected event.""" self.logger.warning('statsd server connection lost: %s', exc) self.connected.clear() @@ -178,9 +186,10 @@ class StatsdProtocol: class TCPProtocol(StatsdProtocol, asyncio.Protocol): """StatsdProtocol implementation over a TCP/IP connection.""" + ip_protocol = socket.IPPROTO_TCP transport: asyncio.WriteTransport - def eof_received(self): + def eof_received(self) -> None: self.logger.warning('received EOF from statsd server') self.connected.clear() @@ -219,6 +228,7 @@ class TCPProtocol(StatsdProtocol, asyncio.Protocol): class UDPProtocol(StatsdProtocol, asyncio.DatagramProtocol): """StatsdProtocol implementation over a UDP/IP connection.""" + ip_protocol = socket.IPPROTO_UDP transport: asyncio.DatagramTransport def send(self, metric: bytes) -> None: @@ -286,18 +296,20 @@ class Processor: """ - protocol: typing.Union[StatsdProtocol, None] + logger: logging.Logger + protocol: typing.Optional[StatsdProtocol] + queue: asyncio.Queue[bytes] _create_transport: typing.Callable[[], typing.Coroutine[ typing.Any, typing.Any, typing.Tuple[asyncio.BaseTransport, StatsdProtocol]]] def __init__(self, *, - host, + host: str, port: int = 8125, reconnect_sleep: float = 1.0, ip_protocol: int = socket.IPPROTO_TCP, - wait_timeout: float = 0.1): + wait_timeout: float = 0.1) -> None: super().__init__() if not host: raise RuntimeError('host must be set') @@ -314,9 +326,11 @@ class Processor: socket.IPPROTO_UDP: self._create_udp_transport, } try: - self._create_transport = transport_creators[ip_protocol] + factory = transport_creators[ip_protocol] except KeyError: raise RuntimeError(f'ip_protocol {ip_protocol} is not supported') + else: + self._create_transport = factory # type: ignore self.host = host self.port = port @@ -332,16 +346,15 @@ class Processor: self.protocol = None self.queue = asyncio.Queue() - self._failed_sends = [] - @property def connected(self) -> bool: """Is the processor connected?""" - if self.protocol is None: - return False - return self.protocol.connected.is_set() + return self.protocol is not None and self.protocol.connected.is_set() - async def run(self): + def enqueue(self, metric: bytes) -> None: + self.queue.put_nowait(metric) + + async def run(self) -> None: """Maintains the connection and processes metric payloads.""" self.running.set() self.stopped.clear() @@ -374,7 +387,7 @@ class Processor: self.running.clear() self.stopped.set() - async def stop(self): + async def stop(self) -> None: """Stop the processor. This is an asynchronous but blocking method. It does not @@ -399,7 +412,7 @@ class Processor: reuse_port=True) return t, typing.cast(StatsdProtocol, p) - async def _connect_if_necessary(self): + async def _connect_if_necessary(self) -> None: if self.protocol is not None: try: await asyncio.wait_for(self.protocol.connected.wait(), @@ -412,7 +425,8 @@ class Processor: buffered_data = b'' if self.protocol is not None: buffered_data = self.protocol.buffered_data - transport, self.protocol = await self._create_transport() + t, p = await self._create_transport() # type: ignore[misc] + transport, self.protocol = t, p self.protocol.buffered_data = buffered_data self.logger.info('connection established to %s', transport.get_extra_info('peername')) @@ -421,7 +435,7 @@ class Processor: self.host, self.port, error) await asyncio.sleep(self._reconnect_sleep) - async def _process_metric(self): + async def _process_metric(self) -> None: try: metric = await asyncio.wait_for(self.queue.get(), self._wait_timeout) @@ -432,4 +446,5 @@ class Processor: # it has queued metrics to send metric = b'' + assert self.protocol is not None # AFAICT, this cannot happen self.protocol.send(metric) diff --git a/sprockets_statsd/tornado.py b/sprockets_statsd/tornado.py index c86e54d..85952ab 100644 --- a/sprockets_statsd/tornado.py +++ b/sprockets_statsd/tornado.py @@ -2,6 +2,7 @@ import contextlib import os import socket import time +import typing from tornado import web @@ -71,7 +72,9 @@ class Application(web.Application): processor quickly responds to connection faults. """ - def __init__(self, *args, **settings): + statsd_connector: typing.Optional[statsd.Connector] + + def __init__(self, *args: typing.Any, **settings: typing.Any): statsd_settings = settings.setdefault('statsd', {}) statsd_settings.setdefault('host', os.environ.get('STATSD_HOST')) statsd_settings.setdefault('port', @@ -98,7 +101,7 @@ class Application(web.Application): self.settings['statsd']['port'] = int(self.settings['statsd']['port']) self.statsd_connector = None - async def start_statsd(self): + async def start_statsd(self) -> None: """Start the connector during startup. Call this method during application startup to enable the statsd @@ -130,7 +133,7 @@ class Application(web.Application): self.statsd_connector = statsd.Connector(**kwargs) await self.statsd_connector.start() - async def stop_statsd(self): + async def stop_statsd(self) -> None: """Stop the connector during shutdown. If the connector was started, then this method will gracefully @@ -145,20 +148,20 @@ class Application(web.Application): class RequestHandler(web.RequestHandler): """Mix this into your handler to send metrics to a statsd server.""" - statsd_connector: statsd.Connector + statsd_connector: typing.Optional[statsd.Connector] - def initialize(self, **kwargs): + def initialize(self, **kwargs: typing.Any) -> None: super().initialize(**kwargs) self.application: Application self.statsd_connector = self.application.statsd_connector - def __build_path(self, *path): + def __build_path(self, *path: typing.Any) -> str: full_path = '.'.join(str(c) for c in path) if self.settings.get('statsd', {}).get('prefix', ''): return f'{self.settings["statsd"]["prefix"]}.{full_path}' return full_path - def record_timing(self, secs: float, *path): + def record_timing(self, secs: float, *path: typing.Any) -> None: """Record the duration. :param secs: number of seconds to record @@ -169,7 +172,7 @@ class RequestHandler(web.RequestHandler): self.statsd_connector.timing(self.__build_path('timers', *path), secs) - def increase_counter(self, *path, amount: int = 1): + def increase_counter(self, *path: typing.Any, amount: int = 1) -> None: """Adjust a counter. :param path: path of the counter to adjust @@ -182,7 +185,8 @@ class RequestHandler(web.RequestHandler): amount) @contextlib.contextmanager - def execution_timer(self, *path): + def execution_timer( + self, *path: typing.Any) -> typing.Generator[None, None, None]: """Record the execution duration of a block of code. :param path: path to record the duration as @@ -194,7 +198,7 @@ class RequestHandler(web.RequestHandler): finally: self.record_timing(time.time() - start, *path) - def on_finish(self): + def on_finish(self) -> None: """Extended to record the request time as a duration. This method extends :meth:`tornado.web.RequestHandler.on_finish` diff --git a/tests/helpers.py b/tests/helpers.py index bab0078..b9bb6da 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -20,7 +20,7 @@ class StatsdServer(asyncio.DatagramProtocol, asyncio.Protocol): self.running = asyncio.Event() self.client_connected = asyncio.Semaphore(value=0) self.message_received = asyncio.Semaphore(value=0) - self.transports: list[asyncio.Transport] = [] + self.transports: list[asyncio.BaseTransport] = [] self._buffer = io.BytesIO() @@ -34,7 +34,8 @@ class StatsdServer(asyncio.DatagramProtocol, asyncio.Protocol): self.port, reuse_port=True) self.server = server - listening_sock = server.sockets[0] + listening_sock = typing.cast(list[socket.socket], + server.sockets)[0] self.host, self.port = listening_sock.getsockname() self.running.set() try: @@ -62,7 +63,8 @@ class StatsdServer(asyncio.DatagramProtocol, asyncio.Protocol): self.running.clear() def close(self): - self.server.close() + if self.server is not None: + self.server.close() for connected_client in self.transports: connected_client.close() self.transports.clear() @@ -74,7 +76,7 @@ class StatsdServer(asyncio.DatagramProtocol, asyncio.Protocol): while self.running.is_set(): await asyncio.sleep(0.1) - def connection_made(self, transport: asyncio.Transport): + def connection_made(self, transport: asyncio.BaseTransport): self.client_connected.release() self.connections_made += 1 self.transports.append(transport) diff --git a/tests/test_processor.py b/tests/test_processor.py index 59df78f..d5faaa1 100644 --- a/tests/test_processor.py +++ b/tests/test_processor.py @@ -2,6 +2,7 @@ import asyncio import logging import socket import time +import typing import asynctest @@ -110,7 +111,7 @@ class ProcessorTests(ProcessorTestCase): def test_that_processor_fails_when_host_is_none(self): with self.assertRaises(RuntimeError) as context: - statsd.Processor(host=None, port=12345) + statsd.Processor(host=None, port=12345) # type: ignore[arg-type] self.assertIn('host', str(context.exception)) async def test_starting_and_stopping_without_connecting(self): @@ -129,7 +130,7 @@ class ProcessorTests(ProcessorTestCase): await self.wait_for(processor.running.wait()) with self.assertLogs(processor.logger, level=logging.ERROR) as cm: - processor.queue.put_nowait('not-bytes') + processor.queue.put_nowait('not-bytes') # type: ignore[arg-type] while processor.queue.qsize() > 0: await asyncio.sleep(0.1) @@ -189,15 +190,16 @@ class TCPProcessingTests(ProcessorTestCase): async def test_socket_closure_while_sending(self): state = {'first_time': True} - real_transport_write = self.processor.protocol.transport.write + protocol = typing.cast(statsd.TCPProtocol, self.processor.protocol) + real_transport_write = protocol.transport.write - def fake_transport_write(buffer): + def fake_transport_write(data): if state['first_time']: self.processor.protocol.transport.close() state['first_time'] = False - return real_transport_write(buffer) + return real_transport_write(data) - self.processor.protocol.transport.write = fake_transport_write + protocol.transport.write = fake_transport_write self.processor.queue.put_nowait(b'counter:1|c') await self.wait_for(self.statsd_server.message_received.acquire()) @@ -265,8 +267,8 @@ class ConnectorTests(ProcessorTestCase): await super().asyncTearDown() def assert_metrics_equal(self, recvd: bytes, path, value, type_code): - recvd = recvd.decode('utf-8') - recvd_path, _, rest = recvd.partition(':') + decoded = recvd.decode('utf-8') + recvd_path, _, rest = decoded.partition(':') recvd_value, _, recvd_code = rest.partition('|') self.assertEqual(path, recvd_path, 'metric path mismatch') self.assertEqual(recvd_value, str(value), 'metric value mismatch') diff --git a/tests/test_tornado.py b/tests/test_tornado.py index 0929caf..8432a04 100644 --- a/tests/test_tornado.py +++ b/tests/test_tornado.py @@ -257,19 +257,19 @@ class RequestHandlerTests(AsyncTestCaseWithTimeout, testing.AsyncHTTPTestCase): timeout_remaining -= (time.time() - start) def parse_metric(self, metric_line: bytes) -> ParsedMetric: - metric_line = metric_line.decode() - path, _, rest = metric_line.partition(':') + decoded = metric_line.decode() + path, _, rest = decoded.partition(':') value, _, type_code = rest.partition('|') try: - value = float(value) + parsed_value = float(value) except ValueError: self.fail(f'value of {path} is not a number: value={value!r}') - return path, value, type_code + return path, parsed_value, type_code def find_metric(self, needle: str) -> ParsedMetric: - needle = needle.encode() + encoded = needle.encode() for line in self.statsd_server.metrics: - if needle in line: + if encoded in line: return self.parse_metric(line) self.fail(f'failed to find metric containing {needle!r}') From 5c947d6798ccf62ffe536a693991a70583f9be88 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Mon, 29 Mar 2021 07:46:23 -0400 Subject: [PATCH 26/31] Make docs a little cleaner. --- README.rst | 2 +- docs/index.rst | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/README.rst b/README.rst index b072e31..aec5e05 100644 --- a/README.rst +++ b/README.rst @@ -47,7 +47,7 @@ the payload. Tornado helpers =============== The ``sprockets_statsd.tornado`` module contains mix-in classes that make reporting metrics from your tornado_ web -application simple. You will need to install the ``sprockets_statsd[tornado]`` mix-in to ensure that the Tornado +application simple. You will need to install the ``sprockets_statsd[tornado]`` extra to ensure that the Tornado requirements for this library are met. .. code-block:: python diff --git a/docs/index.rst b/docs/index.rst index 3e52b51..7777077 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -1,4 +1,3 @@ -================ sprockets-statsd ================ From 0f0bae1199198623acdf74bc5349d80b78c52f67 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Tue, 30 Mar 2021 07:22:22 -0400 Subject: [PATCH 27/31] Set upper bound on metrics queue size. --- sprockets_statsd/statsd.py | 14 +++++++++++--- sprockets_statsd/tornado.py | 22 +++++++--------------- tests/test_processor.py | 22 ++++++++++++++++++++++ 3 files changed, 40 insertions(+), 18 deletions(-) diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py index c2bf9b6..6329e8b 100644 --- a/sprockets_statsd/statsd.py +++ b/sprockets_statsd/statsd.py @@ -35,12 +35,14 @@ class Connector: sends the metric payloads. """ + logger: logging.Logger processor: 'Processor' def __init__(self, host: str, port: int = 8125, **kwargs: typing.Any) -> None: + self.logger = logging.getLogger(__package__).getChild('Connector') self.processor = Processor(host=host, port=port, **kwargs) self._processor_task: typing.Optional[asyncio.Task[None]] = None @@ -124,7 +126,10 @@ class Connector: """ payload = f'{path}:{value}|{type_code}' - self.processor.enqueue(payload.encode('utf-8')) + try: + self.processor.enqueue(payload.encode('utf-8')) + except asyncio.QueueFull: + self.logger.warning('statsd queue is full, discarding metric') class StatsdProtocol(asyncio.BaseProtocol): @@ -245,6 +250,8 @@ class Processor: :param host: statsd server to send metrics to :param port: TCP port that the server is listening on + :param max_queue_size: only allow this many elements to be + stored in the queue before discarding metrics :param reconnect_sleep: number of seconds to sleep after socket error occurs when connecting :param wait_timeout: number os seconds to wait for a message to @@ -307,8 +314,9 @@ class Processor: *, host: str, port: int = 8125, - reconnect_sleep: float = 1.0, ip_protocol: int = socket.IPPROTO_TCP, + max_queue_size: int = 1000, + reconnect_sleep: float = 1.0, wait_timeout: float = 0.1) -> None: super().__init__() if not host: @@ -344,7 +352,7 @@ class Processor: self.logger = logging.getLogger(__package__).getChild('Processor') self.should_terminate = False self.protocol = None - self.queue = asyncio.Queue() + self.queue = asyncio.Queue(maxsize=max_queue_size) @property def connected(self) -> bool: diff --git a/sprockets_statsd/tornado.py b/sprockets_statsd/tornado.py index 85952ab..0da1cbc 100644 --- a/sprockets_statsd/tornado.py +++ b/sprockets_statsd/tornado.py @@ -111,25 +111,17 @@ class Application(web.Application): """ if self.statsd_connector is None: - statsd_settings = self.settings['statsd'] - kwargs = { - 'host': statsd_settings['host'], - 'port': statsd_settings['port'], - } - if 'reconnect_sleep' in statsd_settings: - kwargs['reconnect_sleep'] = statsd_settings['reconnect_sleep'] - if 'wait_timeout' in statsd_settings: - kwargs['wait_timeout'] = statsd_settings['wait_timeout'] - if statsd_settings['protocol'] == 'tcp': + kwargs = self.settings['statsd'].copy() + protocol = kwargs.pop('protocol', None) + if protocol == 'tcp': kwargs['ip_protocol'] = socket.IPPROTO_TCP - elif statsd_settings['protocol'] == 'udp': + elif protocol == 'udp': kwargs['ip_protocol'] = socket.IPPROTO_UDP else: - raise RuntimeError( - f'statsd configuration error:' - f' {statsd_settings["protocol"]} is not a valid' - f' protocol') + raise RuntimeError(f'statsd configuration error: {protocol} ' + f'is not a valid protocol') + kwargs.pop('prefix', None) # TODO move prefixing into Connector self.statsd_connector = statsd.Connector(**kwargs) await self.statsd_connector.start() diff --git a/tests/test_processor.py b/tests/test_processor.py index d5faaa1..0825abd 100644 --- a/tests/test_processor.py +++ b/tests/test_processor.py @@ -376,3 +376,25 @@ class ConnectorOptionTests(ProcessorTestCase): statsd.Connector(host=self.statsd_server.host, port=port) self.assertIn('port', str(context.exception)) self.assertIn(repr(port), str(context.exception)) + + async def test_that_metrics_are_dropped_when_queue_overflows(self): + connector = statsd.Connector(host=self.statsd_server.host, + port=1, + max_queue_size=10) + await connector.start() + self.addCleanup(connector.stop) + + # fill up the queue with incr's + for expected_size in range(1, connector.processor.queue.maxsize + 1): + connector.incr('counter') + self.assertEqual(connector.processor.queue.qsize(), expected_size) + + # the following decr's should be ignored + for _ in range(10): + connector.decr('counter') + self.assertEqual(connector.processor.queue.qsize(), 10) + + # make sure that only the incr's are in the queue + for _ in range(connector.processor.queue.qsize()): + metric = await connector.processor.queue.get() + self.assertEqual(metric, b'counter:1|c') From da95efa5446fe7ea418d7b763184cae62162df00 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Tue, 30 Mar 2021 07:56:20 -0400 Subject: [PATCH 28/31] Move path prefixing into Connector. This makes the low-level code a little more opinionated. If there is a good reason for making `timers`, `counters`, etc. optional, then we can PR it into place. --- sprockets_statsd/statsd.py | 44 ++++++++++++++++++++++++++++++------- sprockets_statsd/tornado.py | 12 +++------- tests/test_processor.py | 31 ++++++++++++++------------ tests/test_tornado.py | 34 ++++++++++++++-------------- 4 files changed, 72 insertions(+), 49 deletions(-) diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py index 6329e8b..43cd329 100644 --- a/sprockets_statsd/statsd.py +++ b/sprockets_statsd/statsd.py @@ -12,6 +12,7 @@ class Connector: :keyword ip_protocol: IP protocol to use for the underlying socket -- either ``socket.IPPROTO_TCP`` for TCP or ``socket.IPPROTO_UDP`` for UDP sockets. + :keyword prefix: optional string to prepend to metric paths :param kwargs: additional keyword parameters are passed to the :class:`.Processor` initializer @@ -23,11 +24,34 @@ class Connector: terminating to ensure that all metrics are flushed to the statsd server. - When the connector is *should_terminate*, metric payloads are sent by - calling the :meth:`.inject_metric` method. The payloads are - stored in an internal queue that is consumed whenever the + Metrics are optionally prefixed with :attr:`prefix` before the + metric type prefix. This *should* be used to prevent metrics + from being overwritten when multiple applications share a StatsD + instance. Each metric type is also prefixed by one of the + following strings based on the metric type: + + +-------------------+---------------+-----------+ + | Method call | Prefix | Type code | + +-------------------+---------------+-----------+ + | :meth:`.incr` | ``counters.`` | ``c`` | + +-------------------+---------------+-----------+ + | :meth:`.decr` | ``counters.`` | ``c`` | + +-------------------+---------------+-----------+ + | :meth:`.gauge` | ``gauges.`` | ``g`` | + +-------------------+---------------+-----------+ + | :meth:`.timing` | ``timers.`` | ``ms`` | + +-------------------+---------------+-----------+ + + When the connector is *should_terminate*, metric payloads are + sent by calling the :meth:`.inject_metric` method. The payloads + are stored in an internal queue that is consumed whenever the connection to the server is active. + .. attribute:: prefix + :type: str + + String to prefix to all metrics *before* the metric type prefix. + .. attribute:: processor :type: Processor @@ -36,13 +60,17 @@ class Connector: """ logger: logging.Logger + prefix: str processor: 'Processor' def __init__(self, host: str, port: int = 8125, + *, + prefix: str = '', **kwargs: typing.Any) -> None: self.logger = logging.getLogger(__package__).getChild('Connector') + self.prefix = f'{prefix}.' if prefix else prefix self.processor = Processor(host=host, port=port, **kwargs) self._processor_task: typing.Optional[asyncio.Task[None]] = None @@ -74,7 +102,7 @@ class Connector: :param value: amount to increment the counter by """ - self.inject_metric(path, str(value), 'c') + self.inject_metric(f'counters.{path}', str(value), 'c') def decr(self, path: str, value: int = 1) -> None: """Decrement a counter metric. @@ -85,7 +113,7 @@ class Connector: This is equivalent to ``self.incr(path, -value)``. """ - self.inject_metric(path, str(-value), 'c') + self.inject_metric(f'counters.{path}', str(-value), 'c') def gauge(self, path: str, value: int, delta: bool = False) -> None: """Manipulate a gauge metric. @@ -103,7 +131,7 @@ class Connector: payload = f'{value:+d}' else: payload = str(value) - self.inject_metric(path, payload, 'g') + self.inject_metric(f'gauges.{path}', payload, 'g') def timing(self, path: str, seconds: float) -> None: """Send a timer metric. @@ -112,7 +140,7 @@ class Connector: :param seconds: number of **seconds** to record """ - self.inject_metric(path, str(seconds * 1000.0), 'ms') + self.inject_metric(f'timers.{path}', str(seconds * 1000.0), 'ms') def inject_metric(self, path: str, value: str, type_code: str) -> None: """Send a metric to the statsd server. @@ -125,7 +153,7 @@ class Connector: internal queue for future processing. """ - payload = f'{path}:{value}|{type_code}' + payload = f'{self.prefix}{path}:{value}|{type_code}' try: self.processor.enqueue(payload.encode('utf-8')) except asyncio.QueueFull: diff --git a/sprockets_statsd/tornado.py b/sprockets_statsd/tornado.py index 0da1cbc..5e368c6 100644 --- a/sprockets_statsd/tornado.py +++ b/sprockets_statsd/tornado.py @@ -121,7 +121,6 @@ class Application(web.Application): raise RuntimeError(f'statsd configuration error: {protocol} ' f'is not a valid protocol') - kwargs.pop('prefix', None) # TODO move prefixing into Connector self.statsd_connector = statsd.Connector(**kwargs) await self.statsd_connector.start() @@ -148,10 +147,7 @@ class RequestHandler(web.RequestHandler): self.statsd_connector = self.application.statsd_connector def __build_path(self, *path: typing.Any) -> str: - full_path = '.'.join(str(c) for c in path) - if self.settings.get('statsd', {}).get('prefix', ''): - return f'{self.settings["statsd"]["prefix"]}.{full_path}' - return full_path + return '.'.join(str(c) for c in path) def record_timing(self, secs: float, *path: typing.Any) -> None: """Record the duration. @@ -161,8 +157,7 @@ class RequestHandler(web.RequestHandler): """ if self.statsd_connector is not None: - self.statsd_connector.timing(self.__build_path('timers', *path), - secs) + self.statsd_connector.timing(self.__build_path(*path), secs) def increase_counter(self, *path: typing.Any, amount: int = 1) -> None: """Adjust a counter. @@ -173,8 +168,7 @@ class RequestHandler(web.RequestHandler): """ if self.statsd_connector is not None: - self.statsd_connector.incr(self.__build_path('counters', *path), - amount) + self.statsd_connector.incr(self.__build_path(*path), amount) @contextlib.contextmanager def execution_timer( diff --git a/tests/test_processor.py b/tests/test_processor.py index 0825abd..4b802da 100644 --- a/tests/test_processor.py +++ b/tests/test_processor.py @@ -225,8 +225,9 @@ class UDPProcessingTests(ProcessorTestCase): await self.wait_for(self.statsd_server.message_received.acquire()) await self.wait_for(self.statsd_server.message_received.acquire()) - self.assertEqual(self.statsd_server.metrics[0], b'counter:1|c') - self.assertEqual(self.statsd_server.metrics[1], b'timer:1.0|ms') + self.assertEqual(self.statsd_server.metrics[0], + b'counters.counter:1|c') + self.assertEqual(self.statsd_server.metrics[1], b'timers.timer:1.0|ms') async def test_that_client_sends_to_new_server(self): self.statsd_server.close() @@ -240,7 +241,8 @@ class UDPProcessingTests(ProcessorTestCase): self.connector.incr('should.be.recvd') await self.wait_for(self.statsd_server.message_received.acquire()) - self.assertEqual(self.statsd_server.metrics[0], b'should.be.recvd:1|c') + self.assertEqual(self.statsd_server.metrics[0], + b'counters.should.be.recvd:1|c') async def test_that_client_handles_socket_closure(self): self.connector.processor.protocol.transport.close() @@ -249,7 +251,8 @@ class UDPProcessingTests(ProcessorTestCase): self.connector.incr('should.be.recvd') await self.wait_for(self.statsd_server.message_received.acquire()) - self.assertEqual(self.statsd_server.metrics[0], b'should.be.recvd:1|c') + self.assertEqual(self.statsd_server.metrics[0], + b'counters.should.be.recvd:1|c') class ConnectorTests(ProcessorTestCase): @@ -278,22 +281,22 @@ class ConnectorTests(ProcessorTestCase): self.connector.incr('simple.counter') await self.wait_for(self.statsd_server.message_received.acquire()) self.assert_metrics_equal(self.statsd_server.metrics[-1], - 'simple.counter', 1, 'c') + 'counters.simple.counter', 1, 'c') self.connector.incr('simple.counter', 10) await self.wait_for(self.statsd_server.message_received.acquire()) self.assert_metrics_equal(self.statsd_server.metrics[-1], - 'simple.counter', 10, 'c') + 'counters.simple.counter', 10, 'c') self.connector.decr('simple.counter') await self.wait_for(self.statsd_server.message_received.acquire()) self.assert_metrics_equal(self.statsd_server.metrics[-1], - 'simple.counter', -1, 'c') + 'counters.simple.counter', -1, 'c') self.connector.decr('simple.counter', 10) await self.wait_for(self.statsd_server.message_received.acquire()) self.assert_metrics_equal(self.statsd_server.metrics[-1], - 'simple.counter', -10, 'c') + 'counters.simple.counter', -10, 'c') async def test_adjusting_gauge(self): self.connector.gauge('simple.gauge', 100) @@ -303,18 +306,18 @@ class ConnectorTests(ProcessorTestCase): await self.wait_for(self.statsd_server.message_received.acquire()) self.assert_metrics_equal(self.statsd_server.metrics[0], - 'simple.gauge', '100', 'g') + 'gauges.simple.gauge', '100', 'g') self.assert_metrics_equal(self.statsd_server.metrics[1], - 'simple.gauge', '-10', 'g') + 'gauges.simple.gauge', '-10', 'g') self.assert_metrics_equal(self.statsd_server.metrics[2], - 'simple.gauge', '+10', 'g') + 'gauges.simple.gauge', '+10', 'g') async def test_sending_timer(self): secs = 12.34 self.connector.timing('simple.timer', secs) await self.wait_for(self.statsd_server.message_received.acquire()) self.assert_metrics_equal(self.statsd_server.metrics[0], - 'simple.timer', 12340.0, 'ms') + 'timers.simple.timer', 12340.0, 'ms') async def test_that_queued_metrics_are_drained(self): # The easiest way to test that the internal metrics queue @@ -348,7 +351,7 @@ class ConnectorTests(ProcessorTestCase): await self.wait_for(self.statsd_server.client_connected.acquire()) for value in range(50): await self.wait_for(self.statsd_server.message_received.acquire()) - self.assertEqual(f'counter:{value}|c'.encode(), + self.assertEqual(f'counters.counter:{value}|c'.encode(), self.statsd_server.metrics.pop(0)) @@ -397,4 +400,4 @@ class ConnectorOptionTests(ProcessorTestCase): # make sure that only the incr's are in the queue for _ in range(connector.processor.queue.qsize()): metric = await connector.processor.queue.get() - self.assertEqual(metric, b'counter:1|c') + self.assertEqual(metric, b'counters.counter:1|c') diff --git a/tests/test_tornado.py b/tests/test_tornado.py index 8432a04..77559b8 100644 --- a/tests/test_tornado.py +++ b/tests/test_tornado.py @@ -62,12 +62,12 @@ class ApplicationTests(AsyncTestCaseWithTimeout): self.unsetenv('STATSD_PREFIX') self.unsetenv('STATSD_PROTOCOL') - app = sprockets_statsd.tornado.Application(statsd={'prefix': None}) + app = sprockets_statsd.tornado.Application(statsd={'prefix': ''}) self.assertIn('statsd', app.settings) self.assertIsNone(app.settings['statsd']['host'], 'default host value should be None') self.assertEqual(8125, app.settings['statsd']['port']) - self.assertEqual(None, app.settings['statsd']['prefix']) + self.assertEqual('', app.settings['statsd']['prefix']) self.assertEqual('tcp', app.settings['statsd']['protocol']) def test_that_statsd_settings_read_from_environment(self): @@ -217,6 +217,20 @@ class ApplicationTests(AsyncTestCaseWithTimeout): app.statsd_connector.processor._ip_protocol) self.run_coroutine(app.stop_statsd()) + def test_disabling_statsd_prefix(self): + app = sprockets_statsd.tornado.Application( + service='my-service', + version='1.0.0', + statsd={ + 'host': 'localhost', + 'prefix': '', + 'protocol': 'udp', + }, + ) + self.run_coroutine(app.start_statsd()) + self.assertEqual(app.statsd_connector.prefix, '') + self.run_coroutine(app.stop_statsd()) + class RequestHandlerTests(AsyncTestCaseWithTimeout, testing.AsyncHTTPTestCase): def setUp(self): @@ -306,19 +320,3 @@ class RequestHandlerTests(AsyncTestCaseWithTimeout, testing.AsyncHTTPTestCase): rsp = self.fetch('/') self.assertEqual(200, rsp.code) - - def test_handling_request_without_prefix(self): - self.app.settings['statsd']['prefix'] = '' - - rsp = self.fetch('/') - self.assertEqual(200, rsp.code) - self.wait_for_metrics() - - path, _, _ = self.find_metric('Handler.GET.200') - self.assertEqual('timers.Handler.GET.200', path) - - path, _, _ = self.find_metric('execution-timer') - self.assertEqual('timers.execution-timer', path) - - path, _, _ = self.find_metric('request-count') - self.assertEqual('counters.request-count', path) From 51e1f2ccaeb63a867e7df01d2068d676d5c88847 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Tue, 30 Mar 2021 08:09:53 -0400 Subject: [PATCH 29/31] Make type annotations safe for Python 3.7. --- sprockets_statsd/statsd.py | 2 +- tests/helpers.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sprockets_statsd/statsd.py b/sprockets_statsd/statsd.py index 43cd329..b35e605 100644 --- a/sprockets_statsd/statsd.py +++ b/sprockets_statsd/statsd.py @@ -333,7 +333,7 @@ class Processor: logger: logging.Logger protocol: typing.Optional[StatsdProtocol] - queue: asyncio.Queue[bytes] + queue: asyncio.Queue _create_transport: typing.Callable[[], typing.Coroutine[ typing.Any, typing.Any, typing.Tuple[asyncio.BaseTransport, StatsdProtocol]]] diff --git a/tests/helpers.py b/tests/helpers.py index b9bb6da..91a21ef 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -20,7 +20,7 @@ class StatsdServer(asyncio.DatagramProtocol, asyncio.Protocol): self.running = asyncio.Event() self.client_connected = asyncio.Semaphore(value=0) self.message_received = asyncio.Semaphore(value=0) - self.transports: list[asyncio.BaseTransport] = [] + self.transports: typing.List[asyncio.BaseTransport] = [] self._buffer = io.BytesIO() @@ -34,7 +34,7 @@ class StatsdServer(asyncio.DatagramProtocol, asyncio.Protocol): self.port, reuse_port=True) self.server = server - listening_sock = typing.cast(list[socket.socket], + listening_sock = typing.cast(typing.List[socket.socket], server.sockets)[0] self.host, self.port = listening_sock.getsockname() self.running.set() From 5a524378578c0365ca89da71c2d3ae9d534ae40d Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Tue, 30 Mar 2021 08:10:24 -0400 Subject: [PATCH 30/31] Remove coverage from tox.ini. This makes running in parallel work properly. --- tox.ini | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tox.ini b/tox.ini index 3b8ffcf..64481a1 100644 --- a/tox.ini +++ b/tox.ini @@ -6,8 +6,7 @@ toxworkdir = ./build/tox deps = .[dev,tornado] commands = - coverage run -m unittest - coverage report + python -m unittest [testenv:lint] commands = From 0d911fc3e225e4361fcf63b4db40da984950c6be Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Tue, 30 Mar 2021 08:24:25 -0400 Subject: [PATCH 31/31] Enable codecov upload. --- .github/workflows/run-tests.yml | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index c65650c..2b88174 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -33,16 +33,15 @@ jobs: run: | coverage run -m unittest coverage report - coverage xml + coverage xml -o ./coverage.xml - name: Generate documentation run: | sphinx-build -b html -W --no-color docs build/sphinx/html -# TODO -# - name: Upload coverage -# uses: codecov/codecov-action@v1.0.2 -# if: github.event_name == 'push' && github.repository == 'sprockets/sprockets-statsd' && github.branch == 'main' -# with: -# token: ${{ secrets.CODECOV_TOKEN }} -# file: build/coverage.xml -# flags: unittests -# fail_ci_if_error: true + - name: Upload coverage + uses: codecov/codecov-action@v1.0.2 + with: + token: ${{ secrets.CODECOV_TOKEN }} + file: ./coverage.xml + flags: unittests + env_vars: OS,PYTHON + fail_ci_if_error: true