Skip to content

ref(hybrid-cloud): use organization_slug in MonitorStats #42384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 15, 2022
Merged

ref(hybrid-cloud): use organization_slug in MonitorStats #42384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d276e8
update monitor tests
cathteng Dec 15, 2022
f4873bc
convert MonitorStats endpoint
cathteng Dec 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/sentry/api/endpoints/monitor_stats.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,24 @@
from __future__ import annotations

from rest_framework.request import Request
from rest_framework.response import Response

from sentry import tsdb
from sentry.api.base import StatsMixin, pending_silo_endpoint
from sentry.api.base import StatsMixin, region_silo_endpoint
from sentry.api.bases.monitor import MonitorEndpoint
from sentry.models import CheckInStatus, MonitorCheckIn


@pending_silo_endpoint
@region_silo_endpoint
class MonitorStatsEndpoint(MonitorEndpoint, StatsMixin):
Copy link
Member Author

@cathteng cathteng Dec 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no test file for this endpoint so there is no test file to convert

Copy link
Contributor

@RyanSkonnord RyanSkonnord Dec 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic looks pretty substantial. Maybe this is a good time to add one?

(To be clear, I'm fine either way. Up to you whether it's worth pausing other work, or if you just want a change of pace.)

Copy link
Member Author

@cathteng cathteng Dec 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was going to attempt to write a test file, but the API appears to be broken given what the frontend is passing to it (see any monitor in https://sentry.io/organizations/sentry/monitors/) so i'm not even sure how it works

Copy link
Member Author

@cathteng cathteng Dec 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i broke it but david helped rearrange the urls

# TODO(dcramer): probably convert to tsdb
def get(self, request: Request, project, monitor) -> Response:
def get(
self, request: Request, project, monitor, organization_slug: str | None = None
) -> Response:
if organization_slug:
if project.organization.slug != organization_slug:
return self.respond_invalid()

args = self._parse_args(request)

stats = {}
Expand Down
5 changes: 5 additions & 0 deletions src/sentry/api/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,6 +704,11 @@
MonitorStatsEndpoint.as_view(),
name="sentry-api-0-monitor-stats",
),
url(
r"^(?P<organization_slug>[^\/]+)/(?P<monitor_id>[^\/]+)/stats/$",
MonitorStatsEndpoint.as_view(),
name="sentry-api-0-monitor-stats-with-org",
),
]
),
),
Expand Down
10 changes: 10 additions & 0 deletions tests/sentry/api/endpoints/test_monitor_checkins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,3 +166,13 @@ def test_with_dsn_auth_invalid_project(self):
)

assert resp.status_code == 400, resp.content

def test_mismatched_org_slugs(self):
monitor = self._create_monitor()
path = f"/api/0/monitors/asdf/{monitor.guid}/checkins/"
self.login_as(user=self.user)

with self.feature("organizations:monitors"):
resp = self.client.post(path)

assert resp.status_code == 400
32 changes: 16 additions & 16 deletions tests/sentry/api/endpoints/test_monitor_details.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ def test_simple(self):
self.login_as(user=self.user)
monitor = self._create_monitor()

with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
for path_func in self._get_path_functions():
path = path_func(monitor)
resp = self.client.get(path)
Expand Down Expand Up @@ -47,7 +47,7 @@ def setUp(self):
def test_name(self):
monitor = self._create_monitor()

with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
for i, path_func in enumerate(self._get_path_functions()):
monitor = self._create_monitor()
path = path_func(monitor)
Expand All @@ -60,7 +60,7 @@ def test_name(self):
assert monitor.name == f"Monitor Name {i}"

def test_can_disable(self):
with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
for path_func in self._get_path_functions():
monitor = self._create_monitor()
path = path_func(monitor)
Expand All @@ -73,7 +73,7 @@ def test_can_disable(self):
assert monitor.status == MonitorStatus.DISABLED

def test_can_enable(self):
with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
for path_func in self._get_path_functions():
monitor = self._create_monitor()
path = path_func(monitor)
Expand All @@ -89,7 +89,7 @@ def test_can_enable(self):
assert monitor.status == MonitorStatus.ACTIVE

def test_cannot_enable_if_enabled(self):
with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
for path_func in self._get_path_functions():
monitor = self._create_monitor()
path = path_func(monitor)
Expand All @@ -105,7 +105,7 @@ def test_cannot_enable_if_enabled(self):
assert monitor.status == MonitorStatus.OK

def test_checkin_margin(self):
with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
for path_func in self._get_path_functions():
monitor = self._create_monitor()
path = path_func(monitor)
Expand All @@ -119,7 +119,7 @@ def test_checkin_margin(self):
assert monitor.config["checkin_margin"] == 30

def test_max_runtime(self):
with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
for path_func in self._get_path_functions():
monitor = self._create_monitor()
path = path_func(monitor)
Expand All @@ -133,7 +133,7 @@ def test_max_runtime(self):
assert monitor.config["max_runtime"] == 30

def test_invalid_config_param(self):
with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
for path_func in self._get_path_functions():
monitor = self._create_monitor()
path = path_func(monitor)
Expand All @@ -147,7 +147,7 @@ def test_invalid_config_param(self):
assert "invalid" not in monitor.config

def test_cronjob_crontab(self):
with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
for path_func in self._get_path_functions():
monitor = self._create_monitor()
path = path_func(monitor)
Expand All @@ -172,7 +172,7 @@ def test_cronjob_crontab(self):
# ['@hourly', '0 * * * *'],
# ))
def test_cronjob_nonstandard(self):
with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
for path_func in self._get_path_functions():
monitor = self._create_monitor()
path = path_func(monitor)
Expand All @@ -187,7 +187,7 @@ def test_cronjob_nonstandard(self):
assert monitor.config["schedule"] == "0 0 1 * *"

def test_cronjob_crontab_invalid(self):
with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
for path_func in self._get_path_functions():
monitor = self._create_monitor()
path = path_func(monitor)
Expand All @@ -201,7 +201,7 @@ def test_cronjob_crontab_invalid(self):
assert resp.status_code == 400, resp.content

def test_cronjob_interval(self):
with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
for path_func in self._get_path_functions():
monitor = self._create_monitor()
path = path_func(monitor)
Expand All @@ -218,7 +218,7 @@ def test_cronjob_interval(self):
assert monitor.config["schedule"] == [1, "month"]

def test_cronjob_interval_invalid_inteval(self):
with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
for path_func in self._get_path_functions():
monitor = self._create_monitor()
path = path_func(monitor)
Expand Down Expand Up @@ -247,7 +247,7 @@ def test_mismatched_org_slugs(self):
path = f"/api/0/monitors/asdf/{monitor.guid}/"
self.login_as(user=self.user)

with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
resp = self.client.put(
path, data={"config": {"schedule_type": "interval", "schedule": [1, "month"]}}
)
Expand All @@ -265,7 +265,7 @@ def setUp(self):

def test_simple(self):
self.login_as(user=self.user)
with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
for path_func in self._get_path_functions():
monitor = self._create_monitor()
path = path_func(monitor)
Expand All @@ -286,7 +286,7 @@ def test_mismatched_org_slugs(self):
path = f"/api/0/monitors/asdf/{monitor.guid}/"
self.login_as(user=self.user)

with self.feature({"organizations:monitors": True}):
with self.feature("organizations:monitors"):
resp = self.client.delete(path)

assert resp.status_code == 400