Prevent random values to become metrics labels values (#3534)

* Prevent random values to become metrics labels values

* Add method and status to size and duration metrics

* Fix assertion

Author: leplatrem

kinto/core/initialization.py

@@ -472,10 +472,18 @@ def on_new_response(event):
             auth, user_id = user_id.split(":")
             metrics_service.count("users", unique=[("auth", auth), ("userid", user_id)])
 
+        status = event.response.status_code
+
+        if status >= 400:
+            # Prevent random values of 404 responses to become label values.
+            request_matchdict = {}
+        else:
+            request_matchdict = dict(request.matchdict or {})
+
         # Add extra labels to metrics, based on fields extracted from the request matchdict.
         metrics_matchdict_fields = aslist(settings["metrics_matchdict_fields"])
         # Turn the `id` field of object endpoints into `{resource}_id` (eg. `mushroom_id`, `bucket_id`)
-        enhanced_matchdict = dict(request.matchdict or {})
+        enhanced_matchdict = request_matchdict
         try:
             enhanced_matchdict[request.current_resource_name + "_id"] = enhanced_matchdict.get(
                 "id", ""
@@ -487,8 +495,6 @@ def on_new_response(event):
             (field, enhanced_matchdict.get(field, "")) for field in metrics_matchdict_fields
         ]
 
-        status = event.response.status_code
-
         service = request.current_service
         if service:
             # Use the service name as endpoint if available.
@@ -501,35 +507,30 @@ def on_new_response(event):
                 "unnamed" if status != 404 else "unknown"
             )  # Do not multiply cardinality for unknown endpoints.
 
+        request_labels = [
+            ("method", request.method.lower()),
+            ("endpoint", endpoint),
+            ("status", str(status)),
+        ] + metrics_matchdict_labels
+
         # Count served requests.
-        metrics_service.count(
-            "request_summary",
-            unique=[
-                ("method", request.method.lower()),
-                ("endpoint", endpoint),
-                ("status", str(status)),
-            ]
-            + metrics_matchdict_labels,
-        )
+        metrics_service.count("request_summary", unique=request_labels)
 
         try:
             current = utils.msec_time()
             duration = current - request._received_at
             metrics_service.timer(
                 "request_duration",
                 value=duration,
-                labels=[("endpoint", endpoint), ("method", request.method.lower())]
-                + metrics_matchdict_labels,
+                labels=request_labels,
             )
         except AttributeError:  # pragma: no cover
             # Logging was not setup in this Kinto app (unlikely but possible)
             pass
 
         # Observe response size.
         metrics_service.observe(
-            "request_size",
-            len(event.response.body or b""),
-            labels=[("endpoint", endpoint)] + metrics_matchdict_labels,
+            "request_size", len(event.response.body or b""), labels=request_labels
         )
 
         # Count authentication verifications.

tests/core/test_initialization.py

@@ -433,7 +433,7 @@ def test_statsd_observe_request_size(self):
         self.mocked().observe.assert_any_call(
             "request_size",
             len("{}"),
-            labels=[("endpoint", "heartbeat")],
+            labels=[("method", "get"), ("endpoint", "heartbeat"), ("status", "200")],
         )
 
     def test_statsd_observe_request_duration(self):
@@ -443,7 +443,7 @@ def test_statsd_observe_request_duration(self):
         self.mocked().timer.assert_any_call(
             "request_duration",
             value=mock.ANY,
-            labels=[("endpoint", "heartbeat"), ("method", "get")],
+            labels=[("method", "get"), ("endpoint", "heartbeat"), ("status", "200")],
         )
 
     def test_statsd_counts_unknown_urls(self):

tests/plugins/test_prometheus.py

@@ -196,10 +196,10 @@ def test_metrics_excluded_labels(self):
         self.assertNotIn("group_id=", resp.text)
         self.assertNotIn("record_id=", resp.text)
         self.assertIn(
-            'kintoprod_request_size_count{bucket_id="bid",collection_id="",endpoint="group-object"}',
+            'kintoprod_request_size_count{bucket_id="bid",collection_id="",endpoint="group-object",method="put",status="201"}',
             resp.text,
         )
         self.assertIn(
-            'kintoprod_request_size_count{bucket_id="bid",collection_id="cid",endpoint="record-object"}',
+            'kintoprod_request_size_count{bucket_id="bid",collection_id="cid",endpoint="record-object",method="put",status="201"}',
             resp.text,
         )

tests/test_views_metrics.py

@@ -31,21 +31,21 @@ def test_metrics_have_matchdict_labels(self):
         self.app.get("/buckets/beers/collections/barley/records", headers=self.headers)
 
         resp = self.app.get("/__metrics__")
-        print(resp.text)
+
         self.assertIn(
-            'request_size_sum{bucket_id="beers",collection_id="",endpoint="bucket-object",group_id="",record_id=""}',
+            'request_size_sum{bucket_id="beers",collection_id="",endpoint="bucket-object",group_id="",method="put",record_id="",status="201"}',
             resp.text,
         )
         self.assertIn(
-            'request_size_sum{bucket_id="beers",collection_id="",endpoint="group-object",group_id="amateurs",record_id=""}',
+            'request_size_sum{bucket_id="beers",collection_id="",endpoint="group-object",group_id="amateurs",method="put",record_id="",status="201"}',
             resp.text,
         )
         self.assertIn(
             'request_summary_total{bucket_id="beers",collection_id="barley",endpoint="collection-object",group_id="",method="put",record_id="",status="201"}',
             resp.text,
         )
         self.assertIn(
-            'request_duration_sum{bucket_id="beers",collection_id="barley",endpoint="record-object",group_id="",method="put",record_id="abc"}',
+            'request_duration_sum{bucket_id="beers",collection_id="barley",endpoint="record-object",group_id="",method="put",record_id="abc",status="201"}',
             resp.text,
         )
 
@@ -61,3 +61,13 @@ def test_metrics_have_matchdict_labels(self):
             'request_summary_total{bucket_id="beers",collection_id="barley",endpoint="record-plural",group_id="",method="get",record_id="",status="200"}',
             resp.text,
         )
+
+    def test_4xx_do_not_have_matchdict_labels_values(self):
+        self.app.get("/buckets/water", headers=self.headers, status=403)
+
+        resp = self.app.get("/__metrics__")
+
+        self.assertIn(
+            'request_summary_total{bucket_id="",collection_id="",endpoint="bucket-object",group_id="",method="get",record_id="",status="403"}',
+            resp.text,
+        )