Skip to content

ref(rules): Add batch_query_hook to EventFrequencyCondition#68559

Merged
ceorourke merged 12 commits into
masterfrom
ceorourke/baseeventfrequencyconditions
Apr 10, 2024
Merged

ref(rules): Add batch_query_hook to EventFrequencyCondition#68559
ceorourke merged 12 commits into
masterfrom
ceorourke/baseeventfrequencyconditions

Conversation

@ceorourke

@ceorourke ceorourke commented Apr 9, 2024

Copy link
Copy Markdown
Member

Add a batch_query_hook to the EventFrequencyCondition so that when we process "slow" rule conditions we can batch the queries by group ids.

Partially closes https://github.com/getsentry/team-core-product-foundations/issues/239

Comment thread src/sentry/rules/conditions/event_frequency.py
Comment thread src/sentry/rules/conditions/event_frequency.py Outdated
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 9, 2024
@codecov

codecov Bot commented Apr 9, 2024

Copy link
Copy Markdown

Bundle Report

Changes will increase total bundle size by 93.75kB ⬆️

Bundle name Size Change
sentry-webpack-bundle-array-push 26.27MB 93.75kB ⬆️

@saponifi3d saponifi3d left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i think the biggest thing here is to make sure we can't do the queries in a single scan, since that has such a big impact on our goals for the project.

Comment thread src/sentry/rules/conditions/event_frequency.py Outdated
Comment thread src/sentry/rules/conditions/event_frequency.py
Comment thread src/sentry/rules/conditions/event_frequency.py Outdated
Comment thread tests/snuba/rules/conditions/test_event_frequency.py Outdated
@ceorourke ceorourke marked this pull request as ready for review April 10, 2024 19:33
@ceorourke ceorourke requested a review from saponifi3d April 10, 2024 20:03
start=start,
end=end,
environment_id=environment_id,
referrer_suffix="alert_event_frequency",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
referrer_suffix="alert_event_frequency",
referrer_suffix=referrer_suffix

generic_issues = [group for group in groups if group.issue_category != GroupCategory.ERROR]

if error_issues:
error_sums = self.get_chunked_sums(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In javascript i'd generally define get_chunked_sums inside of the batch_query_hook function and then reference the start, end, and environment_id from the outer scope. Is that a faux pas in python? (if not, it might help DRY this code up a little more and reduce it by 9 more lines 😎)

It'd be something like:

def get_chunked_sums(groups: list[Group], referrer_suffix: str):
    # same code you have for get_chunked_sums

if error_issues:
    error_sums = get_chunked_sums(
        groups=error_issues,

        # looks like referrer_suffix could be DRY'd up and hardcoded in get_chunked_sums as well
        referrer_suffix="alert_event_frequency"
    )

class EventFrequencyQueryTest(SnubaTestCase, RuleTestCase, PerformanceIssueTestCase):
rule_cls = EventFrequencyCondition

def test_batch_query(self):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎉

@saponifi3d saponifi3d left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm once we either dry the code up or update the param 🎉

@ceorourke ceorourke merged commit 54cf7e5 into master Apr 10, 2024
@ceorourke ceorourke deleted the ceorourke/baseeventfrequencyconditions branch April 10, 2024 21:42
@sentry

sentry Bot commented Apr 10, 2024

Copy link
Copy Markdown
Contributor

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ ProcessingErrorInvalidTransaction: invalid transaction event: end timestamp is smaller than start timestamp pytest.runtest.protocol tests/snuba/rules/condi... View Issue

Did you find this useful? React with a 👍 or 👎

ceorourke added a commit that referenced this pull request Apr 11, 2024
…#68672)

Follow up to #68559 and a part 2
of getsentry/team-core-product-foundations#239
to add a `batch_query_hook` function for the
`EventUniqueUserFrequencyCondition`.
c298lee pushed a commit that referenced this pull request Apr 12, 2024
Add a `batch_query_hook` to the `EventFrequencyCondition` so that when
we process "slow" rule conditions we can batch the queries by group ids.

Partially closes
https://github.com/getsentry/team-core-product-foundations/issues/239
c298lee pushed a commit that referenced this pull request Apr 12, 2024
…#68672)

Follow up to #68559 and a part 2
of https://github.com/getsentry/team-core-product-foundations/issues/239
to add a `batch_query_hook` function for the
`EventUniqueUserFrequencyCondition`.
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants