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

feat(discover-snql): Use the snql query builder for events-meta #29096

Merged
merged 2 commits into from
Oct 6, 2021

Conversation

wmak
Copy link
Member

@wmak wmak commented Oct 6, 2021

  • This adds a feature flag so we can try the new query builder for the
    events-meta endpoint
  • This also adds a function_alias_map so we can correctly convert the
    meta results on the discover endpoints

- This adds a feature flag so we can try the new query builder for the
  events-meta endpoint
- This also adds a function_alias_map so we can correctly convert the
  meta results on the discover endpoints
@wmak wmak requested a review from a team October 6, 2021 00:16
@wmak wmak requested a review from a team as a code owner October 6, 2021 00:16
@@ -2261,6 +2262,7 @@ def __init__(
),
SnQLFunction(
"count",
optional_args=[NullColumn("column")],
Copy link
Member Author

Choose a reason for hiding this comment

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

support older syntax of count(id) that's still floating about

Copy link
Member

@Zylphrex Zylphrex left a comment

Choose a reason for hiding this comment

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

Can we also add a tag to the issue alert in Sentry to make it obvious from the slack notification that it's an snql issue?

Comment on lines +183 to +186
class OrganizationEventsMetaEndpointWithSnql(OrganizationEventsMetaEndpoint):
def setUp(self):
super().setUp()
self.features["organizations:discover-use-snql"] = True
Copy link
Member

Choose a reason for hiding this comment

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

Oh dang, does this actually work? 🔥

Copy link
Member Author

Choose a reason for hiding this comment

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

hell yeah, gonna use this pattern for the other test cases as we convert

@wmak wmak enabled auto-merge (squash) October 6, 2021 15:59
@wmak wmak merged commit 2216ac5 into master Oct 6, 2021
@wmak wmak deleted the wmak/feat/events-meta-snql branch October 6, 2021 16:22
@github-actions github-actions bot locked and limited conversation to collaborators Oct 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants