Skip to content

ref(rules): Refactor get_rate to be used for bulk queries #69446

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
ceorourke merged 6 commits into from
Apr 23, 2024
Merged

ref(rules): Refactor get_rate to be used for bulk queries #69446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a27dca7
ref(rules): Refactor get_rate to handle batch
ceorourke Apr 22, 2024
e071656
break into get rate single and bulk
ceorourke Apr 22, 2024
56120ee
nuke get_start_end_from_duration, add a default value for comparison …
ceorourke Apr 23, 2024
1505c31
fixed after bad rebase
ceorourke Apr 23, 2024
1a19685
fix function call, add docstring
ceorourke Apr 23, 2024
983095f
pull out get_rate_bulk
ceorourke Apr 23, 2024
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
48 changes: 35 additions & 13 deletions src/sentry/rules/conditions/event_frequency.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,17 @@ def passes(self, event: GroupEvent, state: EventState) -> bool:
if state.is_new and value > 1:
return False

# TODO(mgaeta): Bug: Rule is optional.
comparison_type = self.get_option("comparisonType", ComparisonType.COUNT)
comparison_interval_option = self.get_option("comparisonInterval", "5m")
comparison_interval = COMPARISON_INTERVALS[comparison_interval_option][1]
_, duration = self.intervals[interval]
try:
current_value = self.get_rate(event, interval, self.rule.environment_id) # type: ignore[arg-type, union-attr]
current_value = self.get_rate(duration=duration, comparison_interval=comparison_interval, event=event, environment_id=self.rule.environment_id, comparison_type=comparison_type) # type: ignore[arg-type, union-attr]
# XXX(CEO): once inc-666 work is concluded, rm try/except
except RateLimitExceeded:
metrics.incr("rule.event_frequency.snuba_query_limit")
return False

logging.info("event_frequency_rule current: %s, threshold: %s", current_value, value)
return current_value > value

Expand Down Expand Up @@ -214,26 +218,44 @@ def batch_query_hook(
"""
raise NotImplementedError

def get_rate(self, event: GroupEvent, interval: str, environment_id: int) -> int:
_, duration = self.intervals[interval]
end = timezone.now()
def get_option_override(self, duration: timedelta) -> contextlib.AbstractContextManager[object]:
# For conditions with interval >= 1 hour we don't need to worry about read your writes
# consistency. Disable it so that we can scale to more nodes.
option_override_cm: contextlib.AbstractContextManager[object] = contextlib.nullcontext()
if duration >= timedelta(hours=1):
option_override_cm = options_override({"consistent": False})
with option_override_cm:
result: int = self.query(event, end - duration, end, environment_id=environment_id)
comparison_type = self.get_option("comparisonType", ComparisonType.COUNT)
return option_override_cm

def get_comparison_start_end(
self, interval: timedelta, duration: timedelta
) -> tuple[datetime, datetime]:
"""
Calculate the start and end times for the query. `interval` is only used for EventFrequencyPercentCondition
as the '5 minutes' in The issue affects more than 100 percent of sessions in 5 minutes, otherwise it's the current time.
`duration` is the time frame in which the condition is measuring counts, e.g. the '10 minutes' in
"The issue is seen more than 100 times in 10 minutes"
"""
end = timezone.now() - interval
start = end - duration
return (start, end)

def get_rate(
self,
duration: timedelta,
comparison_interval: timedelta,
event: GroupEvent,
environment_id: int,
comparison_type: str,
) -> int:
start, end = self.get_comparison_start_end(timedelta(), duration)
with self.get_option_override(duration):
result = self.query(event, start, end, environment_id=environment_id)
if comparison_type == ComparisonType.PERCENT:
comparison_interval = COMPARISON_INTERVALS[self.get_option("comparisonInterval")][1]
comparison_end = end - comparison_interval
# TODO: Figure out if there's a way we can do this less frequently. All queries are
# automatically cached for 10s. We could consider trying to cache this and the main
# query for 20s to reduce the load.
comparison_result = self.query(
event, comparison_end - duration, comparison_end, environment_id=environment_id
)
start, end = self.get_comparison_start_end(comparison_interval, duration)
comparison_result = self.query(event, start, end, environment_id=environment_id)
result = percent_increase(result, comparison_result)

return result
Expand Down
128 changes: 104 additions & 24 deletions tests/snuba/rules/conditions/test_event_frequency.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,43 +290,63 @@ def _run_test(self, minutes, data, passes, add_events=False):
self.assertDoesNotPass(environment_rule, event, is_new=False)

def test_one_minute_with_events(self):
data = {"interval": "1m", "value": 6}
data = {"interval": "1m", "value": 6, "comparisonType": "count", "comparisonInterval": "5m"}
self._run_test(data=data, minutes=1, passes=True, add_events=True)
data = {"interval": "1m", "value": 16}
data = {
"interval": "1m",
"value": 16,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=1, passes=False)

def test_one_hour_with_events(self):
data = {"interval": "1h", "value": 6}
data = {"interval": "1h", "value": 6, "comparisonType": "count", "comparisonInterval": "5m"}
self._run_test(data=data, minutes=60, passes=True, add_events=True)
data = {"interval": "1h", "value": 16}
data = {
"interval": "1h",
"value": 16,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=60, passes=False)

def test_one_day_with_events(self):
data = {"interval": "1d", "value": 6}
data = {"interval": "1d", "value": 6, "comparisonType": "count", "comparisonInterval": "5m"}
self._run_test(data=data, minutes=1440, passes=True, add_events=True)
data = {"interval": "1d", "value": 16}
data = {
"interval": "1d",
"value": 16,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=1440, passes=False)

def test_one_week_with_events(self):
data = {"interval": "1w", "value": 6}
data = {"interval": "1w", "value": 6, "comparisonType": "count", "comparisonInterval": "5m"}
self._run_test(data=data, minutes=10080, passes=True, add_events=True)
data = {"interval": "1w", "value": 16}
data = {
"interval": "1w",
"value": 16,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=10080, passes=False)

def test_one_minute_no_events(self):
data = {"interval": "1m", "value": 6}
data = {"interval": "1m", "value": 6, "comparisonType": "count", "comparisonInterval": "5m"}
self._run_test(data=data, minutes=1, passes=False)

def test_one_hour_no_events(self):
data = {"interval": "1h", "value": 6}
data = {"interval": "1h", "value": 6, "comparisonType": "count", "comparisonInterval": "5m"}
self._run_test(data=data, minutes=60, passes=False)

def test_one_day_no_events(self):
data = {"interval": "1d", "value": 6}
data = {"interval": "1d", "value": 6, "comparisonType": "count", "comparisonInterval": "5m"}
self._run_test(data=data, minutes=1440, passes=False)

def test_one_week_no_events(self):
data = {"interval": "1w", "value": 6}
data = {"interval": "1w", "value": 6, "comparisonType": "count", "comparisonInterval": "5m"}
self._run_test(data=data, minutes=10080, passes=False)

def test_comparison(self):
Expand Down Expand Up @@ -516,57 +536,117 @@ def increment(self, event, count, environment=None, timestamp=None):
@patch("sentry.rules.conditions.event_frequency.MIN_SESSIONS_TO_FIRE", 1)
def test_five_minutes_with_events(self):
self._make_sessions(60)
data = {"interval": "5m", "value": 39}
data = {
"interval": "5m",
"value": 39,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=5, passes=True, add_events=True)
data = {"interval": "5m", "value": 41}
data = {
"interval": "5m",
"value": 41,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=5, passes=False)

@patch("sentry.rules.conditions.event_frequency.MIN_SESSIONS_TO_FIRE", 1)
def test_ten_minutes_with_events(self):
self._make_sessions(60)
data = {"interval": "10m", "value": 49}
data = {
"interval": "10m",
"value": 49,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=10, passes=True, add_events=True)
data = {"interval": "10m", "value": 51}
data = {
"interval": "10m",
"value": 51,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=10, passes=False)

@patch("sentry.rules.conditions.event_frequency.MIN_SESSIONS_TO_FIRE", 1)
def test_thirty_minutes_with_events(self):
self._make_sessions(60)
data = {"interval": "30m", "value": 49}
data = {
"interval": "30m",
"value": 49,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=30, passes=True, add_events=True)
data = {"interval": "30m", "value": 51}
data = {
"interval": "30m",
"value": 51,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=30, passes=False)

@patch("sentry.rules.conditions.event_frequency.MIN_SESSIONS_TO_FIRE", 1)
def test_one_hour_with_events(self):
self._make_sessions(60)
data = {"interval": "1h", "value": 49}
data = {
"interval": "1h",
"value": 49,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=60, add_events=True, passes=True)
data = {"interval": "1h", "value": 51}
data = {
"interval": "1h",
"value": 51,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=60, passes=False)

@patch("sentry.rules.conditions.event_frequency.MIN_SESSIONS_TO_FIRE", 1)
def test_five_minutes_no_events(self):
self._make_sessions(60)
data = {"interval": "5m", "value": 39}
data = {
"interval": "5m",
"value": 39,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=5, passes=True, add_events=True)

@patch("sentry.rules.conditions.event_frequency.MIN_SESSIONS_TO_FIRE", 1)
def test_ten_minutes_no_events(self):
self._make_sessions(60)
data = {"interval": "10m", "value": 49}
data = {
"interval": "10m",
"value": 49,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=10, passes=True, add_events=True)

@patch("sentry.rules.conditions.event_frequency.MIN_SESSIONS_TO_FIRE", 1)
def test_thirty_minutes_no_events(self):
self._make_sessions(60)
data = {"interval": "30m", "value": 49}
data = {
"interval": "30m",
"value": 49,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=30, passes=True, add_events=True)

@patch("sentry.rules.conditions.event_frequency.MIN_SESSIONS_TO_FIRE", 1)
def test_one_hour_no_events(self):
self._make_sessions(60)
data = {"interval": "1h", "value": 49}
data = {
"interval": "1h",
"value": 49,
"comparisonType": "count",
"comparisonInterval": "5m",
}
self._run_test(data=data, minutes=60, passes=False)

@patch("sentry.rules.conditions.event_frequency.MIN_SESSIONS_TO_FIRE", 1)
Expand Down