From c5489f5db33c6b6b1f1df9f95a9c7fa5cbc528a1 Mon Sep 17 00:00:00 2001
From: Mark Story <mark@sentry.io>
Date: Thu, 8 Aug 2019 11:04:28 -0400
Subject: [PATCH 1/9] feat(discover2) Add aggregate column support and
 additional aliases

This is an alternate approach to modifying the discover endpoints to fit
the requirements of discover2/events search. By extending the existing
events endpoint with rollup and additional functions/aliases we can get
what we need. This approach will require some shim code in the UI to
convert saved discover queries into a compatible field list and query
string, but that work now seems simpler than building out the remaining
ancillary endpoints in discover.

Refs SEN-811
---
 src/sentry/api/bases/organization_events.py   |  96 +++--------
 .../api/endpoints/organization_events.py      |  11 +-
 src/sentry/api/event_search.py                | 159 +++++++++++++++++-
 tests/sentry/api/test_event_search.py         | 139 ++++++++++++++-
 .../endpoints/test_organization_events_v2.py  | 156 +++++------------
 5 files changed, 358 insertions(+), 203 deletions(-)

diff --git a/src/sentry/api/bases/organization_events.py b/src/sentry/api/bases/organization_events.py
index 0f54759c9106..1b078de2c3f7 100644
--- a/src/sentry/api/bases/organization_events.py
+++ b/src/sentry/api/bases/organization_events.py
@@ -1,38 +1,19 @@
 from __future__ import absolute_import
 
-from copy import deepcopy
-from rest_framework.exceptions import PermissionDenied
 import six
+from rest_framework.exceptions import PermissionDenied
 from enum import Enum
 
 from sentry import features
 from sentry.api.bases import OrganizationEndpoint, OrganizationEventsError
-from sentry.api.event_search import get_snuba_query_args, InvalidSearchQuery
+from sentry.api.event_search import (
+    get_snuba_query_args,
+    resolve_field_list,
+    InvalidSearchQuery
+)
 from sentry.models.project import Project
 from sentry.utils import snuba
 
-# We support 4 "special fields" on the v2 events API which perform some
-# additional calculations over aggregated event data
-SPECIAL_FIELDS = {
-    'issue_title': {
-        'aggregations': [['anyHeavy', 'title', 'issue_title']],
-    },
-    'last_seen': {
-        'aggregations': [['max', 'timestamp', 'last_seen']],
-    },
-    'event_count': {
-        'aggregations': [['uniq', 'id', 'event_count']],
-    },
-    'user_count': {
-        'aggregations': [['uniq', 'user', 'user_count']],
-    },
-    'latest_event': {
-        'fields': [
-            ['argMax', ['id', 'timestamp'], 'latest_event'],
-        ],
-    },
-}
-
 
 class Direction(Enum):
     NEXT = 0
@@ -67,47 +48,29 @@ def get_snuba_query_args(self, request, organization, params):
         except InvalidSearchQuery as exc:
             raise OrganizationEventsError(exc.message)
 
-        fields = request.GET.getlist('field')[:]
-        aggregations = []
-        groupby = request.GET.getlist('groupby')
-        special_fields = set()
-
-        if fields:
-            # If project.name is requested, get the project.id from Snuba so we
-            # can use this to look up the name in Sentry
-            if 'project.name' in fields:
-                fields.remove('project.name')
-                if 'project.id' not in fields:
-                    fields.append('project.id')
-
-            for field in fields[:]:
-                if field in SPECIAL_FIELDS:
-                    special_fields.add(field)
-                    special_field = deepcopy(SPECIAL_FIELDS[field])
-                    fields.remove(field)
-                    fields.extend(special_field.get('fields', []))
-                    aggregations.extend(special_field.get('aggregations', []))
-                    groupby.extend(special_field.get('groupby', []))
-
-            snuba_args['selected_columns'] = fields
-
-        self._filter_unspecified_special_fields_in_conditions(snuba_args, special_fields)
-        if aggregations:
-            snuba_args['aggregations'] = aggregations
-
-        if groupby:
-            snuba_args['groupby'] = groupby
-
         sort = request.GET.getlist('sort')
-        if sort and snuba.valid_orderby(sort, SPECIAL_FIELDS):
+        if sort:
             snuba_args['orderby'] = sort
 
         # Deprecated. `sort` should be used as it is supported by
         # more endpoints.
         orderby = request.GET.getlist('orderby')
-        if orderby and snuba.valid_orderby(orderby, SPECIAL_FIELDS) and 'orderby' not in snuba_args:
+        if orderby and 'orderby' not in snuba_args:
             snuba_args['orderby'] = orderby
 
+        if request.GET.get('rollup'):
+            try:
+                snuba_args['rollup'] = int(request.GET.get('rollup'))
+            except ValueError:
+                raise OrganizationEventsError('rollup must be an integer.')
+
+        fields = request.GET.getlist('field')[:]
+        if fields:
+            try:
+                snuba_args.update(resolve_field_list(fields, snuba_args))
+            except InvalidSearchQuery as exc:
+                raise OrganizationEventsError(exc.message)
+
         # TODO(lb): remove once boolean search is fully functional
         has_boolean_op_flag = features.has(
             'organizations:boolean-search',
@@ -147,9 +110,6 @@ def get_snuba_query_args_legacy(self, request, organization):
         except InvalidSearchQuery as exc:
             raise OrganizationEventsError(exc.message)
 
-        # Filter out special aggregates.
-        self._filter_unspecified_special_fields_in_conditions(snuba_args, set())
-
         # TODO(lb): remove once boolean search is fully functional
         has_boolean_op_flag = features.has(
             'organizations:boolean-search',
@@ -212,17 +172,3 @@ def _get_next_or_prev_id(self, direction, request, organization, snuba_args, eve
             return None
 
         return six.text_type(result['data'][0]['event_id'])
-
-    def _filter_unspecified_special_fields_in_conditions(self, snuba_args, special_fields):
-        conditions = []
-        for condition in snuba_args['conditions']:
-            field = condition[0]
-            if (
-                not isinstance(field, (list, tuple))
-                and field in SPECIAL_FIELDS
-                and field not in special_fields
-            ):
-                # skip over special field.
-                continue
-            conditions.append(condition)
-        snuba_args['conditions'] = conditions
diff --git a/src/sentry/api/endpoints/organization_events.py b/src/sentry/api/endpoints/organization_events.py
index a2fec67a37bb..4bf6523245a4 100644
--- a/src/sentry/api/endpoints/organization_events.py
+++ b/src/sentry/api/endpoints/organization_events.py
@@ -18,7 +18,6 @@
 from sentry import features
 from sentry.models.project import Project
 
-ALLOWED_GROUPINGS = frozenset(('issue.id', 'project.id', 'transaction'))
 logger = logging.getLogger(__name__)
 
 
@@ -32,16 +31,12 @@ def get(self, request, organization):
             params = self.get_filter_params(request, organization)
             snuba_args = self.get_snuba_query_args(request, organization, params)
             fields = snuba_args.get('selected_columns')
+            aggregations = snuba_args.get('aggregations')
             groupby = snuba_args.get('groupby', [])
 
-            if not fields and not groupby:
+            if not fields and not aggregations and not groupby:
                 return Response({'detail': 'No fields or groupings provided'}, status=400)
 
-            if any(field for field in groupby if field not in ALLOWED_GROUPINGS):
-                message = ('Invalid groupby value requested. Allowed values are ' +
-                           ', '.join(ALLOWED_GROUPINGS))
-                return Response({'detail': message}, status=400)
-
         except OrganizationEventsError as exc:
             return Response({'detail': exc.message}, status=400)
         except NoProjects:
@@ -136,7 +131,7 @@ def handle_results(self, request, organization, project_ids, results):
 
         fields = request.GET.getlist('field')
 
-        if 'project.name' in fields:
+        if 'project.name' in fields or 'project' in fields:
             for result in results:
                 result['project.name'] = projects[result['project.id']]
                 if 'project.id' not in fields:
diff --git a/src/sentry/api/event_search.py b/src/sentry/api/event_search.py
index 9a9b05f11db2..82ddaead68d3 100644
--- a/src/sentry/api/event_search.py
+++ b/src/sentry/api/event_search.py
@@ -2,6 +2,7 @@
 
 import re
 from collections import namedtuple, defaultdict
+from copy import deepcopy
 from datetime import datetime
 
 import six
@@ -142,9 +143,8 @@ def translate(pat):
     'first_seen': 'first_seen',
     'last_seen': 'last_seen',
     'times_seen': 'times_seen',
-    # OrganizationEvents aggregations
-    'event_count': 'event_count',
-    'user_count': 'user_count',
+    # TODO(mark) figure out how to safelist aggregate functions/field aliases
+    # so they can be used in conditions
 }, **SENTRY_SNUBA_MAP)
 no_conversion = set(['project_id', 'start', 'end'])
 
@@ -215,9 +215,9 @@ class SearchVisitor(NodeVisitor):
         'device.battery_level', 'device.charging', 'device.online',
         'device.simulator', 'error.handled', 'issue.id', 'stack.colno',
         'stack.in_app', 'stack.lineno', 'stack.stack_level',
-        # OrganizationEvents aggregations
-        'event_count', 'user_count',
-
+        'duration',
+        # TODO(mark) figure out how to safelist aggregate functions/field aliases
+        # so they can be used in conditions
     ])
     date_keys = set([
         'start', 'end', 'first_seen', 'last_seen', 'time', 'timestamp',
@@ -662,3 +662,150 @@ def get_snuba_query_args(query=None, params=None):
             kwargs['has_boolean_terms'] = True
             kwargs['conditions'].append(convert_search_boolean_to_snuba_query(term))
     return kwargs
+
+
+FIELD_ALIASES = {
+    'issue_title': {
+        'aggregations': [['anyHeavy', 'title', 'issue_title']],
+    },
+    'last_seen': {
+        'aggregations': [['max', 'timestamp', 'last_seen']],
+    },
+    'latest_event': {
+        'fields': [
+            ['argMax', ['id', 'timestamp'], 'latest_event'],
+        ],
+    },
+    'project': {
+        'fields': ['project.id'],
+    },
+    'user': {
+        'fields': ['user.id', 'user.name', 'user.username', 'user.email', 'user.ip'],
+    }
+    # TODO(mark) Add rpm alias.
+}
+
+VALID_AGGREGATES = {
+    'count_unique': {
+        'snuba_name': 'uniq',
+        'fields': '*',
+    },
+    'count': {
+        'snuba_name': 'count',
+        'fields': '*'
+    },
+    'avg': {
+        'snuba_name': 'avg',
+        'fields': ['issue.id', 'duration'],
+    },
+    'min': {
+        'snuba_name': 'min',
+        'fields': ['timestamp', 'issue.id', 'duration'],
+    },
+    'max': {
+        'snuba_name': 'max',
+        'fields': ['timestamp', 'issue.id', 'duration'],
+    },
+    'sum': {
+        'snuba_name': 'sum',
+        'fields': ['duration'],
+    },
+    # This doesn't work yet, but is an illustration of how it could work
+    'p75': {
+        'snuba_name': 'quantileTiming(0.75)',
+        'fields': ['duration'],
+    },
+}
+
+AGGREGATE_PATTERN = re.compile(r'^(?P<function>[^\(]+)\((?P<column>[a-z\._+]+)\)$')
+
+
+def validate_aggregate(field, match):
+    function_name = match.group('function')
+    if function_name not in VALID_AGGREGATES:
+        raise InvalidSearchQuery("Unknown aggregate function '%s'" % field)
+
+    function_data = VALID_AGGREGATES[function_name]
+    column = match.group('column')
+    if column not in function_data['fields'] and function_data['fields'] != '*':
+        raise InvalidSearchQuery(
+            "Invalid column '%s' in aggregate function '%s'" % (column, function_name))
+
+
+def validate_orderby(orderby, fields):
+    orderby = orderby if isinstance(orderby, (list, tuple)) else [orderby]
+    for column in orderby:
+        column = column.lstrip('-')
+        if column not in fields:
+            raise InvalidSearchQuery('Cannot order by an field that is not selected.')
+
+
+def resolve_field_list(fields, snuba_args):
+    """
+    Expand a list of fields based on aliases and aggregate functions.
+
+    Returns a dist of aggregations, selected_columns, and
+    groupby that can be merged into the result of get_snuba_query_args()
+    to build a more complete snuba query based on event search conventions.
+    """
+    # If project.name is requested, get the project.id from Snuba so we
+    # can use this to look up the name in Sentry
+    if 'project.name' in fields:
+        fields.remove('project.name')
+        if 'project.id' not in fields:
+            fields.append('project.id')
+
+    aggregations = []
+    groupby = []
+    columns = []
+    for field in fields:
+        if field in FIELD_ALIASES:
+            special_field = deepcopy(FIELD_ALIASES[field])
+            columns.extend(special_field.get('fields', []))
+            aggregations.extend(special_field.get('aggregations', []))
+            continue
+
+        # Basic fields don't require additional validation. They could be tag
+        # names which we have no way of validating at this point.
+        match = AGGREGATE_PATTERN.search(field)
+        if not match:
+            columns.append(field)
+            continue
+
+        validate_aggregate(field, match)
+        aggregations.append([
+            VALID_AGGREGATES[match.group('function')]['snuba_name'],
+            match.group('column'),
+            u'{}_{}'.format(match.group('function'), match.group('column'))
+        ])
+
+    rollup = snuba_args.get('rollup')
+    if not rollup:
+        # Ensure fields we require to build a functioning interface
+        # are present. We don't add fields when using a rollup as the additional fields
+        # would be aggregated away.
+        if 'project.id' not in columns:
+            columns.append('project.id')
+        if not aggregations and 'id' not in columns:
+            columns.append('id')
+        if aggregations and 'latest_event' not in fields:
+            columns.extend(deepcopy(FIELD_ALIASES['latest_event']['fields']))
+
+    basic_fields = [col for col in columns if isinstance(col, six.string_types)]
+    if rollup and basic_fields:
+        raise InvalidSearchQuery('You cannot combine rollup with non-aggregate fields')
+
+    orderby = snuba_args.get('orderby')
+    if orderby:
+        validate_orderby(orderby, fields)
+
+    # If aggregations are present all basic fields
+    # need to be added to the group by so that the query is valid.
+    if aggregations:
+        groupby.extend(basic_fields)
+
+    return {
+        'selected_columns': columns,
+        'aggregations': aggregations,
+        'groupby': groupby,
+    }
diff --git a/tests/sentry/api/test_event_search.py b/tests/sentry/api/test_event_search.py
index 189c1ee0b7b1..ac0a63b838f4 100644
--- a/tests/sentry/api/test_event_search.py
+++ b/tests/sentry/api/test_event_search.py
@@ -3,6 +3,7 @@
 import datetime
 import pytest
 import six
+import unittest
 from datetime import timedelta
 
 from django.utils import timezone
@@ -10,13 +11,14 @@
 
 from sentry.api.event_search import (
     convert_endpoint_params, event_search_grammar, get_snuba_query_args,
-    parse_search_query, InvalidSearchQuery, SearchBoolean, SearchFilter, SearchKey,
+    resolve_field_list, parse_search_query,
+    InvalidSearchQuery, SearchBoolean, SearchFilter, SearchKey,
     SearchValue, SearchVisitor,
 )
 from sentry.testutils import TestCase
 
 
-class ParseSearchQueryTest(TestCase):
+class ParseSearchQueryTest(unittest.TestCase):
     def test_simple(self):
         # test with raw search query at the end
         assert parse_search_query('user.email:foo@example.com release:1.2.1 hello') == [
@@ -691,7 +693,7 @@ def test_empty_string(self):
         assert parse_search_query('') == []
 
 
-class ParseBooleanSearchQueryTest(TestCase):
+class ParseBooleanSearchQueryTest(unittest.TestCase):
     def setUp(self):
         super(ParseBooleanSearchQueryTest, self).setUp()
         self.term1 = SearchFilter(
@@ -1163,7 +1165,7 @@ def test_project_name(self):
         }
 
 
-class ConvertEndpointParamsTests(TestCase):
+class ConvertEndpointParamsTests(unittest.TestCase):
     def test_simple(self):
         assert convert_endpoint_params({
             'project_id': [1, 2, 3],
@@ -1204,3 +1206,132 @@ def test_simple(self):
                 )
             ),
         ]
+
+
+class ResolveFieldListTest(unittest.TestCase):
+    def test_automatic_fields_no_aggregates(self):
+        fields = ['event.type', 'message']
+        result = resolve_field_list(fields, {})
+        assert result['selected_columns'] == ['event.type', 'message', 'project.id', 'id']
+        assert result['aggregations'] == []
+        assert result['groupby'] == []
+
+    def test_automatic_fields_with_aggregate_aliases(self):
+        fields = ['issue_title', 'message']
+        result = resolve_field_list(fields, {})
+        # Automatic fields should be inserted
+        assert result['selected_columns'] == [
+            'message',
+            'project.id',
+            ['argMax', ['id', 'timestamp'], 'latest_event'],
+        ]
+        assert result['aggregations'] == [
+            ['anyHeavy', 'title', 'issue_title']
+        ]
+        assert result['groupby'] == ['message', 'project.id']
+
+    def test_field_alias_expansion(self):
+        fields = ['issue_title', 'last_seen', 'latest_event', 'project', 'user', 'message']
+        result = resolve_field_list(fields, {})
+        print result
+        assert result['selected_columns'] == [
+            ['argMax', ['id', 'timestamp'], 'latest_event'],
+            'project.id',
+            'user.id',
+            'user.name',
+            'user.username',
+            'user.email',
+            'user.ip',
+            'message',
+        ]
+        assert result['aggregations'] == [
+            ['anyHeavy', 'title', 'issue_title'],
+            ['max', 'timestamp', 'last_seen']
+        ]
+        assert result['groupby'] == [
+            'project.id',
+            'user.id',
+            'user.name',
+            'user.username',
+            'user.email',
+            'user.ip',
+            'message',
+        ]
+
+    def test_aggregate_function_expansion(self):
+        fields = ['count_unique(user)', 'count(id)', 'avg(duration)']
+        result = resolve_field_list(fields, {})
+        # Automatic fields should be inserted
+        assert result['selected_columns'] == [
+            'project.id',
+            ['argMax', ['id', 'timestamp'], 'latest_event'],
+        ]
+        assert result['aggregations'] == [
+            ['uniq', 'user', 'count_unique_user'],
+            ['count', 'id', 'count_id'],
+            ['avg', 'duration', 'avg_duration'],
+        ]
+        assert result['groupby'] == ['project.id']
+
+    def test_aggregate_function_invalid_name(self):
+        with pytest.raises(InvalidSearchQuery) as err:
+            fields = ['derp(user)']
+            resolve_field_list(fields, {})
+        assert 'Unknown aggregate' in six.text_type(err)
+
+    def test_aggregate_function_case_sensitive(self):
+        with pytest.raises(InvalidSearchQuery) as err:
+            fields = ['MAX(user)']
+            resolve_field_list(fields, {})
+        assert 'Unknown aggregate' in six.text_type(err)
+
+    def test_aggregate_function_invalid_column(self):
+        with pytest.raises(InvalidSearchQuery) as err:
+            fields = ['p75(message)']
+            resolve_field_list(fields, {})
+        assert 'Invalid column' in six.text_type(err)
+
+    def test_rollup_with_unaggregated_fields(self):
+        with pytest.raises(InvalidSearchQuery) as err:
+            fields = ['message']
+            snuba_args = {'rollup': 15}
+            resolve_field_list(fields, snuba_args)
+        assert 'rollup with non-aggregate' in six.text_type(err)
+
+    def test_rollup_with_aggregated_fields(self):
+        fields = ['count_unique(user)']
+        snuba_args = {'rollup': 15}
+        result = resolve_field_list(fields, snuba_args)
+        assert result['aggregations'] == [
+            ['uniq', 'user', 'count_unique_user']
+        ]
+        assert result['selected_columns'] == []
+        assert result['groupby'] == []
+
+    def test_orderby_unselected_field(self):
+        fields = ['message']
+        snuba_args = {'orderby': 'timestamp'}
+        with pytest.raises(InvalidSearchQuery) as err:
+            resolve_field_list(fields, snuba_args)
+        assert 'Cannot order' in six.text_type(err)
+
+    def test_orderby_basic_field(self):
+        fields = ['message']
+        snuba_args = {'orderby': '-message'}
+        result = resolve_field_list(fields, snuba_args)
+        assert result['selected_columns'] == ['message', 'project.id', 'id']
+        assert result['aggregations'] == []
+        assert result['groupby'] == []
+
+    def test_orderby_field_alias(self):
+        fields = ['issue_title']
+        snuba_args = {'orderby': '-issue_title'}
+        result = resolve_field_list(fields, snuba_args)
+        assert result['selected_columns'] == [
+            'project.id',
+            ['argMax', ['id', 'timestamp'], 'latest_event']
+        ]
+        assert result['aggregations'] == [
+            ['anyHeavy', 'title', 'issue_title']
+        ]
+        assert result['groupby'] == ['project.id']
diff --git a/tests/snuba/api/endpoints/test_organization_events_v2.py b/tests/snuba/api/endpoints/test_organization_events_v2.py
index 80f3984b8a3b..f401fcbc4c45 100644
--- a/tests/snuba/api/endpoints/test_organization_events_v2.py
+++ b/tests/snuba/api/endpoints/test_organization_events_v2.py
@@ -6,6 +6,7 @@
 from django.core.urlresolvers import reverse
 
 from sentry.testutils import APITestCase, SnubaTestCase
+import pytest
 
 
 class OrganizationEventsTestBase(APITestCase, SnubaTestCase):
@@ -119,7 +120,7 @@ def test_raw_data(self):
                 self.url,
                 format='json',
                 data={
-                    'field': ['id', 'project.id', 'user.email', 'user.ip', 'time'],
+                    'field': ['id', 'project.id', 'user.email', 'user.ip', 'timestamp'],
                     'orderby': '-timestamp',
                 },
             )
@@ -157,10 +158,10 @@ def test_project_name(self):
         assert 'project.id' not in response.data[0]
         assert response.data[0]['environment'] == 'staging'
 
-    def test_groupby(self):
+    def test_implicit_groupby(self):
         self.login_as(user=self.user)
         project = self.create_project()
-        event1 = self.store_event(
+        self.store_event(
             data={
                 'event_id': 'a' * 32,
                 'timestamp': self.min_ago,
@@ -168,7 +169,7 @@ def test_groupby(self):
             },
             project_id=project.id,
         )
-        self.store_event(
+        event1 = self.store_event(
             data={
                 'event_id': 'b' * 32,
                 'timestamp': self.min_ago,
@@ -190,18 +191,25 @@ def test_groupby(self):
                 self.url,
                 format='json',
                 data={
-                    'field': ['project.id', 'issue.id'],
-                    'groupby': ['project.id', 'issue.id'],
+                    'field': ['count(id)', 'project.id', 'issue.id'],
                     'orderby': 'issue.id',
                 },
             )
 
         assert response.status_code == 200, response.content
         assert len(response.data) == 2
-        assert response.data[0]['project.id'] == project.id
-        assert response.data[0]['issue.id'] == event1.group_id
-        assert response.data[1]['project.id'] == project.id
-        assert response.data[1]['issue.id'] == event2.group_id
+        assert response.data[0] == {
+            'project.id': project.id,
+            'issue.id': event1.group_id,
+            'count_id': 2,
+            'latest_event': event1.event_id,
+        }
+        assert response.data[1] == {
+            'project.id': project.id,
+            'issue.id': event2.group_id,
+            'count_id': 1,
+            'latest_event': event2.event_id,
+        }
 
     def test_orderby(self):
         self.login_as(user=self.user)
@@ -232,11 +240,12 @@ def test_orderby(self):
                 self.url,
                 format='json',
                 data={
-                    'field': ['id'],
+                    'field': ['id', 'timestamp'],
                     'orderby': ['-timestamp', '-id']
                 },
             )
 
+        assert response.status_code == 200, response.content
         assert response.data[0]['id'] == 'c' * 32
         assert response.data[1]['id'] == 'b' * 32
         assert response.data[2]['id'] == 'a' * 32
@@ -273,16 +282,17 @@ def test_sort_title(self):
                 self.url,
                 format='json',
                 data={
-                    'field': ['id'],
+                    'field': ['id', 'title'],
                     'sort': 'title'
                 },
             )
 
+        assert response.status_code == 200, response.content
         assert response.data[0]['id'] == 'c' * 32
         assert response.data[1]['id'] == 'b' * 32
         assert response.data[2]['id'] == 'a' * 32
 
-    def test_sort_ignore_invalid(self):
+    def test_sort_invalid(self):
         self.login_as(user=self.user)
         project = self.create_project()
         self.store_event(
@@ -301,9 +311,10 @@ def test_sort_ignore_invalid(self):
                     'sort': 'garbage'
                 },
             )
-        assert response.status_code == 200
+        assert response.status_code == 400
+        assert 'order by' in response.content
 
-    def test_special_fields(self):
+    def test_aliased_fields(self):
         self.login_as(user=self.user)
         project = self.create_project()
         event1 = self.store_event(
@@ -345,8 +356,7 @@ def test_special_fields(self):
                 self.url,
                 format='json',
                 data={
-                    'field': ['issue_title', 'event_count', 'user_count'],
-                    'groupby': ['issue.id', 'project.id'],
+                    'field': ['issue.id', 'issue_title', 'count(id)', 'count_unique(user)'],
                     'orderby': 'issue.id'
                 },
             )
@@ -354,12 +364,13 @@ def test_special_fields(self):
         assert response.status_code == 200, response.content
         assert len(response.data) == 2
         assert response.data[0]['issue.id'] == event1.group_id
-        assert response.data[0]['event_count'] == 1
-        assert response.data[0]['user_count'] == 1
+        assert response.data[0]['count_id'] == 1
+        assert response.data[0]['count_unique_user'] == 1
         assert response.data[1]['issue.id'] == event2.group_id
-        assert response.data[1]['event_count'] == 2
-        assert response.data[1]['user_count'] == 2
+        assert response.data[1]['count_id'] == 2
+        assert response.data[1]['count_unique_user'] == 2
 
+    @pytest.mark.xfail(reason='aggregate comparisons need parser improvements')
     def test_aggregation_comparison(self):
         self.login_as(user=self.user)
         project = self.create_project()
@@ -424,10 +435,9 @@ def test_aggregation_comparison(self):
                 self.url,
                 format='json',
                 data={
-                    'field': ['issue_title', 'event_count', 'user_count'],
-                    'query': 'event_count:>1 user_count:>1',
-                    'groupby': ['issue.id', 'project.id'],
-                    'orderby': 'issue.id'
+                    'field': ['issue_title', 'count(id)', 'count_unique(user)'],
+                    'query': 'count_id:>1 count_unique_user:>1',
+                    'orderby': 'issue_title'
                 },
             )
 
@@ -435,9 +445,10 @@ def test_aggregation_comparison(self):
 
         assert len(response.data) == 1
         assert response.data[0]['issue.id'] == event.group_id
-        assert response.data[0]['event_count'] == 2
-        assert response.data[0]['user_count'] == 2
+        assert response.data[0]['count_id'] == 2
+        assert response.data[0]['count_unique_user'] == 2
 
+    @pytest.mark.xfail(reason='aggregate comparisons need parser improvements')
     def test_aggregation_comparison_with_conditions(self):
         self.login_as(user=self.user)
         project = self.create_project()
@@ -495,10 +506,9 @@ def test_aggregation_comparison_with_conditions(self):
                 self.url,
                 format='json',
                 data={
-                    'field': ['issue_title', 'event_count'],
-                    'query': 'event_count:>1 user.email:foo@example.com environment:prod',
-                    'groupby': ['issue.id', 'project.id'],
-                    'orderby': 'issue.id'
+                    'field': ['issue_title', 'count(id)'],
+                    'query': 'count_id:>1 user.email:foo@example.com environment:prod',
+                    'orderby': 'issue_title'
                 },
             )
 
@@ -506,56 +516,7 @@ def test_aggregation_comparison_with_conditions(self):
 
         assert len(response.data) == 1
         assert response.data[0]['issue.id'] == event.group_id
-        assert response.data[0]['event_count'] == 2
-
-    def test_invalid_groupby(self):
-        self.login_as(user=self.user)
-
-        project = self.create_project()
-        self.store_event(
-            data={
-                'event_id': 'a' * 32,
-                'message': 'how to make fast',
-                'timestamp': self.min_ago,
-            },
-            project_id=project.id
-        )
-
-        with self.feature('organizations:events-v2'):
-            response = self.client.get(
-                self.url,
-                format='json',
-                data={
-                    'groupby': ['id'],
-                },
-            )
-        assert response.status_code == 400, response.content
-        assert response.data['detail'] == 'Invalid groupby value requested. Allowed values are transaction, project.id, issue.id'
-
-    def test_non_aggregated_fields_with_groupby(self):
-        self.login_as(user=self.user)
-
-        project = self.create_project()
-        self.store_event(
-            data={
-                'event_id': 'a' * 32,
-                'message': 'how to make fast',
-                'timestamp': self.min_ago,
-            },
-            project_id=project.id
-        )
-
-        with self.feature('organizations:events-v2'):
-            response = self.client.get(
-                self.url,
-                format='json',
-                data={
-                    'field': ['project.id'],
-                    'groupby': ['issue.id'],
-                },
-            )
-        assert response.status_code == 400, response.content
-        assert response.data['detail'] == 'Invalid query.'
+        assert response.data[0]['count_id'] == 2
 
     def test_nonexistent_fields(self):
         self.login_as(user=self.user)
@@ -579,7 +540,7 @@ def test_nonexistent_fields(self):
                 },
             )
         assert response.status_code == 200, response.content
-        assert response.data == [{u'issue_world.id': u''}]
+        assert response.data[0]['issue_world.id'] == ''
 
     def test_no_requested_fields_or_grouping(self):
         self.login_as(user=self.user)
@@ -605,31 +566,9 @@ def test_no_requested_fields_or_grouping(self):
         assert response.status_code == 400, response.content
         assert response.data['detail'] == 'No fields or groupings provided'
 
-    def test_irrelevant_special_field_in_query(self):
+    def test_condition_on_aggregate_fails(self):
         self.login_as(user=self.user)
         project = self.create_project()
-        event1 = self.store_event(
-            data={
-                'event_id': 'a' * 32,
-                'timestamp': self.min_ago,
-                'fingerprint': ['group_1'],
-                'user': {
-                    'email': 'foo@example.com',
-                },
-            },
-            project_id=project.id,
-        )
-        event2 = self.store_event(
-            data={
-                'event_id': 'b' * 32,
-                'timestamp': self.min_ago,
-                'fingerprint': ['group_2'],
-                'user': {
-                    'email': 'foo@example.com',
-                },
-            },
-            project_id=project.id,
-        )
         self.store_event(
             data={
                 'event_id': 'c' * 32,
@@ -648,16 +587,13 @@ def test_irrelevant_special_field_in_query(self):
                 format='json',
                 data={
                     'field': ['issue.id'],
-                    'query': 'event_count:>1',
+                    'query': 'event_count:>0',
                     'orderby': 'issue.id'
                 },
             )
 
         assert response.status_code == 200, response.content
-        assert len(response.data) == 3
-        assert response.data[0]['issue.id'] == event1.group_id
-        assert response.data[1]['issue.id'] == event2.group_id
-        assert response.data[2]['issue.id'] == event2.group_id
+        assert len(response.data) == 0
 
     def test_group_filtering(self):
         user = self.create_user()

From bf09008f95f1af01c4f422aaf6dbc8d17e48fd46 Mon Sep 17 00:00:00 2001
From: Mark Story <mark@sentry.io>
Date: Thu, 8 Aug 2019 14:16:20 -0400
Subject: [PATCH 2/9] Update the UI code to use the new aggregate
 functions/field aliases.

---
 .../app/views/organizationEventsV2/data.jsx   | 80 ++++++-------------
 .../eventModalContent.jsx                     |  7 +-
 .../app/views/organizationEventsV2/table.jsx  |  8 +-
 .../app/views/organizationEventsV2/utils.jsx  | 21 +----
 .../views/organizationEventsV2/index.spec.jsx |  4 +-
 .../views/organizationEventsV2/utils.spec.jsx | 24 ------
 6 files changed, 39 insertions(+), 105 deletions(-)

diff --git a/src/sentry/static/sentry/app/views/organizationEventsV2/data.jsx b/src/sentry/static/sentry/app/views/organizationEventsV2/data.jsx
index 75162bb704d3..f2c023c09208 100644
--- a/src/sentry/static/sentry/app/views/organizationEventsV2/data.jsx
+++ b/src/sentry/static/sentry/app/views/organizationEventsV2/data.jsx
@@ -17,14 +17,16 @@ import {QueryLink} from './styles';
 
 export const MODAL_QUERY_KEYS = ['eventSlug'];
 export const PIN_ICON = `image://${pinIcon}`;
+export const AGGREGATE_ALIASES = ['issue_title', 'last_seen', 'latest_event'];
 
 export const ALL_VIEWS = deepFreeze([
   {
     id: 'all',
     name: t('All Events'),
     data: {
-      fields: ['event', 'type', 'project', 'user', 'time'],
-      sort: ['-timestamp', '-id'],
+      fields: ['title', 'event.type', 'project', 'user', 'timestamp'],
+      columnNames: ['title', 'type', 'project', 'user', 'time'],
+      sort: ['-timestamp'],
     },
     tags: [
       'event.type',
@@ -40,9 +42,9 @@ export const ALL_VIEWS = deepFreeze([
     id: 'errors',
     name: t('Errors'),
     data: {
-      fields: ['error', 'event_count', 'user_count', 'project', 'last_seen'],
-      groupby: ['issue.id', 'project.id'],
-      sort: ['-last_seen', '-issue.id'],
+      fields: ['issue_title', 'count(id)', 'count_unique(user)', 'project', 'last_seen'],
+      columnNames: ['error', 'events', 'users', 'project', 'last seen'],
+      sort: ['-last_seen', '-issue_title'],
       query: 'event.type:error',
     },
     tags: ['error.type', 'project.name'],
@@ -52,9 +54,9 @@ export const ALL_VIEWS = deepFreeze([
     id: 'csp',
     name: t('CSP'),
     data: {
-      fields: ['csp', 'event_count', 'user_count', 'project', 'last_seen'],
-      groupby: ['issue.id', 'project.id'],
-      sort: ['-last_seen', '-issue.id'],
+      fields: ['issue_title', 'count(id)', 'count_unique(user)', 'project', 'last_seen'],
+      columnNames: ['csp', 'events', 'users', 'project', 'last seen'],
+      sort: ['-last_seen', '-issue_title'],
       query: 'event.type:csp',
     },
     tags: [
@@ -70,8 +72,8 @@ export const ALL_VIEWS = deepFreeze([
     id: 'transactions',
     name: t('Transactions'),
     data: {
-      fields: ['transaction', 'project'],
-      groupby: ['transaction', 'project.id'],
+      fields: ['transaction', 'project', 'count(id)'],
+      columnNames: ['transaction', 'project', 'volume'],
       sort: ['-transaction'],
       query: 'event.type:transaction',
     },
@@ -83,7 +85,7 @@ export const ALL_VIEWS = deepFreeze([
       'user.ip',
       'environment',
     ],
-    columnWidths: ['3fr', '2fr'],
+    columnWidths: ['3fr', '1fr', '70px'],
   },
 ]);
 
@@ -94,7 +96,6 @@ export const ALL_VIEWS = deepFreeze([
  */
 export const SPECIAL_FIELDS = {
   transaction: {
-    fields: ['project.name', 'transaction', 'latest_event'],
     sortField: 'transaction',
     renderFunc: (data, {organization, location}) => {
       const target = {
@@ -113,8 +114,7 @@ export const SPECIAL_FIELDS = {
       );
     },
   },
-  event: {
-    fields: ['title', 'id', 'project.name'],
+  title: {
     sortField: 'title',
     renderFunc: (data, {organization, location}) => {
       const target = {
@@ -131,7 +131,6 @@ export const SPECIAL_FIELDS = {
     },
   },
   type: {
-    fields: ['event.type'],
     sortField: 'event.type',
     renderFunc: (data, {location, organization}) => {
       const target = {
@@ -146,7 +145,6 @@ export const SPECIAL_FIELDS = {
     },
   },
   project: {
-    fields: ['project.name'],
     sortField: false,
     renderFunc: (data, {organization}) => {
       const project = organization.projects.find(p => p.slug === data['project.name']);
@@ -162,8 +160,7 @@ export const SPECIAL_FIELDS = {
     },
   },
   user: {
-    fields: ['user', 'user.name', 'user.username', 'user.email', 'user.ip', 'user.id'],
-    sortField: 'user',
+    sortField: 'user.id',
     renderFunc: (data, {organization, location}) => {
       const userObj = {
         id: data['user.id'],
@@ -192,8 +189,7 @@ export const SPECIAL_FIELDS = {
       return <QueryLink to={target}>{badge}</QueryLink>;
     },
   },
-  time: {
-    fields: ['timestamp'],
+  timestamp: {
     sortField: 'timestamp',
     renderFunc: data => (
       <Container>
@@ -206,8 +202,7 @@ export const SPECIAL_FIELDS = {
       </Container>
     ),
   },
-  error: {
-    fields: ['issue_title', 'project.name', 'latest_event'],
+  issue_title: {
     sortField: 'issue_title',
     renderFunc: (data, {organization, location}) => {
       const target = {
@@ -226,49 +221,26 @@ export const SPECIAL_FIELDS = {
       );
     },
   },
-  csp: {
-    fields: ['issue_title', 'project.name', 'latest_event'],
-    sortField: 'issue_title',
-    renderFunc: (data, {organization, location}) => {
-      const target = {
-        pathname: `/organizations/${organization.slug}/events/`,
-        query: {
-          ...location.query,
-          eventSlug: `${data['project.name']}:${data.latest_event}`,
-        },
-      };
-      return (
-        <Container>
-          <Link css={overflowEllipsis} to={target} aria-label={data.issue_title}>
-            {data.issue_title}
-          </Link>
-        </Container>
-      );
-    },
-  },
-  event_count: {
-    title: 'events',
-    fields: ['event_count'],
-    sortField: 'event_count',
+  // TODO generalize this.
+  'count(id)': {
+    sortField: 'count_id',
     renderFunc: data => (
       <NumberContainer>
-        {typeof data.event_count === 'number' ? <Count value={data.event_count} /> : null}
+        {typeof data.count_id === 'number' ? <Count value={data.count_id} /> : null}
       </NumberContainer>
     ),
   },
-  user_count: {
-    title: 'users',
-    fields: ['user_count'],
-    sortField: 'user_count',
+  'count_unique(user)': {
+    sortField: 'unique_count_user',
     renderFunc: data => (
       <NumberContainer>
-        {typeof data.user_count === 'number' ? <Count value={data.user_count} /> : null}
+        {typeof data.count_unique_user === 'number' ? (
+          <Count value={data.count_unique_user} />
+        ) : null}
       </NumberContainer>
     ),
   },
   last_seen: {
-    title: 'last seen',
-    fields: ['last_seen'],
     sortField: 'last_seen',
     renderFunc: data => {
       return (
diff --git a/src/sentry/static/sentry/app/views/organizationEventsV2/eventModalContent.jsx b/src/sentry/static/sentry/app/views/organizationEventsV2/eventModalContent.jsx
index 5d29fa967413..36ea59cf328e 100644
--- a/src/sentry/static/sentry/app/views/organizationEventsV2/eventModalContent.jsx
+++ b/src/sentry/static/sentry/app/views/organizationEventsV2/eventModalContent.jsx
@@ -17,6 +17,7 @@ import ModalPagination from './modalPagination';
 import ModalLineGraph from './modalLineGraph';
 import RelatedEvents from './relatedEvents';
 import TagsTable from './tagsTable';
+import {AGGREGATE_ALIASES} from './data';
 
 /**
  * Render the columns and navigation elements inside the event modal view.
@@ -24,7 +25,11 @@ import TagsTable from './tagsTable';
  */
 const EventModalContent = props => {
   const {event, projectId, organization, location, view} = props;
-  const isGroupedView = !!view.data.groupby;
+
+  // Known aggregate aliases and functions indicated grouped views.
+  const isGroupedView = !!view.data.fields.find(
+    field => AGGREGATE_ALIASES.includes(field) || field.match(/[a-z_]+\([a-z_\.]+\)/)
+  );
   const eventJsonUrl = `/api/0/projects/${organization.slug}/${projectId}/events/${
     event.eventID
   }/json/`;
diff --git a/src/sentry/static/sentry/app/views/organizationEventsV2/table.jsx b/src/sentry/static/sentry/app/views/organizationEventsV2/table.jsx
index 2a90023bbc7c..45b0e4b538a0 100644
--- a/src/sentry/static/sentry/app/views/organizationEventsV2/table.jsx
+++ b/src/sentry/static/sentry/app/views/organizationEventsV2/table.jsx
@@ -49,7 +49,6 @@ export default class Table extends React.Component {
               query: `${field}:${row[field]}`,
             },
           };
-
           return (
             <Cell key={field}>
               {SPECIAL_FIELDS.hasOwnProperty(field) ? (
@@ -66,17 +65,16 @@ export default class Table extends React.Component {
 
   render() {
     const {isLoading, location, view} = this.props;
-    const {fields, sort} = view.data;
+    const {fields, columnNames, sort} = view.data;
     const defaultSort = sort.length ? sort[0] : null;
 
     return (
       <Panel>
         <TableHeader className={getGridStyle(view)}>
-          {fields.map(field => {
-            let title = field;
+          {fields.map((field, i) => {
+            const title = columnNames[i] || field;
             let sortKey = field;
             if (SPECIAL_FIELDS.hasOwnProperty(field)) {
-              title = SPECIAL_FIELDS[field].title || field;
               sortKey = SPECIAL_FIELDS[field].sortField;
             }
 
diff --git a/src/sentry/static/sentry/app/views/organizationEventsV2/utils.jsx b/src/sentry/static/sentry/app/views/organizationEventsV2/utils.jsx
index 827ef7293052..f4f880453541 100644
--- a/src/sentry/static/sentry/app/views/organizationEventsV2/utils.jsx
+++ b/src/sentry/static/sentry/app/views/organizationEventsV2/utils.jsx
@@ -2,7 +2,7 @@ import {pick, get} from 'lodash';
 
 import {DEFAULT_PER_PAGE} from 'app/constants';
 import {URL_PARAM} from 'app/constants/globalSelectionHeader';
-import {ALL_VIEWS, SPECIAL_FIELDS} from './data';
+import {ALL_VIEWS} from './data';
 
 /**
  * Given a view id, return the corresponding view object
@@ -22,25 +22,8 @@ export function getCurrentView(requestedView) {
  * @returns {Object}
  */
 export function getQuery(view, location) {
-  const fields = [];
   const groupby = view.data.groupby ? [...view.data.groupby] : [];
-
-  const viewFields = get(view, 'data.fields', []);
-
-  viewFields.forEach(field => {
-    if (SPECIAL_FIELDS.hasOwnProperty(field)) {
-      const specialField = SPECIAL_FIELDS[field];
-
-      if (specialField.hasOwnProperty('fields')) {
-        fields.push(...specialField.fields);
-      }
-      if (specialField.hasOwnProperty('groupby')) {
-        groupby.push(...specialField.groupby);
-      }
-    } else {
-      fields.push(field);
-    }
-  });
+  const fields = get(view, 'data.fields', []);
 
   const data = pick(location.query, [
     'project',
diff --git a/tests/js/spec/views/organizationEventsV2/index.spec.jsx b/tests/js/spec/views/organizationEventsV2/index.spec.jsx
index 1c594939d55e..f7fc728f72c3 100644
--- a/tests/js/spec/views/organizationEventsV2/index.spec.jsx
+++ b/tests/js/spec/views/organizationEventsV2/index.spec.jsx
@@ -94,9 +94,9 @@ describe('OrganizationEventsV2', function() {
     // Sort link should reverse.
     expect(timestamp.props().to.query).toEqual({sort: 'timestamp'});
 
-    const userlink = findLink('user');
+    const userlink = findLink('user.id');
     // User link should be descending.
-    expect(userlink.props().to.query).toEqual({sort: '-user'});
+    expect(userlink.props().to.query).toEqual({sort: '-user.id'});
   });
 
   it('generates links to modals', async function() {
diff --git a/tests/js/spec/views/organizationEventsV2/utils.spec.jsx b/tests/js/spec/views/organizationEventsV2/utils.spec.jsx
index cb7fbdf59978..2b6d23a54968 100644
--- a/tests/js/spec/views/organizationEventsV2/utils.spec.jsx
+++ b/tests/js/spec/views/organizationEventsV2/utils.spec.jsx
@@ -20,30 +20,6 @@ describe('getCurrentView()', function() {
 });
 
 describe('getQuery()', function() {
-  it('expands special "event" and "user" fields', function() {
-    const view = {
-      id: 'test',
-      name: 'test view',
-      data: {
-        fields: ['event', 'user', 'issue.id'],
-      },
-      tags: [],
-    };
-
-    expect(getQuery(view, {}).field).toEqual([
-      'title',
-      'id',
-      'project.name',
-      'user',
-      'user.name',
-      'user.username',
-      'user.email',
-      'user.ip',
-      'user.id',
-      'issue.id',
-    ]);
-  });
-
   it('appends any additional conditions defined for view', function() {
     const view = {
       id: 'test',

From 89be14f312c5f55cfec1c48c45d75685b6a8f0d1 Mon Sep 17 00:00:00 2001
From: Mark Story <mark@sentry.io>
Date: Thu, 8 Aug 2019 15:42:01 -0400
Subject: [PATCH 3/9] Don't allow aggregates on issue.id

Generally not a useful scenario
---
 src/sentry/api/event_search.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/sentry/api/event_search.py b/src/sentry/api/event_search.py
index 82ddaead68d3..10eb3b0fdbe3 100644
--- a/src/sentry/api/event_search.py
+++ b/src/sentry/api/event_search.py
@@ -696,15 +696,15 @@ def get_snuba_query_args(query=None, params=None):
     },
     'avg': {
         'snuba_name': 'avg',
-        'fields': ['issue.id', 'duration'],
+        'fields': ['duration'],
     },
     'min': {
         'snuba_name': 'min',
-        'fields': ['timestamp', 'issue.id', 'duration'],
+        'fields': ['timestamp', 'duration'],
     },
     'max': {
         'snuba_name': 'max',
-        'fields': ['timestamp', 'issue.id', 'duration'],
+        'fields': ['timestamp', 'duration'],
     },
     'sum': {
         'snuba_name': 'sum',

From f52a75d0d6fc3be74ae42dc2168d510a175987b2 Mon Sep 17 00:00:00 2001
From: Mark Story <mark@sentry.io>
Date: Thu, 8 Aug 2019 15:53:09 -0400
Subject: [PATCH 4/9] Remove redundant conditions

---
 src/sentry/api/endpoints/organization_events.py          | 7 ++-----
 tests/snuba/api/endpoints/test_organization_events_v2.py | 2 +-
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/sentry/api/endpoints/organization_events.py b/src/sentry/api/endpoints/organization_events.py
index 4bf6523245a4..d21e91f3a137 100644
--- a/src/sentry/api/endpoints/organization_events.py
+++ b/src/sentry/api/endpoints/organization_events.py
@@ -31,11 +31,8 @@ def get(self, request, organization):
             params = self.get_filter_params(request, organization)
             snuba_args = self.get_snuba_query_args(request, organization, params)
             fields = snuba_args.get('selected_columns')
-            aggregations = snuba_args.get('aggregations')
-            groupby = snuba_args.get('groupby', [])
-
-            if not fields and not aggregations and not groupby:
-                return Response({'detail': 'No fields or groupings provided'}, status=400)
+            if not fields:
+                return Response({'detail': 'No fields provided'}, status=400)
 
         except OrganizationEventsError as exc:
             return Response({'detail': exc.message}, status=400)
diff --git a/tests/snuba/api/endpoints/test_organization_events_v2.py b/tests/snuba/api/endpoints/test_organization_events_v2.py
index f401fcbc4c45..849aa7e8c183 100644
--- a/tests/snuba/api/endpoints/test_organization_events_v2.py
+++ b/tests/snuba/api/endpoints/test_organization_events_v2.py
@@ -564,7 +564,7 @@ def test_no_requested_fields_or_grouping(self):
                 },
             )
         assert response.status_code == 400, response.content
-        assert response.data['detail'] == 'No fields or groupings provided'
+        assert response.data['detail'] == 'No fields provided'
 
     def test_condition_on_aggregate_fails(self):
         self.login_as(user=self.user)

From d4dc8a80bccb27530b16f54d283b9b5b64a57c05 Mon Sep 17 00:00:00 2001
From: Mark Story <mark@sentry.io>
Date: Thu, 8 Aug 2019 16:38:42 -0400
Subject: [PATCH 5/9] Enable cross project aggregations

Don't inject a project.id field when aggregating across non-project
fields. Instead use argMax() to get a project.name from the latest
event.
---
 .../api/endpoints/organization_events.py      | 22 ++++++----
 src/sentry/api/event_search.py                | 13 ++++--
 tests/sentry/api/test_event_search.py         | 27 +++++++-----
 .../endpoints/test_organization_events_v2.py  | 43 +++++++++++++++++++
 4 files changed, 83 insertions(+), 22 deletions(-)

diff --git a/src/sentry/api/endpoints/organization_events.py b/src/sentry/api/endpoints/organization_events.py
index d21e91f3a137..fa3078f104cb 100644
--- a/src/sentry/api/endpoints/organization_events.py
+++ b/src/sentry/api/endpoints/organization_events.py
@@ -122,16 +122,22 @@ def get_legacy(self, request, organization):
         )
 
     def handle_results(self, request, organization, project_ids, results):
+        if not results:
+            return results
+
+        first_row = results[0]
+        if not ('project.id' in first_row or 'projectid' in first_row):
+            return results
+
+        fields = request.GET.getlist('field')
         projects = {p['id']: p['slug'] for p in Project.objects.filter(
             organization=organization,
             id__in=project_ids).values('id', 'slug')}
-
-        fields = request.GET.getlist('field')
-
-        if 'project.name' in fields or 'project' in fields:
-            for result in results:
-                result['project.name'] = projects[result['project.id']]
-                if 'project.id' not in fields:
-                    del result['project.id']
+        for result in results:
+            for key in ('projectid', 'project.id'):
+                if key in result:
+                    result['project.name'] = projects[result[key]]
+                    if key not in fields:
+                        del result[key]
 
         return results
diff --git a/src/sentry/api/event_search.py b/src/sentry/api/event_search.py
index 10eb3b0fdbe3..43599745133f 100644
--- a/src/sentry/api/event_search.py
+++ b/src/sentry/api/event_search.py
@@ -759,6 +759,9 @@ def resolve_field_list(fields, snuba_args):
     groupby = []
     columns = []
     for field in fields:
+        if not isinstance(field, six.string_types):
+            raise InvalidSearchQuery('Field names must be strings')
+
         if field in FIELD_ALIASES:
             special_field = deepcopy(FIELD_ALIASES[field])
             columns.extend(special_field.get('fields', []))
@@ -783,13 +786,17 @@ def resolve_field_list(fields, snuba_args):
     if not rollup:
         # Ensure fields we require to build a functioning interface
         # are present. We don't add fields when using a rollup as the additional fields
-        # would be aggregated away.
-        if 'project.id' not in columns:
-            columns.append('project.id')
+        # would be aggregated away. When there are aggregations
+        # we use argMax to get the latest event/projectid so we can create links.
+        # The `projectid` output name is not a typo, using `project_id` triggers
+        # generates invalid queries.
         if not aggregations and 'id' not in columns:
             columns.append('id')
+            columns.append('project.id')
         if aggregations and 'latest_event' not in fields:
             columns.extend(deepcopy(FIELD_ALIASES['latest_event']['fields']))
+        if aggregations and 'project.id' not in columns:
+            columns.append(['argMax', ['project_id', 'timestamp'], 'projectid'])
 
     basic_fields = [col for col in columns if isinstance(col, six.string_types)]
     if rollup and basic_fields:
diff --git a/tests/sentry/api/test_event_search.py b/tests/sentry/api/test_event_search.py
index ac0a63b838f4..03b36daa2e4b 100644
--- a/tests/sentry/api/test_event_search.py
+++ b/tests/sentry/api/test_event_search.py
@@ -1209,10 +1209,16 @@ def test_simple(self):
 
 
 class ResolveFieldListTest(unittest.TestCase):
+    def test_non_string_field_error(self):
+        fields = [['any', 'thing', 'lol']]
+        with pytest.raises(InvalidSearchQuery) as err:
+            resolve_field_list(fields, {})
+        assert 'Field names' in six.text_type(err)
+
     def test_automatic_fields_no_aggregates(self):
         fields = ['event.type', 'message']
         result = resolve_field_list(fields, {})
-        assert result['selected_columns'] == ['event.type', 'message', 'project.id', 'id']
+        assert result['selected_columns'] == ['event.type', 'message', 'id', 'project.id']
         assert result['aggregations'] == []
         assert result['groupby'] == []
 
@@ -1222,18 +1228,17 @@ def test_automatic_fields_with_aggregate_aliases(self):
         # Automatic fields should be inserted
         assert result['selected_columns'] == [
             'message',
-            'project.id',
             ['argMax', ['id', 'timestamp'], 'latest_event'],
+            ['argMax', ['project_id', 'timestamp'], 'projectid'],
         ]
         assert result['aggregations'] == [
             ['anyHeavy', 'title', 'issue_title']
         ]
-        assert result['groupby'] == ['message', 'project.id']
+        assert result['groupby'] == ['message']
 
     def test_field_alias_expansion(self):
         fields = ['issue_title', 'last_seen', 'latest_event', 'project', 'user', 'message']
         result = resolve_field_list(fields, {})
-        print result
         assert result['selected_columns'] == [
             ['argMax', ['id', 'timestamp'], 'latest_event'],
             'project.id',
@@ -1263,15 +1268,15 @@ def test_aggregate_function_expansion(self):
         result = resolve_field_list(fields, {})
         # Automatic fields should be inserted
         assert result['selected_columns'] == [
-            'project.id',
             ['argMax', ['id', 'timestamp'], 'latest_event'],
+            ['argMax', ['project_id', 'timestamp'], 'projectid'],
         ]
         assert result['aggregations'] == [
             ['uniq', 'user', 'count_unique_user'],
             ['count', 'id', 'count_id'],
             ['avg', 'duration', 'avg_duration'],
         ]
-        assert result['groupby'] == ['project.id']
+        assert result['groupby'] == []
 
     def test_aggregate_function_invalid_name(self):
         with pytest.raises(InvalidSearchQuery) as err:
@@ -1319,7 +1324,7 @@ def test_orderby_basic_field(self):
         fields = ['message']
         snuba_args = {'orderby': '-message'}
         result = resolve_field_list(fields, snuba_args)
-        assert result['selected_columns'] == ['message', 'project.id', 'id']
+        assert result['selected_columns'] == ['message', 'id', 'project.id']
         assert result['aggregations'] == []
         assert result['groupby'] == []
 
@@ -1328,10 +1333,10 @@ def test_orderby_field_alias(self):
         snuba_args = {'orderby': '-issue_title'}
         result = resolve_field_list(fields, snuba_args)
         assert result['selected_columns'] == [
-            'project.id',
-            ['argMax', ['id', 'timestamp'], 'latest_event']
+            ['argMax', ['id', 'timestamp'], 'latest_event'],
+            ['argMax', ['project_id', 'timestamp'], 'projectid'],
         ]
         assert result['aggregations'] == [
-            ['anyHeavy', 'title', 'issue_title']
+            ['anyHeavy', 'title', 'issue_title'],
         ]
-        assert result['groupby'] == ['project.id']
+        assert result['groupby'] == []
diff --git a/tests/snuba/api/endpoints/test_organization_events_v2.py b/tests/snuba/api/endpoints/test_organization_events_v2.py
index 849aa7e8c183..5a2b62dc908a 100644
--- a/tests/snuba/api/endpoints/test_organization_events_v2.py
+++ b/tests/snuba/api/endpoints/test_organization_events_v2.py
@@ -200,17 +200,56 @@ def test_implicit_groupby(self):
         assert len(response.data) == 2
         assert response.data[0] == {
             'project.id': project.id,
+            'project.name': project.slug,
             'issue.id': event1.group_id,
             'count_id': 2,
             'latest_event': event1.event_id,
         }
         assert response.data[1] == {
             'project.id': project.id,
+            'project.name': project.slug,
             'issue.id': event2.group_id,
             'count_id': 1,
             'latest_event': event2.event_id,
         }
 
+    def test_automatic_id_and_project(self):
+        self.login_as(user=self.user)
+        project = self.create_project()
+        self.store_event(
+            data={
+                'event_id': 'a' * 32,
+                'timestamp': self.min_ago,
+                'fingerprint': ['group_1'],
+            },
+            project_id=project.id,
+        )
+        event1 = self.store_event(
+            data={
+                'event_id': 'b' * 32,
+                'timestamp': self.min_ago,
+                'fingerprint': ['group_1'],
+            },
+            project_id=project.id,
+        )
+
+        with self.feature('organizations:events-v2'):
+            response = self.client.get(
+                self.url,
+                format='json',
+                data={
+                    'field': ['count(id)'],
+                },
+            )
+
+        assert response.status_code == 200, response.content
+        assert len(response.data) == 1
+        assert response.data[0] == {
+            'project.name': project.slug,
+            'count_id': 2,
+            'latest_event': event1.event_id,
+        }
+
     def test_orderby(self):
         self.login_as(user=self.user)
         project = self.create_project()
@@ -366,6 +405,10 @@ def test_aliased_fields(self):
         assert response.data[0]['issue.id'] == event1.group_id
         assert response.data[0]['count_id'] == 1
         assert response.data[0]['count_unique_user'] == 1
+        assert 'latest_event' in response.data[0]
+        assert 'project.name' in response.data[0]
+        assert 'projectid' not in response.data[0]
+        assert 'project.id' not in response.data[0]
         assert response.data[1]['issue.id'] == event2.group_id
         assert response.data[1]['count_id'] == 2
         assert response.data[1]['count_unique_user'] == 2

From a897c35bbdbca8c2148470f99683ef9046a18ec7 Mon Sep 17 00:00:00 2001
From: Mark Story <mark@sentry.io>
Date: Fri, 9 Aug 2019 10:08:37 -0400
Subject: [PATCH 6/9] Fix tests

* Remove non-deterministic results.
* Remove condition test for fields that are no longer supported.
---
 .../test_organization_events_stats.py         | 22 -------------------
 .../endpoints/test_organization_events_v2.py  |  9 ++++----
 2 files changed, 5 insertions(+), 26 deletions(-)

diff --git a/tests/snuba/api/endpoints/test_organization_events_stats.py b/tests/snuba/api/endpoints/test_organization_events_stats.py
index 7bcfba9590d9..2444f8fd3974 100644
--- a/tests/snuba/api/endpoints/test_organization_events_stats.py
+++ b/tests/snuba/api/endpoints/test_organization_events_stats.py
@@ -164,25 +164,3 @@ def test_with_event_count_flag(self):
             [{'count': 1}],
             [{'count': 2}],
         ]
-
-    def test_special_fields_ignored(self):
-        url = reverse(
-            'sentry-api-0-organization-events-stats',
-            kwargs={
-                'organization_slug': self.project.organization.slug,
-            }
-        )
-        response = self.client.get('%s?%s' % (url, urlencode({
-            'start': self.day_ago.isoformat()[:19],
-            'end': (self.day_ago + timedelta(hours=1, minutes=59)).isoformat()[:19],
-            'interval': '1h',
-            'yAxis': 'event_count',
-            'query': 'event_count:>5'
-        })), format='json')
-
-        assert response.status_code == 200, response.content
-        assert [attrs for time, attrs in response.data['data']] == [
-            [],
-            [{'count': 1}],
-            [{'count': 2}],
-        ]
diff --git a/tests/snuba/api/endpoints/test_organization_events_v2.py b/tests/snuba/api/endpoints/test_organization_events_v2.py
index 5a2b62dc908a..3f548a73dfd7 100644
--- a/tests/snuba/api/endpoints/test_organization_events_v2.py
+++ b/tests/snuba/api/endpoints/test_organization_events_v2.py
@@ -164,7 +164,7 @@ def test_implicit_groupby(self):
         self.store_event(
             data={
                 'event_id': 'a' * 32,
-                'timestamp': self.min_ago,
+                'timestamp': self.two_min_ago,
                 'fingerprint': ['group_1'],
             },
             project_id=project.id,
@@ -198,6 +198,7 @@ def test_implicit_groupby(self):
 
         assert response.status_code == 200, response.content
         assert len(response.data) == 2
+        print response.content
         assert response.data[0] == {
             'project.id': project.id,
             'project.name': project.slug,
@@ -219,12 +220,12 @@ def test_automatic_id_and_project(self):
         self.store_event(
             data={
                 'event_id': 'a' * 32,
-                'timestamp': self.min_ago,
+                'timestamp': self.two_min_ago,
                 'fingerprint': ['group_1'],
             },
             project_id=project.id,
         )
-        event1 = self.store_event(
+        event = self.store_event(
             data={
                 'event_id': 'b' * 32,
                 'timestamp': self.min_ago,
@@ -247,7 +248,7 @@ def test_automatic_id_and_project(self):
         assert response.data[0] == {
             'project.name': project.slug,
             'count_id': 2,
-            'latest_event': event1.event_id,
+            'latest_event': event.event_id,
         }
 
     def test_orderby(self):

From 89a7ee871701308a4b0b67c5df2d05d0309838b5 Mon Sep 17 00:00:00 2001
From: Mark Story <mark@sentry.io>
Date: Fri, 9 Aug 2019 12:11:36 -0400
Subject: [PATCH 7/9] Remove print

---
 tests/snuba/api/endpoints/test_organization_events_v2.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/snuba/api/endpoints/test_organization_events_v2.py b/tests/snuba/api/endpoints/test_organization_events_v2.py
index 3f548a73dfd7..dfedbef93eb5 100644
--- a/tests/snuba/api/endpoints/test_organization_events_v2.py
+++ b/tests/snuba/api/endpoints/test_organization_events_v2.py
@@ -198,7 +198,6 @@ def test_implicit_groupby(self):
 
         assert response.status_code == 200, response.content
         assert len(response.data) == 2
-        print response.content
         assert response.data[0] == {
             'project.id': project.id,
             'project.name': project.slug,

From ef18130f8be1ac4a77ef0a4b547c5876c2443130 Mon Sep 17 00:00:00 2001
From: Mark Story <mark@sentry.io>
Date: Fri, 9 Aug 2019 16:49:30 -0400
Subject: [PATCH 8/9] Move aggregate fields into the aggregate slot.

We need to use string function names in the short term as snuba's json
schema validation prevents aggregates on more than one column. I've also
changed the logic to allow rollup with basic fields as long as at least
one aggregate is also specified.
---
 .../api/endpoints/organization_events.py      |  3 +-
 src/sentry/api/event_search.py                | 23 ++++++------
 tests/sentry/api/test_event_search.py         | 37 ++++++++++++-------
 3 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/src/sentry/api/endpoints/organization_events.py b/src/sentry/api/endpoints/organization_events.py
index fa3078f104cb..7cd3e3a1d68c 100644
--- a/src/sentry/api/endpoints/organization_events.py
+++ b/src/sentry/api/endpoints/organization_events.py
@@ -30,8 +30,7 @@ def get(self, request, organization):
         try:
             params = self.get_filter_params(request, organization)
             snuba_args = self.get_snuba_query_args(request, organization, params)
-            fields = snuba_args.get('selected_columns')
-            if not fields:
+            if not snuba_args.get('selected_columns') and not snuba_args.get('aggregations'):
                 return Response({'detail': 'No fields provided'}, status=400)
 
         except OrganizationEventsError as exc:
diff --git a/src/sentry/api/event_search.py b/src/sentry/api/event_search.py
index 43599745133f..59059b3db211 100644
--- a/src/sentry/api/event_search.py
+++ b/src/sentry/api/event_search.py
@@ -672,8 +672,10 @@ def get_snuba_query_args(query=None, params=None):
         'aggregations': [['max', 'timestamp', 'last_seen']],
     },
     'latest_event': {
-        'fields': [
-            ['argMax', ['id', 'timestamp'], 'latest_event'],
+        'aggregations': [
+            # TODO(mark) This is a hack to work around jsonschema limitations
+            # in snuba.
+            ['argMax(event_id, timestamp)', '', 'latest_event'],
         ],
     },
     'project': {
@@ -717,7 +719,7 @@ def get_snuba_query_args(query=None, params=None):
     },
 }
 
-AGGREGATE_PATTERN = re.compile(r'^(?P<function>[^\(]+)\((?P<column>[a-z\._+]+)\)$')
+AGGREGATE_PATTERN = re.compile(r'^(?P<function>[^\(]+)\((?P<column>[a-z\._]*)\)$')
 
 
 def validate_aggregate(field, match):
@@ -779,7 +781,7 @@ def resolve_field_list(fields, snuba_args):
         aggregations.append([
             VALID_AGGREGATES[match.group('function')]['snuba_name'],
             match.group('column'),
-            u'{}_{}'.format(match.group('function'), match.group('column'))
+            u'{}_{}'.format(match.group('function'), match.group('column')).rstrip('_')
         ])
 
     rollup = snuba_args.get('rollup')
@@ -794,22 +796,21 @@ def resolve_field_list(fields, snuba_args):
             columns.append('id')
             columns.append('project.id')
         if aggregations and 'latest_event' not in fields:
-            columns.extend(deepcopy(FIELD_ALIASES['latest_event']['fields']))
+            aggregations.extend(deepcopy(FIELD_ALIASES['latest_event']['aggregations']))
         if aggregations and 'project.id' not in columns:
-            columns.append(['argMax', ['project_id', 'timestamp'], 'projectid'])
+            aggregations.append(['argMax(project_id, timestamp)', '', 'projectid'])
 
-    basic_fields = [col for col in columns if isinstance(col, six.string_types)]
-    if rollup and basic_fields:
-        raise InvalidSearchQuery('You cannot combine rollup with non-aggregate fields')
+    if rollup and columns and not aggregations:
+        raise InvalidSearchQuery('You cannot use rollup without an aggregate field.')
 
     orderby = snuba_args.get('orderby')
     if orderby:
         validate_orderby(orderby, fields)
 
-    # If aggregations are present all basic fields
+    # If aggregations are present all columns
     # need to be added to the group by so that the query is valid.
     if aggregations:
-        groupby.extend(basic_fields)
+        groupby.extend(columns)
 
     return {
         'selected_columns': columns,
diff --git a/tests/sentry/api/test_event_search.py b/tests/sentry/api/test_event_search.py
index 03b36daa2e4b..046706ed9667 100644
--- a/tests/sentry/api/test_event_search.py
+++ b/tests/sentry/api/test_event_search.py
@@ -1228,11 +1228,11 @@ def test_automatic_fields_with_aggregate_aliases(self):
         # Automatic fields should be inserted
         assert result['selected_columns'] == [
             'message',
-            ['argMax', ['id', 'timestamp'], 'latest_event'],
-            ['argMax', ['project_id', 'timestamp'], 'projectid'],
         ]
         assert result['aggregations'] == [
-            ['anyHeavy', 'title', 'issue_title']
+            ['anyHeavy', 'title', 'issue_title'],
+            ['argMax(event_id, timestamp)', '', 'latest_event'],
+            ['argMax(project_id, timestamp)', '', 'projectid'],
         ]
         assert result['groupby'] == ['message']
 
@@ -1240,7 +1240,6 @@ def test_field_alias_expansion(self):
         fields = ['issue_title', 'last_seen', 'latest_event', 'project', 'user', 'message']
         result = resolve_field_list(fields, {})
         assert result['selected_columns'] == [
-            ['argMax', ['id', 'timestamp'], 'latest_event'],
             'project.id',
             'user.id',
             'user.name',
@@ -1251,7 +1250,8 @@ def test_field_alias_expansion(self):
         ]
         assert result['aggregations'] == [
             ['anyHeavy', 'title', 'issue_title'],
-            ['max', 'timestamp', 'last_seen']
+            ['max', 'timestamp', 'last_seen'],
+            ['argMax(event_id, timestamp)', '', 'latest_event'],
         ]
         assert result['groupby'] == [
             'project.id',
@@ -1267,14 +1267,13 @@ def test_aggregate_function_expansion(self):
         fields = ['count_unique(user)', 'count(id)', 'avg(duration)']
         result = resolve_field_list(fields, {})
         # Automatic fields should be inserted
-        assert result['selected_columns'] == [
-            ['argMax', ['id', 'timestamp'], 'latest_event'],
-            ['argMax', ['project_id', 'timestamp'], 'projectid'],
-        ]
+        assert result['selected_columns'] == []
         assert result['aggregations'] == [
             ['uniq', 'user', 'count_unique_user'],
             ['count', 'id', 'count_id'],
             ['avg', 'duration', 'avg_duration'],
+            ['argMax(event_id, timestamp)', '', 'latest_event'],
+            ['argMax(project_id, timestamp)', '', 'projectid'],
         ]
         assert result['groupby'] == []
 
@@ -1301,7 +1300,18 @@ def test_rollup_with_unaggregated_fields(self):
             fields = ['message']
             snuba_args = {'rollup': 15}
             resolve_field_list(fields, snuba_args)
-        assert 'rollup with non-aggregate' in six.text_type(err)
+        assert 'rollup without an aggregate' in six.text_type(err)
+
+    def test_rollup_with_basic_and_aggregated_fields(self):
+        fields = ['message', 'count()']
+        snuba_args = {'rollup': 15}
+        result = resolve_field_list(fields, snuba_args)
+
+        assert result['aggregations'] == [
+            ['count', '', 'count']
+        ]
+        assert result['selected_columns'] == ['message']
+        assert result['groupby'] == ['message']
 
     def test_rollup_with_aggregated_fields(self):
         fields = ['count_unique(user)']
@@ -1332,11 +1342,10 @@ def test_orderby_field_alias(self):
         fields = ['issue_title']
         snuba_args = {'orderby': '-issue_title'}
         result = resolve_field_list(fields, snuba_args)
-        assert result['selected_columns'] == [
-            ['argMax', ['id', 'timestamp'], 'latest_event'],
-            ['argMax', ['project_id', 'timestamp'], 'projectid'],
-        ]
+        assert result['selected_columns'] == []
         assert result['aggregations'] == [
             ['anyHeavy', 'title', 'issue_title'],
+            ['argMax(event_id, timestamp)', '', 'latest_event'],
+            ['argMax(project_id, timestamp)', '', 'projectid'],
         ]
         assert result['groupby'] == []

From 0fb02355b4eb3afd7ea528ecceb66aa730597de4 Mon Sep 17 00:00:00 2001
From: Mark Story <mark@sentry.io>
Date: Mon, 12 Aug 2019 14:15:47 -0400
Subject: [PATCH 9/9] Remove duration as a field

Duration is currently in use as a tag by a few customers.
---
 src/sentry/api/event_search.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/sentry/api/event_search.py b/src/sentry/api/event_search.py
index 59059b3db211..c8464144462d 100644
--- a/src/sentry/api/event_search.py
+++ b/src/sentry/api/event_search.py
@@ -215,7 +215,6 @@ class SearchVisitor(NodeVisitor):
         'device.battery_level', 'device.charging', 'device.online',
         'device.simulator', 'error.handled', 'issue.id', 'stack.colno',
         'stack.in_app', 'stack.lineno', 'stack.stack_level',
-        'duration',
         # TODO(mark) figure out how to safelist aggregate functions/field aliases
         # so they can be used in conditions
     ])