From 03de34f709bc2e2c02cc68272029c1eadf14dfe7 Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Wed, 16 Oct 2024 09:57:16 -0700 Subject: [PATCH 1/5] Make query, origin, and user_id optional --- src/sentry/api/analytics.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sentry/api/analytics.py b/src/sentry/api/analytics.py index 9e170c8a51e545..e875453f138fea 100644 --- a/src/sentry/api/analytics.py +++ b/src/sentry/api/analytics.py @@ -39,15 +39,15 @@ class DevToolbarApiRequestEvent(analytics.Event): attributes = ( analytics.Attribute("view_name"), analytics.Attribute("route"), - analytics.Attribute("query_string"), - analytics.Attribute("origin"), + analytics.Attribute("query_string", required=False), + analytics.Attribute("origin", required=False), analytics.Attribute("method"), analytics.Attribute("status_code", type=int), analytics.Attribute("organization_id", type=int, required=False), analytics.Attribute("organization_slug", required=False), analytics.Attribute("project_id", type=int, required=False), analytics.Attribute("project_slug", required=False), - analytics.Attribute("user_id", type=int), + analytics.Attribute("user_id", type=int, required=False), ) From e43c983310d4656c637b5ee6dfcb7ed5729bab79 Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Wed, 16 Oct 2024 12:19:59 -0700 Subject: [PATCH 2/5] Check query param instead of header, refactor tests --- src/sentry/middleware/devtoolbar.py | 11 ++- tests/sentry/middleware/test_devtoolbar.py | 79 +++++++++------------- 2 files changed, 41 insertions(+), 49 deletions(-) diff --git a/src/sentry/middleware/devtoolbar.py b/src/sentry/middleware/devtoolbar.py index dba707ab6b974a..135c096710fe21 100644 --- a/src/sentry/middleware/devtoolbar.py +++ b/src/sentry/middleware/devtoolbar.py @@ -17,8 +17,8 @@ def __init__(self, get_response): def __call__(self, request): response = self.get_response(request) try: - # note ordering of conditions to avoid extra option queries. - if request.headers.get("queryReferrer") == "devtoolbar" and options.get( + # Note ordering of conditions to reduce option queries. GET contains the query params, regardless of method. + if request.GET.get("queryReferrer") == "devtoolbar" and options.get( "devtoolbar.analytics.enabled" ): _record_api_request(request, response) @@ -45,7 +45,12 @@ def _record_api_request(request: HttpRequest, response: HttpResponse) -> None: project_id, project_slug = parse_id_or_slug_param(project_id_or_slug) origin = origin_from_request(request) - query_string: str = get_query_string(request) # starts with '?' + query_string: str = get_query_string(request) or None + if query_string: + params = query_string.lstrip("?").split("&") + params = list(filter(lambda s: s != "queryReferrer=devtoolbar", params)) + query_string = f"?{'&'.join(params)}" if params else None + analytics.record( "devtoolbar.api_request", view_name=view_name, diff --git a/tests/sentry/middleware/test_devtoolbar.py b/tests/sentry/middleware/test_devtoolbar.py index 419c3d56570df1..c9b32a0ad84e4a 100644 --- a/tests/sentry/middleware/test_devtoolbar.py +++ b/tests/sentry/middleware/test_devtoolbar.py @@ -21,28 +21,13 @@ def factory(self): def setUp(self): # Allows changing the get_response mock for each test. - self.get_response = MagicMock(return_value=HttpResponse(status=200)) - self.middleware.get_response = self.get_response - - def make_toolbar_request( - self, - path="/", - method="GET", - headers=None, - incl_toolbar_header=True, - resolver_match="mock", - ): - headers = headers or {} - if incl_toolbar_header: - headers["queryReferrer"] = "devtoolbar" - request = getattr(self.factory, method.lower())(path, headers=headers) - request.resolver_match = MagicMock() if resolver_match == "mock" else resolver_match - return request + self.middleware.get_response = MagicMock(return_value=HttpResponse(status=200)) @override_options({"devtoolbar.analytics.enabled": True}) @patch("sentry.analytics.record") def test_basic(self, mock_record: MagicMock): - request = self.make_toolbar_request() + request = self.factory.get("/?queryReferrer=devtoolbar") + request.resolver_match = MagicMock() self.middleware(request) mock_record.assert_called() assert mock_record.call_args[0][0] == self.analytics_event_name @@ -50,13 +35,13 @@ def test_basic(self, mock_record: MagicMock): @override_options({"devtoolbar.analytics.enabled": True}) @patch("sentry.analytics.record") def test_no_devtoolbar_header(self, mock_record: MagicMock): - request = self.make_toolbar_request(incl_toolbar_header=False) + request = self.factory.get("/") + request.resolver_match = MagicMock() self.middleware(request) mock_record.assert_not_called() - request = self.make_toolbar_request( - headers={"queryReferrer": "not-toolbar"}, incl_toolbar_header=False - ) + request = self.factory.get("/?queryReferrer=not-toolbar") + request.resolver_match = MagicMock() self.middleware(request) mock_record.assert_not_called() @@ -64,7 +49,7 @@ def test_no_devtoolbar_header(self, mock_record: MagicMock): @patch("sentry.middleware.devtoolbar.logger.exception") @patch("sentry.analytics.record") def test_request_not_resolved(self, mock_record: MagicMock, mock_logger: MagicMock): - request = self.make_toolbar_request() + request = self.factory.get("/?queryReferrer=devtoolbar") request.resolver_match = None self.middleware(request) @@ -81,9 +66,8 @@ def test_view_name_and_route(self, mock_record: MagicMock): # Integration tests do a better job of testing these fields, since they involve route resolver. view_name = "my-endpoint" route = "/issues/(?P)/" - request = self.make_toolbar_request( - resolver_match=MagicMock(view_name=view_name, route=route), - ) + request = self.factory.get("/?queryReferrer=devtoolbar") + request.resolver_match = MagicMock(view_name=view_name, route=route) self.middleware(request) mock_record.assert_called() @@ -95,9 +79,8 @@ def test_view_name_and_route(self, mock_record: MagicMock): @patch("sentry.analytics.record") def test_query_string(self, mock_record: MagicMock): query = "?a=b&statsPeriod=14d" - request = self.make_toolbar_request( - path="https://sentry.io/replays/" + query, - ) + request = self.factory.get(f"/{query}&queryReferrer=devtoolbar") + request.resolver_match = MagicMock() self.middleware(request) mock_record.assert_called() @@ -108,7 +91,10 @@ def test_query_string(self, mock_record: MagicMock): @patch("sentry.analytics.record") def test_origin(self, mock_record: MagicMock): origin = "https://potato.com" - request = self.make_toolbar_request(headers={"Origin": origin}) + request = self.factory.get( + f"{origin}/?queryReferrer=devtoolbar", headers={"Origin": origin} + ) + request.resolver_match = MagicMock() self.middleware(request) mock_record.assert_called() @@ -119,7 +105,9 @@ def test_origin(self, mock_record: MagicMock): @patch("sentry.analytics.record") def test_origin_from_referrer(self, mock_record: MagicMock): origin = "https://potato.com" - request = self.make_toolbar_request(headers={"Referer": origin + "/issues/?a=b"}) + url = origin + "/issues/?a=b&queryReferrer=devtoolbar" + request = self.factory.get(url, headers={"Referer": url}) + request.resolver_match = MagicMock() self.middleware(request) mock_record.assert_called() @@ -129,8 +117,9 @@ def test_origin_from_referrer(self, mock_record: MagicMock): @override_options({"devtoolbar.analytics.enabled": True}) @patch("sentry.analytics.record") def test_response_status_code(self, mock_record: MagicMock): - request = self.make_toolbar_request() - self.get_response.return_value = HttpResponse(status=420) + request = self.factory.get("/?queryReferrer=devtoolbar") + request.resolver_match = MagicMock() + self.middleware.get_response.return_value = HttpResponse(status=420) self.middleware(request) mock_record.assert_called() @@ -141,7 +130,8 @@ def test_response_status_code(self, mock_record: MagicMock): @patch("sentry.analytics.record") def test_methods(self, mock_record: MagicMock): for method in ["GET", "POST", "PUT", "DELETE"]: - request = self.make_toolbar_request(method=method) + request = getattr(self.factory, method.lower())("/?queryReferrer=devtoolbar") + request.resolver_match = MagicMock() self.middleware(request) mock_record.assert_called() @@ -168,18 +158,17 @@ def setUp(self): @patch("sentry.analytics.record") def _test_endpoint( self, - path: str, - query_string: str, + url: str, method: str, expected_view_name: str, expected_route: str, mock_record: MagicMock, + expected_query_string=None, expected_org_id=None, expected_org_slug=None, expected_proj_id=None, expected_proj_slug=None, ): - url = path + query_string response: HttpResponse = getattr(self.client, method.lower())( url, headers={ @@ -192,7 +181,7 @@ def _test_endpoint( "devtoolbar.api_request", view_name=expected_view_name, route=expected_route, - query_string=query_string, + query_string=expected_query_string, origin=self.origin, method=method, status_code=response.status_code, @@ -205,27 +194,26 @@ def _test_endpoint( def test_organization_replays(self): self._test_endpoint( - f"/api/0/organizations/{self.organization.slug}/replays/", - "?field=id", + f"/api/0/organizations/{self.organization.slug}/replays/?field=id&queryReferrer=devtoolbar", "GET", "sentry-api-0-organization-replay-index", "^api/0/organizations/(?P[^\\/]+)/replays/$", + expected_query_string="?field=id", expected_org_slug=self.organization.slug, ) self._test_endpoint( - f"/api/0/organizations/{self.organization.id}/replays/", - "?field=id", + f"/api/0/organizations/{self.organization.id}/replays/?queryReferrer=devtoolbar&field=id", "GET", "sentry-api-0-organization-replay-index", "^api/0/organizations/(?P[^\\/]+)/replays/$", + expected_query_string="?field=id", expected_org_id=self.organization.id, ) def test_group_details(self): group = self.create_group(substatus=GroupSubStatus.NEW) self._test_endpoint( - f"/api/0/organizations/{self.organization.slug}/issues/{group.id}/", - "", + f"/api/0/organizations/{self.organization.slug}/issues/{group.id}/?queryReferrer=devtoolbar", "GET", "sentry-api-0-organization-group-group-details", "^api/0/organizations/(?P[^\\/]+)/(?:issues|groups)/(?P[^\\/]+)/$", @@ -235,8 +223,7 @@ def test_group_details(self): def test_project_user_feedback(self): # Should return 400 (no POST data) self._test_endpoint( - f"/api/0/projects/{self.organization.slug}/{self.project.id}/user-feedback/", - "", + f"/api/0/projects/{self.organization.slug}/{self.project.id}/user-feedback/?queryReferrer=devtoolbar", "POST", "sentry-api-0-project-user-reports", r"^api/0/projects/(?P[^\/]+)/(?P[^\/]+)/(?:user-feedback|user-reports)/$", From 60158fac6042c1c06068a14fa04720dfadc258ed Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Wed, 16 Oct 2024 12:31:37 -0700 Subject: [PATCH 3/5] Fix query_string typing --- src/sentry/middleware/devtoolbar.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/middleware/devtoolbar.py b/src/sentry/middleware/devtoolbar.py index 135c096710fe21..d65d07fb1e2a70 100644 --- a/src/sentry/middleware/devtoolbar.py +++ b/src/sentry/middleware/devtoolbar.py @@ -45,7 +45,7 @@ def _record_api_request(request: HttpRequest, response: HttpResponse) -> None: project_id, project_slug = parse_id_or_slug_param(project_id_or_slug) origin = origin_from_request(request) - query_string: str = get_query_string(request) or None + query_string = get_query_string(request) or None if query_string: params = query_string.lstrip("?").split("&") params = list(filter(lambda s: s != "queryReferrer=devtoolbar", params)) From a8b3ff73752cc8bf7424c9dd983d1f05d57ed2c2 Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Wed, 16 Oct 2024 14:05:41 -0700 Subject: [PATCH 4/5] Undo query_string parsing changes --- src/sentry/middleware/devtoolbar.py | 6 +----- tests/sentry/middleware/test_devtoolbar.py | 10 ++++++---- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/sentry/middleware/devtoolbar.py b/src/sentry/middleware/devtoolbar.py index d65d07fb1e2a70..0d1ef60c8e0117 100644 --- a/src/sentry/middleware/devtoolbar.py +++ b/src/sentry/middleware/devtoolbar.py @@ -45,11 +45,7 @@ def _record_api_request(request: HttpRequest, response: HttpResponse) -> None: project_id, project_slug = parse_id_or_slug_param(project_id_or_slug) origin = origin_from_request(request) - query_string = get_query_string(request) or None - if query_string: - params = query_string.lstrip("?").split("&") - params = list(filter(lambda s: s != "queryReferrer=devtoolbar", params)) - query_string = f"?{'&'.join(params)}" if params else None + query_string: str = get_query_string(request) # starts with ? if non-empty analytics.record( "devtoolbar.api_request", diff --git a/tests/sentry/middleware/test_devtoolbar.py b/tests/sentry/middleware/test_devtoolbar.py index c9b32a0ad84e4a..712dcfa3280605 100644 --- a/tests/sentry/middleware/test_devtoolbar.py +++ b/tests/sentry/middleware/test_devtoolbar.py @@ -78,8 +78,8 @@ def test_view_name_and_route(self, mock_record: MagicMock): @override_options({"devtoolbar.analytics.enabled": True}) @patch("sentry.analytics.record") def test_query_string(self, mock_record: MagicMock): - query = "?a=b&statsPeriod=14d" - request = self.factory.get(f"/{query}&queryReferrer=devtoolbar") + query = "?a=b&statsPeriod=14d&queryReferrer=devtoolbar" + request = self.factory.get("/" + query) request.resolver_match = MagicMock() self.middleware(request) @@ -198,7 +198,7 @@ def test_organization_replays(self): "GET", "sentry-api-0-organization-replay-index", "^api/0/organizations/(?P[^\\/]+)/replays/$", - expected_query_string="?field=id", + expected_query_string="?field=id&queryReferrer=devtoolbar", expected_org_slug=self.organization.slug, ) self._test_endpoint( @@ -206,7 +206,7 @@ def test_organization_replays(self): "GET", "sentry-api-0-organization-replay-index", "^api/0/organizations/(?P[^\\/]+)/replays/$", - expected_query_string="?field=id", + expected_query_string="?queryReferrer=devtoolbar&field=id", expected_org_id=self.organization.id, ) @@ -217,6 +217,7 @@ def test_group_details(self): "GET", "sentry-api-0-organization-group-group-details", "^api/0/organizations/(?P[^\\/]+)/(?:issues|groups)/(?P[^\\/]+)/$", + expected_query_string="?queryReferrer=devtoolbar", expected_org_slug=self.organization.slug, ) @@ -227,6 +228,7 @@ def test_project_user_feedback(self): "POST", "sentry-api-0-project-user-reports", r"^api/0/projects/(?P[^\/]+)/(?P[^\/]+)/(?:user-feedback|user-reports)/$", + expected_query_string="?queryReferrer=devtoolbar", expected_org_slug=self.organization.slug, expected_proj_id=self.project.id, ) From e24567013561c2f396b999904315d29aa37d9963 Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Wed, 16 Oct 2024 14:11:45 -0700 Subject: [PATCH 5/5] Ref integration test helper --- tests/sentry/middleware/test_devtoolbar.py | 23 +++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tests/sentry/middleware/test_devtoolbar.py b/tests/sentry/middleware/test_devtoolbar.py index 712dcfa3280605..ff40bfc5ee589a 100644 --- a/tests/sentry/middleware/test_devtoolbar.py +++ b/tests/sentry/middleware/test_devtoolbar.py @@ -158,17 +158,18 @@ def setUp(self): @patch("sentry.analytics.record") def _test_endpoint( self, - url: str, + path: str, + query_string: str, method: str, expected_view_name: str, expected_route: str, mock_record: MagicMock, - expected_query_string=None, expected_org_id=None, expected_org_slug=None, expected_proj_id=None, expected_proj_slug=None, ): + url = path + query_string response: HttpResponse = getattr(self.client, method.lower())( url, headers={ @@ -181,7 +182,7 @@ def _test_endpoint( "devtoolbar.api_request", view_name=expected_view_name, route=expected_route, - query_string=expected_query_string, + query_string=query_string, origin=self.origin, method=method, status_code=response.status_code, @@ -194,41 +195,41 @@ def _test_endpoint( def test_organization_replays(self): self._test_endpoint( - f"/api/0/organizations/{self.organization.slug}/replays/?field=id&queryReferrer=devtoolbar", + f"/api/0/organizations/{self.organization.slug}/replays/", + "?field=id&queryReferrer=devtoolbar", "GET", "sentry-api-0-organization-replay-index", "^api/0/organizations/(?P[^\\/]+)/replays/$", - expected_query_string="?field=id&queryReferrer=devtoolbar", expected_org_slug=self.organization.slug, ) self._test_endpoint( - f"/api/0/organizations/{self.organization.id}/replays/?queryReferrer=devtoolbar&field=id", + f"/api/0/organizations/{self.organization.id}/replays/", + "?queryReferrer=devtoolbar&field=id", "GET", "sentry-api-0-organization-replay-index", "^api/0/organizations/(?P[^\\/]+)/replays/$", - expected_query_string="?queryReferrer=devtoolbar&field=id", expected_org_id=self.organization.id, ) def test_group_details(self): group = self.create_group(substatus=GroupSubStatus.NEW) self._test_endpoint( - f"/api/0/organizations/{self.organization.slug}/issues/{group.id}/?queryReferrer=devtoolbar", + f"/api/0/organizations/{self.organization.slug}/issues/{group.id}/", + "?queryReferrer=devtoolbar", "GET", "sentry-api-0-organization-group-group-details", "^api/0/organizations/(?P[^\\/]+)/(?:issues|groups)/(?P[^\\/]+)/$", - expected_query_string="?queryReferrer=devtoolbar", expected_org_slug=self.organization.slug, ) def test_project_user_feedback(self): # Should return 400 (no POST data) self._test_endpoint( - f"/api/0/projects/{self.organization.slug}/{self.project.id}/user-feedback/?queryReferrer=devtoolbar", + f"/api/0/projects/{self.organization.slug}/{self.project.id}/user-feedback/", + "?queryReferrer=devtoolbar", "POST", "sentry-api-0-project-user-reports", r"^api/0/projects/(?P[^\/]+)/(?P[^\/]+)/(?:user-feedback|user-reports)/$", - expected_query_string="?queryReferrer=devtoolbar", expected_org_slug=self.organization.slug, expected_proj_id=self.project.id, )