Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v7.4.x] Elasticsearch: Fix query initialization logic & query transformation from Prometheus/Loki #31441

Merged
merged 1 commit into from
Feb 24, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { bucketAggregationConfig } from '../utils';
import { removeEmpty } from '../../../../utils';

export const reducer = (
state: BucketAggregation[],
state: ElasticsearchQuery['bucketAggs'],
action: BucketAggregationAction | ChangeMetricTypeAction | InitAction
): ElasticsearchQuery['bucketAggs'] => {
switch (action.type) {
Expand All @@ -28,18 +28,18 @@ export const reducer = (
};

// If the last bucket aggregation is a `date_histogram` we add the new one before it.
const lastAgg = state[state.length - 1];
const lastAgg = state![state!.length - 1];
if (lastAgg?.type === 'date_histogram') {
return [...state.slice(0, state.length - 1), newAgg, lastAgg];
return [...state!.slice(0, state!.length - 1), newAgg, lastAgg];
}

return [...state, newAgg];
return [...state!, newAgg];

case REMOVE_BUCKET_AGG:
return state.filter((bucketAgg) => bucketAgg.id !== action.payload.id);
return state!.filter((bucketAgg) => bucketAgg.id !== action.payload.id);

case CHANGE_BUCKET_AGG_TYPE:
return state.map((bucketAgg) => {
return state!.map((bucketAgg) => {
if (bucketAgg.id !== action.payload.id) {
return bucketAgg;
}
Expand All @@ -58,7 +58,7 @@ export const reducer = (
});

case CHANGE_BUCKET_AGG_FIELD:
return state.map((bucketAgg) => {
return state!.map((bucketAgg) => {
if (bucketAgg.id !== action.payload.id) {
return bucketAgg;
}
Expand All @@ -74,7 +74,7 @@ export const reducer = (
// we remove all of them.
if (metricAggregationConfig[action.payload.type].isSingleMetric) {
return [];
} else if (state.length === 0) {
} else if (state!.length === 0) {
// Else, if there are no bucket aggregations we restore a default one.
// This happens when switching from a metric that requires the absence of bucket aggregations to
// one that requires it.
Expand All @@ -83,7 +83,7 @@ export const reducer = (
return state;

case CHANGE_BUCKET_AGG_SETTING:
return state.map((bucketAgg) => {
return state!.map((bucketAgg) => {
if (bucketAgg.id !== action.payload.bucketAgg.id) {
return bucketAgg;
}
Expand All @@ -102,6 +102,9 @@ export const reducer = (
});

case INIT:
if (state?.length || 0 > 0) {
return state;
}
return [defaultBucketAgg('2')];

default:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { ElasticDatasource } from '../../datasource';

const query: ElasticsearchQuery = {
refId: 'A',
query: '',
metrics: [{ id: '1', type: 'count' }],
bucketAggs: [{ type: 'date_histogram', id: '2' }],
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export const ElasticsearchProvider: FunctionComponent<Props> = ({

// This initializes the query by dispatching an init action to each reducer.
// useStatelessReducer will then call `onChange` with the newly generated query
if (!query.metrics && !query.bucketAggs) {
if (!query.metrics || !query.bucketAggs || query.query === undefined) {
dispatch(initQuery());

return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ describe('Settings Editor', () => {
const initialSize = '500';
const query: ElasticsearchQuery = {
refId: 'A',
query: '',
metrics: [
{
id: metricId,
Expand All @@ -21,6 +22,7 @@ describe('Settings Editor', () => {
},
},
],
bucketAggs: [],
};

const onChange = jest.fn();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,24 +22,26 @@ import {
} from './types';

export const reducer = (
state: MetricAggregation[],
state: ElasticsearchQuery['metrics'],
action: MetricAggregationAction | InitAction
): ElasticsearchQuery['metrics'] => {
switch (action.type) {
case ADD_METRIC:
return [...state, defaultMetricAgg(action.payload.id)];
return [...state!, defaultMetricAgg(action.payload.id)];

case REMOVE_METRIC:
const metricToRemove = state.find((m) => m.id === action.payload.id)!;
const metricsToRemove = [metricToRemove, ...getChildren(metricToRemove, state)];
const resultingMetrics = state.filter((metric) => !metricsToRemove.some((toRemove) => toRemove.id === metric.id));
const metricToRemove = state!.find((m) => m.id === action.payload.id)!;
const metricsToRemove = [metricToRemove, ...getChildren(metricToRemove, state!)];
const resultingMetrics = state!.filter(
(metric) => !metricsToRemove.some((toRemove) => toRemove.id === metric.id)
);
if (resultingMetrics.length === 0) {
return [defaultMetricAgg('1')];
}
return resultingMetrics;

case CHANGE_METRIC_TYPE:
return state
return state!
.filter((metric) =>
// When the new metric type is `isSingleMetric` we remove all other metrics from the query
// leaving only the current one.
Expand All @@ -64,7 +66,7 @@ export const reducer = (
});

case CHANGE_METRIC_FIELD:
return state.map((metric) => {
return state!.map((metric) => {
if (metric.id !== action.payload.id) {
return metric;
}
Expand All @@ -82,7 +84,7 @@ export const reducer = (
});

case TOGGLE_METRIC_VISIBILITY:
return state.map((metric) => {
return state!.map((metric) => {
if (metric.id !== action.payload.id) {
return metric;
}
Expand All @@ -94,7 +96,7 @@ export const reducer = (
});

case CHANGE_METRIC_SETTING:
return state.map((metric) => {
return state!.map((metric) => {
if (metric.id !== action.payload.metric.id) {
return metric;
}
Expand All @@ -119,7 +121,7 @@ export const reducer = (
});

case CHANGE_METRIC_META:
return state.map((metric) => {
return state!.map((metric) => {
if (metric.id !== action.payload.metric.id) {
return metric;
}
Expand All @@ -140,7 +142,7 @@ export const reducer = (
});

case CHANGE_METRIC_ATTRIBUTE:
return state.map((metric) => {
return state!.map((metric) => {
if (metric.id !== action.payload.metric.id) {
return metric;
}
Expand All @@ -152,6 +154,9 @@ export const reducer = (
});

case INIT:
if (state?.length || 0 > 0) {
return state;
}
return [defaultMetricAgg('1')];

default:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ describe('QueryEditor', () => {
const alias = '{{metric}}';
const query: ElasticsearchQuery = {
refId: 'A',
query: '',
alias,
metrics: [
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,29 @@
import { reducerTester } from 'test/core/redux/reducerTester';
import { ElasticsearchQuery } from '../../types';
import { aliasPatternReducer, changeAliasPattern, changeQuery, queryReducer } from './state';
import { aliasPatternReducer, changeAliasPattern, changeQuery, initQuery, queryReducer } from './state';

describe('Query Reducer', () => {
describe('On Init', () => {
it('Should maintain the previous `query` if present', () => {
const initialQuery: ElasticsearchQuery['query'] = 'Some lucene query';

reducerTester()
.givenReducer(queryReducer, initialQuery)
.whenActionIsDispatched(initQuery())
.thenStateShouldEqual(initialQuery);
});

it('Should set an empty `query` if it is not already set', () => {
const initialQuery: ElasticsearchQuery['query'] = undefined;
const expectedQuery = '';

reducerTester()
.givenReducer(queryReducer, initialQuery)
.whenActionIsDispatched(initQuery())
.thenStateShouldEqual(expectedQuery);
});
});

it('Should correctly set `query`', () => {
const expectedQuery: ElasticsearchQuery['query'] = 'Some lucene query';

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Action } from '../../hooks/useStatelessReducer';
import { ElasticsearchQuery } from '../../types';

export const INIT = 'init';
const CHANGE_QUERY = 'change_query';
Expand All @@ -18,6 +19,10 @@ interface ChangeAliasPatternAction extends Action<typeof CHANGE_ALIAS_PATTERN> {
};
}

/**
* When the `initQuery` Action is dispatched, the query gets populated with default values where values are not present.
* This means it won't override any existing value in place, but just ensure the query is in a "runnable" state.
*/
export const initQuery = (): InitAction => ({ type: INIT });

export const changeQuery = (query: string): ChangeQueryAction => ({
Expand All @@ -34,26 +39,29 @@ export const changeAliasPattern = (aliasPattern: string): ChangeAliasPatternActi
},
});

export const queryReducer = (prevQuery: string, action: ChangeQueryAction | InitAction) => {
export const queryReducer = (prevQuery: ElasticsearchQuery['query'], action: ChangeQueryAction | InitAction) => {
switch (action.type) {
case CHANGE_QUERY:
return action.payload.query;

case INIT:
return '';
return prevQuery || '';

default:
return prevQuery;
}
};

export const aliasPatternReducer = (prevAliasPattern: string, action: ChangeAliasPatternAction | InitAction) => {
export const aliasPatternReducer = (
prevAliasPattern: ElasticsearchQuery['alias'],
action: ChangeAliasPatternAction | InitAction
) => {
switch (action.type) {
case CHANGE_ALIAS_PATTERN:
return action.payload.aliasPattern;

case INIT:
return '';
return prevAliasPattern || '';

default:
return prevAliasPattern;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ describe('useNextId', () => {
it('Should return the next available id', () => {
const query: ElasticsearchQuery = {
refId: 'A',
query: '',
metrics: [{ id: '1', type: 'avg' }],
bucketAggs: [{ id: '2', type: 'date_histogram' }],
};
Expand Down