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

ref(aci): tagged event and event attribute subfilters for event frequency conditions #87807

Merged

Conversation

cathteng
Copy link
Member

Event frequency conditions that query Snuba will also include additional query filters for tags and event attributes.

The tag filter is currently implemented -- this queries the tags column in Snuba for the match and the value.
However, for event attributes, each attribute is a separate Snuba column that we need to query separately for the match and value.

This PR prepares for querying each event attribute filter separately by storing the event attribute filter differently from tagged event filter, using each filter's respective comparison_json_schema.

@cathteng cathteng requested a review from a team as a code owner March 24, 2025 21:56
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 24, 2025
Comment on lines +365 to +368
case EventAttributeFilter.id:
comparison_filter["attribute"] = condition["attribute"]
case TaggedEventFilter.id:
comparison_filter["key"] = condition["key"]
Copy link
Member Author

Choose a reason for hiding this comment

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

these two conditions only differ in that one has an attribute key and the other one has key, and we'll need to query snuba differently

Copy link
Contributor

Choose a reason for hiding this comment

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

feels so weird to have a switch statement (match) but not have to worry about fall through from the cases. haha

Copy link
Contributor

@saponifi3d saponifi3d left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for adding types to the ATTR_CHOICES too 🫶

Comment on lines +365 to +368
case EventAttributeFilter.id:
comparison_filter["attribute"] = condition["attribute"]
case TaggedEventFilter.id:
comparison_filter["key"] = condition["key"]
Copy link
Contributor

Choose a reason for hiding this comment

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

feels so weird to have a switch statement (match) but not have to worry about fall through from the cases. haha

@cathteng cathteng merged commit ab90e6e into master Mar 25, 2025
49 checks passed
@cathteng cathteng deleted the cathy/aci/save-slow-condition-subfilters-separately branch March 25, 2025 17:12
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.

2 participants