openstack/swift
Code Issues Proposed changes

Allow StatsdClients to no-op if no host provided

Browse Source 
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 <alistairncoles@gmail.com>
Change-Id: I6eb5b27a387cc2b7310ee11cc49d38fd2b6cbab8
This commit is contained in:
Tim Burke 2024-05-15 15:11:17 -07:00 committed by Clay Gerrard
parent a1916855c2
commit b447234b2f
7 changed files with 98 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

43
swift/common/statsd_client.py
View File

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

3
swift/common/utils/logs.py
View File

@ -735,7 +735,6 @@ 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(
@ -747,8 +746,6 @@ def get_logger(conf, name=None, log_to_console=False, log_route=None,
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
adapted_logger = LogAdapter(logger, name)
other_handlers = conf.get('log_custom_handlers', None)

2
test/debug_logger.py
View File

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

3
test/unit/common/middleware/s3api/test_s3api.py
View File

@ -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):

6
test/unit/common/middleware/test_tempauth.py
View File

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

64
test/unit/common/test_statsd_client.py
View File

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

15
test/unit/common/test_utils.py
View File

@ -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):