From b84e52592dfa902789249a0643ab20794bab0990 Mon Sep 17 00:00:00 2001 From: Dave Shawley Date: Wed, 24 Mar 2021 08:10:07 -0400 Subject: [PATCH] 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):