feat(discover): Add snql support to the events-facets endpoint #29674
Conversation
wmak
commented
Oct 29, 2021
- This also adds support for turbo + sample rate to the QueryBuilder
- This updates the tests so feature modifying for snql is easier
- Otherwise mostly copy paste from the existing code
- This also adds support for turbo + sample rate to the QueryBuilder - This updates the tests so feature modifying for snql is easier - Otherwise mostly copy paste from the existing code
src/sentry/snuba/discover.py
Outdated
| + f" tags_key:[{','.join(aggregate_tags)}]", | ||
| selected_columns=["count()", "tags_key", "tags_value"], | ||
| orderby=["tags_key", "-count()"], | ||
| limitby=["tags_key", TOP_VALUES_DEFAULT_LIMIT], |
There was a problem hiding this comment.
Nit: The limitby arg is of type Tuple(str, int), so you'll probably run into problems with Mypy when enabling it on this file.
There was a problem hiding this comment.
Looks like you fixed the wrong one here. It's QueryBuilder that's expecting the Tuple but you changed it for the raw_query call instead.
src/sentry/snuba/discover.py
Outdated
| def get_facets( | ||
| query: str, | ||
| params: ParamsType, | ||
| referrer: str = None, |
There was a problem hiding this comment.
Is this type hint not contradictory? It has type str but a default value of None.
There was a problem hiding this comment.
🤔 I'm shocked pyright doesn't complain about this.
src/sentry/snuba/discover.py
Outdated
| key_name_builder = QueryBuilder( | ||
| Dataset.Discover, | ||
| params, | ||
| # Exclude tracing tags as they are noisy and generally not helpful. |
There was a problem hiding this comment.
This comment doesn't seem relevant anymore if we don't need to explicitly exclude tracing tags
src/sentry/snuba/discover.py
Outdated
| if tag == "environment": | ||
| # Add here tags that you want to be individual | ||
| individual_tags.append(tag) | ||
| elif i >= len(top_tags) - 10: |
There was a problem hiding this comment.
Nit: Can we declare this 10 as a constant somewhere?
src/sentry/snuba/discover.py
Outdated
| + f" tags_key:[{','.join(aggregate_tags)}]", | ||
| selected_columns=["count()", "tags_key", "tags_value"], | ||
| orderby=["tags_key", "-count()"], | ||
| limitby=["tags_key", TOP_VALUES_DEFAULT_LIMIT], |
There was a problem hiding this comment.
Looks like you fixed the wrong one here. It's QueryBuilder that's expecting the Tuple but you changed it for the raw_query call instead.
src/sentry/snuba/discover.py
Outdated
| @@ -1099,7 +1228,7 @@ def get_facets(query, params, limit=10, referrer=None): | |||
| orderby="-count", | |||
| dataset=Dataset.Discover, | |||
| referrer=referrer, | |||
| sample=sample_rate, | |||
| sample_rate=sample_rate, | |||
There was a problem hiding this comment.
DId you mean to change this? This is the non snql code path.
src/sentry/snuba/discover.py
Outdated
| @@ -1170,7 +1299,7 @@ def get_facets(query, params, limit=10, referrer=None): | |||
| sample=sample_rate, | |||
| # Ensures Snuba will not apply FINAL | |||
| turbo=sample_rate is not None, | |||
| limitby=[TOP_VALUES_DEFAULT_LIMIT, "tags_key"], | |||
| limitby=(TOP_VALUES_DEFAULT_LIMIT, "tags_key"), | |||
There was a problem hiding this comment.
Looks like you changed the wrong one, see comment above.
src/sentry/snuba/discover.py
Outdated
| if tag == "environment": | ||
| # Add here tags that you want to be individual | ||
| individual_tags.append(tag) | ||
| elif i >= len(top_tags) - (TOP_VALUES_DEFAULT_LIMIT + 1): |
There was a problem hiding this comment.
Any reason why this is using TOP_VALUES_DEFAULT_LIMIT + 1 other than the fact that it's 9? Would it be better to declare a separate constant with the value 10?
There was a problem hiding this comment.
wait I'm being dumb, yeah needs to be a separate constant 🤦