Browse Source

feat(discover) Add tracing functions to column builder (#17513)

Add expanded tracing functions to the column editor. The new functions
require a more complete description of the parameters and input types as
well as managing more input values. I've included required state, and
default values to start with so that using complex fields like apdex()
and impact() are more straightforward to use.

While I've added the idea of a 'refinement' to the EventView Field type,
I don't think it is a good idea long term. Instead I would like to
refactor the Field type to be `{field, parameters: []}` which should
help streamline other discover's guts.

The onchange event fires on every keypress. This would
re-render the entire row on each press which causes focus to be lost
making the fields frustrating to deal with. We need to handle state
closer to the source to prevent elements from being remounted. Having
a stateful input also enables validation rules to be enforced better.
Mark Story 5 years ago
parent
commit
d85fd6ecdd

+ 5 - 0
docs-ui/components/columnEditor.stories.js

@@ -45,6 +45,11 @@ storiesOf('Discover|ColumnEditor', module).add(
         field: 'issue.id',
         aggregation: 'count_unique',
       },
+      {
+        refinement: '0.81',
+        field: 'transaction.duration',
+        aggregation: 'percentile',
+      },
     ];
 
     const showModal = () => {

+ 7 - 7
src/sentry/static/sentry/app/views/eventsV2/data.tsx

@@ -35,7 +35,7 @@ export const AGGREGATE_ALIASES = [
 
 // default list of yAxis options
 export const CHART_AXIS_OPTIONS = [
-  {label: 'count(id)', value: 'count(id)'},
+  {label: 'count()', value: 'count()'},
   {label: 'count_unique(users)', value: 'count_unique(user)'},
 ];
 
@@ -57,12 +57,12 @@ export const TRANSACTION_VIEWS: Readonly<Array<NewQuery>> = [
     fields: [
       'transaction',
       'project',
-      'count(id)',
+      'count()',
       'avg(transaction.duration)',
       'p75()',
       'p95()',
     ],
-    orderby: '-count_id',
+    orderby: '-count',
     query: 'event.type:transaction',
     projects: [],
     version: 2,
@@ -75,8 +75,8 @@ export const ALL_VIEWS: Readonly<Array<NewQuery>> = [
   {
     id: undefined,
     name: t('Errors by Title'),
-    fields: ['title', 'count(id)', 'count_unique(user)', 'project'],
-    orderby: '-count_id',
+    fields: ['title', 'count()', 'count_unique(user)', 'project'],
+    orderby: '-count',
     query: 'event.type:error',
     projects: [],
     version: 2,
@@ -85,8 +85,8 @@ export const ALL_VIEWS: Readonly<Array<NewQuery>> = [
   {
     id: undefined,
     name: t('Errors by URL'),
-    fields: ['url', 'count(id)', 'count_unique(issue.id)'],
-    orderby: '-count_id',
+    fields: ['url', 'count()', 'count_unique(issue.id)'],
+    orderby: '-count',
     query: 'event.type:error',
     projects: [],
     version: 2,

+ 92 - 9
src/sentry/static/sentry/app/views/eventsV2/eventQueryParams.tsx

@@ -11,11 +11,19 @@ export type ColumnType =
 
 export type ColumnValueType = ColumnType | 'never'; // Matches to nothing
 
-export type AggregateParameter = {
-  kind: 'column' | 'value';
-  columnTypes: Readonly<ColumnType[]>;
-  required: boolean;
-};
+export type AggregateParameter =
+  | {
+      kind: 'column';
+      columnTypes: Readonly<ColumnType[]>;
+      defaultValue?: string;
+      required: boolean;
+    }
+  | {
+      kind: 'value';
+      dataType: ColumnType;
+      defaultValue?: string;
+      required: boolean;
+    };
 
 // Refer to src/sentry/api/event_search.py
 export const AGGREGATIONS = {
@@ -84,6 +92,8 @@ export const AGGREGATIONS = {
     outputType: 'timestamp',
     isSortable: true,
   },
+
+  // Tracing functions.
   p75: {
     parameters: [],
     outputType: 'duration',
@@ -100,6 +110,75 @@ export const AGGREGATIONS = {
     outputType: 'duration',
     isSortable: true,
   },
+  percentile: {
+    parameters: [
+      {
+        kind: 'column',
+        columnTypes: ['duration'],
+        defaultValue: 'transaction.duration',
+        required: true,
+      },
+      {
+        kind: 'value',
+        dataType: 'number',
+        defaultValue: '0.5',
+        required: true,
+      },
+    ],
+    outputType: null,
+    isSortable: true,
+  },
+  error_rate: {
+    parameters: [],
+    outputType: 'number',
+    isSortable: true,
+  },
+  apdex: {
+    parameters: [
+      {
+        kind: 'column',
+        columnTypes: ['duration'],
+        defaultValue: 'transaction.duration',
+        required: true,
+      },
+      {
+        kind: 'value',
+        dataType: 'number',
+        defaultValue: '300',
+        required: true,
+      },
+    ],
+    outputType: 'number',
+    isSortable: true,
+  },
+  impact: {
+    parameters: [
+      {
+        kind: 'column',
+        columnTypes: ['duration'],
+        defaultValue: 'transaction.duration',
+        required: true,
+      },
+      {
+        kind: 'value',
+        dataType: 'number',
+        defaultValue: '300',
+        required: true,
+      },
+    ],
+    outputType: 'number',
+    isSortable: true,
+  },
+  rps: {
+    parameters: [],
+    outputType: 'number',
+    isSortable: true,
+  },
+  rpm: {
+    parameters: [],
+    outputType: 'number',
+    isSortable: true,
+  },
 } as const;
 
 assert(
@@ -116,6 +195,7 @@ assert(
 );
 
 export type Aggregation = keyof typeof AGGREGATIONS | '';
+export type AggregationRefinement = string | undefined;
 
 /**
  * Refer to src/sentry/snuba/events.py, search for Columns
@@ -202,12 +282,15 @@ export const TRACING_FIELDS = [
   'transaction.duration',
   'transaction.op',
   'transaction.status',
-  'apdex',
-  'impact',
-  'p99',
-  'p95',
   'p75',
+  'p95',
+  'p99',
+  'percentile',
   'error_rate',
+  'apdex',
+  'impact',
+  'rps',
+  'rpm',
 ];
 
 // In the early days of discover2 these functions were exposed

+ 12 - 11
src/sentry/static/sentry/app/views/eventsV2/eventView.tsx

@@ -55,6 +55,7 @@ export type Field = {
 export type Column = {
   aggregation: string;
   field: string;
+  refinement?: string;
   width?: number;
 };
 
@@ -106,11 +107,13 @@ export function isFieldSortable(field: Field, tableMeta: MetaType | undefined):
   return !!getSortKeyFromField(field, tableMeta);
 }
 
-const generateFieldAsString = (props: {aggregation: string; field: string}): string => {
-  const {aggregation, field} = props;
-  const hasAggregation = aggregation.length > 0;
-
-  return hasAggregation ? `${aggregation}(${field})` : field;
+const generateFieldAsString = (col: Column): string => {
+  const {aggregation, field, refinement} = col;
+  const parameters = [field, refinement].filter(i => i);
+  if (aggregation) {
+    return `${aggregation}(${parameters.join(',')})`;
+  }
+  return field;
 };
 
 const decodeFields = (location: Location): Array<Field> => {
@@ -572,6 +575,7 @@ class EventView {
   withColumns(columns: Column[]): EventView {
     const newEventView = this.clone();
     const fields: Field[] = columns
+      .filter(col => col.field || col.aggregation)
       .map(col => generateFieldAsString(col))
       .map((field, i) => {
         // newly added field
@@ -626,13 +630,11 @@ class EventView {
     updatedColumn: Column,
     tableMeta: MetaType | undefined
   ): EventView {
-    const {aggregation, field, width} = updatedColumn;
-
     const columnToBeUpdated = this.fields[columnIndex];
-    const fieldAsString = generateFieldAsString({field, aggregation});
+    const fieldAsString = generateFieldAsString(updatedColumn);
 
     const updateField = columnToBeUpdated.field !== fieldAsString;
-    const updateWidth = columnToBeUpdated.width !== width;
+    const updateWidth = columnToBeUpdated.width !== updatedColumn.width;
 
     if (!updateField && !updateWidth) {
       return this;
@@ -645,7 +647,7 @@ class EventView {
 
     const updatedField: Field = {
       field: fieldAsString,
-      width: width || COL_WIDTH_UNDEFINED,
+      width: updatedColumn.width || COL_WIDTH_UNDEFINED,
     };
 
     const fields = [...newEventView.fields];
@@ -954,7 +956,6 @@ class EventView {
     const yAxisOptions = this.getYAxisOptions();
 
     const yAxis = this.yAxis;
-
     const defaultOption = yAxisOptions[0].value;
 
     if (!yAxis) {

+ 37 - 19
src/sentry/static/sentry/app/views/eventsV2/table/columnEditCollection.tsx

@@ -9,7 +9,7 @@ import {
 } from 'app/components/events/interfaces/spans/utils';
 import {IconAdd, IconDelete, IconGrabbable} from 'app/icons';
 import {t} from 'app/locale';
-import {SelectValue, OrganizationSummary} from 'app/types';
+import {SelectValue, OrganizationSummary, StringMap} from 'app/types';
 import space from 'app/styles/space';
 import theme from 'app/utils/theme';
 
@@ -35,7 +35,7 @@ type State = {
   left: undefined | number;
   top: undefined | number;
   // Stored as a object so we can find elements later.
-  fieldOptions: {[key: string]: SelectValue<FieldValue>};
+  fieldOptions: StringMap<SelectValue<FieldValue>>;
 };
 
 const DRAG_CLASS = 'draggable-item';
@@ -100,15 +100,16 @@ class ColumnEditCollection extends React.Component<Props, State> {
       fields = fields.filter(item => !TRACING_FIELDS.includes(item));
       functions = functions.filter(item => !TRACING_FIELDS.includes(item));
     }
-    const fieldOptions: {[key: string]: SelectValue<FieldValue>} = {};
+    const fieldOptions: StringMap<SelectValue<FieldValue>> = {};
 
     // Index items by prefixed keys as custom tags
     // can overlap both fields and function names.
     // Having a mapping makes finding the value objects easier
     // later as well.
     functions.forEach(func => {
+      const ellipsis = AGGREGATIONS[func].parameters.length ? '\u2026' : '';
       fieldOptions[`function:${func}`] = {
-        label: `${func}(...)`,
+        label: `${func}(${ellipsis})`,
         value: {
           kind: FieldValueKind.FUNCTION,
           meta: {
@@ -156,7 +157,10 @@ class ColumnEditCollection extends React.Component<Props, State> {
 
   // Signal to the parent that a new column has been added.
   handleAddColumn = () => {
-    const newColumns = [...this.props.columns, {aggregation: '', field: ''}];
+    const newColumns = [
+      ...this.props.columns,
+      {aggregation: '', field: '', refinement: undefined},
+    ];
     this.props.onChange(newColumns);
   };
 
@@ -263,7 +267,7 @@ class ColumnEditCollection extends React.Component<Props, State> {
     });
   };
 
-  renderGhost() {
+  renderGhost(gridColumns: number) {
     const index = this.state.draggingIndex;
     if (typeof index !== 'number' || !this.state.isDragging || !this.portal) {
       return null;
@@ -278,7 +282,7 @@ class ColumnEditCollection extends React.Component<Props, State> {
     };
     const ghost = (
       <Ghost ref={this.dragGhostRef} style={style}>
-        {this.renderItem(col, index, {isGhost: true})}
+        {this.renderItem(col, index, {isGhost: true, gridColumns})}
       </Ghost>
     );
 
@@ -288,7 +292,11 @@ class ColumnEditCollection extends React.Component<Props, State> {
   renderItem(
     col: Column,
     i: number,
-    {canDelete = true, isGhost = false}: {canDelete?: boolean; isGhost?: boolean}
+    {
+      canDelete = true,
+      isGhost = false,
+      gridColumns = 2,
+    }: {canDelete?: boolean; isGhost?: boolean; gridColumns: number}
   ) {
     const {isDragging, draggingTargetIndex, draggingIndex, fieldOptions} = this.state;
 
@@ -297,7 +305,7 @@ class ColumnEditCollection extends React.Component<Props, State> {
     if (isDragging && isGhost === false && draggingTargetIndex === i) {
       placeholder = (
         <DragPlaceholder
-          key={`placeholder:${col.aggregation}:${col.field}`}
+          key={`placeholder:${col.aggregation}:${col.field}:${col.refinement}`}
           className={DRAG_CLASS}
         />
       );
@@ -315,12 +323,11 @@ class ColumnEditCollection extends React.Component<Props, State> {
         : PlaceholderPosition.BOTTOM;
 
     return (
-      <React.Fragment>
+      <React.Fragment
+        key={`${i}:${col.aggregation}:${col.field}:${col.refinement}:${isGhost}`}
+      >
         {position === PlaceholderPosition.TOP && placeholder}
-        <RowContainer
-          className={isGhost ? '' : DRAG_CLASS}
-          key={`container:${col.aggregation}:${col.field}:${isGhost}`}
-        >
+        <RowContainer className={isGhost ? '' : DRAG_CLASS}>
           {canDelete ? (
             <Button
               aria-label={t('Drag to reorder')}
@@ -333,9 +340,11 @@ class ColumnEditCollection extends React.Component<Props, State> {
           )}
           <ColumnEditRow
             fieldOptions={fieldOptions}
+            gridColumns={gridColumns}
             column={col}
             parentIndex={i}
             onChange={this.handleUpdateColumn}
+            takeFocus={i === this.props.columns.length - 1}
           />
           {canDelete ? (
             <Button
@@ -356,16 +365,25 @@ class ColumnEditCollection extends React.Component<Props, State> {
   render() {
     const {columns} = this.props;
     const canDelete = columns.length > 1;
+
+    // Get the longest number of columns so we can layout the rows.
+    // We always want at least 2 columns.
+    const gridColumns = Math.max(
+      ...columns.map(col => (col.field && col.refinement !== undefined ? 3 : 2))
+    );
+
     return (
       <div>
-        {this.renderGhost()}
+        {this.renderGhost(gridColumns)}
         <RowContainer>
-          <Heading>
+          <Heading gridColumns={gridColumns}>
             <StyledSectionHeading>{t('Tag / Field / Function')}</StyledSectionHeading>
             <StyledSectionHeading>{t('Field Parameter')}</StyledSectionHeading>
           </Heading>
         </RowContainer>
-        {columns.map((col: Column, i: number) => this.renderItem(col, i, {canDelete}))}
+        {columns.map((col: Column, i: number) =>
+          this.renderItem(col, i, {canDelete, gridColumns})
+        )}
         <RowContainer>
           <Actions>
             <Button
@@ -422,12 +440,12 @@ const Actions = styled('div')`
   grid-column: 2 / 3;
 `;
 
-const Heading = styled('div')`
+const Heading = styled('div')<{gridColumns: number}>`
   grid-column: 2 / 3;
 
   /* Emulate the grid used in the column editor rows */
   display: grid;
-  grid-template-columns: repeat(2, 1fr);
+  grid-template-columns: repeat(${p => p.gridColumns}, 1fr);
   grid-column-gap: ${space(1)};
 `;
 

+ 266 - 68
src/sentry/static/sentry/app/views/eventsV2/table/columnEditRow.tsx

@@ -4,85 +4,123 @@ import {components} from 'react-select';
 
 import Badge from 'app/components/badge';
 import SelectControl from 'app/components/forms/selectControl';
-import {SelectValue} from 'app/types';
+import {SelectValue, StringMap} from 'app/types';
 import {t} from 'app/locale';
 import space from 'app/styles/space';
 
 import {FieldValueKind, FieldValue} from './types';
-import {FIELD_ALIASES, AggregateParameter} from '../eventQueryParams';
+import {FIELD_ALIASES, ColumnType, AggregateParameter} from '../eventQueryParams';
 import {Column} from '../eventView';
 
-type FieldOptions = {[key: string]: SelectValue<FieldValue>};
+type FieldOptions = StringMap<SelectValue<FieldValue>>;
+
+// Intermediate type that combines the current column
+// data with the AggregateParameter type.
+type ParameterDescription =
+  | {
+      kind: 'value';
+      value: string;
+      dataType: ColumnType;
+      required: boolean;
+    }
+  | {
+      kind: 'column';
+      value: FieldValue | null;
+      options: SelectValue<FieldValue>[];
+      required: boolean;
+    };
+
 type Props = {
   className?: string;
+  takeFocus: boolean;
   parentIndex: number;
   column: Column;
+  gridColumns: number;
   fieldOptions: FieldOptions;
   onChange: (index: number, column: Column) => void;
 };
 
-const NO_OPTIONS: SelectValue<string>[] = [{label: t('N/A'), value: ''}];
-
 class ColumnEditRow extends React.Component<Props> {
   handleFieldChange = ({value}) => {
     const {column} = this.props;
-    let field = column.field,
-      aggregation = column.aggregation;
+    let currentParams: [string, string, string | undefined] = [
+      column.aggregation,
+      column.field,
+      column.refinement,
+    ];
 
     switch (value.kind) {
       case FieldValueKind.TAG:
       case FieldValueKind.FIELD:
-        field = value.meta.name;
-        aggregation = '';
+        currentParams = ['', value.meta.name, ''];
         break;
       case FieldValueKind.FUNCTION:
-        aggregation = value.meta.name;
+        currentParams[0] = value.meta.name;
         // Backwards compatibility for field alias versions of functions.
-        if (FIELD_ALIASES.includes(field)) {
-          field = '';
+        if (currentParams[1] && FIELD_ALIASES.includes(currentParams[1])) {
+          currentParams = [currentParams[0], '', ''];
         }
         break;
       default:
         throw new Error('Invalid field type found in column picker');
     }
 
-    const currentField = this.getFieldOrTagValue(field);
-    if (aggregation && currentField !== null) {
-      const parameterSpec: AggregateParameter = value.meta.parameters[0];
-
-      if (parameterSpec === undefined) {
-        // New function has no parameter, clear the field
-        field = '';
-      } else if (
-        (currentField.kind === FieldValueKind.FIELD ||
-          currentField.kind === FieldValueKind.TAG) &&
-        parameterSpec.columnTypes.includes(currentField.meta.dataType)
-      ) {
-        // New function accepts current field.
-        field = currentField.meta.name;
-      } else {
-        // field does not fit within new function requirements.
-        field = '';
+    if (value.kind === FieldValueKind.FUNCTION) {
+      value.meta.parameters.forEach((param: AggregateParameter, i: number) => {
+        if (param.kind === 'column') {
+          const field = this.getFieldOrTagValue(currentParams[i + 1]);
+          if (field === null) {
+            currentParams[i + 1] = param.defaultValue || '';
+          } else if (
+            (field.kind === FieldValueKind.FIELD || field.kind === FieldValueKind.TAG) &&
+            param.columnTypes.includes(field.meta.dataType)
+          ) {
+            // New function accepts current field.
+            currentParams[i + 1] = field.meta.name;
+          } else {
+            // field does not fit within new function requirements, use the default.
+            currentParams[i + 1] = param.defaultValue || '';
+            currentParams[i + 2] = '';
+          }
+        }
+        if (param.kind === 'value') {
+          currentParams[i + 1] = param.defaultValue;
+        }
+      });
+
+      if (value.meta.parameters.length === 0) {
+        currentParams = [currentParams[0], '', undefined];
       }
     }
 
-    this.triggerChange(field, aggregation);
+    this.triggerChange(...currentParams);
   };
 
   handleFieldParameterChange = ({value}) => {
-    this.triggerChange(value.meta.name, this.props.column.aggregation);
+    const {column} = this.props;
+    this.triggerChange(column.aggregation, value.meta.name, column.refinement);
   };
 
-  triggerChange(field: string, aggregation: string) {
+  handleRefinementChange = (value: string) => {
+    const {column} = this.props;
+    this.triggerChange(column.aggregation, column.field, value);
+  };
+
+  triggerChange(aggregation: string, field: string, refinement?: string) {
     const {parentIndex} = this.props;
     this.props.onChange(parentIndex, {
-      field,
       aggregation,
+      field,
+      refinement,
     });
   }
 
-  getFieldOrTagValue(name: string): FieldValue | null {
+  getFieldOrTagValue(name: string | undefined): FieldValue | null {
     const {fieldOptions} = this.props;
+    if (name === undefined) {
+      return null;
+    }
+
     const fieldName = `field:${name}`;
     const tagName = `tag:${name}`;
     if (fieldOptions[fieldName]) {
@@ -109,8 +147,7 @@ class ColumnEditRow extends React.Component<Props> {
 
   getFieldData() {
     let field: FieldValue | null = null,
-      fieldParameter: FieldValue | null = null,
-      fieldParameterOptions: SelectValue<FieldValue>[] = [];
+      fieldParameter: FieldValue | null = null;
 
     const {column} = this.props;
     let {fieldOptions} = this.props;
@@ -118,6 +155,7 @@ class ColumnEditRow extends React.Component<Props> {
 
     if (fieldOptions[funcName] !== undefined) {
       field = fieldOptions[funcName].value;
+      // TODO move this closer to where it is used.
       fieldParameter = this.getFieldOrTagValue(column.field);
     } else if (!column.aggregation && FIELD_ALIASES.includes(column.field)) {
       // Handle backwards compatible field aliases.
@@ -133,20 +171,39 @@ class ColumnEditRow extends React.Component<Props> {
     fieldOptions = this.appendFieldIfUnknown(fieldOptions, field);
     fieldOptions = this.appendFieldIfUnknown(fieldOptions, fieldParameter);
 
+    let parameterDescriptions: ParameterDescription[] = [];
+    // Generate options and values for each parameter.
     if (
       field &&
       field.kind === FieldValueKind.FUNCTION &&
       field.meta.parameters.length > 0
     ) {
-      const parameters = field.meta.parameters;
-      fieldParameterOptions = Object.values(fieldOptions).filter(
-        ({value}) =>
-          (value.kind === FieldValueKind.FIELD || value.kind === FieldValueKind.TAG) &&
-          parameters[0].columnTypes.includes(value.meta.dataType)
+      parameterDescriptions = field.meta.parameters.map(
+        (param): ParameterDescription => {
+          if (param.kind === 'column') {
+            return {
+              kind: 'column',
+              value: fieldParameter,
+              required: param.required,
+              options: Object.values(fieldOptions).filter(
+                ({value}) =>
+                  (value.kind === FieldValueKind.FIELD ||
+                    value.kind === FieldValueKind.TAG) &&
+                  param.columnTypes.includes(value.meta.dataType)
+              ),
+            };
+          }
+          return {
+            kind: 'value',
+            value: column.refinement || param.defaultValue || '',
+            dataType: param.dataType,
+            required: param.required,
+          };
+        }
       );
     }
 
-    return {field, fieldOptions, fieldParameter, fieldParameterOptions};
+    return {field, fieldOptions, parameterDescriptions};
   }
 
   appendFieldIfUnknown(
@@ -166,20 +223,103 @@ class ColumnEditRow extends React.Component<Props> {
     return fieldOptions;
   }
 
+  renderParameterInputs(parameters: ParameterDescription[]): React.ReactNode[] {
+    const {gridColumns} = this.props;
+    const inputs = parameters.map((descriptor: ParameterDescription) => {
+      if (descriptor.kind === 'column' && descriptor.options.length > 0) {
+        return (
+          <SelectControl
+            key="select"
+            name="parameter"
+            placeholder={t('Select value')}
+            options={descriptor.options}
+            value={descriptor.value}
+            required={descriptor.required}
+            onChange={this.handleFieldParameterChange}
+          />
+        );
+      }
+      if (descriptor.kind === 'value') {
+        const inputProps = {
+          required: descriptor.required,
+          value: descriptor.value,
+          onUpdate: this.handleRefinementChange,
+        };
+        switch (descriptor.dataType) {
+          case 'number':
+            return (
+              <BufferedInput
+                name="refinement"
+                key="parameter:number"
+                type="text"
+                inputMode="numeric"
+                pattern="[0-9]*(\.[0-9]*)?"
+                {...inputProps}
+              />
+            );
+          case 'integer':
+            return (
+              <BufferedInput
+                name="refinement"
+                key="parameter:integer"
+                type="text"
+                inputMode="numeric"
+                pattern="[0-9]*"
+                {...inputProps}
+              />
+            );
+          default:
+            return (
+              <BufferedInput
+                name="refinement"
+                key="parameter:text"
+                type="text"
+                {...inputProps}
+              />
+            );
+        }
+      }
+      throw new Error(`Unknown parameter type encountered for ${this.props.column}`);
+    });
+
+    // Add enough disabled inputs to fill the grid up.
+    // We always have 1 input.
+    const requiredInputs = gridColumns - inputs.length - 1;
+    if (requiredInputs > 0) {
+      for (let i = 0; i < requiredInputs; i++) {
+        inputs.push(
+          <StyledInput
+            className="form-control"
+            key={`disabled:${i}`}
+            placeholder={t('N/A')}
+            disabled
+          />
+        );
+      }
+    }
+
+    return inputs;
+  }
+
   render() {
-    const {className} = this.props;
-    const {
-      field,
-      fieldOptions,
-      fieldParameter,
-      fieldParameterOptions,
-    } = this.getFieldData();
+    const {className, takeFocus, gridColumns} = this.props;
+    const {field, fieldOptions, parameterDescriptions} = this.getFieldData();
+
+    const selectProps: React.ComponentProps<SelectControl> = {
+      name: 'field',
+      options: Object.values(fieldOptions),
+      placeholder: t('(Required)'),
+      value: field,
+      onChange: this.handleFieldChange,
+    };
+    if (takeFocus && field === null) {
+      selectProps.autoFocus = true;
+    }
 
     return (
-      <Container className={className}>
+      <Container className={className} gridColumns={gridColumns}>
         <SelectControl
-          name="field"
-          options={Object.values(fieldOptions)}
+          {...selectProps}
           components={{
             Option: ({label, value, ...props}) => (
               <components.Option label={label} {...props}>
@@ -190,29 +330,16 @@ class ColumnEditRow extends React.Component<Props> {
               </components.Option>
             ),
           }}
-          placeholder={t('Select (Required)')}
-          value={field}
-          onChange={this.handleFieldChange}
         />
-        {fieldParameterOptions.length === 0 ? (
-          <SelectControl name="parameter" options={NO_OPTIONS} value="" isDisabled />
-        ) : (
-          <SelectControl
-            name="parameter"
-            placeholder={t('Select (Required)')}
-            options={fieldParameterOptions}
-            value={fieldParameter}
-            onChange={this.handleFieldParameterChange}
-          />
-        )}
+        {this.renderParameterInputs(parameterDescriptions)}
       </Container>
     );
   }
 }
 
-const Container = styled('div')`
+const Container = styled('div')<{gridColumns: number}>`
   display: grid;
-  grid-template-columns: repeat(2, 1fr);
+  grid-template-columns: repeat(${p => p.gridColumns}, 1fr);
   grid-column-gap: ${space(1)};
   align-items: center;
 
@@ -226,4 +353,75 @@ const Label = styled('span')`
   width: 100%;
 `;
 
+type InputProps = React.HTMLProps<HTMLInputElement> & {
+  onUpdate: (value: string) => void;
+  value: string;
+};
+type InputState = {value: string};
+
+/**
+ * Because controlled inputs fire onChange on every key stroke,
+ * we can't update the ColumnEditRow that often as it would re-render
+ * the input elements causing focus to be lost.
+ *
+ * Using a buffered input lets us throttle rendering and enforce data
+ * constraints better.
+ */
+class BufferedInput extends React.Component<InputProps, InputState> {
+  state = {
+    value: this.props.value,
+  };
+
+  private input: React.RefObject<HTMLInputElement>;
+
+  constructor(props: InputProps) {
+    super(props);
+    this.input = React.createRef();
+  }
+
+  get isValid() {
+    if (!this.input.current) {
+      return true;
+    }
+    return this.input.current.validity.valid;
+  }
+
+  handleBlur = () => {
+    if (this.isValid) {
+      this.props.onUpdate(this.state.value);
+    } else {
+      this.setState({value: this.props.value});
+    }
+  };
+
+  handleChange = (event: React.ChangeEvent<HTMLInputElement>) => {
+    if (this.isValid) {
+      this.setState({value: event.target.value});
+    }
+  };
+
+  render() {
+    const {onUpdate, ...props} = this.props;
+    return (
+      <StyledInput
+        {...props}
+        ref={this.input}
+        className="form-control"
+        value={this.state.value}
+        onChange={this.handleChange}
+        onBlur={this.handleBlur}
+      />
+    );
+  }
+}
+
+// Set a min-width to allow shrinkage in grid.
+const StyledInput = styled('input')`
+  min-width: 50px;
+
+  &:not([disabled='true']):invalid {
+    border-color: ${p => p.theme.red};
+  }
+`;
+
 export {ColumnEditRow};

+ 1 - 0
src/sentry/static/sentry/app/views/eventsV2/table/tableView.tsx

@@ -65,6 +65,7 @@ class TableView extends React.Component<TableViewProps> {
       aggregation: String(nextColumn.aggregation),
       field: String(nextColumn.field),
       width: nextColumn.width ? Number(nextColumn.width) : COL_WIDTH_UNDEFINED,
+      refinement: nextColumn.refinement,
     };
 
     const tableMeta = (tableData && tableData.meta) || undefined;

+ 2 - 0
src/sentry/static/sentry/app/views/eventsV2/table/types.tsx

@@ -6,6 +6,7 @@ import {
   AggregateParameter,
   Aggregation,
   Field,
+  AggregationRefinement,
 } from '../eventQueryParams';
 import {Field as FieldType} from '../eventView';
 import {MetaType} from '../utils';
@@ -18,6 +19,7 @@ export type TableColumn<K> = GridColumnOrder<K> & {
   // name: string               From GridColumnHeader
   aggregation: Aggregation;
   field: Field;
+  refinement: AggregationRefinement;
   eventViewField: Readonly<FieldType>;
 
   type: ColumnValueType;

+ 24 - 19
src/sentry/static/sentry/app/views/eventsV2/utils.tsx

@@ -26,7 +26,13 @@ import {
   TRANSACTION_VIEWS,
 } from './data';
 import EventView, {Field as FieldType, Column} from './eventView';
-import {Aggregation, Field, AGGREGATIONS, FIELDS} from './eventQueryParams';
+import {
+  Aggregation,
+  AggregationRefinement,
+  Field,
+  AGGREGATIONS,
+  FIELDS,
+} from './eventQueryParams';
 import {TableColumn, TableDataRow} from './table/types';
 
 export type EventQuery = {
@@ -37,14 +43,13 @@ export type EventQuery = {
   per_page?: number;
 };
 
-const AGGREGATE_PATTERN = /^([^\(]+)\((.*)\)$/;
-const ROUND_BRACKETS_PATTERN = /[\(\)]/;
+const AGGREGATE_PATTERN = /^([^\(]+)\((.*?)(?:\s*,\s*(.*))?\)$/;
 
-function explodeFieldString(field: string): {aggregation: string; field: string} {
+function explodeFieldString(field: string): Column {
   const results = field.match(AGGREGATE_PATTERN);
 
   if (results && results.length >= 3) {
-    return {aggregation: results[1], field: results[2]};
+    return {aggregation: results[1], field: results[2], refinement: results[3]};
   }
 
   return {aggregation: '', field};
@@ -56,6 +61,7 @@ export function explodeField(field: FieldType): Column {
   return {
     aggregation: results.aggregation,
     field: results.field,
+    refinement: results.refinement,
     width: field.width || COL_WIDTH_UNDEFINED,
   };
 }
@@ -174,8 +180,9 @@ export function getAggregateAlias(field: string): string {
     return field;
   }
   return field
-    .replace(AGGREGATE_PATTERN, '$1_$2')
+    .replace(AGGREGATE_PATTERN, '$1_$2_$3')
     .replace(/\./g, '_')
+    .replace(/\,/g, '_')
     .replace(/_+$/, '');
 }
 
@@ -190,6 +197,7 @@ const TEMPLATE_TABLE_COLUMN: TableColumn<React.ReactText> = {
   key: '',
   aggregation: '',
   field: '',
+  refinement: undefined,
   name: '',
   width: COL_WIDTH_UNDEFINED,
 
@@ -203,22 +211,19 @@ export function decodeColumnOrder(
   fields: Readonly<FieldType[]>
 ): TableColumn<React.ReactText>[] {
   return fields.map((f: FieldType) => {
-    const col = {aggregationField: f.field, name: f.field, width: f.width};
     const column: TableColumn<React.ReactText> = {...TEMPLATE_TABLE_COLUMN};
+    const col = explodeField(f);
 
-    // "field" will be split into ["field"]
-    // "agg()" will be split into ["agg", "", ""]
-    // "agg(field)" will be split to ["agg", "field", ""]
-    // Any column without brackets are assumed to be a field
-    const aggregationField = col.aggregationField.split(ROUND_BRACKETS_PATTERN);
-
-    if (aggregationField.length === 1) {
-      column.field = aggregationField[0] as Field;
-    } else {
-      column.aggregation = aggregationField[0] as Aggregation;
-      column.field = aggregationField[1] as Field;
+    if (col.aggregation) {
+      column.aggregation = col.aggregation as Aggregation;
+    }
+    if (col.field) {
+      column.field = col.field as Field;
+    }
+    if (col.refinement) {
+      column.refinement = col.refinement as AggregationRefinement;
     }
-    column.key = col.aggregationField;
+    column.key = f.field;
 
     // Aggregations can have a strict outputType or they can inherit from their field.
     // Otherwise use the FIELDS data to infer types.

+ 66 - 32
tests/js/spec/views/eventsV2/eventView.spec.jsx

@@ -225,9 +225,9 @@ describe('EventView.fromSavedQuery()', function() {
 
   it('maps saved query with no conditions', function() {
     const saved = {
-      orderby: '-count_id',
+      orderby: '-count',
       name: 'foo bar',
-      fields: ['release', 'count(id)'],
+      fields: ['release', 'count()'],
       widths: [111, 222],
       dateCreated: '2019-10-30T06:13:17.632078Z',
       environment: ['dev', 'production'],
@@ -236,7 +236,7 @@ describe('EventView.fromSavedQuery()', function() {
       dateUpdated: '2019-10-30T06:13:17.632096Z',
       id: '5',
       projects: [1],
-      yAxis: 'count(id)',
+      yAxis: 'count()',
     };
 
     const eventView = EventView.fromSavedQuery(saved);
@@ -246,13 +246,13 @@ describe('EventView.fromSavedQuery()', function() {
       name: 'foo bar',
       fields: [
         {field: 'release', width: 111},
-        {field: 'count(id)', width: 222},
+        {field: 'count()', width: 222},
       ],
-      sorts: generateSorts(['count_id']),
+      sorts: generateSorts(['count']),
       query: '',
       project: [1],
       environment: ['dev', 'production'],
-      yAxis: 'count(id)',
+      yAxis: 'count()',
     };
 
     expect(eventView).toMatchObject(expected);
@@ -552,7 +552,7 @@ describe('EventView.generateQueryStringObject()', function() {
       end: '2019-10-02T00:00:00',
       statsPeriod: '14d',
       environment: ['staging'],
-      yAxis: 'count(id)',
+      yAxis: 'count()',
     };
 
     const eventView = new EventView(state);
@@ -569,7 +569,7 @@ describe('EventView.generateQueryStringObject()', function() {
       end: '2019-10-02T00:00:00',
       statsPeriod: '14d',
       environment: ['staging'],
-      yAxis: 'count(id)',
+      yAxis: 'count()',
     };
 
     expect(eventView.generateQueryStringObject()).toEqual(expected);
@@ -681,7 +681,7 @@ describe('EventView.getEventsAPIPayload()', function() {
         utc: 'true',
         statsPeriod: '14d',
         cursor: 'some cursor',
-        yAxis: 'count(id)',
+        yAxis: 'count()',
 
         // irrelevant query strings
         bestCountry: 'canada',
@@ -1095,6 +1095,18 @@ describe('EventView.withColumns()', function() {
     ]);
   });
 
+  it('drops empty columns', function() {
+    const newView = eventView.withColumns([
+      {field: 'issue', aggregation: ''},
+      {field: '', aggregation: 'count'},
+      {field: '', aggregation: ''},
+    ]);
+    expect(newView.fields).toEqual([
+      {field: 'issue', width: COL_WIDTH_UNDEFINED},
+      {field: 'count()', width: COL_WIDTH_UNDEFINED},
+    ]);
+  });
+
   it('inherits widths from existing columns when names match', function() {
     const newView = eventView.withColumns([
       {field: '', aggregation: 'count'},
@@ -1189,7 +1201,7 @@ describe('EventView.withNewColumn()', function() {
     expect(eventView2).toMatchObject(nextState);
   });
 
-  it('add an aggregate function with arguments', function() {
+  it('add an aggregate function with field', function() {
     const eventView = new EventView(state);
     const newColumn = {
       aggregation: 'avg',
@@ -1205,6 +1217,20 @@ describe('EventView.withNewColumn()', function() {
     };
     expect(eventView2).toMatchObject(nextState);
   });
+
+  it('add an aggregate function with field & refinement', function() {
+    const eventView = new EventView(state);
+    const newColumn = {
+      aggregation: 'percentile',
+      field: 'transaction.duration',
+      refinement: '0.5',
+    };
+    const updated = eventView.withNewColumn(newColumn);
+    expect(updated.fields).toEqual([
+      ...state.fields,
+      {field: 'percentile(transaction.duration,0.5)', width: COL_WIDTH_UNDEFINED},
+    ]);
+  });
 });
 
 describe('EventView.withUpdatedColumn()', function() {
@@ -1273,18 +1299,16 @@ describe('EventView.withUpdatedColumn()', function() {
     const eventView2 = eventView.withUpdatedColumn(1, newColumn, meta);
 
     expect(eventView2 !== eventView).toBeTruthy();
-
     expect(eventView).toMatchObject(state);
 
     const nextState = {
       ...state,
       fields: [state.fields[0], {field: 'count()'}],
     };
-
     expect(eventView2).toMatchObject(nextState);
   });
 
-  it('update a column to an aggregate function with arguments', function() {
+  it('update a column to an aggregate function with field', function() {
     const eventView = new EventView(state);
 
     const newColumn = {
@@ -1295,17 +1319,31 @@ describe('EventView.withUpdatedColumn()', function() {
     const eventView2 = eventView.withUpdatedColumn(1, newColumn, meta);
 
     expect(eventView2 !== eventView).toBeTruthy();
-
     expect(eventView).toMatchObject(state);
 
     const nextState = {
       ...state,
       fields: [state.fields[0], {field: 'avg(transaction.duration)'}],
     };
-
     expect(eventView2).toMatchObject(nextState);
   });
 
+  it('update a column to an aggregate function with field & refinement', function() {
+    const eventView = new EventView(state);
+
+    const newColumn = {
+      aggregation: 'percentile',
+      field: 'transaction.duration',
+      refinement: '0.5',
+    };
+
+    const newView = eventView.withUpdatedColumn(1, newColumn, meta);
+    expect(newView.fields).toEqual([
+      state.fields[0],
+      {field: 'percentile(transaction.duration,0.5)', width: COL_WIDTH_UNDEFINED},
+    ]);
+  });
+
   describe('update a column that is sorted', function() {
     it('the sorted column is the only sorted column', function() {
       const eventView = new EventView(state);
@@ -1318,7 +1356,6 @@ describe('EventView.withUpdatedColumn()', function() {
       const eventView2 = eventView.withUpdatedColumn(0, newColumn, meta);
 
       expect(eventView2 !== eventView).toBeTruthy();
-
       expect(eventView).toMatchObject(state);
 
       const nextState = {
@@ -1326,7 +1363,6 @@ describe('EventView.withUpdatedColumn()', function() {
         sorts: [{field: 'title', kind: 'desc'}],
         fields: [{field: 'title'}, state.fields[1]],
       };
-
       expect(eventView2).toMatchObject(nextState);
     });
 
@@ -1346,14 +1382,12 @@ describe('EventView.withUpdatedColumn()', function() {
       const eventView2 = eventView.withUpdatedColumn(0, newColumn, meta);
 
       expect(eventView2 !== eventView).toBeTruthy();
-
       expect(eventView).toMatchObject(modifiedState);
 
       const nextState = {
         ...state,
         fields: [{field: 'title'}, state.fields[1], {field: 'count()'}],
       };
-
       expect(eventView2).toMatchObject(nextState);
     });
 
@@ -1486,7 +1520,7 @@ describe('EventView.withDeletedColumn()', function() {
 
       const state2 = {
         ...state,
-        fields: [{field: 'title'}, {field: 'timestamp'}, {field: 'count(id)'}],
+        fields: [{field: 'title'}, {field: 'timestamp'}, {field: 'count()'}],
         sorts: generateSorts(['timestamp']),
       };
 
@@ -1495,7 +1529,7 @@ describe('EventView.withDeletedColumn()', function() {
       const expected = {
         ...state,
         sorts: generateSorts(['title']),
-        fields: [{field: 'title'}, {field: 'count(id)'}],
+        fields: [{field: 'title'}, {field: 'count()'}],
       };
 
       const eventView2 = eventView.withDeletedColumn(1, {});
@@ -1990,18 +2024,18 @@ describe('EventView.getYAxisOptions', function() {
   it('should add aggregate fields as options', function() {
     let thisEventView = new EventView({
       ...state,
-      fields: generateFields(['ignored-field', 'count(user)']),
+      fields: generateFields(['ignored-field', 'count_unique(issue)']),
     });
 
     expect(thisEventView.getYAxisOptions()).toEqual([
-      generateYaxis('count(user)'),
+      generateYaxis('count_unique(issue)'),
       ...CHART_AXIS_OPTIONS,
     ]);
 
     // should de-duplicate entries
     thisEventView = new EventView({
       ...state,
-      fields: generateFields(['ignored-field', 'count(id)']),
+      fields: generateFields(['ignored-field', 'count()']),
     });
 
     expect(thisEventView.getYAxisOptions()).toEqual([...CHART_AXIS_OPTIONS]);
@@ -2012,14 +2046,14 @@ describe('EventView.getYAxisOptions', function() {
       ...state,
       fields: generateFields([
         'ignored-field',
-        'count(user)',
+        'count_unique(issue)',
         'last_seen',
         'latest_event',
       ]),
     });
 
     expect(thisEventView.getYAxisOptions()).toEqual([
-      generateYaxis('count(user)'),
+      generateYaxis('count_unique(issue)'),
       ...CHART_AXIS_OPTIONS,
     ]);
   });
@@ -2038,7 +2072,7 @@ describe('EventView.getYAxis', function() {
   it('should return first default yAxis', function() {
     const thisEventView = new EventView(state);
 
-    expect(thisEventView.getYAxis()).toEqual('count(id)');
+    expect(thisEventView.getYAxis()).toEqual('count()');
   });
 
   it('should return valid yAxis', function() {
@@ -2046,21 +2080,21 @@ describe('EventView.getYAxis', function() {
       ...state,
       fields: generateFields([
         'ignored-field',
-        'count(user)',
+        'count_unique(user)',
         'last_seen',
         'latest_event',
       ]),
-      yAxis: 'count(user)',
+      yAxis: 'count_unique(user)',
     });
 
-    expect(thisEventView.getYAxis()).toEqual('count(user)');
+    expect(thisEventView.getYAxis()).toEqual('count_unique(user)');
   });
 
   it('should ignore invalid yAxis', function() {
     const invalid = [
       'last_seen',
       'latest_event',
-      'count(user)', // this is not one of the selected fields
+      'count_unique(issue)', // this is not one of the selected fields
     ];
 
     for (const option of invalid) {
@@ -2071,7 +2105,7 @@ describe('EventView.getYAxis', function() {
       });
 
       // yAxis defaults to the first entry of the default yAxis options
-      expect(thisEventView.getYAxis()).toEqual('count(id)');
+      expect(thisEventView.getYAxis()).toEqual('count()');
     }
   });
 });

Some files were not shown because too many files changed in this diff