Browse Source

fix(ds): SDK versions accepts start and end [TET-334] (#38026)

* fix(ds): SDK versions accepts start and end

Modified sdk versions endpoint to accept
start and end timestamps that are at most
1 day apart. Does not accept statsPeriod anymore

* PR comments
Ahmed Etefy 2 years ago
parent
commit
362e5c6f59

+ 40 - 17
src/sentry/api/endpoints/organization_dynamic_sampling_sdk_versions.py

@@ -2,7 +2,8 @@ from datetime import timedelta
 from functools import cmp_to_key
 from typing import Any, Dict
 
-from django.utils import timezone
+from dateutil.parser import parse as parse_date
+from rest_framework import status
 from rest_framework.request import Request
 from rest_framework.response import Response
 from sentry_relay.processing import compare_version as compare_version_relay
@@ -10,7 +11,7 @@ from sentry_relay.processing import compare_version as compare_version_relay
 from sentry import features
 from sentry.api.bases.organization import OrganizationEndpoint
 from sentry.snuba import discover
-from sentry.utils.dates import parse_stats_period
+from sentry.utils.dates import ensure_aware
 
 SDK_NAME_FILTER_THRESHOLD = 0.1
 SDK_VERSION_FILTER_THRESHOLD = 0.05
@@ -37,9 +38,36 @@ ALLOWED_SDK_NAMES = frozenset(
 )
 
 
+class QueryBoundsException(Exception):
+    pass
+
+
 class OrganizationDynamicSamplingSDKVersionsEndpoint(OrganizationEndpoint):
     private = True
 
+    @staticmethod
+    def __validate_query_bounds(query_start, query_end):
+        if not query_start or not query_end:
+            raise QueryBoundsException("'start' and 'end' are required")
+
+        query_start = ensure_aware(parse_date(query_start))
+        query_end = ensure_aware(parse_date(query_end))
+
+        if query_start > query_end:
+            raise QueryBoundsException("'start' has to be before 'end'")
+
+        if query_end - query_start > timedelta(days=1):
+            raise QueryBoundsException("'start' and 'end' have to be a maximum of 1 day apart")
+
+        stats_period = query_end - query_start
+        # Quantize time boundary down so that during a 5-minute interval, the query time boundaries
+        # remain the same to leverage the snuba cache
+        query_end = query_end.replace(
+            minute=(query_end.minute - query_end.minute % 5), second=0, microsecond=0
+        )
+        query_start = query_end - stats_period
+        return query_start, query_end
+
     def get(self, request: Request, organization) -> Response:
         """
         Return a list of project SDK versions based on project filter that are used in the
@@ -49,8 +77,8 @@ class OrganizationDynamicSamplingSDKVersionsEndpoint(OrganizationEndpoint):
 
         :pparam string organization_slug: the slug of the organization.
         :qparam array[string] project: A required list of project ids to filter
-        :qparam string statsPeriod: an optional stat period (can be one of
-                                    ``"24h"``, ``"14d"``, and ``""``).
+        :qparam string start: specify a date to begin at. Format must be iso format
+        :qparam string end:  specify a date to end at. Format must be iso format
         :auth: required
         """
         if not features.has("organizations:server-side-sampling", organization, actor=request.user):
@@ -75,17 +103,12 @@ class OrganizationDynamicSamplingSDKVersionsEndpoint(OrganizationEndpoint):
             )
         ]
 
-        stats_period = min(
-            parse_stats_period(request.GET.get("statsPeriod", "24h")), timedelta(days=2)
-        )
-
-        end_time = timezone.now()
-        # Quantize time boundary down so that during a 5-minute interval, the query time boundaries
-        # remain the same to leverage the snuba cache
-        end_time = end_time.replace(
-            minute=(end_time.minute - end_time.minute % 5), second=0, microsecond=0
-        )
-        start_time = end_time - stats_period
+        try:
+            query_start, query_end = self.__validate_query_bounds(
+                request.GET.get("start"), request.GET.get("end")
+            )
+        except QueryBoundsException as e:
+            return Response({"detail": str(e)}, status=status.HTTP_400_BAD_REQUEST)
 
         sample_rate_count_if = 'count_if(trace.client_sample_rate, notEquals, "")'
         avg_sample_rate_equation = f"{sample_rate_count_if} / count()"
@@ -103,8 +126,8 @@ class OrganizationDynamicSamplingSDKVersionsEndpoint(OrganizationEndpoint):
             ],
             query="event.type:transaction",
             params={
-                "start": start_time,
-                "end": end_time,
+                "start": query_start,
+                "end": query_end,
                 "project_id": project_ids,
                 "organization_id": organization,
             },

+ 69 - 10
tests/sentry/api/endpoints/test_organization_dynamic_sampling_sdk_versions.py

@@ -4,7 +4,6 @@ from unittest import mock
 
 from django.urls import reverse
 from django.utils import timezone
-from freezegun import freeze_time
 
 from sentry.testutils import APITestCase
 from sentry.testutils.helpers import Feature
@@ -211,27 +210,80 @@ class OrganizationDynamicSamplingSDKVersionsTest(APITestCase):
         user = self.create_user("foo@example.com")
         self.login_as(user)
         with Feature({"organizations:server-side-sampling": True}):
-            response = self.client.get(f"{self.endpoint}?project={self.project.id}")
+            response = self.client.get(
+                f"{self.endpoint}?project={self.project.id}&"
+                f"start=2022-08-06T00:02:00+00:00&"
+                f"end=2022-08-07T00:00:02+00:00"
+            )
             assert response.status_code == 403
 
     def test_feature_flag_disabled(self):
         self.login_as(self.user)
-        response = self.client.get(self.endpoint)
+        response = self.client.get(
+            f"{self.endpoint}?project={self.project.id}&"
+            f"start=2022-08-06T00:02:00+00:00&"
+            f"end=2022-08-07T00:00:02+00:00"
+        )
         assert response.status_code == 404
 
     def test_no_project_ids_filter_requested(self):
         self.login_as(self.user)
         with Feature({"organizations:server-side-sampling": True}):
-            response = self.client.get(self.endpoint)
+            response = self.client.get(
+                f"{self.endpoint}?"
+                f"start=2022-08-06T00:02:00+00:00&"
+                f"end=2022-08-07T00:00:02+00:00"
+            )
             assert response.status_code == 200
             assert response.data == []
 
+    def test_no_query_start_or_no_query_end(self):
+        self.login_as(self.user)
+        with Feature({"organizations:server-side-sampling": True}):
+            response = self.client.get(
+                f"{self.endpoint}?project={self.project.id}&end=2022-08-07T00:00:02+00:00"
+            )
+            assert response.status_code == 400
+            assert response.json()["detail"] == "'start' and 'end' are required"
+
+            response = self.client.get(
+                f"{self.endpoint}?project={self.project.id}&start=2022-08-06T00:02:00+00:00"
+            )
+            assert response.status_code == 400
+            assert response.json()["detail"] == "'start' and 'end' are required"
+
+    def test_query_start_is_before_query_end(self):
+        self.login_as(self.user)
+        with Feature({"organizations:server-side-sampling": True}):
+            response = self.client.get(
+                f"{self.endpoint}?project="
+                f"{self.project.id}&start=2022-08-10T00:02:00+00:00&end=2022-08-07T00:00:02+00:00"
+            )
+            assert response.status_code == 400
+            assert response.json()["detail"] == "'start' has to be before 'end'"
+
+    def test_query_start_and_query_end_are_atmost_one_day_apart(self):
+        self.login_as(self.user)
+        with Feature({"organizations:server-side-sampling": True}):
+            response = self.client.get(
+                f"{self.endpoint}?project="
+                f"{self.project.id}&start=2022-08-05T00:02:00+00:00&end=2022-08-07T00:00:02+00:00"
+            )
+            assert response.status_code == 400
+            assert (
+                response.json()["detail"] == "'start' and 'end' have to be a maximum of 1 day apart"
+            )
+
     @mock.patch("sentry.api.endpoints.organization_dynamic_sampling_sdk_versions.discover.query")
     def test_successful_response(self, mock_query):
         self.login_as(self.user)
         mock_query.return_value = mocked_discover_query()
         with Feature({"organizations:server-side-sampling": True}):
-            response = self.client.get(f"{self.endpoint}?project={self.project.id}")
+            response = self.client.get(
+                f"{self.endpoint}?project={self.project.id}&"
+                f"start=2022-08-06T00:02:00+00:00&"
+                f"end=2022-08-07T00:00:02+00:00"
+            )
             assert response.json() == [
                 {
                     "project": "wind",
@@ -288,17 +340,20 @@ class OrganizationDynamicSamplingSDKVersionsTest(APITestCase):
         self.login_as(self.user)
         mock_query.return_value = {"data": []}
         with Feature({"organizations:server-side-sampling": True}):
-            response = self.client.get(f"{self.endpoint}?project={self.project.id}")
+            response = self.client.get(
+                f"{self.endpoint}?project={self.project.id}&"
+                f"start=2022-08-06T00:02:00+00:00&"
+                f"end=2022-08-07T00:00:02+00:00"
+            )
             assert response.json() == []
 
-    @freeze_time("2022-07-07 03:21:34")
     @mock.patch("sentry.api.endpoints.organization_dynamic_sampling_sdk_versions.discover.query")
     def test_request_params_are_applied_to_discover_query(self, mock_query):
         self.login_as(self.user)
         mock_query.return_value = mocked_discover_query()
 
-        end_time = datetime.datetime(2022, 7, 7, 3, 20, 0, tzinfo=timezone.utc)
-        start_time = end_time - timedelta(hours=6)
+        end_time = datetime.datetime(2022, 8, 7, 0, 0, 0, tzinfo=timezone.utc)
+        start_time = end_time - timedelta(hours=24)
 
         calls = [
             mock.call(
@@ -334,6 +389,10 @@ class OrganizationDynamicSamplingSDKVersionsTest(APITestCase):
         ]
 
         with Feature({"organizations:server-side-sampling": True}):
-            response = self.client.get(f"{self.endpoint}?project={self.project.id}&statsPeriod=6h")
+            response = self.client.get(
+                f"{self.endpoint}?project={self.project.id}&"
+                f"start=2022-08-06T00:02:00+00:00&"
+                f"end=2022-08-07T00:00:02+00:00"
+            )
             assert response.status_code == 200
             assert mock_query.mock_calls == calls