From c88af6e22138a011886de9717cbdf0d59387d064 Mon Sep 17 00:00:00 2001 From: Giordano Ricci Date: Mon, 26 Apr 2021 16:54:23 +0100 Subject: [PATCH] Elasticsearch: Add generic support for template variables (#32762) * Elasticsearch: Add generic support for template variables * format MovingAverage settings as numbers * Move formatting logic to query builder & forma serial_diff settings as numbers * modify presence check * add todo * minor fixes * transform string values to numbers * Move casting logic * Slightly cleaner implementation * Add BE tests * Leverage elastic validation when string doesn't resolve to a numeric value * move newly introduced test to testify * add FE query_builder tests * check error * Parse values to float instead of int * Fix tests & ParseFloat bit size --- pkg/tsdb/elasticsearch/time_series_query.go | 27 ++- .../elasticsearch/time_series_query_test.go | 78 +++++++++ .../MovingAverageSettingsEditor.tsx | 163 ++++++------------ .../SettingsEditor/index.tsx | 9 +- .../MetricAggregationsEditor/aggregations.ts | 27 +-- .../MetricAggregationsEditor/utils.ts | 8 +- .../datasource/elasticsearch/datasource.ts | 54 ++++-- .../datasource/elasticsearch/query_builder.ts | 41 +++++ .../elasticsearch/specs/query_builder.test.ts | 63 ++++++- 9 files changed, 318 insertions(+), 152 deletions(-) diff --git a/pkg/tsdb/elasticsearch/time_series_query.go b/pkg/tsdb/elasticsearch/time_series_query.go index a41e92ecbc7..9b6958d9bc5 100644 --- a/pkg/tsdb/elasticsearch/time_series_query.go +++ b/pkg/tsdb/elasticsearch/time_series_query.go @@ -164,7 +164,7 @@ func (e *timeSeriesQuery) processQuery(q *Query, ms *es.MultiSearchRequestBuilde } aggBuilder.Pipeline(m.ID, m.Type, bucketPath, func(a *es.PipelineAggregation) { - a.Settings = m.Settings.MustMap() + a.Settings = m.generateSettingsForDSL() }) } } else { @@ -181,6 +181,31 @@ func (e *timeSeriesQuery) processQuery(q *Query, ms *es.MultiSearchRequestBuilde return nil } +// Casts values to int when required by Elastic's query DSL +func (metricAggregation MetricAgg) generateSettingsForDSL() map[string]interface{} { + setFloatPath := func(path ...string) { + if stringValue, err := metricAggregation.Settings.GetPath(path...).String(); err == nil { + if value, err := strconv.ParseFloat(stringValue, 64); err == nil { + metricAggregation.Settings.SetPath(path, value) + } + } + } + + switch metricAggregation.Type { + case "moving_avg": + setFloatPath("window") + setFloatPath("predict") + setFloatPath("settings", "alpha") + setFloatPath("settings", "beta") + setFloatPath("settings", "gamma") + setFloatPath("settings", "period") + case "serial_diff": + setFloatPath("lag") + } + + return metricAggregation.Settings.MustMap() +} + func addDateHistogramAgg(aggBuilder es.AggBuilder, bucketAgg *BucketAgg, timeFrom, timeTo string) es.AggBuilder { aggBuilder.DateHistogram(bucketAgg.ID, bucketAgg.Field, func(a *es.DateHistogramAgg, b es.AggBuilder) { a.Interval = bucketAgg.Settings.Get("interval").MustString("auto") diff --git a/pkg/tsdb/elasticsearch/time_series_query_test.go b/pkg/tsdb/elasticsearch/time_series_query_test.go index 401a299ce94..976bd3200c5 100644 --- a/pkg/tsdb/elasticsearch/time_series_query_test.go +++ b/pkg/tsdb/elasticsearch/time_series_query_test.go @@ -8,6 +8,7 @@ import ( "github.com/grafana/grafana/pkg/plugins" es "github.com/grafana/grafana/pkg/tsdb/elasticsearch/client" "github.com/grafana/grafana/pkg/tsdb/interval" + "github.com/stretchr/testify/assert" "github.com/grafana/grafana/pkg/components/simplejson" . "github.com/smartystreets/goconvey/convey" @@ -856,6 +857,83 @@ func TestExecuteTimeSeriesQuery(t *testing.T) { }) } +func TestSettingsCasting(t *testing.T) { + from := time.Date(2018, 5, 15, 17, 50, 0, 0, time.UTC) + to := time.Date(2018, 5, 15, 17, 55, 0, 0, time.UTC) + + t.Run("Correctly transforms moving_average settings", func(t *testing.T) { + c := newFakeClient(5) + _, err := executeTsdbQuery(c, `{ + "timeField": "@timestamp", + "bucketAggs": [ + { "type": "date_histogram", "field": "@timestamp", "id": "2" } + ], + "metrics": [ + { "id": "1", "type": "average", "field": "@value" }, + { + "id": "3", + "type": "moving_avg", + "field": "1", + "pipelineAgg": "1", + "settings": { + "model": "holt_winters", + "window": "10", + "predict": "5", + "settings": { + "alpha": "0.5", + "beta": "0.7", + "gamma": "SHOULD NOT CHANGE", + "period": "4" + } + } + } + ] + }`, from, to, 15*time.Second) + assert.Nil(t, err) + sr := c.multisearchRequests[0].Requests[0] + + movingAvgSettings := sr.Aggs[0].Aggregation.Aggs[1].Aggregation.Aggregation.(*es.PipelineAggregation).Settings + + assert.Equal(t, 10., movingAvgSettings["window"]) + assert.Equal(t, 5., movingAvgSettings["predict"]) + + modelSettings := movingAvgSettings["settings"].(map[string]interface{}) + + assert.Equal(t, .5, modelSettings["alpha"]) + assert.Equal(t, .7, modelSettings["beta"]) + assert.Equal(t, "SHOULD NOT CHANGE", modelSettings["gamma"]) + assert.Equal(t, 4., modelSettings["period"]) + }) + + t.Run("Correctly transforms serial_diff settings", func(t *testing.T) { + c := newFakeClient(5) + _, err := executeTsdbQuery(c, `{ + "timeField": "@timestamp", + "bucketAggs": [ + { "type": "date_histogram", "field": "@timestamp", "id": "2" } + ], + "metrics": [ + { "id": "1", "type": "average", "field": "@value" }, + { + "id": "3", + "type": "serial_diff", + "field": "1", + "pipelineAgg": "1", + "settings": { + "lag": "1" + } + } + ] + }`, from, to, 15*time.Second) + assert.Nil(t, err) + sr := c.multisearchRequests[0].Requests[0] + + serialDiffSettings := sr.Aggs[0].Aggregation.Aggs[1].Aggregation.Aggregation.(*es.PipelineAggregation).Settings + + assert.Equal(t, 1., serialDiffSettings["lag"]) + }) +} + type fakeClient struct { version int timeField string diff --git a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/MovingAverageSettingsEditor.tsx b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/MovingAverageSettingsEditor.tsx index 5103c5110f3..b577a32edbd 100644 --- a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/MovingAverageSettingsEditor.tsx +++ b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/MovingAverageSettingsEditor.tsx @@ -1,21 +1,24 @@ -import { Input, InlineField, Select, Switch } from '@grafana/ui'; +import { Input, InlineField, Select, InlineSwitch } from '@grafana/ui'; import React, { FunctionComponent } from 'react'; import { useDispatch } from '../../../../hooks/useStatelessReducer'; import { movingAvgModelOptions } from '../../../../query_def'; import { isEWMAMovingAverage, isHoltMovingAverage, isHoltWintersMovingAverage, MovingAverage } from '../aggregations'; import { changeMetricSetting } from '../state/actions'; +import { SettingField } from './SettingField'; interface Props { metric: MovingAverage; } // The way we handle changes for those settings is not ideal compared to the other components in the editor +// FIXME: using `changeMetricSetting` will cause an error when switching from models that have different options +// as they might be incompatible. We should clear all other options on model change. export const MovingAverageSettingsEditor: FunctionComponent = ({ metric }) => { const dispatch = useDispatch(); return ( <> - + dispatch(changeMetricSetting(metric, 'window', parseInt(e.target.value!, 10)))} - defaultValue={metric.settings?.window} - /> - + - - dispatch(changeMetricSetting(metric, 'predict', parseInt(e.target.value!, 10)))} - defaultValue={metric.settings?.predict} - /> - + - {isEWMAMovingAverage(metric) && ( - <> - - dispatch(changeMetricSetting(metric, 'alpha', parseInt(e.target.value!, 10)))} - defaultValue={metric.settings?.alpha} - /> - - - - ) => - dispatch(changeMetricSetting(metric, 'minimize', e.target.checked)) - } - checked={!!metric.settings?.minimize} - /> - - + {(isEWMAMovingAverage(metric) || isHoltMovingAverage(metric) || isHoltWintersMovingAverage(metric)) && ( + + + dispatch( + changeMetricSetting(metric, 'settings', { + ...metric.settings?.settings, + alpha: e.target.value, + }) + ) + } + defaultValue={metric.settings?.settings?.alpha} + /> + )} - {isHoltMovingAverage(metric) && ( - <> - - - dispatch( - changeMetricSetting(metric, 'settings', { - ...metric.settings?.settings, - alpha: parseInt(e.target.value!, 10), - }) - ) - } - defaultValue={metric.settings?.settings?.alpha} - /> - - - - dispatch( - changeMetricSetting(metric, 'settings', { - ...metric.settings?.settings, - beta: parseInt(e.target.value!, 10), - }) - ) - } - defaultValue={metric.settings?.settings?.beta} - /> - - - - ) => - dispatch(changeMetricSetting(metric, 'minimize', e.target.checked)) - } - checked={!!metric.settings?.minimize} - /> - - + {(isHoltMovingAverage(metric) || isHoltWintersMovingAverage(metric)) && ( + + + dispatch( + changeMetricSetting(metric, 'settings', { + ...metric.settings?.settings, + beta: e.target.value, + }) + ) + } + defaultValue={metric.settings?.settings?.beta} + /> + )} {isHoltWintersMovingAverage(metric) && ( <> - + dispatch( changeMetricSetting(metric, 'settings', { ...metric.settings?.settings, - alpha: parseInt(e.target.value!, 10), - }) - ) - } - defaultValue={metric.settings?.settings?.alpha} - /> - - - - dispatch( - changeMetricSetting(metric, 'settings', { - ...metric.settings?.settings, - beta: parseInt(e.target.value!, 10), - }) - ) - } - defaultValue={metric.settings?.settings?.beta} - /> - - - - dispatch( - changeMetricSetting(metric, 'settings', { - ...metric.settings?.settings, - gamma: parseInt(e.target.value!, 10), + gamma: e.target.value, }) ) } defaultValue={metric.settings?.settings?.gamma} /> - + dispatch( changeMetricSetting(metric, 'settings', { ...metric.settings?.settings, - period: parseInt(e.target.value!, 10), + period: e.target.value!, }) ) } @@ -152,8 +91,8 @@ export const MovingAverageSettingsEditor: FunctionComponent = ({ metric } /> - - + ) => dispatch( changeMetricSetting(metric, 'settings', { ...metric.settings?.settings, pad: e.target.checked }) @@ -162,17 +101,19 @@ export const MovingAverageSettingsEditor: FunctionComponent = ({ metric } checked={!!metric.settings?.settings?.pad} /> - - - ) => - dispatch(changeMetricSetting(metric, 'minimize', e.target.checked)) - } - checked={!!metric.settings?.minimize} - /> - )} + + {(isEWMAMovingAverage(metric) || isHoltMovingAverage(metric) || isHoltWintersMovingAverage(metric)) && ( + + ) => + dispatch(changeMetricSetting(metric, 'minimize', e.target.checked)) + } + checked={!!metric.settings?.minimize} + /> + + )} ); }; diff --git a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/index.tsx b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/index.tsx index 3c5a65f7170..86582ea545f 100644 --- a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/index.tsx +++ b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/index.tsx @@ -37,14 +37,7 @@ export const SettingsEditor: FunctionComponent = ({ metric, previousMetri