From 2eb67919cfb18135f34cbf2d77f28f171ec0bf31 Mon Sep 17 00:00:00 2001 From: Philip Jenvey Date: Fri, 26 Jun 2020 18:40:43 -0700 Subject: [PATCH] feat: fixup metrics + tags remove datadog's ThreadStats (as it only speaks datadog's http protocol) w/ markus backed by the plain statsd compatible Datadog backend upgrade datadog to avoid its own telemetry metrics producing garbage tags Co-authored-by: JR Conlin Closes #1400 --- autopush/metrics.py | 62 +++++++++++++++--------------- autopush/tests/test_integration.py | 12 +++--- autopush/tests/test_metrics.py | 30 +++++++-------- docs/api/metrics.rst | 2 +- requirements.in | 4 +- requirements.txt | 7 ++-- 6 files changed, 56 insertions(+), 61 deletions(-) diff --git a/autopush/metrics.py b/autopush/metrics.py index f09bc176..e5b37632 100644 --- a/autopush/metrics.py +++ b/autopush/metrics.py @@ -8,8 +8,7 @@ from twisted.internet import reactor -import datadog -from datadog import ThreadStats +import markus from autopush import logging @@ -65,56 +64,55 @@ def make_tags(base=None, **kwargs): return tags -class DatadogMetrics(object): - """DataDog Metric backend""" - def __init__(self, api_key=None, app_key=None, - hostname=None, statsd_host=None, statsd_port=None, - flush_interval=10, namespace="autopush"): - - datadog.initialize( - api_key=api_key, - app_key=app_key, - host_name=hostname, - statsd_host=statsd_host, - statsd_port=statsd_port, - ) - self._client = ThreadStats() - self._flush_interval = flush_interval +class TaggedMetrics(IMetrics): + """DataDog like tagged Metric backend""" + def __init__(self, hostname, statsd_host=None, statsd_port=None, + namespace="autopush"): + markus.configure( + backends=[{ + 'class': 'markus.backends.datadog.DatadogMetrics', + 'options': { + 'statsd_host': statsd_host, + 'statsd_port': statsd_port, + }}]) + self._client = markus.get_metrics(namespace) self._host = hostname self._namespace = namespace def _prefix_name(self, name): - return "%s.%s" % (self._namespace, name) + return name def start(self): - self._client.start(flush_interval=self._flush_interval, - roll_up_interval=self._flush_interval) + pass - def increment(self, name, count=1, **kwargs): - self._client.increment(self._prefix_name(name), count, host=self._host, - **kwargs) + def _make_tags(self, tags): + if tags is None: + tags = [] + tags.append('host:%s' % self._host) + return tags - def gauge(self, name, count, **kwargs): - self._client.gauge(self._prefix_name(name), count, host=self._host, - **kwargs) + def increment(self, name, count=1, tags=None, **kwargs): + self._client.incr(self._prefix_name(name), count, + tags=self._make_tags(tags)) - def timing(self, name, duration, **kwargs): + def gauge(self, name, count, tags=None, **kwargs): + self._client.gauge(self._prefix_name(name), count, + tags=self._make_tags(tags)) + + def timing(self, name, duration, tags=None, **kwargs): self._client.timing(self._prefix_name(name), value=duration, - host=self._host, **kwargs) + tags=self._make_tags(tags)) def from_config(conf): # type: (AutopushConfig) -> IMetrics """Create an IMetrics from the given config""" if conf.statsd_host: - return DatadogMetrics( + return TaggedMetrics( hostname=logging.instance_id_or_hostname if conf.ami_id else conf.hostname, - api_key=conf.datadog_api_key, - app_key=conf.datadog_app_key, statsd_host=conf.statsd_host, statsd_port=conf.statsd_port, - flush_interval=conf.datadog_flush_interval, ) else: return SinkMetrics() diff --git a/autopush/tests/test_integration.py b/autopush/tests/test_integration.py index 6b9ad104..280455e4 100644 --- a/autopush/tests/test_integration.py +++ b/autopush/tests/test_integration.py @@ -49,7 +49,7 @@ from autopush.logging import begin_or_register from autopush.main import ConnectionApplication, EndpointApplication from autopush.utils import base64url_encode, normalize_id -from autopush.metrics import SinkMetrics, DatadogMetrics +from autopush.metrics import SinkMetrics, TaggedMetrics import autopush.tests from autopush.tests.support import _TestingLogObserver from autopush.websocket import PushServerFactory @@ -538,7 +538,7 @@ def test_legacy_simplepush_record(self): db.Message.all_channels = safe yield self.shut_down(client) - @patch("autopush.metrics.datadog") + @patch("autopush.metrics.markus") @inlineCallbacks def test_webpush_data_delivery_to_disconnected_client(self, m_ddog): tests = { @@ -553,8 +553,8 @@ def test_webpush_data_delivery_to_disconnected_client(self, m_ddog): data=b"\xc3\x28\xa0\xa1\xe2\x28\xa1", result="wyigoeIooQ"), } # Piggy back a check for stored source metrics - self.conn.db.metrics = DatadogMetrics( - "someapikey", "someappkey", namespace="testpush", + self.conn.db.metrics = TaggedMetrics( + namespace="testpush", hostname="localhost") self.conn.db.metrics._client = Mock() @@ -584,8 +584,8 @@ def test_webpush_data_delivery_to_disconnected_client(self, m_ddog): yield client.ack(chan, result["version"]) assert self.logs.logged_ci(lambda ci: 'message_size' in ci) - inc_call = self.conn.db.metrics._client.increment.call_args_list[5] - assert inc_call[1]['tags'] == ['source:Stored'] + inc_call = self.conn.db.metrics._client.incr.call_args_list[5] + assert inc_call[1]['tags'] == ['source:Stored', 'host:localhost'] yield self.shut_down(client) @inlineCallbacks diff --git a/autopush/tests/test_metrics.py b/autopush/tests/test_metrics.py index 5b0f49c0..2fa8401b 100644 --- a/autopush/tests/test_metrics.py +++ b/autopush/tests/test_metrics.py @@ -5,7 +5,7 @@ from autopush.metrics import ( IMetrics, - DatadogMetrics, + TaggedMetrics, SinkMetrics, periodic_reporter, ) @@ -32,27 +32,23 @@ def test_passing(self): assert sm.timing("test", 10) is None -class DatadogMetricsTestCase(unittest.TestCase): - @patch("autopush.metrics.datadog") - def test_basic(self, mock_dog): - hostname = "localhost" - - m = DatadogMetrics("someapikey", "someappkey", namespace="testpush", - hostname="localhost") - assert len(mock_dog.mock_calls) > 0 +class TaggedMetricsTestCase(unittest.TestCase): + @patch("autopush.metrics.markus") + def test_basic(self, mock_tag): + m = TaggedMetrics(namespace="testpush", hostname="localhost") + assert len(mock_tag.mock_calls) > 0 m._client = Mock() m.start() - m._client.start.assert_called_with(flush_interval=10, - roll_up_interval=10) m.increment("test", 5) - m._client.increment.assert_called_with("testpush.test", 5, - host=hostname) + # Namespace is now auto-prefixed by the underlying markus lib + m._client.incr.assert_called_with("test", 5, + tags=['host:localhost']) m.gauge("connection_count", 200) - m._client.gauge.assert_called_with("testpush.connection_count", 200, - host=hostname) + m._client.gauge.assert_called_with("connection_count", 200, + tags=['host:localhost']) m.timing("lifespan", 113) - m._client.timing.assert_called_with("testpush.lifespan", value=113, - host=hostname) + m._client.timing.assert_called_with("lifespan", value=113, + tags=['host:localhost']) class PeriodicReporterTestCase(unittest.TestCase): diff --git a/docs/api/metrics.rst b/docs/api/metrics.rst index 82258202..5ac98719 100644 --- a/docs/api/metrics.rst +++ b/docs/api/metrics.rst @@ -21,7 +21,7 @@ Implementations :special-members: __init__ :member-order: bysource -.. autoclass:: DatadogMetrics +.. autoclass:: TaggedMetrics :members: :special-members: __init__ :member-order: bysource diff --git a/requirements.in b/requirements.in index 229c8795..4582ccc1 100644 --- a/requirements.in +++ b/requirements.in @@ -2,7 +2,7 @@ apns attrs autobahn[twisted] boto3 -cffi +cffi ; platform_python_implementation == "CPython" click configargparse cryptography @@ -13,6 +13,7 @@ firebase-admin hyper marshmallow marshmallow-polyfield +markus oauth2client objgraph pyasn1 @@ -25,7 +26,6 @@ service-identity simplejson treq twisted --e git+https://github.com/habnabit/txstatsd.git@157ef85fbdeafe23865c7c4e176237ffcb3c3f1f#egg=txStatsD-master typing ua-parser wsaccel ; platform_python_implementation == "CPython" diff --git a/requirements.txt b/requirements.txt index 5bc3932c..aa3f2a1f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,3 @@ --e git+https://github.com/habnabit/txstatsd.git@157ef85fbdeafe23865c7c4e176237ffcb3c3f1f#egg=txStatsD-master apns==2.0.1 attrs==19.3.0 autobahn[twisted]==19.11.2 @@ -8,15 +7,16 @@ botocore==1.14.11 # via boto3, s3transfer cachecontrol==0.12.6 # via firebase-admin cachetools==3.1.1 # via google-auth certifi==2019.11.28 # via requests -cffi==1.13.2 +cffi==1.13.2 ; platform_python_implementation == "CPython" chardet==3.0.4 # via requests click==7.0 configargparse==1.0 +configparser==4.0.2 # via datadog constantly==15.1.0 # via twisted contextlib2==0.6.0.post1 # via raven cryptography==2.8 cyclone==1.2 -datadog==0.34.0 +datadog==0.37.1 decorator==4.4.1 # via datadog docutils==0.15.2 # via botocore ecdsa==0.15 # via python-jose @@ -45,6 +45,7 @@ idna==2.8 # via hyperlink, requests, twisted incremental==17.5.0 # via treq, twisted ipaddress==1.0.23 # via cryptography, service-identity jmespath==0.9.4 # via boto3, botocore +markus==2.2.0 marshmallow-polyfield==4.2 marshmallow==2.19.5 msgpack==0.6.2 # via cachecontrol