From b447234b2f1e005025b5ce50010993ca226c2283 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Wed, 15 May 2024 15:11:17 -0700 Subject: [PATCH] Allow StatsdClients to no-op if no host provided We've been working toward separating our logger from our statsd client. This is generally a good idea; it's always been a little weird to have our special-case loggers that would allow you to *also* increment some counters. The end goal is to take a bunch of places that look like logger = utils.get_logger(conf) ... logger.info(...) logger.increment(...) and turn them into something more like logger = logs.get_adapted_logger(conf) stats = statsd_client.get_statsd_client(conf, logger=logger) ... logger.info(...) stats.increment(...) Take a lesson from logging: callers don't need to know whether the log_level is high enough that their message will be logged, or even whether logging is enabled at all. Code wanting to emit stats shouldn't need to know whether statsd collection has been configured, either. Co-Authored-By: Alistair Coles Change-Id: I6eb5b27a387cc2b7310ee11cc49d38fd2b6cbab8 --- swift/common/statsd_client.py | 43 +++++++------ swift/common/utils/logs.py | 25 ++++---- test/debug_logger.py | 2 +- .../common/middleware/s3api/test_s3api.py | 3 +- test/unit/common/middleware/test_tempauth.py | 6 +- test/unit/common/test_statsd_client.py | 64 +++++++++++++++++-- test/unit/common/test_utils.py | 15 ----- 7 files changed, 98 insertions(+), 60 deletions(-) diff --git a/swift/common/statsd_client.py b/swift/common/statsd_client.py index d8e05f0dc1..492d264bbf 100644 --- a/swift/common/statsd_client.py +++ b/swift/common/statsd_client.py @@ -35,9 +35,28 @@ class StatsdClient(object): self._sample_rate_factor = sample_rate_factor self.random = random self.logger = logger + self._sock_family = self._target = None + if self._host: + self._set_sock_family_and_target(host, port) + + def _set_sock_family_and_target(self, host, port): # Determine if host is IPv4 or IPv6 - addr_info, self._sock_family = self._determine_sock_family(host, port) + addr_info = None + try: + addr_info = socket.getaddrinfo(host, port, socket.AF_INET) + self._sock_family = socket.AF_INET + except socket.gaierror: + try: + addr_info = socket.getaddrinfo(host, port, socket.AF_INET6) + self._sock_family = socket.AF_INET6 + except socket.gaierror: + # Don't keep the server from starting from what could be a + # transient DNS failure. Any hostname will get re-resolved as + # necessary in the .sendto() calls. + # However, we don't know if we're IPv4 or IPv6 in this case, so + # we assume legacy IPv4. + self._sock_family = socket.AF_INET # NOTE: we use the original host value, not the DNS-resolved one # because if host is a hostname, we don't want to cache the DNS @@ -57,24 +76,6 @@ class StatsdClient(object): else: self._target = (host, port) - def _determine_sock_family(self, host, port): - addr_info = sock_family = None - try: - addr_info = socket.getaddrinfo(host, port, socket.AF_INET) - sock_family = socket.AF_INET - except socket.gaierror: - try: - addr_info = socket.getaddrinfo(host, port, socket.AF_INET6) - sock_family = socket.AF_INET6 - except socket.gaierror: - # Don't keep the server from starting from what could be a - # transient DNS failure. Any hostname will get re-resolved as - # necessary in the .sendto() calls. - # However, we don't know if we're IPv4 or IPv6 in this case, so - # we assume legacy IPv4. - sock_family = socket.AF_INET - return addr_info, sock_family - def _set_prefix(self, tail_prefix): """ Modifies the prefix that is added to metric names. The resulting prefix @@ -115,6 +116,10 @@ class StatsdClient(object): self._set_prefix(tail_prefix) def _send(self, m_name, m_value, m_type, sample_rate): + if not self._host: + # StatsD not configured + return + if sample_rate is None: sample_rate = self._default_sample_rate sample_rate = sample_rate * self._sample_rate_factor diff --git a/swift/common/utils/logs.py b/swift/common/utils/logs.py index 3e918a34e4..35c161fb6f 100644 --- a/swift/common/utils/logs.py +++ b/swift/common/utils/logs.py @@ -735,20 +735,17 @@ def get_logger(conf, name=None, log_to_console=False, log_route=None, # Setup logger with a StatsD client if so configured statsd_host = conf.get('log_statsd_host') - if statsd_host: - statsd_port = int(conf.get('log_statsd_port', 8125)) - base_prefix = conf.get('log_statsd_metric_prefix', '') - default_sample_rate = float(conf.get( - 'log_statsd_default_sample_rate', 1)) - sample_rate_factor = float(conf.get( - 'log_statsd_sample_rate_factor', 1)) - if statsd_tail_prefix is None: - statsd_tail_prefix = name - logger.statsd_client = statsd_client.StatsdClient( - statsd_host, statsd_port, base_prefix, statsd_tail_prefix, - default_sample_rate, sample_rate_factor, logger=logger) - else: - logger.statsd_client = None + statsd_port = int(conf.get('log_statsd_port', 8125)) + base_prefix = conf.get('log_statsd_metric_prefix', '') + default_sample_rate = float(conf.get( + 'log_statsd_default_sample_rate', 1)) + sample_rate_factor = float(conf.get( + 'log_statsd_sample_rate_factor', 1)) + if statsd_tail_prefix is None: + statsd_tail_prefix = name + logger.statsd_client = statsd_client.StatsdClient( + statsd_host, statsd_port, base_prefix, statsd_tail_prefix, + default_sample_rate, sample_rate_factor, logger=logger) adapted_logger = LogAdapter(logger, name) other_handlers = conf.get('log_custom_handlers', None) diff --git a/test/debug_logger.py b/test/debug_logger.py index 52e39777ca..b59dfe9e70 100644 --- a/test/debug_logger.py +++ b/test/debug_logger.py @@ -149,7 +149,7 @@ class FakeLogger(logging.Logger, CaptureLog): self.level = logging.NOTSET if 'facility' in kwargs: self.facility = kwargs['facility'] - self.statsd_client = FakeStatsdClient("host", 8125) + self.statsd_client = FakeStatsdClient(None, 8125) self.thread_locals = None self.parent = None # ensure the NOTICE level has been named, in case it has not already diff --git a/test/unit/common/middleware/s3api/test_s3api.py b/test/unit/common/middleware/s3api/test_s3api.py index b5f715d990..39fd39a3a2 100644 --- a/test/unit/common/middleware/s3api/test_s3api.py +++ b/test/unit/common/middleware/s3api/test_s3api.py @@ -223,7 +223,8 @@ class TestS3ApiMiddleware(S3ApiTestCase): self.assertEqual('s3api', s3api.logger.logger.name) self.assertIsNot(s3api.logger.logger, proxy_logger) self.assertEqual('swift', s3api.logger.server) - self.assertIsNone(s3api.logger.logger.statsd_client) + # there's a stats client, but with no host, it can't send anything + self.assertIsNone(s3api.logger.logger.statsd_client._host) with mock.patch('swift.common.statsd_client.StatsdClient', FakeStatsdClient): diff --git a/test/unit/common/middleware/test_tempauth.py b/test/unit/common/middleware/test_tempauth.py index 3cdb1b5ffd..5ed83953f9 100644 --- a/test/unit/common/middleware/test_tempauth.py +++ b/test/unit/common/middleware/test_tempauth.py @@ -100,7 +100,7 @@ class TestAuth(unittest.TestCase): def test_statsd_prefix(self): app = FakeApp() - ath = auth.filter_factory({'log_statsd_host': 'example.com'})(app) + ath = auth.filter_factory({})(app) self.assertIsNotNone(ath.logger.logger.statsd_client) self.assertIsInstance(ath.logger.logger.statsd_client, StatsdClient) @@ -108,8 +108,7 @@ class TestAuth(unittest.TestCase): ath.logger.logger.statsd_client._prefix) ath = auth.filter_factory({'log_statsd_metric_prefix': 'foo', - 'log_name': 'bar', - 'log_statsd_host': 'example.com'})(app) + 'log_name': 'bar'})(app) self.assertIsNotNone(ath.logger.logger.statsd_client) self.assertIsInstance(ath.logger.logger.statsd_client, StatsdClient) @@ -118,7 +117,6 @@ class TestAuth(unittest.TestCase): ath = auth.filter_factory({'log_statsd_metric_prefix': 'foo', 'log_name': 'bar', - 'log_statsd_host': 'example.com', 'reseller_prefix': 'TEST'})(app) self.assertIsNotNone(ath.logger.logger.statsd_client) self.assertIsInstance(ath.logger.logger.statsd_client, diff --git a/test/unit/common/test_statsd_client.py b/test/unit/common/test_statsd_client.py index 16b8ee8a86..12329f2118 100644 --- a/test/unit/common/test_statsd_client.py +++ b/test/unit/common/test_statsd_client.py @@ -30,16 +30,34 @@ from mock import patch from swift.common import utils from swift.common import statsd_client +from swift.common.statsd_client import StatsdClient from test.debug_logger import debug_logger -from test.unit.common.test_utils import MockUdpSocket from swift.common.swob import Response -class TestStatsdLogging(unittest.TestCase): +class MockUdpSocket(object): + def __init__(self, sendto_errno=None): + self.sent = [] + self.sendto_errno = sendto_errno + + def sendto(self, data, target): + if self.sendto_errno: + raise socket.error(self.sendto_errno, + 'test errno %s' % self.sendto_errno) + self.sent.append((data, target)) + return len(data) + + def close(self): + pass + + +class BaseTestStasdClient(unittest.TestCase): def setUp(self): + self.getaddrinfo_calls = [] def fake_getaddrinfo(host, port, *args): + self.getaddrinfo_calls.append((host, port)) # this is what a real getaddrinfo('localhost', port, # socket.AF_INET) returned once return [(socket.AF_INET, # address family @@ -59,10 +77,43 @@ class TestStatsdLogging(unittest.TestCase): self.mock_getaddrinfo = self.getaddrinfo_patcher.start() self.addCleanup(self.getaddrinfo_patcher.stop) + +class TestStatsdClient(BaseTestStasdClient): + def test_init_host(self): + client = StatsdClient('myhost', 1234) + self.assertEqual([('myhost', 1234)], self.getaddrinfo_calls) + with mock.patch.object(client, '_open_socket') as mock_open: + self.assertIs(client.increment('tunafish'), + mock_open.return_value.sendto.return_value) + self.assertEqual(mock_open.mock_calls, [ + mock.call(), + mock.call().sendto(b'tunafish:1|c', ('myhost', 1234)), + mock.call().close(), + ]) + + def test_init_host_is_none(self): + client = StatsdClient(None, None) + self.assertIsNone(client._host) + self.assertFalse(self.getaddrinfo_calls) + with mock.patch.object(client, '_open_socket') as mock_open: + self.assertIsNone(client.increment('tunafish')) + self.assertFalse(mock_open.mock_calls) + self.assertFalse(self.getaddrinfo_calls) + + +class TestStatsdLogging(BaseTestStasdClient): def test_get_logger_statsd_client_not_specified(self): logger = utils.get_logger({}, 'some-name', log_route='some-route') - # white-box construction validation - self.assertIsNone(logger.logger.statsd_client) + # white-box construction validation: + # there may be a client instance, but it won't send anything; + # won't even open a socket + self.assertIsInstance(logger.logger.statsd_client, + statsd_client.StatsdClient) + self.assertIsNone(logger.logger.statsd_client._host) + with mock.patch.object(logger.logger.statsd_client, + '_open_socket') as mock_open: + logger.increment('tunafish') + self.assertFalse(mock_open.mock_calls) def test_get_logger_statsd_client_defaults(self): logger = utils.get_logger({'log_statsd_host': 'some.host.com'}, @@ -325,11 +376,12 @@ class TestStatsdLogging(unittest.TestCase): client._open_socket = lambda *_: mock_socket client.random = lambda: 0.50001 - logger.increment('tribbles', sample_rate=0.5) + self.assertIsNone(logger.increment('tribbles', sample_rate=0.5)) self.assertEqual(len(mock_socket.sent), 0) client.random = lambda: 0.49999 - logger.increment('tribbles', sample_rate=0.5) + rv = logger.increment('tribbles', sample_rate=0.5) + self.assertIsInstance(rv, int) self.assertEqual(len(mock_socket.sent), 1) payload = mock_socket.sent[0][0] diff --git a/test/unit/common/test_utils.py b/test/unit/common/test_utils.py index df0e3caaca..829ce7867a 100644 --- a/test/unit/common/test_utils.py +++ b/test/unit/common/test_utils.py @@ -136,21 +136,6 @@ class MockOs(object): return getattr(os, name) -class MockUdpSocket(object): - def __init__(self, sendto_errno=None): - self.sent = [] - self.sendto_errno = sendto_errno - - def sendto(self, data, target): - if self.sendto_errno: - raise socket.error(self.sendto_errno, - 'test errno %s' % self.sendto_errno) - self.sent.append((data, target)) - - def close(self): - pass - - class MockSys(object): def __init__(self):