From 189029600dbd86b3af5880ec9d0e64ac99ba19d7 Mon Sep 17 00:00:00 2001 From: Jerry Feng Date: Fri, 28 Mar 2025 16:11:47 -0400 Subject: [PATCH 1/3] Bundle Analysis: Add metric label for upload result --- upload/tests/views/test_bundle_analysis.py | 32 +++++++++++ upload/views/bundle_analysis.py | 65 +++++++++++++--------- 2 files changed, 71 insertions(+), 26 deletions(-) diff --git a/upload/tests/views/test_bundle_analysis.py b/upload/tests/views/test_bundle_analysis.py index 3e65c0b7c0..277d79aecf 100644 --- a/upload/tests/views/test_bundle_analysis.py +++ b/upload/tests/views/test_bundle_analysis.py @@ -810,3 +810,35 @@ def test_upload_bundle_analysis_tokenless_mismatched_branch( assert res.status_code == 401 assert res.json() == {"detail": "Not valid tokenless upload"} assert not upload.called + + +def test_bundle_analysis_view_exception_handling(db, client, mocker, mock_redis): + with patch( + "upload.views.bundle_analysis.BundleAnalysisView._handle_upload", + side_effect=Exception("Test Exception"), + ): + client = APIClient() + repository = RepositoryFactory.create() + client.credentials(HTTP_AUTHORIZATION=f"token {repository.upload_token}") + + mock_inc_counter = mocker.patch("upload.views.bundle_analysis.inc_counter") + + res = client.post( + reverse("upload-bundle-analysis"), + { + "commit": "6fd5b89357fc8cdf34d6197549ac7c6d7e5977ef", + "slug": f"{repository.author.username}::::{repository.name}", + }, + format="json", + ) + + assert res.status_code == 201 + assert res.json()["url"] is None + + # Check that the labels["result"] is "error" + mock_inc_counter.assert_any_call( + ANY, + labels=mocker.ANY, + ) + labels = mock_inc_counter.call_args[1]["labels"] + assert labels["result"] == "error" diff --git a/upload/views/bundle_analysis.py b/upload/views/bundle_analysis.py index 6bfc369654..35dd3bc5b2 100644 --- a/upload/views/bundle_analysis.py +++ b/upload/views/bundle_analysis.py @@ -48,6 +48,7 @@ "endpoint", "is_using_shelter", "position", + "result", ], ) @@ -85,20 +86,7 @@ class BundleAnalysisView(APIView, ShelterMixin): def get_exception_handler(self) -> Callable: return repo_auth_custom_exception_handler - def post(self, request: HttpRequest) -> Response: - labels = generate_upload_prometheus_metrics_labels( - action="bundle_analysis", - endpoint="bundle_analysis", - request=self.request, - is_shelter_request=self.is_shelter_request(), - position="start", - include_empty_labels=False, - ) - inc_counter( - BUNDLE_ANALYSIS_UPLOAD_VIEWS_COUNTER, - labels=labels, - ) - + def _handle_upload(self, request: HttpRequest) -> str | None: serializer = UploadSerializer(data=request.data) if not serializer.is_valid(): return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) @@ -191,18 +179,6 @@ def post(self, request: HttpRequest) -> Response: task_arguments=task_arguments, ), ) - labels = generate_upload_prometheus_metrics_labels( - action="bundle_analysis", - endpoint="bundle_analysis", - request=self.request, - is_shelter_request=self.is_shelter_request(), - position="end", - include_empty_labels=False, - ) - inc_counter( - BUNDLE_ANALYSIS_UPLOAD_VIEWS_COUNTER, - labels=labels, - ) dispatch_upload_task( task_arguments, @@ -235,4 +211,41 @@ def post(self, request: HttpRequest) -> Response: ), ) + return url + + def post(self, request: HttpRequest) -> Response: + labels = generate_upload_prometheus_metrics_labels( + action="bundle_analysis", + endpoint="bundle_analysis", + request=self.request, + is_shelter_request=self.is_shelter_request(), + position="start", + include_empty_labels=False, + ) + labels["result"] = "pending" + url = None + inc_counter( + BUNDLE_ANALYSIS_UPLOAD_VIEWS_COUNTER, + labels=labels, + ) + + upload_result = "success" + try: + url = self._handle_upload(request) + except Exception as e: + log.error( + "Error handling bundle analysis upload", + extra=dict( + error=e, + ), + exc_info=True, + ) + upload_result = "error" + + labels["position"] = "end" + labels["result"] = upload_result + inc_counter( + BUNDLE_ANALYSIS_UPLOAD_VIEWS_COUNTER, + labels=labels, + ) return Response({"url": url}, status=201) From 0d404d166efe2d69a93fd3cefaab97b95c25b7ed Mon Sep 17 00:00:00 2001 From: Jerry Feng Date: Fri, 28 Mar 2025 16:50:38 -0400 Subject: [PATCH 2/3] fix the thing --- upload/tests/views/test_bundle_analysis.py | 14 ++++++++-- upload/views/bundle_analysis.py | 32 ++++++++++++---------- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/upload/tests/views/test_bundle_analysis.py b/upload/tests/views/test_bundle_analysis.py index 277d79aecf..8037f54ca1 100644 --- a/upload/tests/views/test_bundle_analysis.py +++ b/upload/tests/views/test_bundle_analysis.py @@ -104,6 +104,7 @@ def test_upload_bundle_analysis_success(db, client, mocker, mock_redis): "endpoint": "bundle_analysis", "is_using_shelter": "no", "position": "end", + "result": "success", }, ) @@ -207,6 +208,7 @@ def test_upload_bundle_analysis_success_shelter(db, client, mocker, mock_redis): "endpoint": "bundle_analysis", "is_using_shelter": "no", "position": "end", + "result": "success", }, ) @@ -245,6 +247,7 @@ def test_upload_bundle_analysis_org_token(db, client, mocker, mock_redis): "endpoint": "bundle_analysis", "is_using_shelter": "no", "position": "end", + "result": "success", }, ) @@ -292,6 +295,7 @@ def test_upload_bundle_analysis_existing_commit(db, client, mocker, mock_redis): "endpoint": "bundle_analysis", "is_using_shelter": "no", "position": "end", + "result": "success", }, ) @@ -340,7 +344,8 @@ def test_upload_bundle_analysis_missing_args(db, client, mocker, mock_redis): "action": "bundle_analysis", "endpoint": "bundle_analysis", "is_using_shelter": "no", - "position": "start", + "position": "end", + "result": "bad_request", }, ) @@ -412,6 +417,7 @@ def test_upload_bundle_analysis_github_oidc_auth( "endpoint": "bundle_analysis", "is_using_shelter": "no", "position": "end", + "result": "success", }, ) @@ -472,6 +478,7 @@ def test_upload_bundle_analysis_measurement_datasets_created( "endpoint": "bundle_analysis", "is_using_shelter": "no", "position": "end", + "result": "success", }, ) @@ -533,6 +540,7 @@ def test_upload_bundle_analysis_measurement_timeseries_disabled( "endpoint": "bundle_analysis", "is_using_shelter": "no", "position": "end", + "result": "success", }, ) @@ -574,7 +582,8 @@ def test_upload_bundle_analysis_no_repo(db, client, mocker, mock_redis): "action": "bundle_analysis", "endpoint": "bundle_analysis", "is_using_shelter": "no", - "position": "start", + "position": "end", + "result": "error", }, ) @@ -629,6 +638,7 @@ def test_upload_bundle_analysis_tokenless_success(db, client, mocker, mock_redis "endpoint": "bundle_analysis", "is_using_shelter": "no", "position": "end", + "result": "success", }, ) diff --git a/upload/views/bundle_analysis.py b/upload/views/bundle_analysis.py index 35dd3bc5b2..c500868c4f 100644 --- a/upload/views/bundle_analysis.py +++ b/upload/views/bundle_analysis.py @@ -1,6 +1,6 @@ import logging import uuid -from typing import Any, Callable +from typing import Any, Callable, Tuple from django.conf import settings from django.http import HttpRequest @@ -86,10 +86,13 @@ class BundleAnalysisView(APIView, ShelterMixin): def get_exception_handler(self) -> Callable: return repo_auth_custom_exception_handler - def _handle_upload(self, request: HttpRequest) -> str | None: + def _handle_upload(self, request: HttpRequest) -> Tuple[str, Response]: serializer = UploadSerializer(data=request.data) if not serializer.is_valid(): - return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) + return ( + "bad_request", + Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST), + ) data = serializer.validated_data if isinstance(request.user, Owner): @@ -211,7 +214,7 @@ def _handle_upload(self, request: HttpRequest) -> str | None: ), ) - return url + return ("success", Response({"url": url}, status=201)) def post(self, request: HttpRequest) -> Response: labels = generate_upload_prometheus_metrics_labels( @@ -223,15 +226,14 @@ def post(self, request: HttpRequest) -> Response: include_empty_labels=False, ) labels["result"] = "pending" - url = None inc_counter( BUNDLE_ANALYSIS_UPLOAD_VIEWS_COUNTER, labels=labels, ) - upload_result = "success" try: - url = self._handle_upload(request) + upload_result, response = self._handle_upload(request) + return response except Exception as e: log.error( "Error handling bundle analysis upload", @@ -241,11 +243,11 @@ def post(self, request: HttpRequest) -> Response: exc_info=True, ) upload_result = "error" - - labels["position"] = "end" - labels["result"] = upload_result - inc_counter( - BUNDLE_ANALYSIS_UPLOAD_VIEWS_COUNTER, - labels=labels, - ) - return Response({"url": url}, status=201) + raise + finally: + labels["position"] = "end" + labels["result"] = upload_result + inc_counter( + BUNDLE_ANALYSIS_UPLOAD_VIEWS_COUNTER, + labels=labels, + ) From 93d7442074fa42a444e0ae9705abb0aba5595d39 Mon Sep 17 00:00:00 2001 From: Jerry Feng Date: Fri, 28 Mar 2025 17:04:02 -0400 Subject: [PATCH 3/3] fix test --- upload/tests/views/test_bundle_analysis.py | 47 +++++++++++----------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/upload/tests/views/test_bundle_analysis.py b/upload/tests/views/test_bundle_analysis.py index 8037f54ca1..b227081251 100644 --- a/upload/tests/views/test_bundle_analysis.py +++ b/upload/tests/views/test_bundle_analysis.py @@ -822,30 +822,29 @@ def test_upload_bundle_analysis_tokenless_mismatched_branch( assert not upload.called -def test_bundle_analysis_view_exception_handling(db, client, mocker, mock_redis): - with patch( - "upload.views.bundle_analysis.BundleAnalysisView._handle_upload", - side_effect=Exception("Test Exception"), - ): - client = APIClient() - repository = RepositoryFactory.create() - client.credentials(HTTP_AUTHORIZATION=f"token {repository.upload_token}") - - mock_inc_counter = mocker.patch("upload.views.bundle_analysis.inc_counter") - - res = client.post( - reverse("upload-bundle-analysis"), - { - "commit": "6fd5b89357fc8cdf34d6197549ac7c6d7e5977ef", - "slug": f"{repository.author.username}::::{repository.name}", - }, - format="json", - ) - - assert res.status_code == 201 - assert res.json()["url"] is None - - # Check that the labels["result"] is "error" +def test_upload_bundle_analysis_view_exception_handling(db, client, mocker, mock_redis): + try: + with patch( + "upload.views.bundle_analysis.BundleAnalysisView._handle_upload", + side_effect=Exception("Test Exception"), + ): + client = APIClient() + repository = RepositoryFactory.create() + client.credentials(HTTP_AUTHORIZATION=f"token {repository.upload_token}") + + mock_inc_counter = mocker.patch("upload.views.bundle_analysis.inc_counter") + + client.post( + reverse("upload-bundle-analysis"), + { + "commit": "6fd5b89357fc8cdf34d6197549ac7c6d7e5977ef", + "slug": f"{repository.author.username}::::{repository.name}", + }, + format="json", + ) + except Exception as e: + # Check that the Test Exception was raised and the inc_counter went up + assert str(e) == "Test Exception" mock_inc_counter.assert_any_call( ANY, labels=mocker.ANY,