Elasticsearch: Allow omitting field when metric supports inline script (#32839)

* Elasticsearch: Allow omitting field when metric supports inline script

* Add tests for MetricEditor to show a None option

* Add tests for useFields hook

* Alerting: allow elasticsearch metrics without field

pkg/tsdb/elasticsearch/client/models.go

@@ -280,8 +280,10 @@ type MetricAggregation struct {
 
 // MarshalJSON returns the JSON encoding of the metric aggregation
 func (a *MetricAggregation) MarshalJSON() ([]byte, error) {
-	root := map[string]interface{}{
-		"field": a.Field,
+	root := map[string]interface{}{}
+
+	if a.Field != "" {
+		root["field"] = a.Field
 	}
 
 	for k, v := range a.Settings {

public/app/plugins/datasource/elasticsearch/components/QueryEditor/BucketAggregationsEditor/BucketAggregationEditor.tsx

@@ -1,8 +1,8 @@
-import { MetricFindValue, SelectableValue } from '@grafana/data';
+import { SelectableValue } from '@grafana/data';
 import { InlineSegmentGroup, Segment, SegmentAsync } from '@grafana/ui';
 import React, { FunctionComponent } from 'react';
+import { useFields } from '../../../hooks/useFields';
 import { useDispatch } from '../../../hooks/useStatelessReducer';
-import { useDatasource } from '../ElasticsearchQueryContext';
 import { segmentStyles } from '../styles';
 import { BucketAggregation, BucketAggregationType, isBucketAggregationWithField } from './aggregations';
 import { SettingsEditor } from './SettingsEditor';
@@ -17,11 +17,6 @@ const bucketAggOptions: Array<SelectableValue<BucketAggregationType>> = Object.e
   })
 );
 
-const toSelectableValue = ({ value, text }: MetricFindValue): SelectableValue<string> => ({
-  label: text,
-  value: `${value || text}`,
-});
-
 const toOption = (bucketAgg: BucketAggregation) => ({
   label: bucketAggregationConfig[bucketAgg.type].label,
   value: bucketAgg.type,
@@ -32,24 +27,8 @@ interface QueryMetricEditorProps {
 }
 
 export const BucketAggregationEditor: FunctionComponent<QueryMetricEditorProps> = ({ value }) => {
-  const datasource = useDatasource();
   const dispatch = useDispatch<BucketAggregationAction>();
-
-  // TODO: Move this in a separate hook (and simplify)
-  const getFields = async () => {
-    const get = () => {
-      switch (value.type) {
-        case 'date_histogram':
-          return datasource.getFields('date');
-        case 'geohash_grid':
-          return datasource.getFields('geo_point');
-        default:
-          return datasource.getFields();
-      }
-    };
-
-    return (await get().toPromise()).map(toSelectableValue);
-  };
+  const getFields = useFields(value.type);
 
   return (
     <>

public/app/plugins/datasource/elasticsearch/components/QueryEditor/ElasticsearchQueryContext.test.tsx

@@ -1,9 +1,10 @@
 import React, { FunctionComponent } from 'react';
 import { renderHook } from '@testing-library/react-hooks';
 import { render } from '@testing-library/react';
-import { ElasticsearchProvider, useDatasource, useQuery } from './ElasticsearchQueryContext';
+import { ElasticsearchProvider, useQuery } from './ElasticsearchQueryContext';
 import { ElasticsearchQuery } from '../../types';
 import { ElasticDatasource } from '../../datasource';
+import { getDefaultTimeRange } from '@grafana/data';
 
 const query: ElasticsearchQuery = {
   refId: 'A',
@@ -24,6 +25,7 @@ describe('ElasticsearchQueryContext', () => {
         onChange={onChange}
         datasource={datasource}
         onRunQuery={onRunQuery}
+        range={getDefaultTimeRange()}
       />
     );
 
@@ -39,6 +41,7 @@ describe('ElasticsearchQueryContext', () => {
     expect(onRunQuery).toHaveBeenCalled();
   });
 
+  // the following applies to all hooks in ElasticsearchQueryContext as they all share the same code.
   describe('useQuery Hook', () => {
     it('Should throw when used outside of ElasticsearchQueryContext', () => {
       const { result } = renderHook(() => useQuery());
@@ -53,6 +56,7 @@ describe('ElasticsearchQueryContext', () => {
           query={query}
           onChange={() => {}}
           onRunQuery={() => {}}
+          range={getDefaultTimeRange()}
         >
           {children}
         </ElasticsearchProvider>
@@ -65,28 +69,4 @@ describe('ElasticsearchQueryContext', () => {
       expect(result.current).toBe(query);
     });
   });
-
-  describe('useDatasource Hook', () => {
-    it('Should throw when used outside of ElasticsearchQueryContext', () => {
-      const { result } = renderHook(() => useDatasource());
-
-      expect(result.error).toBeTruthy();
-    });
-
-    it('Should return the current datasource instance', () => {
-      const datasource = {} as ElasticDatasource;
-
-      const wrapper: FunctionComponent = ({ children }) => (
-        <ElasticsearchProvider datasource={datasource} query={query} onChange={() => {}} onRunQuery={() => {}}>
-          {children}
-        </ElasticsearchProvider>
-      );
-
-      const { result } = renderHook(() => useDatasource(), {
-        wrapper,
-      });
-
-      expect(result.current).toBe(datasource);
-    });
-  });
 });

public/app/plugins/datasource/elasticsearch/components/QueryEditor/ElasticsearchQueryContext.tsx

@@ -1,4 +1,4 @@
-import React, { createContext, FunctionComponent, useCallback, useContext } from 'react';
+import React, { Context, createContext, FunctionComponent, useCallback, useContext } from 'react';
 import { ElasticDatasource } from '../../datasource';
 import { combineReducers, useStatelessReducer, DispatchContext } from '../../hooks/useStatelessReducer';
 import { ElasticsearchQuery } from '../../types';
@@ -6,15 +6,18 @@ import { ElasticsearchQuery } from '../../types';
 import { reducer as metricsReducer } from './MetricAggregationsEditor/state/reducer';
 import { reducer as bucketAggsReducer } from './BucketAggregationsEditor/state/reducer';
 import { aliasPatternReducer, queryReducer, initQuery } from './state';
+import { TimeRange } from '@grafana/data';
 
 const DatasourceContext = createContext<ElasticDatasource | undefined>(undefined);
 const QueryContext = createContext<ElasticsearchQuery | undefined>(undefined);
+const RangeContext = createContext<TimeRange | undefined>(undefined);
 
 interface Props {
   query: ElasticsearchQuery;
   onChange: (query: ElasticsearchQuery) => void;
   onRunQuery: () => void;
   datasource: ElasticDatasource;
+  range: TimeRange;
 }
 
 export const ElasticsearchProvider: FunctionComponent<Props> = ({
@@ -23,6 +26,7 @@ export const ElasticsearchProvider: FunctionComponent<Props> = ({
   onRunQuery,
   query,
   datasource,
+  range,
 }) => {
   const onStateChange = useCallback(
     (query: ElasticsearchQuery) => {
@@ -57,27 +61,28 @@ export const ElasticsearchProvider: FunctionComponent<Props> = ({
   return (
     <DatasourceContext.Provider value={datasource}>
       <QueryContext.Provider value={query}>
-        <DispatchContext.Provider value={dispatch}>{children}</DispatchContext.Provider>
+        <RangeContext.Provider value={range}>
+          <DispatchContext.Provider value={dispatch}>{children}</DispatchContext.Provider>
+        </RangeContext.Provider>
       </QueryContext.Provider>
     </DatasourceContext.Provider>
   );
 };
 
-export const useQuery = (): ElasticsearchQuery => {
-  const query = useContext(QueryContext);
+interface GetHook {
+  <T>(context: Context<T>): () => NonNullable<T>;
+}
 
-  if (!query) {
+const getHook: GetHook = (c) => () => {
+  const contextValue = useContext(c);
+
+  if (!contextValue) {
     throw new Error('use ElasticsearchProvider first.');
   }
 
-  return query;
+  return contextValue as NonNullable<typeof contextValue>;
 };
 
-export const useDatasource = () => {
-  const datasource = useContext(DatasourceContext);
-  if (!datasource) {
-    throw new Error('use ElasticsearchProvider first.');
-  }
-
-  return datasource;
-};
+export const useQuery = getHook(QueryContext);
+export const useDatasource = getHook(DatasourceContext);
+export const useRange = getHook(RangeContext);

public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/MetricEditor.test.tsx

@@ -0,0 +1,85 @@
+import { act, fireEvent, render, screen } from '@testing-library/react';
+import { ElasticsearchProvider } from '../ElasticsearchQueryContext';
+import { MetricEditor } from './MetricEditor';
+import React, { ReactNode } from 'react';
+import { ElasticDatasource } from '../../../datasource';
+import { getDefaultTimeRange } from '@grafana/data';
+import { ElasticsearchQuery } from '../../../types';
+import { Average, UniqueCount } from './aggregations';
+import { defaultBucketAgg } from '../../../query_def';
+import { from } from 'rxjs';
+
+describe('Metric Editor', () => {
+  it('Should display a "None" option for "field" if the metric supports inline script', async () => {
+    const avg: Average = {
+      id: '1',
+      type: 'avg',
+    };
+
+    const query: ElasticsearchQuery = {
+      refId: 'A',
+      query: '',
+      metrics: [avg],
+      bucketAggs: [defaultBucketAgg('2')],
+    };
+
+    const getFields: ElasticDatasource['getFields'] = jest.fn(() => from([[]]));
+
+    const wrapper = ({ children }: { children: ReactNode }) => (
+      <ElasticsearchProvider
+        datasource={{ getFields } as ElasticDatasource}
+        query={query}
+        range={getDefaultTimeRange()}
+        onChange={() => {}}
+        onRunQuery={() => {}}
+      >
+        {children}
+      </ElasticsearchProvider>
+    );
+
+    render(<MetricEditor value={avg} />, { wrapper });
+
+    act(() => {
+      fireEvent.click(screen.getByText('Select Field'));
+    });
+
+    expect(await screen.findByText('None')).toBeInTheDocument();
+  });
+
+  it('Should not display a "None" option for "field" if the metric does not support inline script', async () => {
+    const avg: UniqueCount = {
+      id: '1',
+      type: 'cardinality',
+    };
+
+    const query: ElasticsearchQuery = {
+      refId: 'A',
+      query: '',
+      metrics: [avg],
+      bucketAggs: [defaultBucketAgg('2')],
+    };
+
+    const getFields: ElasticDatasource['getFields'] = jest.fn(() => from([[]]));
+
+    const wrapper = ({ children }: { children: ReactNode }) => (
+      <ElasticsearchProvider
+        datasource={{ getFields } as ElasticDatasource}
+        query={query}
+        range={getDefaultTimeRange()}
+        onChange={() => {}}
+        onRunQuery={() => {}}
+      >
+        {children}
+      </ElasticsearchProvider>
+    );
+
+    render(<MetricEditor value={avg} />, { wrapper });
+
+    act(() => {
+      fireEvent.click(screen.getByText('Select Field'));
+    });
+
+    expect(await screen.findByText('No options found')).toBeInTheDocument();
+    expect(screen.queryByText('None')).not.toBeInTheDocument();
+  });
+});

public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/MetricEditor.tsx

@@ -1,7 +1,7 @@
-import { MetricFindValue, SelectableValue } from '@grafana/data';
+import { SelectableValue } from '@grafana/data';
 import { InlineSegmentGroup, Segment, SegmentAsync, useTheme } from '@grafana/ui';
 import { cx } from '@emotion/css';
-import React, { FunctionComponent } from 'react';
+import React, { FunctionComponent, useCallback } from 'react';
 import { useDatasource, useQuery } from '../ElasticsearchQueryContext';
 import { useDispatch } from '../../../hooks/useStatelessReducer';
 import { getStyles } from './styles';
@@ -13,23 +13,20 @@ import { MetricPicker } from '../../MetricPicker';
 import { segmentStyles } from '../styles';
 import {
   isMetricAggregationWithField,
+  isMetricAggregationWithInlineScript,
   isMetricAggregationWithSettings,
   isPipelineAggregation,
   isPipelineAggregationWithMultipleBucketPaths,
   MetricAggregation,
   MetricAggregationType,
 } from './aggregations';
+import { useFields } from '../../../hooks/useFields';
 
 const toOption = (metric: MetricAggregation) => ({
   label: metricAggregationConfig[metric.type].label,
   value: metric.type,
 });
 
-const toSelectableValue = ({ value, text }: MetricFindValue): SelectableValue<string> => ({
-  label: text,
-  value: `${value || text}`,
-});
-
 interface Props {
   value: MetricAggregation;
 }
@@ -68,24 +65,24 @@ export const MetricEditor: FunctionComponent<Props> = ({ value }) => {
   const datasource = useDatasource();
   const query = useQuery();
   const dispatch = useDispatch<MetricAggregationAction>();
+  const getFields = useFields(value.type);
+
+  const loadOptions = useCallback(async () => {
+    const remoteFields = await getFields();
+
+    // Metric aggregations that have inline script support don't require a field to be set.
+    if (isMetricAggregationWithInlineScript(value)) {
+      return [{ label: 'None' }, ...remoteFields];
+    }
+
+    return remoteFields;
+  }, [getFields, value]);
 
   const previousMetrics = query.metrics!.slice(
     0,
     query.metrics!.findIndex((m) => m.id === value.id)
   );
 
-  // TODO: This could be common with the one in BucketAggregationEditor
-  const getFields = async () => {
-    const get = () => {
-      if (value.type === 'cardinality') {
-        return datasource.getFields();
-      }
-      return datasource.getFields('number');
-    };
-
-    return (await get().toPromise()).map(toSelectableValue);
-  };
-
   return (
     <>
       <InlineSegmentGroup>
@@ -99,7 +96,7 @@ export const MetricEditor: FunctionComponent<Props> = ({ value }) => {
         {isMetricAggregationWithField(value) && !isPipelineAggregation(value) && (
           <SegmentAsync
             className={cx(styles.color, segmentStyles)}
-            loadOptions={getFields}
+            loadOptions={loadOptions}
             onChange={(e) => dispatch(changeMetricField(value.id, e.value!))}
             placeholder="Select Field"
             value={value.field}

public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/index.test.tsx

@@ -4,6 +4,7 @@ import { SettingsEditor } from '.';
 import { ElasticsearchProvider } from '../../ElasticsearchQueryContext';
 import { ElasticDatasource } from '../../../../datasource';
 import { ElasticsearchQuery } from '../../../../types';
+import { getDefaultTimeRange } from '@grafana/data';
 
 describe('Settings Editor', () => {
   describe('Raw Data', () => {
@@ -33,6 +34,7 @@ describe('Settings Editor', () => {
           datasource={{} as ElasticDatasource}
           onChange={onChange}
           onRunQuery={() => {}}
+          range={getDefaultTimeRange()}
         >
           <SettingsEditor metric={query.metrics![0]} previousMetrics={[]} />
         </ElasticsearchProvider>
@@ -67,6 +69,7 @@ describe('Settings Editor', () => {
           datasource={{} as ElasticDatasource}
           onChange={onChange}
           onRunQuery={() => {}}
+          range={getDefaultTimeRange()}
         >
           <SettingsEditor metric={onChange.mock.calls[0][0].metrics![0]} previousMetrics={[]} />
         </ElasticsearchProvider>

public/app/plugins/datasource/elasticsearch/components/QueryEditor/index.tsx

@@ -1,5 +1,5 @@
 import React, { FunctionComponent } from 'react';
-import { QueryEditorProps } from '@grafana/data';
+import { getDefaultTimeRange, QueryEditorProps } from '@grafana/data';
 import { ElasticDatasource } from '../../datasource';
 import { ElasticsearchOptions, ElasticsearchQuery } from '../../types';
 import { ElasticsearchProvider } from './ElasticsearchQueryContext';
@@ -17,8 +17,15 @@ export const QueryEditor: FunctionComponent<ElasticQueryEditorProps> = ({
   onChange,
   onRunQuery,
   datasource,
+  range,
 }) => (
-  <ElasticsearchProvider datasource={datasource} onChange={onChange} onRunQuery={onRunQuery} query={query}>
+  <ElasticsearchProvider
+    datasource={datasource}
+    onChange={onChange}
+    onRunQuery={onRunQuery}
+    query={query}
+    range={range || getDefaultTimeRange()}
+  >
     <QueryEditorForm value={query} />
   </ElasticsearchProvider>
 );

public/app/plugins/datasource/elasticsearch/datasource.ts

@@ -619,6 +619,8 @@ export class ElasticDatasource extends DataSourceApi<ElasticsearchQuery, Elastic
   }
 
   // TODO: instead of being a string, this could be a custom type representing all the elastic types
+  // FIXME: This doesn't seem to return actual MetricFindValues, we should either change the return type
+  // or fix the implementation.
   getFields(type?: string, range?: TimeRange): Observable<MetricFindValue[]> {
     const configuredEsVersion = this.esVersion;
     return this.get('/_mapping', range).pipe(

public/app/plugins/datasource/elasticsearch/hooks/useFields.test.tsx

@@ -0,0 +1,73 @@
+import React, { ReactNode } from 'react';
+import { ElasticDatasource } from '../datasource';
+import { from } from 'rxjs';
+import { ElasticsearchProvider } from '../components/QueryEditor/ElasticsearchQueryContext';
+import { getDefaultTimeRange } from '@grafana/data';
+import { ElasticsearchQuery } from '../types';
+import { defaultBucketAgg, defaultMetricAgg } from '../query_def';
+import { renderHook } from '@testing-library/react-hooks';
+import { useFields } from './useFields';
+import { MetricAggregationType } from '../components/QueryEditor/MetricAggregationsEditor/aggregations';
+import { BucketAggregationType } from '../components/QueryEditor/BucketAggregationsEditor/aggregations';
+
+describe('useFields hook', () => {
+  // TODO: If we move the field type to the configuration objects as described in the hook's source
+  // we can stop testing for getField to be called with the correct parameters.
+  it("returns a function that calls datasource's getFields with the correct parameters", async () => {
+    const timeRange = getDefaultTimeRange();
+    const query: ElasticsearchQuery = {
+      refId: 'A',
+      query: '',
+      metrics: [defaultMetricAgg()],
+      bucketAggs: [defaultBucketAgg()],
+    };
+
+    const getFields: ElasticDatasource['getFields'] = jest.fn(() => from([[]]));
+
+    const wrapper = ({ children }: { children: ReactNode }) => (
+      <ElasticsearchProvider
+        datasource={{ getFields } as ElasticDatasource}
+        query={query}
+        range={timeRange}
+        onChange={() => {}}
+        onRunQuery={() => {}}
+      >
+        {children}
+      </ElasticsearchProvider>
+    );
+
+    //
+    // METRIC AGGREGATIONS
+    //
+    // Cardinality works on every kind of data
+    const { result, rerender } = renderHook(
+      (aggregationType: BucketAggregationType | MetricAggregationType) => useFields(aggregationType),
+      { wrapper, initialProps: 'cardinality' }
+    );
+    result.current();
+    expect(getFields).toHaveBeenLastCalledWith(undefined, timeRange);
+
+    // All other metric aggregations only work on numbers
+    rerender('avg');
+    result.current();
+    expect(getFields).toHaveBeenLastCalledWith('number', timeRange);
+
+    //
+    // BUCKET AGGREGATIONS
+    //
+    // Date Histrogram only works on dates
+    rerender('date_histogram');
+    result.current();
+    expect(getFields).toHaveBeenLastCalledWith('date', timeRange);
+
+    // Geohash Grid only works on geo_point data
+    rerender('geohash_grid');
+    result.current();
+    expect(getFields).toHaveBeenLastCalledWith('geo_point', timeRange);
+
+    // All other bucket aggregation work on any kind of data
+    rerender('terms');
+    result.current();
+    expect(getFields).toHaveBeenLastCalledWith(undefined, timeRange);
+  });
+});

public/app/plugins/datasource/elasticsearch/hooks/useFields.ts

@@ -0,0 +1,53 @@
+import { MetricFindValue, SelectableValue } from '@grafana/data';
+import { BucketAggregationType } from '../components/QueryEditor/BucketAggregationsEditor/aggregations';
+import { useDatasource, useRange } from '../components/QueryEditor/ElasticsearchQueryContext';
+import {
+  isMetricAggregationType,
+  MetricAggregationType,
+} from '../components/QueryEditor/MetricAggregationsEditor/aggregations';
+
+type AggregationType = BucketAggregationType | MetricAggregationType;
+
+const getFilter = (aggregationType: AggregationType) => {
+  // For all metric types we want only numbers, except for cardinality
+  // TODO: To have a more configuration-driven editor, it would be nice to move this logic in
+  // metricAggregationConfig and bucketAggregationConfig so that each aggregation type can specify on
+  // which kind of data it operates.
+  if (isMetricAggregationType(aggregationType)) {
+    if (aggregationType !== 'cardinality') {
+      return 'number';
+    }
+
+    return void 0;
+  }
+
+  switch (aggregationType) {
+    case 'date_histogram':
+      return 'date';
+    case 'geohash_grid':
+      return 'geo_point';
+    default:
+      return void 0;
+  }
+};
+
+const toSelectableValue = ({ text }: MetricFindValue): SelectableValue<string> => ({
+  label: text,
+  value: text,
+});
+
+/**
+ * Returns a function to query the configured datasource for autocomplete values for the specified aggregation type.
+ * Each aggregation can be run on different types, for example avg only operates on numeric fields, geohash_grid only on geo_point fields.
+ * @param aggregationType the type of aggregation to get fields for
+ */
+export const useFields = (aggregationType: AggregationType) => {
+  const datasource = useDatasource();
+  const range = useRange();
+  const filter = getFilter(aggregationType);
+
+  return async () => {
+    const rawFields = await datasource.getFields(filter, range).toPromise();
+    return rawFields.map(toSelectableValue);
+  };
+};

public/app/plugins/datasource/elasticsearch/hooks/useNextId.test.tsx

@@ -3,6 +3,7 @@ import { renderHook } from '@testing-library/react-hooks';
 import { ElasticsearchProvider } from '../components/QueryEditor/ElasticsearchQueryContext';
 import { useNextId } from './useNextId';
 import { ElasticsearchQuery } from '../types';
+import { getDefaultTimeRange } from '@grafana/data';
 
 describe('useNextId', () => {
   it('Should return the next available id', () => {
@@ -14,7 +15,13 @@ describe('useNextId', () => {
     };
     const wrapper: FunctionComponent = ({ children }) => {
       return (
-        <ElasticsearchProvider query={query} datasource={{} as any} onChange={() => {}} onRunQuery={() => {}}>
+        <ElasticsearchProvider
+          query={query}
+          datasource={{} as any}
+          onChange={() => {}}
+          onRunQuery={() => {}}
+          range={getDefaultTimeRange()}
+        >
           {children}
         </ElasticsearchProvider>
       );