ref: Remove now unused discover query code (#16306)

Remove the automatic dataset detection logic from sentry. We have
switched over to the snuba based implementation of this. Not having
dataset selection in sentry also means we can remove some (but not all)
of the supporting logic as well.

Author: markstory

src/sentry/api/bases/organization_events.py

@@ -5,9 +5,7 @@
 from sentry.api.bases import OrganizationEndpoint, OrganizationEventsError
 from sentry.api.event_search import get_filter, InvalidSearchQuery
 from sentry.models.project import Project
-from sentry.snuba.dataset import Dataset
 from sentry.snuba.discover import ReferenceEvent
-from sentry.utils import snuba
 
 
 class OrganizationEventsEndpointBase(OrganizationEndpoint):
@@ -70,12 +68,4 @@ def get_snuba_query_args_legacy(self, request, organization):
             "filter_keys": _filter.filter_keys,
         }
 
-        # 'legacy' endpoints cannot access transactions dataset.
-        # as they often have assumptions about which columns are returned.
-        dataset = snuba.detect_dataset(snuba_args)
-        if dataset != Dataset.Events:
-            raise OrganizationEventsError(
-                "Invalid query. You cannot reference non-events data in this endpoint."
-            )
-
         return snuba_args

src/sentry/eventstore/snuba/backend.py

@@ -68,6 +68,7 @@ def get_events(
             limit=limit,
             offset=offset,
             referrer=referrer,
+            dataset=snuba.Dataset.Events,
         )
 
         if "error" not in result:
@@ -188,6 +189,9 @@ def __get_event_id_from_filter(self, filter=None, orderby=None):
         columns = [Columns.EVENT_ID.value.alias, Columns.PROJECT_ID.value.alias]
 
         try:
+            # This query uses the discover dataset to enable
+            # getting events across both errors and transactions, which is
+            # required when doing pagination in discover
             result = snuba.dataset_query(
                 selected_columns=columns,
                 conditions=filter.conditions,
@@ -197,7 +201,7 @@ def __get_event_id_from_filter(self, filter=None, orderby=None):
                 limit=1,
                 referrer="eventstore.get_next_or_prev_event_id",
                 orderby=orderby,
-                dataset=snuba.detect_dataset({"conditions": filter.conditions}),
+                dataset=snuba.Dataset.Discover,
             )
         except (snuba.QueryOutsideRetentionError, snuba.QueryOutsideGroupActivityError):
             # This can happen when the date conditions for paging

src/sentry/utils/snuba.py

@@ -292,88 +292,6 @@ def get_snuba_column_name(name, dataset=Dataset.Events):
     return DATASETS[dataset].get(name, u"tags[{}]".format(name))
 
 
-def detect_dataset(query_args, aliased_conditions=False):
-    """
-    Determine the dataset to use based on the conditions, selected_columns,
-    groupby clauses.
-
-    This function operates on the end user field aliases and not the internal column
-    names that have been converted using the field mappings.
-
-    The aliased_conditions parameter switches column detection between
-    the public aliases and the internal names. When query conditions
-    have been pre-parsed by api.event_search set aliased_conditions=True
-    as we need to look for internal names.
-
-    :deprecated: This method and the automatic dataset resolution is deprecated.
-    You should use sentry.snuba.discover instead.
-    """
-    if query_args.get("dataset", None):
-        return query_args["dataset"]
-
-    dataset = Dataset.Events
-    transaction_fields = set(DATASETS[Dataset.Transactions].keys()) - set(
-        DATASETS[Dataset.Events].keys()
-    )
-    condition_fieldset = transaction_fields
-
-    if aliased_conditions:
-        # Release and user are also excluded as they are present on both
-        # datasets and don't trigger usage of transactions.
-        condition_fieldset = (
-            set(DATASET_FIELDS[Dataset.Transactions])
-            - set(DATASET_FIELDS[Dataset.Events])
-            - set(["release", "user"])
-        )
-
-    for condition in query_args.get("conditions") or []:
-        if isinstance(condition[0], six.string_types) and condition[0] in condition_fieldset:
-            return Dataset.Transactions
-        if condition == ["event.type", "=", "transaction"] or condition == [
-            "type",
-            "=",
-            "transaction",
-        ]:
-            return Dataset.Transactions
-
-        if condition == ["event.type", "!=", "transaction"] or condition == [
-            "type",
-            "!=",
-            "transaction",
-        ]:
-            return Dataset.Events
-
-    for field in query_args.get("selected_columns") or []:
-        if isinstance(field, six.string_types) and field in transaction_fields:
-            return Dataset.Transactions
-
-    for field in query_args.get("aggregations") or []:
-        if len(field) != 3:
-            continue
-        # Check field or fields
-        if isinstance(field[1], six.string_types) and field[1] in transaction_fields:
-            return Dataset.Transactions
-        if isinstance(field[1], (list, tuple)):
-            is_transaction = [column for column in field[1] if column in transaction_fields]
-            if is_transaction:
-                return Dataset.Transactions
-        # Check for transaction only field aliases
-        if isinstance(field[2], six.string_types) and field[2] in (
-            "apdex",
-            "impact",
-            "p75",
-            "p95",
-            "p99",
-        ):
-            return Dataset.Transactions
-
-    for field in query_args.get("groupby") or []:
-        if field in transaction_fields:
-            return Dataset.Transactions
-
-    return dataset
-
-
 def get_function_index(column_expr, depth=0):
     """
     If column_expr list contains a function, returns the index of its function name
@@ -411,7 +329,7 @@ def get_function_index(column_expr, depth=0):
         return None
 
 
-def parse_columns_in_functions(col, context=None, index=None, dataset=Dataset.Events):
+def parse_columns_in_functions(col, context=None, index=None):
     """
     Checks expressions for arguments that should be considered a column while
     ignoring strings that represent clickhouse function names
@@ -434,23 +352,23 @@ def parse_columns_in_functions(col, context=None, index=None, dataset=Dataset.Ev
         if function_name_index > 0:
             for i in six.moves.xrange(0, function_name_index):
                 if context is not None:
-                    context[i] = get_snuba_column_name(col[i], dataset)
+                    context[i] = get_snuba_column_name(col[i])
 
         args = col[function_name_index + 1]
 
         # check for nested functions in args
         if get_function_index(args):
             # look for columns
-            return parse_columns_in_functions(args, args, dataset=dataset)
+            return parse_columns_in_functions(args, args)
 
         # check each argument for column names
         else:
             for (i, arg) in enumerate(args):
-                parse_columns_in_functions(arg, args, i, dataset=dataset)
+                parse_columns_in_functions(arg, args, i)
     else:
         # probably a column name
         if context is not None and index is not None:
-            context[index] = get_snuba_column_name(col, dataset)
+            context[index] = get_snuba_column_name(col)
 
 
 def get_arrayjoin(column):
@@ -459,23 +377,6 @@ def get_arrayjoin(column):
         return match.groups()[0]
 
 
-def valid_orderby(orderby, custom_fields=None, dataset=Dataset.Events):
-    """
-    Check if a field can be used in sorting. We don't allow
-    sorting on fields that would be aliased as tag[foo] because those
-    fields are unlikely to be selected.
-    """
-    if custom_fields is None:
-        custom_fields = []
-    fields = orderby if isinstance(orderby, (list, tuple)) else [orderby]
-    mapping = DATASETS[dataset]
-    for field in fields:
-        field = field.lstrip("-")
-        if field not in mapping and field not in custom_fields:
-            return False
-    return True
-
-
 def transform_aliases_and_query(**kwargs):
     """
     Convert aliases in selected_columns, groupby, aggregation, conditions,
@@ -499,7 +400,7 @@ def transform_aliases_and_query(**kwargs):
     arrayjoin = kwargs.get("arrayjoin")
     orderby = kwargs.get("orderby")
     having = kwargs.get("having", [])
-    dataset = detect_dataset(kwargs)
+    dataset = Dataset.Events
 
     if selected_columns:
         for (idx, col) in enumerate(selected_columns):
@@ -511,14 +412,14 @@ def transform_aliases_and_query(**kwargs):
                 translated_columns[col[2]] = col[2]
                 derived_columns.add(col[2])
             else:
-                name = get_snuba_column_name(col, dataset)
+                name = get_snuba_column_name(col)
                 selected_columns[idx] = name
                 translated_columns[name] = col
 
     if groupby:
         for (idx, col) in enumerate(groupby):
             if col not in derived_columns:
-                name = get_snuba_column_name(col, dataset)
+                name = get_snuba_column_name(col)
             else:
                 name = col
 
@@ -528,12 +429,12 @@ def transform_aliases_and_query(**kwargs):
     for aggregation in aggregations or []:
         derived_columns.add(aggregation[2])
         if isinstance(aggregation[1], six.string_types):
-            aggregation[1] = get_snuba_column_name(aggregation[1], dataset)
+            aggregation[1] = get_snuba_column_name(aggregation[1])
         elif isinstance(aggregation[1], (set, tuple, list)):
-            aggregation[1] = [get_snuba_column_name(col, dataset) for col in aggregation[1]]
+            aggregation[1] = [get_snuba_column_name(col) for col in aggregation[1]]
 
     for col in filter_keys.keys():
-        name = get_snuba_column_name(col, dataset)
+        name = get_snuba_column_name(col)
         filter_keys[name] = filter_keys.pop(col)
 
     if conditions:
@@ -558,7 +459,7 @@ def transform_aliases_and_query(**kwargs):
             translated_orderby.append(
                 u"{}{}".format(
                     "-" if field_with_order.startswith("-") else "",
-                    field if field in derived_columns else get_snuba_column_name(field, dataset),
+                    field if field in derived_columns else get_snuba_column_name(field),
                 )
             )
 
@@ -1052,15 +953,7 @@ def dataset_query(
     You should use sentry.snuba.discover instead.
     """
     if dataset is None:
-        dataset = detect_dataset(
-            dict(
-                dataset=dataset,
-                aggregations=aggregations,
-                conditions=conditions,
-                selected_columns=selected_columns,
-                groupby=groupby,
-            )
-        )
+        raise ValueError("A dataset is required, and is no longer automatically detected.")
 
     derived_columns = []
     if selected_columns:

tests/sentry/eventstore/test_base.py

@@ -2,6 +2,7 @@
 
 import logging
 import mock
+import pytest
 import six
 
 from sentry import eventstore
@@ -71,8 +72,8 @@ def setUp(self):
 
         self.transaction_event = self.store_event(data=event_data, project_id=self.project.id)
 
+    @pytest.mark.skip(reason="There is no longer a difference in underlying dataset.")
     def test_logs_differences(self):
-
         logger = logging.getLogger("sentry.eventstore")
 
         with mock.patch.object(logger, "info") as mock_logger: