From 5e9995834e64acf5e8a8cc6656b6a4b91cd81aaa Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Mon, 12 Aug 2024 15:14:03 -0400 Subject: [PATCH] bug-1906108: rework pageview counters to timers This reworks all the pageview counting metrics to a timing metric single key `socorro.collector.pageview_duration_ms` with the method and path as tags. This makes it easier to do a dashboard for. --- antenna/breakpad_resource.py | 4 +++- antenna/health_resource.py | 16 ++++++++++++---- antenna/statsd_metrics.yaml | 22 ++++++---------------- tests/test_breakpad_resource.py | 6 +++++- tests/test_sentry.py | 11 +++++++++++ 5 files changed, 37 insertions(+), 22 deletions(-) diff --git a/antenna/breakpad_resource.py b/antenna/breakpad_resource.py index 90805f6d..2e8379aa 100644 --- a/antenna/breakpad_resource.py +++ b/antenna/breakpad_resource.py @@ -302,7 +302,9 @@ def cleanup_crash_report(self, raw_crash): del raw_crash[bad_field] notes.append("Removed %s from raw crash." % bad_field) - @METRICS.timer_decorator("collector.breakpad_resource.on_post.time") + @METRICS.timer_decorator( + "collector.pageview_duration_ms", tags=["method:post", "path:/submit"] + ) def on_post(self, req, resp): """Handle incoming HTTP POSTs. diff --git a/antenna/health_resource.py b/antenna/health_resource.py index 680cacca..8002ad75 100644 --- a/antenna/health_resource.py +++ b/antenna/health_resource.py @@ -14,9 +14,11 @@ class BrokenResource: """Handle ``/__broken__`` endpoint.""" + @METRICS.timer_decorator( + "collector.pageview_duration_ms", tags=["method:get", "path:/__broken__"] + ) def on_get(self, req, resp): """Implement GET HTTP request.""" - METRICS.incr("collector.health.broken.count") # This is intentional breakage raise Exception("intentional exception") @@ -27,9 +29,11 @@ class VersionResource: def __init__(self, basedir): self.basedir = basedir + @METRICS.timer_decorator( + "collector.pageview_duration_ms", tags=["method:get", "path:/__version__"] + ) def on_get(self, req, resp): """Implement GET HTTP request.""" - METRICS.incr("collector.health.version.count") version_info = get_version_info(self.basedir) resp.content_type = "application/json; charset=utf-8" @@ -40,9 +44,11 @@ def on_get(self, req, resp): class LBHeartbeatResource: """Handle ``/__lbheartbeat__`` to let the load balancing know application health.""" + @METRICS.timer_decorator( + "collector.pageview_duration_ms", tags=["method:get", "path:/__lbheartbeat__"] + ) def on_get(self, req, resp): """Implement GET HTTP request.""" - METRICS.incr("collector.health.lbheartbeat.count") resp.content_type = "application/json; charset=utf-8" resp.status = falcon.HTTP_200 @@ -75,9 +81,11 @@ class HeartbeatResource: def __init__(self, app): self.antenna_app = app + @METRICS.timer_decorator( + "collector.pageview_duration_ms", tags=["method:get", "path:/__heartbeat__"] + ) def on_get(self, req, resp): """Implement GET HTTP request.""" - METRICS.incr("collector.health.heartbeat.count") state = HealthState() # So we're going to think of Antenna like a big object graph and diff --git a/antenna/statsd_metrics.yaml b/antenna/statsd_metrics.yaml index 1ae132b1..83ba79b5 100644 --- a/antenna/statsd_metrics.yaml +++ b/antenna/statsd_metrics.yaml @@ -118,22 +118,12 @@ socorro.collector.crashmover.crash_publish.time: description: | Timer for how long it takes to publish a crash report for processing. -socorro.collector.health.broken.count: - type: "incr" - description: | - Counter for ``/__broken__`` view. - -socorro.collector.health.version.count: - type: "incr" +socorro.collector.pageview_duration_ms: + type: "timing" description: | - Counter for ``/__version__`` view. + Timer for how long it took to handle an HTTP request. -socorro.collector.health.lbheartbeat.count: - type: "incr" - description: | - Counter for ``/__lbheartbeat__`` view. + Tags: -socorro.collector.health.heartbeat.count: - type: "incr" - description: | - Counter for ``/__heartbeat__`` view. + * ``path``: the request path + * ``method``: the request method diff --git a/tests/test_breakpad_resource.py b/tests/test_breakpad_resource.py index 3cab1838..d02eb8eb 100644 --- a/tests/test_breakpad_resource.py +++ b/tests/test_breakpad_resource.py @@ -474,7 +474,11 @@ def test_submit_crash_report(self, client, metricsmock): mm.assert_timing("socorro.collector.crashmover.crash_publish.time") mm.assert_incr("socorro.collector.crashmover.save_crash.count") mm.assert_timing("socorro.collector.crashmover.crash_handling.time") - mm.assert_timing("socorro.collector.breakpad_resource.on_post.time") + + mm.assert_timing( + "socorro.collector.pageview_duration_ms", + tags=["method:post", "path:/submit", AnyTagValue("host")], + ) def test_existing_uuid(self, client): """Verify if the crash report has a uuid already, it's reused.""" diff --git a/tests/test_sentry.py b/tests/test_sentry.py index 9392fad6..bc85e66d 100644 --- a/tests/test_sentry.py +++ b/tests/test_sentry.py @@ -47,6 +47,17 @@ "post_context": ANY, "pre_context": ANY, }, + { + "abs_path": "/usr/local/lib/python3.11/site-packages/markus/main.py", + "context_line": ANY, + "filename": "markus/main.py", + "function": "_timer_decorator", + "in_app": False, + "lineno": ANY, + "module": "markus.main", + "post_context": ANY, + "pre_context": ANY, + }, { "abs_path": "/app/antenna/health_resource.py", "context_line": ANY,