Merge "Improve StatsD metric precision"

openstack/proxy.py

@@ -50,6 +50,12 @@ def _check_resource(strict=False):
     return wrap
 
 
+def normalize_metric_name(name):
+    name = name.replace('.', '_')
+    name = name.replace(':', '_')
+    return name
+
+
 class Proxy(adapter.Adapter):
     """Represents a service."""
 
@@ -204,22 +210,35 @@ class Proxy(adapter.Adapter):
             self._report_stats_influxdb(response, url, method, exc)
 
     def _report_stats_statsd(self, response, url=None, method=None, exc=None):
-        if response is not None and not url:
+        try:
-            url = response.request.url
+            if response is not None and not url:
-        if response is not None and not method:
+                url = response.request.url
-            method = response.request.method
+            if response is not None and not method:
-        name_parts = self._extract_name(url,
+                method = response.request.method
-                                        self.service_type,
+            name_parts = [
-                                        self.session.get_project_id())
+                normalize_metric_name(f) for f in
-        key = '.'.join(
+                self._extract_name(
-            [self._statsd_prefix, self.service_type, method]
+                    url, self.service_type, self.session.get_project_id())
-            + name_parts)
+            ]
-        with self._statsd_client.pipeline() as pipe:
+            key = '.'.join(
-            if response is not None:
+                [self._statsd_prefix,
-                pipe.timing(key, response.elapsed)
+                 normalize_metric_name(self.service_type), method,
-                pipe.incr(key)
+                 '_'.join(name_parts)
-            elif exc is not None:
+                 ])
-                pipe.incr('%s.failed' % key)
+            with self._statsd_client.pipeline() as pipe:
+                if response is not None:
+                    duration = int(response.elapsed.total_seconds() * 1000)
+                    metric_name = '%s.%s' % (key, str(response.status_code))
+                    pipe.timing(metric_name, duration)
+                    pipe.incr(metric_name)
+                    if duration > 1000:
+                        pipe.incr('%s.over_1000' % key)
+                elif exc is not None:
+                    pipe.incr('%s.failed' % key)
+                pipe.incr('%s.attempted' % key)
+        except Exception:
+            # We do not want errors in metric reporting ever break client
+            self.log.exception("Exception reporting metrics")
 
     def _report_stats_prometheus(self, response, url=None, method=None,
                                  exc=None):
@@ -253,9 +272,12 @@ class Proxy(adapter.Adapter):
             method = response.request.method
         tags = dict(
             method=method,
-            name='_'.join(self._extract_name(
+            name='_'.join([
-                url, self.service_type,
+                normalize_metric_name(f) for f in
-                self.session.get_project_id()))
+                self._extract_name(
+                    url, self.service_type,
+                    self.session.get_project_id())
+            ])
         )
         fields = dict(
             attempted=1

openstack/tests/unit/test_stats.py

@@ -23,7 +23,9 @@ import threading
 import time
 
 import fixtures
+from keystoneauth1 import exceptions
 import prometheus_client
+from requests import exceptions as rexceptions
 import testtools.content
 
 from openstack.tests.unit import base
@@ -175,7 +177,7 @@ class TestStats(base.TestCase):
         self.assert_calls()
 
         self.assert_reported_stat(
-            'openstack.api.identity.GET.projects', value='1', kind='c')
+            'openstack.api.identity.GET.projects.200', value='1', kind='c')
         self.assert_prometheus_stat(
             'openstack_http_requests_total', 1, dict(
                 service_type='identity',
@@ -196,7 +198,7 @@ class TestStats(base.TestCase):
         self.assert_calls()
 
         self.assert_reported_stat(
-            'openstack.api.identity.GET.projects', value='1', kind='c')
+            'openstack.api.identity.GET.projects.200', value='1', kind='c')
         self.assert_prometheus_stat(
             'openstack_http_requests_total', 1, dict(
                 service_type='identity',
@@ -217,7 +219,11 @@ class TestStats(base.TestCase):
         self.assert_calls()
 
         self.assert_reported_stat(
-            'openstack.api.compute.GET.servers.detail', value='1', kind='c')
+            'openstack.api.compute.GET.servers_detail.200',
+            value='1', kind='c')
+        self.assert_reported_stat(
+            'openstack.api.compute.GET.servers_detail.200',
+            value='0', kind='ms')
         self.assert_prometheus_stat(
             'openstack_http_requests_total', 1, dict(
                 service_type='compute',
@@ -237,7 +243,11 @@ class TestStats(base.TestCase):
         self.assert_calls()
 
         self.assert_reported_stat(
-            'openstack.api.compute.GET.servers', value='1', kind='c')
+            'openstack.api.compute.GET.servers.200', value='1', kind='c')
+        self.assert_reported_stat(
+            'openstack.api.compute.GET.servers.200', value='0', kind='ms')
+        self.assert_reported_stat(
+            'openstack.api.compute.GET.servers.attempted', value='1', kind='c')
         self.assert_prometheus_stat(
             'openstack_http_requests_total', 1, dict(
                 service_type='compute',
@@ -245,6 +255,49 @@ class TestStats(base.TestCase):
                 method='GET',
                 status_code='200'))
 
+    def test_servers_error(self):
+
+        mock_uri = 'https://compute.example.com/v2.1/servers'
+
+        self.register_uris([
+            dict(method='GET', uri=mock_uri, status_code=500,
+                 json={})])
+
+        self.cloud.compute.get('/servers')
+        self.assert_calls()
+
+        self.assert_reported_stat(
+            'openstack.api.compute.GET.servers.500', value='1', kind='c')
+        self.assert_reported_stat(
+            'openstack.api.compute.GET.servers.500', value='0', kind='ms')
+        self.assert_reported_stat(
+            'openstack.api.compute.GET.servers.attempted', value='1', kind='c')
+        self.assert_prometheus_stat(
+            'openstack_http_requests_total', 1, dict(
+                service_type='compute',
+                endpoint=mock_uri,
+                method='GET',
+                status_code='500'))
+
+    def test_timeout(self):
+
+        mock_uri = 'https://compute.example.com/v2.1/servers'
+
+        self.register_uris([
+            dict(method='GET', uri=mock_uri,
+                 exc=rexceptions.ConnectTimeout)
+        ])
+
+        try:
+            self.cloud.compute.get('/servers')
+        except exceptions.ConnectTimeout:
+            pass
+
+        self.assert_reported_stat(
+            'openstack.api.compute.GET.servers.failed', value='1', kind='c')
+        self.assert_reported_stat(
+            'openstack.api.compute.GET.servers.attempted', value='1', kind='c')
+
 
 class TestNoStats(base.TestCase):
 

releasenotes/notes/improve-metrics-5d7ce70ce4021d72.yaml

@@ -0,0 +1,5 @@
+---
+upgrade:
+  - |
+    API metrics emitted by OpenStackSDK to StatsD now contain status_code
+    part of the metric name in order to improve information precision.