fix(discover2): For count() drilldowns, preserve aggregated fields by converting them to their non-aggregated form (#16737)

src/sentry/static/sentry/app/views/eventsV2/utils.tsx

@@ -384,13 +384,29 @@ export function downloadAsCsv(tableData, columnOrder, filename) {
   link.remove();
 }
 
+// transform a given aggregated field to its un-aggregated form.
+// the given field can be transformed into another field, or undefined if it'll need to be dropped.
+type AggregateTransformer = (field: string) => string | undefined;
+
+// a map between a field alias to a transform function to convert the aggregated field alias into
+// its un-aggregated form
+const TRANSFORM_AGGREGATES: {[field: string]: AggregateTransformer} = {
+  p99: () => 'transaction.duration',
+  p95: () => 'transaction.duration',
+  p75: () => 'transaction.duration',
+  last_seen: () => 'timestamp',
+  latest_event: () => 'id',
+  apdex: () => undefined,
+  impact: () => undefined,
+};
+
 export function getExpandedResults(
   eventView: EventView,
   additionalConditions: {[key: string]: string},
   dataRow?: TableDataRow | Event
 ): EventView {
   let nextView = eventView.clone();
-  const fieldsToRemove: number[] = [];
+  const fieldsToUpdate: number[] = [];
 
   // Workaround around readonly typing
   const aggregateAliases: string[] = [...AGGREGATE_ALIASES];
@@ -398,9 +414,9 @@ export function getExpandedResults(
   nextView.fields.forEach((field: FieldType, index: number) => {
     const column = explodeField(field);
 
-    // Remove aggregates as the expanded results have no aggregates.
+    // Mark aggregated fields to be transformed into its un-aggregated form
     if (column.aggregation || aggregateAliases.includes(column.field)) {
-      fieldsToRemove.push(index);
+      fieldsToUpdate.push(index);
       return;
     }
 
@@ -423,10 +439,79 @@ export function getExpandedResults(
     }
   });
 
-  // Remove fields from view largest index to smallest to not
-  // disturbe lower indexes until the end.
-  fieldsToRemove.reverse().forEach((i: number) => {
-    nextView = nextView.withDeletedColumn(i, undefined);
+  const transformedFields = new Set();
+  const fieldsToDelete: number[] = [];
+
+  // make a best effort to transform aggregated columns with its non-aggregated form
+  fieldsToUpdate.forEach((indexToUpdate: number) => {
+    const currentField: FieldType = nextView.fields[indexToUpdate];
+    const exploded = explodeField(currentField);
+
+    // check if we can use an aggregated transform function
+
+    const fieldNameAlias = TRANSFORM_AGGREGATES[exploded.aggregation]
+      ? exploded.aggregation
+      : TRANSFORM_AGGREGATES[exploded.field]
+      ? exploded.field
+      : undefined;
+
+    const transform = fieldNameAlias && TRANSFORM_AGGREGATES[fieldNameAlias];
+
+    if (fieldNameAlias && transform) {
+      const nextFieldName = transform(fieldNameAlias);
+
+      if (!nextFieldName || transformedFields.has(nextFieldName)) {
+        // this field is either duplicated in another column, or nextFieldName is undefined.
+        // in either case, we remove this column
+        fieldsToDelete.push(indexToUpdate);
+        return;
+      }
+
+      const updatedColumn = {
+        aggregation: '',
+        field: nextFieldName,
+        width: exploded.width,
+      };
+
+      transformedFields.add(nextFieldName);
+      nextView = nextView.withUpdatedColumn(indexToUpdate, updatedColumn, undefined);
+
+      return;
+    }
+
+    // otherwise just use exploded.field as a column
+
+    if (!exploded.field) {
+      // edge case: transform count() into id
+
+      if (exploded.aggregation !== 'count') {
+        fieldsToDelete.push(indexToUpdate);
+        return;
+      }
+
+      exploded.field = 'id';
+    }
+
+    if (transformedFields.has(exploded.field)) {
+      // this field is duplicated in another column. we remove this column
+      fieldsToDelete.push(indexToUpdate);
+      return;
+    }
+
+    transformedFields.add(exploded.field);
+
+    const updatedColumn = {
+      aggregation: '',
+      field: exploded.field,
+      width: exploded.width,
+    };
+
+    nextView = nextView.withUpdatedColumn(indexToUpdate, updatedColumn, undefined);
+  });
+
+  // delete any columns marked for deletion
+  fieldsToDelete.reverse().forEach((index: number) => {
+    nextView = nextView.withDeletedColumn(index, undefined);
   });
 
   // filter out any aggregates from the search conditions.

tests/js/spec/views/eventsV2/utils.spec.jsx

@@ -305,12 +305,75 @@ describe('getExpandedResults()', function() {
     environment: ['staging'],
   };
 
-  it('removes aggregate fields from result view', () => {
-    const view = new EventView(state);
-    const result = getExpandedResults(view, {}, {});
+  it('preserve aggregated fields', () => {
+    let view = new EventView(state);
+    let result = getExpandedResults(view, {}, {});
 
-    expect(result.fields).toEqual([{field: 'title'}, {field: 'custom_tag'}]);
+    expect(result.fields).toEqual([
+      {field: 'id', width: 300}, // expect count() to be converted to id
+      {field: 'timestamp', width: 300},
+      {field: 'title'},
+      {field: 'custom_tag'},
+    ]);
     expect(result.query).toEqual('event.type:error');
+
+    // de-duplicate transformed columns
+
+    view = new EventView({
+      ...state,
+      fields: [
+        {field: 'count()'},
+        {field: 'last_seen'},
+        {field: 'title'},
+        {field: 'custom_tag'},
+        {field: 'count(id)'},
+      ],
+    });
+
+    result = getExpandedResults(view, {}, {});
+
+    expect(result.fields).toEqual([
+      {field: 'id', width: 300}, // expect count() to be converted to id
+      {field: 'timestamp', width: 300},
+      {field: 'title'},
+      {field: 'custom_tag'},
+    ]);
+
+    // transform aliased fields, & de-duplicate any transforms
+
+    view = new EventView({
+      ...state,
+      fields: [
+        {field: 'last_seen'}, // expect this to be transformed to transaction.duration
+        {field: 'latest_event'},
+        {field: 'title'},
+        {field: 'avg(transaction.duration)'}, // expect this to be dropped
+        {field: 'p75()'},
+        {field: 'p95()'},
+        {field: 'p99()'},
+        // legacy parameterless functions
+        {field: 'p75'},
+        {field: 'p95'},
+        {field: 'p99'},
+        {field: 'custom_tag'},
+        {field: 'title'}, // not expected to be dropped
+        {field: 'unique_count(id)'},
+        // expect these aliases to be dropped
+        {field: 'apdex'},
+        {field: 'impact'},
+      ],
+    });
+
+    result = getExpandedResults(view, {}, {});
+
+    expect(result.fields).toEqual([
+      {field: 'timestamp', width: 300},
+      {field: 'id', width: 300},
+      {field: 'title'},
+      {field: 'transaction.duration', width: 300},
+      {field: 'custom_tag'},
+      {field: 'title'},
+    ]);
   });
 
   it('applies provided conditions', () => {