From 7acd220c4427363c093f42a51229681e72321bd4 Mon Sep 17 00:00:00 2001 From: Erik Olof Gunnar Andersson Date: Sun, 19 May 2019 01:09:04 -0700 Subject: [PATCH] Simplified metrics implementation If monascastatsd is installed, the current implementation will always open a socket when starting designate, even if metrics is disabled in the configuration. This patch changes this by changing the way the timer was implemented, making sure that the metrics implemention is only loaded when needed. * Made the error message obvious when metrics is disabled, and when we are missing the monasca-statsd client. * Removed support for the timed decorator. Change-Id: I508269ae3baac9882a51fb61ce55d6169ffddad3 --- designate/mdns/notify.py | 14 ++- designate/mdns/xfr.py | 31 ++--- designate/metrics.py | 102 ++++++---------- designate/metrics_client/noop.py | 10 +- designate/service.py | 3 +- .../{test_metrics => metrics}/__init__.py | 0 .../{test_metrics => metrics}/test_metrics.py | 110 ++++++++++-------- 7 files changed, 133 insertions(+), 137 deletions(-) rename designate/tests/unit/{test_metrics => metrics}/__init__.py (100%) rename designate/tests/unit/{test_metrics => metrics}/test_metrics.py (52%) diff --git a/designate/mdns/notify.py b/designate/mdns/notify.py index 328f8819c..4aa017d1e 100644 --- a/designate/mdns/notify.py +++ b/designate/mdns/notify.py @@ -41,7 +41,6 @@ class NotifyEndpoint(base.BaseEndpoint): RPC_API_VERSION = '2.0' RPC_API_NAMESPACE = 'notify' - @metrics.timed('mdns.notify_zone_changed') def notify_zone_changed(self, context, zone, host, port, timeout, retry_interval, max_retries, delay): """ @@ -61,10 +60,15 @@ class NotifyEndpoint(base.BaseEndpoint): current_retry is the current retry number. The return value is just used for testing and not by pool manager. """ - time.sleep(delay) - return self._make_and_send_dns_message( - zone, host, port, timeout, retry_interval, max_retries, - notify=True) + start_time = time.time() + try: + time.sleep(delay) + return self._make_and_send_dns_message( + zone, host, port, timeout, retry_interval, max_retries, + notify=True) + finally: + metrics.timing('mdns.notify_zone_changed', + time.time() - start_time) def poll_for_serial_number(self, context, zone, nameserver, timeout, retry_interval, max_retries, delay): diff --git a/designate/mdns/xfr.py b/designate/mdns/xfr.py index e6c8bf831..11751bb06 100644 --- a/designate/mdns/xfr.py +++ b/designate/mdns/xfr.py @@ -13,6 +13,8 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. +import time + from oslo_config import cfg from oslo_utils import timeutils from oslo_log import log as logging @@ -30,24 +32,27 @@ class XFRMixin(object): """ Utility mixin that holds common methods for XFR functionality. """ - @metrics.timed('mdns.xfr.zone_sync') def zone_sync(self, context, zone, servers=None): - servers = servers or zone.masters - servers = servers.to_list() - - timeout = cfg.CONF["service:mdns"].xfr_timeout + start_time = time.time() try: - dnspython_zone = dnsutils.do_axfr(zone.name, servers, - timeout=timeout) - except exceptions.XFRFailure as e: - LOG.warning(e) - return + servers = servers or zone.masters + servers = servers.to_list() - zone.update(dnsutils.from_dnspython_zone(dnspython_zone)) + timeout = cfg.CONF["service:mdns"].xfr_timeout + try: + dnspython_zone = dnsutils.do_axfr(zone.name, servers, + timeout=timeout) + except exceptions.XFRFailure as e: + LOG.warning(e) + return - zone.transferred_at = timeutils.utcnow() + zone.update(dnsutils.from_dnspython_zone(dnspython_zone)) - self.central_api.update_zone(context, zone, increment_serial=False) + zone.transferred_at = timeutils.utcnow() + + self.central_api.update_zone(context, zone, increment_serial=False) + finally: + metrics.timing('mdns.xfr.zone_sync', time.time() - start_time) class XfrEndpoint(base.BaseEndpoint, XFRMixin): diff --git a/designate/metrics.py b/designate/metrics.py index 656f77683..d7418980d 100644 --- a/designate/metrics.py +++ b/designate/metrics.py @@ -11,96 +11,70 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. - -""" -Monasca-Statsd based metrics -~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -Based on metrics-and-stats blueprint - -Usage examples: - -.. code-block:: python - - from designate.metrics import metrics - - @metrics.timed('dot.separated.name') - def your_function(): - pass - - with metrics.time('dot.separated.name'): - pass - - # Increment and decrement a counter. - metrics.counter(name='foo.bar').increment() - metrics.counter(name='foo.bar') -= 10 - -""" - +from oslo_config import cfg from oslo_log import log as logging from oslo_utils import importutils -import designate.conf.metrics +import designate.conf +from designate.metrics_client import noop -stats_client = importutils.import_any('monascastatsd', - 'designate.metrics_client.noop') +monascastatsd = importutils.try_import('monascastatsd') CFG_GROUP_NAME = 'monasca:statsd' CONF = designate.conf.CONF LOG = logging.getLogger(__name__) - # Global metrics client to be imported by other modules metrics = None class Metrics(object): - def __init__(self): - """Initialize Monasca-Statsd client with its default configuration. - Do not start sending metrics yet. - """ - self._client = stats_client.Client(dimensions={ - 'service_name': 'dns' - }) - # cfg.CONF is not available at this time - # Buffer all metrics until init() is called - # https://bugs.launchpad.net/monasca/+bug/1616060 - self._client.connection.open_buffer() - self._client.connection.max_buffer_size = 50000 + self._client = None def init(self): - """Setup client connection or disable metrics based on configuration. - This is called once the cfg.CONF is ready. - """ - conf = CONF[CFG_GROUP_NAME] - if conf.enabled: - LOG.info("Statsd reports to %(host)s %(port)d", - { - 'host': conf.hostname, - 'port': conf.port - }) - self._client.connection._flush_buffer() - self._client.connection.close_buffer() - self._client.connection.connect(conf.hostname, conf.port) + conf = cfg.CONF[CFG_GROUP_NAME] + if conf.enabled and monascastatsd: + LOG.info( + 'Statsd reports to %(host)s:%(port)d', + { + 'host': conf.hostname, + 'port': conf.port + } + ) + self._client = monascastatsd.Client( + host=conf.hostname, port=conf.port, + dimensions={ + 'service_name': 'dns' + }) + return + + if conf.enabled and not monascastatsd: + LOG.error('monasca-statsd client not installed. ' + 'Metrics will be ignored.') else: - LOG.info("Statsd disabled") - # The client cannot be disabled: mock out report() - self._client.connection.report = lambda *a, **kw: None - # There's no clean way to drain the outgoing buffer + LOG.info('Statsd disabled') + + self._client = noop.Client() def counter(self, *a, **kw): - return self._client.get_counter(*a, **kw) + return self.client.get_counter(*a, **kw) def gauge(self, *a, **kw): - return self._client.get_gauge(*a, **kw) + return self.client.get_gauge(*a, **kw) @property - def timed(self): - return self._client.get_timer().timed + def timing(self): + return self.client.get_timer().timing def timer(self): - return self._client.get_timer() + return self.client.get_timer() + + @property + def client(self): + if not self._client: + self.init() + return self._client metrics = Metrics() diff --git a/designate/metrics_client/noop.py b/designate/metrics_client/noop.py index d45ef8565..004193d63 100644 --- a/designate/metrics_client/noop.py +++ b/designate/metrics_client/noop.py @@ -29,10 +29,9 @@ class NoopConnection(object): pass def connect(self, *a, **kw): - LOG.error('Using noop metrics client. Metrics will be ignored.') pass - def open_buffer(self): + def open_buffer(self, *a, **kw): pass @@ -65,10 +64,8 @@ class NoopTimer(object): def __init__(self): pass - def timed(self, *a, **kw): - def wrapper(func): - return func - return wrapper + def timing(self, *a, **kw): + pass class Client(object): @@ -77,7 +74,6 @@ class Client(object): self._gauge = NoopGauge() self._timer = NoopTimer() self.connection = NoopConnection() - pass def get_counter(self, *a, **kw): return self._counter diff --git a/designate/service.py b/designate/service.py index 83477eb85..54f5d1914 100644 --- a/designate/service.py +++ b/designate/service.py @@ -64,7 +64,6 @@ class Service(service.Service): self._service_config = CONF['service:%s' % self.service_name] policy.init() - metrics.init() # NOTE(kiall): All services need RPC initialized, as this is used # for clients AND servers. Hence, this is common to @@ -253,6 +252,8 @@ class DNSService(object): def __init__(self, *args, **kwargs): super(DNSService, self).__init__(*args, **kwargs) + metrics.init() + # Eventet will complain loudly about our use of multiple greentheads # reading/writing to the UDP socket at once. Disable this warning. eventlet.debug.hub_prevent_multiple_readers(False) diff --git a/designate/tests/unit/test_metrics/__init__.py b/designate/tests/unit/metrics/__init__.py similarity index 100% rename from designate/tests/unit/test_metrics/__init__.py rename to designate/tests/unit/metrics/__init__.py diff --git a/designate/tests/unit/test_metrics/test_metrics.py b/designate/tests/unit/metrics/test_metrics.py similarity index 52% rename from designate/tests/unit/test_metrics/test_metrics.py rename to designate/tests/unit/metrics/test_metrics.py index 44ab16f9f..d5c5d1839 100644 --- a/designate/tests/unit/test_metrics/test_metrics.py +++ b/designate/tests/unit/metrics/test_metrics.py @@ -13,97 +13,113 @@ # License for the specific language governing permissions and limitations # under the License. # +import time import mock import monascastatsd - -from designate.metrics import Metrics -from designate.metrics_client import noop -from designate.tests import fixtures -from designate.tests import TestCase - from oslo_config import cfg from oslo_config import fixture as cfg_fixture +from designate import metrics +from designate.metrics_client import noop +from designate.tests import TestCase +from designate.tests import fixtures + class TestNoopMetrics(TestCase): - def setUp(self): super(TestCase, self).setUp() self.stdlog = fixtures.StandardLogging() self.useFixture(self.stdlog) self.CONF = self.useFixture(cfg_fixture.Config(cfg.CONF)).conf - self.metrics = Metrics() - self.metrics._client = noop.Client() + self.CONF.set_override('enabled', False, 'monasca:statsd') - def test_noop_metrics_enabled(self): - self.CONF.set_override('enabled', True, 'monasca:statsd') - self.metrics.init() - self.assertIn("Using noop metrics client. Metrics will be ignored.", - self.stdlog.logger.output) - - def test_noop_metrics_disabled(self): - with mock.patch('designate.metrics_client.noop.LOG') as log_mock: - self.metrics.init() - log_mock.error.assert_not_called() + def test_monasca_metrics_disabled(self): + self.metrics = metrics.Metrics() + self.assertIsInstance(self.metrics.client, noop.Client) + self.assertIn('Statsd disabled', self.stdlog.logger.output) def test_noop_metrics_client_getters(self): - self.CONF.set_override('enabled', True, 'monasca:statsd') - self.metrics.init() + self.metrics = metrics.Metrics() self.assertIsInstance(self.metrics.counter('name'), noop.NoopCounter) self.assertIsInstance(self.metrics.gauge(), noop.NoopGauge) self.assertIsInstance(self.metrics.timer(), noop.NoopTimer) - self.assertIsNotNone(self.metrics.timed.__self__) + self.assertIsNotNone(self.metrics.timer.__self__) def test_noop_metrics_client_timed(self): - timer = self.metrics._client.get_timer() + self.metrics = metrics.Metrics() + timer = self.metrics.client.get_timer() - @timer.timed('timed.test') def func(a): - return a + start_time = time.time() + try: + return a + finally: + timer.timing('mdns.xfr.zone_sync', time.time() - start_time) + result = func(1) self.assertEqual(result, 1) class TestMonascaMetrics(TestCase): - def setUp(self): super(TestCase, self).setUp() self.stdlog = fixtures.StandardLogging() self.useFixture(self.stdlog) self.CONF = self.useFixture(cfg_fixture.Config(cfg.CONF)).conf - self.metrics = Metrics() - - def test_monasca_metrics_enabled(self): self.CONF.set_override('enabled', True, 'monasca:statsd') - self.metrics.init() - self.assertIn("Statsd reports to 127.0.0.1 8125", - self.stdlog.logger.output) - - def test_monasca_metrics_disabled(self): - self.metrics.init() - self.assertIn( - "Statsd disabled", - self.stdlog.logger.output) @mock.patch('socket.socket.connect') - @mock.patch('socket.socket.send') - def test_monasca_metrics_client_getters(self, conn_mock, send_mock): - self.CONF.set_override('enabled', True, 'monasca:statsd') - self.metrics.init() + def test_monasca_metrics_enabled(self, conn_mock): + self.metrics = metrics.Metrics() + + self.assertIsInstance(self.metrics.client, monascastatsd.client.Client) + self.assertIn('Statsd reports to 127.0.0.1:8125', + self.stdlog.logger.output) + self.assertTrue(conn_mock.called) + + @mock.patch('socket.socket.connect') + def test_monasca_metrics_client_getters(self, conn_mock): + self.metrics = metrics.Metrics() + self.assertIsInstance(self.metrics.counter('name'), monascastatsd.counter.Counter) self.assertIsInstance(self.metrics.gauge(), monascastatsd.gauge.Gauge) self.assertIsInstance(self.metrics.timer(), monascastatsd.timer.Timer) - self.assertIsNotNone(self.metrics.timed.__self__) + self.assertIsNotNone(self.metrics.timer.__self__) - def test_monasca_metrics_client_timed(self): - timer = self.metrics._client.get_timer() + self.assertTrue(conn_mock.called) + + @mock.patch('socket.socket.send') + @mock.patch('socket.socket.connect') + def test_monasca_metrics_client_timed(self, conn_mock, send_mock): + self.metrics = metrics.Metrics() + timer = self.metrics.client.get_timer() - @timer.timed('timed.test') def func(a): - return a + start_time = time.time() + try: + return a + finally: + timer.timing('mdns.xfr.zone_sync', time.time() - start_time) + result = func(1) self.assertEqual(result, 1) + self.assertTrue(conn_mock.called) + self.assertTrue(send_mock.called) + + def test_monasca_enabled_but_client_not_installed(self): + restore = metrics.monascastatsd + try: + metrics.monascastatsd = None + self.metrics = metrics.Metrics() + self.assertIsInstance(self.metrics.client, noop.Client) + self.assertIn( + 'monasca-statsd client not installed. ' + 'Metrics will be ignored.', + self.stdlog.logger.output + ) + finally: + metrics.monascastatsd = restore