Merge pull request #13774 from miqh/feat/sum-query-suggestion

Implement sum aggregation suggestion with placeholder support

public/app/features/explore/Explore.tsx

@@ -373,9 +373,10 @@ export class Explore extends React.PureComponent<ExploreProps, ExploreState> {
     this.onModifyQueries({ type: 'ADD_FILTER', key: columnKey, value: rowValue });
     this.onModifyQueries({ type: 'ADD_FILTER', key: columnKey, value: rowValue });
   };
   };
 
 
-  onModifyQueries = (action: object, index?: number) => {
+  onModifyQueries = (action, index?: number) => {
     const { datasource } = this.state;
     const { datasource } = this.state;
     if (datasource && datasource.modifyQuery) {
     if (datasource && datasource.modifyQuery) {
+      const preventSubmit = action.preventSubmit;
       this.setState(
       this.setState(
         state => {
         state => {
           const { queries, queryTransactions } = state;
           const { queries, queryTransactions } = state;
@@ -391,16 +392,26 @@ export class Explore extends React.PureComponent<ExploreProps, ExploreState> {
             nextQueryTransactions = [];
             nextQueryTransactions = [];
           } else {
           } else {
             // Modify query only at index
             // Modify query only at index
-            nextQueries = [
-              ...queries.slice(0, index),
-              {
-                key: generateQueryKey(index),
-                query: datasource.modifyQuery(this.queryExpressions[index], action),
-              },
-              ...queries.slice(index + 1),
-            ];
-            // Discard transactions related to row query
-            nextQueryTransactions = queryTransactions.filter(qt => qt.rowIndex !== index);
+            nextQueries = queries.map((q, i) => {
+              // Synchronise all queries with local query cache to ensure consistency
+              q.query = this.queryExpressions[i];
+              return i === index
+                ? {
+                    key: generateQueryKey(index),
+                    query: datasource.modifyQuery(q.query, action),
+                  }
+                : q;
+            });
+            nextQueryTransactions = queryTransactions
+              // Consume the hint corresponding to the action
+              .map(qt => {
+                if (qt.hints != null && qt.rowIndex === index) {
+                  qt.hints = qt.hints.filter(hint => hint.fix.action !== action);
+                }
+                return qt;
+              })
+              // Preserve previous row query transaction to keep results visible if next query is incomplete
+              .filter(qt => preventSubmit || qt.rowIndex !== index);
           }
           }
           this.queryExpressions = nextQueries.map(q => q.query);
           this.queryExpressions = nextQueries.map(q => q.query);
           return {
           return {
@@ -408,7 +419,8 @@ export class Explore extends React.PureComponent<ExploreProps, ExploreState> {
             queryTransactions: nextQueryTransactions,
             queryTransactions: nextQueryTransactions,
           };
           };
         },
         },
-        () => this.onSubmit()
+        // Accepting certain fixes do not result in a well-formed query which should not be submitted
+        !preventSubmit ? () => this.onSubmit() : null
       );
       );
     }
     }
   };
   };

public/app/features/explore/PlaceholdersBuffer.test.ts

+import PlaceholdersBuffer from './PlaceholdersBuffer';
+
+describe('PlaceholdersBuffer', () => {
+  it('does nothing if no placeholders are defined', () => {
+    const text = 'metric';
+    const buffer = new PlaceholdersBuffer(text);
+
+    expect(buffer.hasPlaceholders()).toBe(false);
+    expect(buffer.toString()).toBe(text);
+    expect(buffer.getNextMoveOffset()).toBe(0);
+  });
+
+  it('respects the traversal order of placeholders', () => {
+    const text = 'sum($2 offset $1) by ($3)';
+    const buffer = new PlaceholdersBuffer(text);
+
+    expect(buffer.hasPlaceholders()).toBe(true);
+    expect(buffer.toString()).toBe('sum( offset ) by ()');
+    expect(buffer.getNextMoveOffset()).toBe(12);
+
+    buffer.setNextPlaceholderValue('1h');
+
+    expect(buffer.hasPlaceholders()).toBe(true);
+    expect(buffer.toString()).toBe('sum( offset 1h) by ()');
+    expect(buffer.getNextMoveOffset()).toBe(-10);
+
+    buffer.setNextPlaceholderValue('metric');
+
+    expect(buffer.hasPlaceholders()).toBe(true);
+    expect(buffer.toString()).toBe('sum(metric offset 1h) by ()');
+    expect(buffer.getNextMoveOffset()).toBe(16);
+
+    buffer.setNextPlaceholderValue('label');
+
+    expect(buffer.hasPlaceholders()).toBe(false);
+    expect(buffer.toString()).toBe('sum(metric offset 1h) by (label)');
+    expect(buffer.getNextMoveOffset()).toBe(0);
+  });
+
+  it('respects the traversal order of adjacent placeholders', () => {
+    const text = '$1$3$2$4';
+    const buffer = new PlaceholdersBuffer(text);
+
+    expect(buffer.hasPlaceholders()).toBe(true);
+    expect(buffer.toString()).toBe('');
+    expect(buffer.getNextMoveOffset()).toBe(0);
+
+    buffer.setNextPlaceholderValue('1');
+
+    expect(buffer.hasPlaceholders()).toBe(true);
+    expect(buffer.toString()).toBe('1');
+    expect(buffer.getNextMoveOffset()).toBe(0);
+
+    buffer.setNextPlaceholderValue('2');
+
+    expect(buffer.hasPlaceholders()).toBe(true);
+    expect(buffer.toString()).toBe('12');
+    expect(buffer.getNextMoveOffset()).toBe(-1);
+
+    buffer.setNextPlaceholderValue('3');
+
+    expect(buffer.hasPlaceholders()).toBe(true);
+    expect(buffer.toString()).toBe('132');
+    expect(buffer.getNextMoveOffset()).toBe(1);
+
+    buffer.setNextPlaceholderValue('4');
+
+    expect(buffer.hasPlaceholders()).toBe(false);
+    expect(buffer.toString()).toBe('1324');
+    expect(buffer.getNextMoveOffset()).toBe(0);
+  });
+});

public/app/features/explore/PlaceholdersBuffer.ts

+/**
+ * Provides a stateful means of managing placeholders in text.
+ *
+ * Placeholders are numbers prefixed with the `$` character (e.g. `$1`).
+ * Each number value represents the order in which a placeholder should
+ * receive focus if multiple placeholders exist.
+ *
+ * Example scenario given `sum($3 offset $1) by($2)`:
+ * 1. `sum( offset |) by()`
+ * 2. `sum( offset 1h) by(|)`
+ * 3. `sum(| offset 1h) by (label)`
+ */
+export default class PlaceholdersBuffer {
+  private nextMoveOffset: number;
+  private orders: number[];
+  private parts: string[];
+
+  constructor(text: string) {
+    const result = this.parse(text);
+    const nextPlaceholderIndex = result.orders.length ? result.orders[0] : 0;
+    this.nextMoveOffset = this.getOffsetBetween(result.parts, 0, nextPlaceholderIndex);
+    this.orders = result.orders;
+    this.parts = result.parts;
+  }
+
+  clearPlaceholders() {
+    this.nextMoveOffset = 0;
+    this.orders = [];
+  }
+
+  getNextMoveOffset(): number {
+    return this.nextMoveOffset;
+  }
+
+  hasPlaceholders(): boolean {
+    return this.orders.length > 0;
+  }
+
+  setNextPlaceholderValue(value: string) {
+    if (this.orders.length === 0) {
+      return;
+    }
+    const currentPlaceholderIndex = this.orders[0];
+    this.parts[currentPlaceholderIndex] = value;
+    this.orders = this.orders.slice(1);
+    if (this.orders.length === 0) {
+      this.nextMoveOffset = 0;
+      return;
+    }
+    const nextPlaceholderIndex = this.orders[0];
+    // Case should never happen but handle it gracefully in case
+    if (currentPlaceholderIndex === nextPlaceholderIndex) {
+      this.nextMoveOffset = 0;
+      return;
+    }
+    const backwardMove = currentPlaceholderIndex > nextPlaceholderIndex;
+    const indices = backwardMove
+      ? { start: nextPlaceholderIndex + 1, end: currentPlaceholderIndex + 1 }
+      : { start: currentPlaceholderIndex + 1, end: nextPlaceholderIndex };
+    this.nextMoveOffset = (backwardMove ? -1 : 1) * this.getOffsetBetween(this.parts, indices.start, indices.end);
+  }
+
+  toString(): string {
+    return this.parts.join('');
+  }
+
+  private getOffsetBetween(parts: string[], startIndex: number, endIndex: number) {
+    return parts.slice(startIndex, endIndex).reduce((offset, part) => offset + part.length, 0);
+  }
+
+  private parse(text: string): ParseResult {
+    const placeholderRegExp = /\$(\d+)/g;
+    const parts = [];
+    const orders = [];
+    let textOffset = 0;
+    while (true) {
+      const match = placeholderRegExp.exec(text);
+      if (!match) {
+        break;
+      }
+      const part = text.slice(textOffset, match.index);
+      parts.push(part);
+      // Accounts for placeholders at text boundaries
+      if (part !== '') {
+        parts.push('');
+      }
+      const order = parseInt(match[1], 10);
+      orders.push({ index: parts.length - 1, order });
+      textOffset += part.length + match.length;
+    }
+    // Ensures string serialisation still works if no placeholders were parsed
+    // and also accounts for the remainder of text with placeholders
+    parts.push(text.slice(textOffset));
+    return {
+      // Placeholder values do not necessarily appear sequentially so sort the
+      // indices to traverse in priority order
+      orders: orders.sort((o1, o2) => o1.order - o2.order).map(o => o.index),
+      parts,
+    };
+  }
+}
+
+type ParseResult = {
+  /**
+   * Indices to placeholder items in `parts` in traversal order.
+   */
+  orders: number[];
+  /**
+   * Parts comprising the original text with placeholders occupying distinct items.
+   */
+  parts: string[];
+};

public/app/features/explore/QueryField.tsx

@@ -12,6 +12,7 @@ import NewlinePlugin from './slate-plugins/newline';
 
 
 import Typeahead from './Typeahead';
 import Typeahead from './Typeahead';
 import { makeFragment, makeValue } from './Value';
 import { makeFragment, makeValue } from './Value';
+import PlaceholdersBuffer from './PlaceholdersBuffer';
 
 
 export const TYPEAHEAD_DEBOUNCE = 100;
 export const TYPEAHEAD_DEBOUNCE = 100;
 
 
@@ -61,12 +62,15 @@ export interface TypeaheadInput {
 
 
 class QueryField extends React.PureComponent<TypeaheadFieldProps, TypeaheadFieldState> {
 class QueryField extends React.PureComponent<TypeaheadFieldProps, TypeaheadFieldState> {
   menuEl: HTMLElement | null;
   menuEl: HTMLElement | null;
+  placeholdersBuffer: PlaceholdersBuffer;
   plugins: any[];
   plugins: any[];
   resetTimer: any;
   resetTimer: any;
 
 
   constructor(props, context) {
   constructor(props, context) {
     super(props, context);
     super(props, context);
 
 
+    this.placeholdersBuffer = new PlaceholdersBuffer(props.initialValue || '');
+
     // Base plugins
     // Base plugins
     this.plugins = [ClearPlugin(), NewlinePlugin(), ...props.additionalPlugins];
     this.plugins = [ClearPlugin(), NewlinePlugin(), ...props.additionalPlugins];
 
 
@@ -76,7 +80,7 @@ class QueryField extends React.PureComponent<TypeaheadFieldProps, TypeaheadField
       typeaheadIndex: 0,
       typeaheadIndex: 0,
       typeaheadPrefix: '',
       typeaheadPrefix: '',
       typeaheadText: '',
       typeaheadText: '',
-      value: makeValue(props.initialValue || '', props.syntax),
+      value: makeValue(this.placeholdersBuffer.toString(), props.syntax),
     };
     };
   }
   }
 
 
@@ -101,12 +105,14 @@ class QueryField extends React.PureComponent<TypeaheadFieldProps, TypeaheadField
   componentWillReceiveProps(nextProps: TypeaheadFieldProps) {
   componentWillReceiveProps(nextProps: TypeaheadFieldProps) {
     if (nextProps.syntaxLoaded && !this.props.syntaxLoaded) {
     if (nextProps.syntaxLoaded && !this.props.syntaxLoaded) {
       // Need a bogus edit to re-render the editor after syntax has fully loaded
       // Need a bogus edit to re-render the editor after syntax has fully loaded
-      this.onChange(
-        this.state.value
-          .change()
-          .insertText(' ')
-          .deleteBackward()
-      );
+      const change = this.state.value
+        .change()
+        .insertText(' ')
+        .deleteBackward();
+      if (this.placeholdersBuffer.hasPlaceholders()) {
+        change.move(this.placeholdersBuffer.getNextMoveOffset()).focus();
+      }
+      this.onChange(change);
     }
     }
   }
   }
 
 
@@ -289,7 +295,17 @@ class QueryField extends React.PureComponent<TypeaheadFieldProps, TypeaheadField
           }
           }
 
 
           const suggestion = getSuggestionByIndex(suggestions, typeaheadIndex);
           const suggestion = getSuggestionByIndex(suggestions, typeaheadIndex);
-          this.applyTypeahead(change, suggestion);
+          const nextChange = this.applyTypeahead(change, suggestion);
+
+          const insertTextOperation = nextChange.operations.find(operation => operation.type === 'insert_text');
+          if (insertTextOperation) {
+            const suggestionText = insertTextOperation.text;
+            this.placeholdersBuffer.setNextPlaceholderValue(suggestionText);
+            if (this.placeholdersBuffer.hasPlaceholders()) {
+              nextChange.move(this.placeholdersBuffer.getNextMoveOffset()).focus();
+            }
+          }
+
           return true;
           return true;
         }
         }
         break;
         break;
@@ -336,6 +352,8 @@ class QueryField extends React.PureComponent<TypeaheadFieldProps, TypeaheadField
     // If we dont wait here, menu clicks wont work because the menu
     // If we dont wait here, menu clicks wont work because the menu
     // will be gone.
     // will be gone.
     this.resetTimer = setTimeout(this.resetTypeahead, 100);
     this.resetTimer = setTimeout(this.resetTypeahead, 100);
+    // Disrupting placeholder entry wipes all remaining placeholders needing input
+    this.placeholdersBuffer.clearPlaceholders();
     if (onBlur) {
     if (onBlur) {
       onBlur();
       onBlur();
     }
     }

public/app/plugins/datasource/prometheus/datasource.ts

@@ -464,6 +464,9 @@ export class PrometheusDatasource {
       case 'ADD_RATE': {
       case 'ADD_RATE': {
         return `rate(${query}[5m])`;
         return `rate(${query}[5m])`;
       }
       }
+      case 'ADD_SUM': {
+        return `sum(${query.trim()}) by ($1)`;
+      }
       case 'EXPAND_RULES': {
       case 'EXPAND_RULES': {
         const mapping = action.mapping;
         const mapping = action.mapping;
         if (mapping) {
         if (mapping) {

public/app/plugins/datasource/prometheus/query_hints.ts

@@ -2,6 +2,11 @@ import _ from 'lodash';
 
 
 import { QueryHint } from 'app/types/explore';
 import { QueryHint } from 'app/types/explore';
 
 
+/**
+ * Number of time series results needed before starting to suggest sum aggregation hints
+ */
+export const SUM_HINT_THRESHOLD_COUNT = 20;
+
 export function getQueryHints(query: string, series?: any[], datasource?: any): QueryHint[] {
 export function getQueryHints(query: string, series?: any[], datasource?: any): QueryHint[] {
   const hints = [];
   const hints = [];
 
 
@@ -90,5 +95,24 @@ export function getQueryHints(query: string, series?: any[], datasource?: any):
       });
       });
     }
     }
   }
   }
+
+  if (series.length >= SUM_HINT_THRESHOLD_COUNT) {
+    const simpleMetric = query.trim().match(/^\w+$/);
+    if (simpleMetric) {
+      hints.push({
+        type: 'ADD_SUM',
+        label: 'Many time series results returned.',
+        fix: {
+          label: 'Consider aggregating with sum().',
+          action: {
+            type: 'ADD_SUM',
+            query: query,
+            preventSubmit: true,
+          },
+        },
+      });
+    }
+  }
+
   return hints.length > 0 ? hints : null;
   return hints.length > 0 ? hints : null;
 }
 }

public/app/plugins/datasource/prometheus/specs/query_hints.test.ts

-import { getQueryHints } from '../query_hints';
+import { getQueryHints, SUM_HINT_THRESHOLD_COUNT } from '../query_hints';
 
 
 describe('getQueryHints()', () => {
 describe('getQueryHints()', () => {
   it('returns no hints for no series', () => {
   it('returns no hints for no series', () => {
@@ -79,4 +79,23 @@ describe('getQueryHints()', () => {
       },
       },
     });
     });
   });
   });
+
+  it('returns a sum hint when many time series results are returned for a simple metric', () => {
+    const seriesCount = SUM_HINT_THRESHOLD_COUNT;
+    const series = Array.from({ length: seriesCount }, _ => ({
+      datapoints: [[0, 0], [0, 0]],
+    }));
+    const hints = getQueryHints('metric', series);
+    expect(hints.length).toBe(seriesCount);
+    expect(hints[0]).toMatchObject({
+      label: 'Many time series results returned.',
+      index: 0,
+      fix: {
+        action: {
+          type: 'ADD_SUM',
+          query: 'metric',
+        },
+      },
+    });
+  });
 });
 });

public/app/types/explore.ts

@@ -119,6 +119,7 @@ export interface QueryFix {
 export interface QueryFixAction {
 export interface QueryFixAction {
   type: string;
   type: string;
   query?: string;
   query?: string;
+  preventSubmit?: boolean;
 }
 }
 
 
 export interface QueryHint {
 export interface QueryHint {