Skip to content
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.

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

fix(uptime): Enforce domain limits everywhere #86086

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wedamija
Copy link
Member

Currently we're only enforcing these at the api level. Move them to autodetection as well, and add an index to make sure we can still query them cheaply.

@wedamija wedamija requested review from a team as code owners February 28, 2025 00:35
@wedamija
Copy link
Member Author

Note that I'll merge this after my other stack that contains migrations merges

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 28, 2025
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/uptime/migrations/0027_uptime_subscription_index_domain_cols.py ()

--
-- Create index uptime_upti_url_dom_ead522_idx on field(s) url_domain_suffix, url_domain of model uptimesubscription
--
CREATE INDEX CONCURRENTLY "uptime_upti_url_dom_ead522_idx" ON "uptime_uptimesubscription" ("url_domain_suffix", "url_domain");

Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Schema change looks good.

self.max_urls = max_urls


def check_url_limits(url):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def check_url_limits(url):
def check_url_limits(url):
"""
Determines if a URL's domain has reached the global maximum (MAX_MONITORS_PER_DOMAIN).
In the case that it has a `MaxUrlsForDomainReachedException` will be raised.
"""

Copy link

codecov bot commented Feb 28, 2025

Codecov Report

Attention: Patch coverage is 97.87234% with 1 line in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/uptime/detectors/ranking.py 92.30% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #86086   +/-   ##
=======================================
  Coverage   87.88%   87.88%           
=======================================
  Files        9713     9714    +1     
  Lines      550582   550619   +37     
  Branches    21441    21441           
=======================================
+ Hits       483875   483920   +45     
+ Misses      66327    66319    -8     
  Partials      380      380           

Currently we're only enforcing these at the api level. Move them to autodetection as well, and add an index to make sure we can still query them cheaply.
@wedamija wedamija force-pushed the danf/uptime-enforce-domain-limits branch from 53f5065 to cbe7b42 Compare March 1, 2025 00:28
@wedamija wedamija force-pushed the danf/uptime-enforce-domain-limits branch from b63a201 to af3f9e1 Compare March 1, 2025 00:29
Copy link
Contributor

github-actions bot commented Mar 1, 2025

This PR has a migration; here is the generated SQL for src/sentry/uptime/migrations/0029_uptime_subscription_index_domain_cols.py ()

--
-- Create index uptime_upti_url_dom_ead522_idx on field(s) url_domain_suffix, url_domain of model uptimesubscription
--
CREATE INDEX CONCURRENTLY "uptime_upti_url_dom_ead522_idx" ON "uptime_uptimesubscription" ("url_domain_suffix", "url_domain");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants