From 9de2f1bb8f95107e117f9a46fe7b0a2c31298fce Mon Sep 17 00:00:00 2001 From: Marcus Andersson Date: Thu, 29 Apr 2021 15:10:14 +0200 Subject: [PATCH] Alerting: moving data source uid to query instead of model (#33416) * initial commit. * Some more improvements to the expression data source support. * added tests to verify that time range picker and data source picker only is visible when callbacks is passed to row. * fixing issue with filter in alerting list. * minor refactoring. * removed guarding code, should be fixed in backend. * cleaning the data query if we change to a different data source. --- .../src/selectors/components.ts | 3 + .../components/TimePicker/TimeRangeInput.tsx | 8 +- .../components/AlertingQueryEditor.tsx | 50 +++---- .../alerting/components/AlertingQueryRows.tsx | 78 +++++++---- .../unified/components/rules/RuleDetails.tsx | 31 +++-- .../hooks/useCombinedRuleNamespaces.ts | 2 - .../unified/hooks/useFilteredRules.ts | 26 +++- .../alerting/unified/mocks/grafana-queries.ts | 6 +- .../alerting/unified/utils/rule-form.ts | 1 - .../features/alerting/unified/utils/rules.ts | 4 +- .../expressions/ExpressionDatasource.ts | 36 ++++- public/app/features/plugins/datasource_srv.ts | 13 +- .../query/components/QueryEditorRow.tsx | 25 ++-- .../components/QueryEditorRowHeader.test.tsx | 124 ++++++++++++++++++ ...rRowTitle.tsx => QueryEditorRowHeader.tsx} | 73 ++++++----- .../components/QueryEditorRowTitle.test.tsx | 66 ---------- .../query/components/QueryEditorRows.tsx | 79 ++++++++--- .../features/query/components/QueryGroup.tsx | 5 +- public/app/features/query/state/runRequest.ts | 8 +- public/app/types/unified-alerting-dto.ts | 8 +- public/app/types/unified-alerting.ts | 2 - 21 files changed, 424 insertions(+), 224 deletions(-) create mode 100644 public/app/features/query/components/QueryEditorRowHeader.test.tsx rename public/app/features/query/components/{QueryEditorRowTitle.tsx => QueryEditorRowHeader.tsx} (78%) delete mode 100644 public/app/features/query/components/QueryEditorRowTitle.test.tsx diff --git a/packages/grafana-e2e-selectors/src/selectors/components.ts b/packages/grafana-e2e-selectors/src/selectors/components.ts index 69552ada7da..b1450b8f981 100644 --- a/packages/grafana-e2e-selectors/src/selectors/components.ts +++ b/packages/grafana-e2e-selectors/src/selectors/components.ts @@ -1,4 +1,7 @@ export const Components = { + TimePicker: { + openButton: 'TimePicker Open Button', + }, DataSource: { TestData: { QueryTab: { diff --git a/packages/grafana-ui/src/components/TimePicker/TimeRangeInput.tsx b/packages/grafana-ui/src/components/TimePicker/TimeRangeInput.tsx index e7c9c79bdbf..c5fb84bdc40 100644 --- a/packages/grafana-ui/src/components/TimePicker/TimeRangeInput.tsx +++ b/packages/grafana-ui/src/components/TimePicker/TimeRangeInput.tsx @@ -9,6 +9,7 @@ import { getFocusStyle } from '../Forms/commonStyles'; import { TimePickerButtonLabel } from './TimeRangePicker'; import { TimePickerContent } from './TimeRangePicker/TimePickerContent'; import { otherOptions, quickOptions } from './rangeOptions'; +import { selectors } from '@grafana/e2e-selectors'; const isValidTimeRange = (range: any) => { return dateMath.isValid(range.from) && dateMath.isValid(range.to); @@ -66,7 +67,12 @@ export const TimeRangeInput: FC = ({ return (
-
+
{isValidTimeRange(value) ? ( ) : ( diff --git a/public/app/features/alerting/components/AlertingQueryEditor.tsx b/public/app/features/alerting/components/AlertingQueryEditor.tsx index c9592f12c1e..07bedc56869 100644 --- a/public/app/features/alerting/components/AlertingQueryEditor.tsx +++ b/public/app/features/alerting/components/AlertingQueryEditor.tsx @@ -5,15 +5,11 @@ import { selectors } from '@grafana/e2e-selectors'; import { Button, HorizontalGroup, Icon, stylesFactory, Tooltip } from '@grafana/ui'; import { config, getDataSourceSrv } from '@grafana/runtime'; import { AlertingQueryRows } from './AlertingQueryRows'; -import { - expressionDatasource, - ExpressionDatasourceID, - ExpressionDatasourceUID, -} from '../../expressions/ExpressionDatasource'; +import { dataSource as expressionDatasource, ExpressionDatasourceUID } from '../../expressions/ExpressionDatasource'; import { getNextRefIdChar } from 'app/core/utils/query'; import { defaultCondition } from '../../expressions/utils/expressionTypes'; import { ExpressionQueryType } from '../../expressions/types'; -import { GrafanaQuery, GrafanaQueryModel } from 'app/types/unified-alerting-dto'; +import { GrafanaQuery } from 'app/types/unified-alerting-dto'; interface Props { value?: GrafanaQuery[]; @@ -53,27 +49,29 @@ export class AlertingQueryEditor extends PureComponent { return; } - const alertingQuery: GrafanaQueryModel = { - refId: '', - datasourceUid: defaultDataSource.uid, - datasource: defaultDataSource.name, - }; - - onChange(addQuery(value, alertingQuery)); + onChange( + addQuery(value, { + datasourceUid: defaultDataSource.uid, + model: { + refId: '', + datasource: defaultDataSource.name, + }, + }) + ); }; onNewExpressionQuery = () => { const { onChange, value = [] } = this.props; - const expressionQuery: GrafanaQueryModel = { - ...expressionDatasource.newQuery({ - type: ExpressionQueryType.classic, - conditions: [defaultCondition], - }), - datasourceUid: ExpressionDatasourceUID, - datasource: ExpressionDatasourceID, - }; - onChange(addQuery(value, expressionQuery)); + onChange( + addQuery(value, { + datasourceUid: ExpressionDatasourceUID, + model: expressionDatasource.newQuery({ + type: ExpressionQueryType.classic, + conditions: [defaultCondition], + }), + }) + ); }; renderAddQueryRow(styles: ReturnType) { @@ -123,14 +121,18 @@ export class AlertingQueryEditor extends PureComponent { } } -const addQuery = (queries: GrafanaQuery[], model: GrafanaQueryModel): GrafanaQuery[] => { +const addQuery = ( + queries: GrafanaQuery[], + queryToAdd: Pick +): GrafanaQuery[] => { const refId = getNextRefIdChar(queries); const query: GrafanaQuery = { + ...queryToAdd, refId, queryType: '', model: { - ...model, + ...queryToAdd.model, hide: false, refId: refId, }, diff --git a/public/app/features/alerting/components/AlertingQueryRows.tsx b/public/app/features/alerting/components/AlertingQueryRows.tsx index 4feea83cf65..7124379115d 100644 --- a/public/app/features/alerting/components/AlertingQueryRows.tsx +++ b/public/app/features/alerting/components/AlertingQueryRows.tsx @@ -1,6 +1,6 @@ import React, { PureComponent } from 'react'; import { DragDropContext, Droppable, DropResult } from 'react-beautiful-dnd'; -import { DataQuery, DataSourceApi, DataSourceInstanceSettings, rangeUtil, PanelData, TimeRange } from '@grafana/data'; +import { DataQuery, DataSourceInstanceSettings, rangeUtil, PanelData, TimeRange } from '@grafana/data'; import { getDataSourceSrv } from '@grafana/runtime'; import { QueryEditorRow } from 'app/features/query/components/QueryEditorRow'; import { isExpressionQuery } from 'app/features/expressions/guards'; @@ -18,18 +18,12 @@ interface Props { interface State { dataPerQuery: Record; - defaultDataSource: DataSourceApi; } export class AlertingQueryRows extends PureComponent { constructor(props: Props) { super(props); - this.state = { dataPerQuery: {}, defaultDataSource: {} as DataSourceApi }; - } - - async componentDidMount() { - const defaultDataSource = await getDataSourceSrv().get(); - this.setState({ defaultDataSource }); + this.state = { dataPerQuery: {} }; } onRemoveQuery = (query: DataQuery) => { @@ -40,10 +34,42 @@ export class AlertingQueryRows extends PureComponent { const { queries, onQueriesChange } = this.props; onQueriesChange( queries.map((item, itemIndex) => { - if (itemIndex === index) { - return { ...item, relativeTimeRange: rangeUtil.timeRangeToRelative(timeRange) }; + if (itemIndex !== index) { + return item; } - return item; + return { + ...item, + relativeTimeRange: rangeUtil.timeRangeToRelative(timeRange), + }; + }) + ); + } + + onChangeDataSource(settings: DataSourceInstanceSettings, index: number) { + const { queries, onQueriesChange } = this.props; + + onQueriesChange( + queries.map((item, itemIndex) => { + if (itemIndex !== index) { + return item; + } + + const previous = getDataSourceSrv().getInstanceSettings(item.datasourceUid); + + if (previous?.type === settings.uid) { + return { + ...item, + datasourceUid: settings.uid, + }; + } + + const { refId, hide } = item.model; + + return { + ...item, + datasourceUid: settings.uid, + model: { refId, hide }, + }; }) ); } @@ -52,10 +78,17 @@ export class AlertingQueryRows extends PureComponent { const { queries, onQueriesChange } = this.props; onQueriesChange( queries.map((item, itemIndex) => { - if (itemIndex === index) { - return { ...item, model: { ...item.model, ...query, datasource: query.datasource! } }; + if (itemIndex !== index) { + return item; } - return item; + return { + ...item, + model: { + ...item.model, + ...query, + datasource: query.datasource!, + }, + }; }) ); } @@ -79,14 +112,8 @@ export class AlertingQueryRows extends PureComponent { onQueriesChange(update); }; - getDataSourceSettings = (query: DataQuery): DataSourceInstanceSettings | undefined => { - const { defaultDataSource } = this.state; - - if (isExpressionQuery(query)) { - return getDataSourceSrv().getInstanceSettings(defaultDataSource.name); - } - - return getDataSourceSrv().getInstanceSettings(query.datasource); + getDataSourceSettings = (query: GrafanaQuery): DataSourceInstanceSettings | undefined => { + return getDataSourceSrv().getInstanceSettings(query.datasourceUid); }; render() { @@ -108,7 +135,12 @@ export class AlertingQueryRows extends PureComponent { return ( this.onChangeDataSource(settings, index) + : undefined + } id={query.refId} index={index} key={query.refId} diff --git a/public/app/features/alerting/unified/components/rules/RuleDetails.tsx b/public/app/features/alerting/unified/components/rules/RuleDetails.tsx index 3e2aacaece3..0ef93dc0a05 100644 --- a/public/app/features/alerting/unified/components/rules/RuleDetails.tsx +++ b/public/app/features/alerting/unified/components/rules/RuleDetails.tsx @@ -3,14 +3,15 @@ import React, { FC, useMemo } from 'react'; import { useStyles } from '@grafana/ui'; import { css, cx } from '@emotion/css'; import { GrafanaTheme } from '@grafana/data'; -import { isAlertingRule } from '../../utils/rules'; +import { isAlertingRule, isGrafanaRulerRule } from '../../utils/rules'; import { isCloudRulesSource, isGrafanaRulesSource } from '../../utils/datasource'; import { Annotation } from '../Annotation'; import { AlertLabels } from '../AlertLabels'; import { AlertInstancesTable } from './AlertInstancesTable'; import { DetailsField } from '../DetailsField'; import { RuleQuery } from './RuleQuery'; -import { getDataSourceSrv } from '@grafana/runtime'; +import { getDatasourceSrv } from 'app/features/plugins/datasource_srv'; +import { ExpressionDatasourceUID } from 'app/features/expressions/ExpressionDatasource'; interface Props { rule: CombinedRule; @@ -27,17 +28,23 @@ export const RuleDetails: FC = ({ rule, rulesSource }) => { const dataSources: Array<{ name: string; icon?: string }> = useMemo(() => { if (isCloudRulesSource(rulesSource)) { return [{ name: rulesSource.name, icon: rulesSource.meta.info.logos.small }]; - } else if (rule.queries) { - return rule.queries - .map(({ datasource }) => { - const ds = getDataSourceSrv().getInstanceSettings(datasource); - if (ds) { - return { name: ds.name, icon: ds.meta.info.logos.small }; - } - return { name: datasource }; - }) - .filter(({ name }) => name !== '__expr__'); } + + if (isGrafanaRulerRule(rule.rulerRule)) { + const { data } = rule.rulerRule.grafana_alert; + + return data.reduce((dataSources, query) => { + const ds = getDatasourceSrv().getInstanceSettings(query.datasourceUid); + + if (!ds || ds.uid === ExpressionDatasourceUID) { + return dataSources; + } + + dataSources.push({ name: ds.name, icon: ds.meta.info.logos.small }); + return dataSources; + }, [] as Array<{ name: string; icon?: string }>); + } + return []; }, [rule, rulesSource]); diff --git a/public/app/features/alerting/unified/hooks/useCombinedRuleNamespaces.ts b/public/app/features/alerting/unified/hooks/useCombinedRuleNamespaces.ts index 99dab3a66fd..d5819d15032 100644 --- a/public/app/features/alerting/unified/hooks/useCombinedRuleNamespaces.ts +++ b/public/app/features/alerting/unified/hooks/useCombinedRuleNamespaces.ts @@ -120,7 +120,6 @@ function promRuleToCombinedRule(rule: Rule, namespace: CombinedRuleNamespace, gr return { name: rule.name, query: rule.query, - queries: rule.query && isGrafanaRulesSource(namespace.rulesSource) ? JSON.parse(rule.query) : undefined, labels: rule.labels || {}, annotations: isAlertingRule(rule) ? rule.annotations || {} : {}, promRule: rule, @@ -156,7 +155,6 @@ function rulerRuleToCombinedRule( } : { name: rule.grafana_alert.title, - queries: (rule.grafana_alert.data ?? []).map((d) => d.model), query: '', labels: rule.labels || {}, annotations: rule.annotations || {}, diff --git a/public/app/features/alerting/unified/hooks/useFilteredRules.ts b/public/app/features/alerting/unified/hooks/useFilteredRules.ts index 1a9e8c3ffd5..149eb50f0e8 100644 --- a/public/app/features/alerting/unified/hooks/useFilteredRules.ts +++ b/public/app/features/alerting/unified/hooks/useFilteredRules.ts @@ -1,10 +1,12 @@ import { useMemo } from 'react'; import { CombinedRuleGroup, CombinedRuleNamespace, RuleFilterState } from 'app/types/unified-alerting'; -import { isCloudRulesSource, isGrafanaRulesSource } from '../utils/datasource'; -import { isAlertingRule } from '../utils/rules'; +import { isCloudRulesSource } from '../utils/datasource'; +import { isAlertingRule, isGrafanaRulerRule } from '../utils/rules'; import { getFiltersFromUrlParams } from '../utils/misc'; import { useQueryParams } from 'app/core/hooks/useQueryParams'; +import { RulerGrafanaRuleDTO } from 'app/types/unified-alerting-dto'; +import { getDataSourceSrv } from '@grafana/runtime'; export const useFilteredRules = (namespaces: CombinedRuleNamespace[]) => { const [queryParams] = useQueryParams(); @@ -45,11 +47,7 @@ const reduceNamespaces = (filters: RuleFilterState) => { const reduceGroups = (filters: RuleFilterState) => { return (groupAcc: CombinedRuleGroup[], group: CombinedRuleGroup) => { const rules = group.rules.filter((rule) => { - if ( - filters.dataSource && - isGrafanaRulesSource(rule.namespace.rulesSource) && - !rule.queries?.find(({ datasource }) => datasource === filters.dataSource) - ) { + if (filters.dataSource && isGrafanaRulerRule(rule.rulerRule) && !isQueryingDataSource(rule.rulerRule, filters)) { return false; } // Query strings can match alert name, label keys, and label values @@ -84,3 +82,17 @@ const reduceGroups = (filters: RuleFilterState) => { return groupAcc; }; }; + +const isQueryingDataSource = (rulerRule: RulerGrafanaRuleDTO, filter: RuleFilterState): boolean => { + if (!filter.dataSource) { + return true; + } + + return !!rulerRule.grafana_alert.data.find((query) => { + if (!query.datasourceUid) { + return false; + } + const ds = getDataSourceSrv().getInstanceSettings(query.datasourceUid); + return ds?.name === filter.dataSource; + }); +}; diff --git a/public/app/features/alerting/unified/mocks/grafana-queries.ts b/public/app/features/alerting/unified/mocks/grafana-queries.ts index cfe8a400432..61a191f7801 100644 --- a/public/app/features/alerting/unified/mocks/grafana-queries.ts +++ b/public/app/features/alerting/unified/mocks/grafana-queries.ts @@ -6,9 +6,8 @@ export const SAMPLE_QUERIES = [ from: 30, to: 0, }, + datasourceUid: '000000004', model: { - datasource: 'gdev-testdata', - datasourceUid: '000000004', intervalMs: 1000, maxDataPoints: 100, pulseWave: { @@ -30,6 +29,7 @@ export const SAMPLE_QUERIES = [ from: 0, to: 0, }, + datasourceUid: '-100', model: { conditions: [ { @@ -48,8 +48,6 @@ export const SAMPLE_QUERIES = [ }, }, ], - datasource: '__expr__', - datasourceUid: '-100', intervalMs: 1000, maxDataPoints: 100, refId: 'B', diff --git a/public/app/features/alerting/unified/utils/rule-form.ts b/public/app/features/alerting/unified/utils/rule-form.ts index 5f1e2aff8f1..29e4e489fcc 100644 --- a/public/app/features/alerting/unified/utils/rule-form.ts +++ b/public/app/features/alerting/unified/utils/rule-form.ts @@ -86,7 +86,6 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF ...defaultFormValues, name: ga.title, type: RuleFormType.threshold, - dataSourceName: ga.data[0]?.model.datasource, evaluateFor: rule.for, evaluateEvery: group.interval || defaultFormValues.evaluateEvery, noDataState: ga.no_data_state, diff --git a/public/app/features/alerting/unified/utils/rules.ts b/public/app/features/alerting/unified/utils/rules.ts index 9ba40a4605b..45c5e66069f 100644 --- a/public/app/features/alerting/unified/utils/rules.ts +++ b/public/app/features/alerting/unified/utils/rules.ts @@ -37,8 +37,8 @@ export function isRecordingRulerRule(rule: RulerRuleDTO): rule is RulerRecording return 'record' in rule; } -export function isGrafanaRulerRule(rule: RulerRuleDTO): rule is RulerGrafanaRuleDTO { - return 'grafana_alert' in rule; +export function isGrafanaRulerRule(rule?: RulerRuleDTO): rule is RulerGrafanaRuleDTO { + return typeof rule === 'object' && 'grafana_alert' in rule; } export function alertInstanceKey(alert: Alert): string { diff --git a/public/app/features/expressions/ExpressionDatasource.ts b/public/app/features/expressions/ExpressionDatasource.ts index b5091576434..1500e304aca 100644 --- a/public/app/features/expressions/ExpressionDatasource.ts +++ b/public/app/features/expressions/ExpressionDatasource.ts @@ -1,4 +1,4 @@ -import { DataSourceInstanceSettings, DataSourcePluginMeta } from '@grafana/data'; +import { DataSourceInstanceSettings, DataSourcePluginMeta, PluginType } from '@grafana/data'; import { ExpressionQuery, ExpressionQueryType } from './types'; import { ExpressionQueryEditor } from './ExpressionQueryEditor'; import { DataSourceWithBackend } from '@grafana/runtime'; @@ -29,11 +29,37 @@ export class ExpressionDatasourceApi extends DataSourceWithBackend> { // Expression Datasource (not a real datasource) - if (name === expressionDatasource.name) { + if (name === ExpressionDatasourceID || name === ExpressionDatasourceUID) { this.datasources[name] = expressionDatasource as any; return Promise.resolve(expressionDatasource); } diff --git a/public/app/features/query/components/QueryEditorRow.tsx b/public/app/features/query/components/QueryEditorRow.tsx index 45e7d278206..7ad508a2b04 100644 --- a/public/app/features/query/components/QueryEditorRow.tsx +++ b/public/app/features/query/components/QueryEditorRow.tsx @@ -19,7 +19,7 @@ import { TimeRange, toLegacyResponseData, } from '@grafana/data'; -import { QueryEditorRowTitle } from './QueryEditorRowTitle'; +import { QueryEditorRowHeader } from './QueryEditorRowHeader'; import { QueryOperationRow, QueryOperationRowRenderProps, @@ -33,10 +33,11 @@ interface Props { data: PanelData; query: DataQuery; queries: DataQuery[]; - dsSettings: DataSourceInstanceSettings; id: string; index: number; timeRange?: TimeRange; + dataSource: DataSourceInstanceSettings; + onChangeDataSource?: (dsSettings: DataSourceInstanceSettings) => void; onChangeTimeRange?: (timeRange: TimeRange) => void; onAddQuery: (query: DataQuery) => void; onRemoveQuery: (query: DataQuery) => void; @@ -111,7 +112,7 @@ export class QueryEditorRow extends PureComponent { } getQueryDataSourceIdentifier(): string | null | undefined { - const { query, dsSettings } = this.props; + const { query, dataSource: dsSettings } = this.props; return query.datasource ?? dsSettings.name; } @@ -302,20 +303,18 @@ export class QueryEditorRow extends PureComponent { ); }; - renderTitle = (props: QueryOperationRowRenderProps) => { - const { query, dsSettings, onChange, queries, onChangeTimeRange, timeRange } = this.props; - const { datasource } = this.state; - const isDisabled = query.hide; + renderHeader = (props: QueryOperationRowRenderProps) => { + const { query, dataSource, onChangeDataSource, onChange, queries, onChangeTimeRange, timeRange } = this.props; return ( - this.onToggleEditMode(e, props)} onChange={onChange} collapsedText={!props.isOpen ? this.renderCollapsedText() : null} @@ -346,7 +345,7 @@ export class QueryEditorRow extends PureComponent { id={id} draggable={true} index={index} - headerElement={this.renderTitle} + headerElement={this.renderHeader} actions={this.renderActions} onOpen={this.onOpen} > diff --git a/public/app/features/query/components/QueryEditorRowHeader.test.tsx b/public/app/features/query/components/QueryEditorRowHeader.test.tsx new file mode 100644 index 00000000000..6aeea8060a0 --- /dev/null +++ b/public/app/features/query/components/QueryEditorRowHeader.test.tsx @@ -0,0 +1,124 @@ +import React from 'react'; +import { fireEvent, render, screen } from '@testing-library/react'; +import { Props, QueryEditorRowHeader } from './QueryEditorRowHeader'; +import { DataSourceInstanceSettings, dateTime } from '@grafana/data'; +import { selectors } from '@grafana/e2e-selectors'; + +jest.mock('@grafana/runtime/src/services/dataSourceSrv', () => { + return { + getDataSourceSrv: () => ({ + getInstanceSettings: jest.fn(), + getList: jest.fn().mockReturnValue([]), + }), + }; +}); + +describe('QueryEditorRowHeader', () => { + it('Can edit title', () => { + const scenario = renderScenario({}); + screen.getByTestId('query-name-div').click(); + + const input = screen.getByTestId('query-name-input'); + fireEvent.change(input, { target: { value: 'new name' } }); + fireEvent.blur(input); + + expect((scenario.props.onChange as any).mock.calls[0][0].refId).toBe('new name'); + }); + + it('Show error when other query with same name exists', async () => { + renderScenario({}); + + screen.getByTestId('query-name-div').click(); + const input = screen.getByTestId('query-name-input'); + fireEvent.change(input, { target: { value: 'B' } }); + const alert = await screen.findByRole('alert'); + + expect(alert.textContent).toBe('Query name already exists'); + }); + + it('Show error when empty name is specified', async () => { + renderScenario({}); + + screen.getByTestId('query-name-div').click(); + const input = screen.getByTestId('query-name-input'); + fireEvent.change(input, { target: { value: '' } }); + const alert = await screen.findByRole('alert'); + + expect(alert.textContent).toBe('An empty query name is not allowed'); + }); + + it('should show data source picker when callback is passed', async () => { + renderScenario({ onChangeDataSource: () => {} }); + + expect(screen.queryByLabelText(selectors.components.DataSourcePicker.container)).not.toBeNull(); + }); + + it('should not show data source picker when no callback is passed', async () => { + renderScenario({ onChangeDataSource: undefined }); + + expect(screen.queryByLabelText(selectors.components.DataSourcePicker.container)).toBeNull(); + }); + + it('should show time range picker when callback and value is passed', async () => { + renderScenario({ + onChangeTimeRange: () => {}, + timeRange: { + from: dateTime(), + to: dateTime(), + raw: { from: 'now', to: 'now' }, + }, + }); + + expect(screen.queryByLabelText(selectors.components.TimePicker.openButton)).not.toBeNull(); + }); + + it('should not show time range picker when no value is passed', async () => { + renderScenario({ + onChangeTimeRange: () => {}, + timeRange: undefined, + }); + + expect(screen.queryByLabelText(selectors.components.DataSourcePicker.container)).toBeNull(); + }); + + it('should not show time range picker when no callback is passed', async () => { + renderScenario({ + onChangeTimeRange: undefined, + timeRange: { + from: dateTime(), + to: dateTime(), + raw: { from: 'now', to: 'now' }, + }, + }); + + expect(screen.queryByLabelText(selectors.components.DataSourcePicker.container)).toBeNull(); + }); +}); + +function renderScenario(overrides: Partial) { + const props: Props = { + query: { + refId: 'A', + }, + queries: [ + { + refId: 'A', + }, + { + refId: 'B', + }, + ], + dataSource: {} as DataSourceInstanceSettings, + disabled: false, + onChange: jest.fn(), + onClick: jest.fn(), + collapsedText: '', + }; + + Object.assign(props, overrides); + + return { + props, + renderResult: render(), + }; +} diff --git a/public/app/features/query/components/QueryEditorRowTitle.tsx b/public/app/features/query/components/QueryEditorRowHeader.tsx similarity index 78% rename from public/app/features/query/components/QueryEditorRowTitle.tsx rename to public/app/features/query/components/QueryEditorRowHeader.tsx index 62a809cc9bc..23ebf2d6fad 100644 --- a/public/app/features/query/components/QueryEditorRowTitle.tsx +++ b/public/app/features/query/components/QueryEditorRowHeader.tsx @@ -2,37 +2,27 @@ import React, { useState } from 'react'; import { css, cx } from '@emotion/css'; import { DataQuery, DataSourceInstanceSettings, GrafanaTheme, TimeRange } from '@grafana/data'; import { DataSourcePicker } from '@grafana/runtime'; -import { Icon, Input, stylesFactory, useTheme, FieldValidationMessage, TimeRangeInput } from '@grafana/ui'; +import { Icon, Input, FieldValidationMessage, TimeRangeInput, useStyles } from '@grafana/ui'; import { selectors } from '@grafana/e2e-selectors'; -import { ExpressionDatasourceID } from '../../expressions/ExpressionDatasource'; +import { ExpressionDatasourceUID } from '../../expressions/ExpressionDatasource'; export interface Props { query: DataQuery; queries: DataQuery[]; - dataSourceName: string; - inMixedMode?: boolean; disabled?: boolean; timeRange?: TimeRange; - onTimeRangeChange?: (timeRange: TimeRange) => void; + dataSource: DataSourceInstanceSettings; + onChangeDataSource?: (settings: DataSourceInstanceSettings) => void; + onChangeTimeRange?: (timeRange: TimeRange) => void; onChange: (query: DataQuery) => void; onClick: (e: React.MouseEvent) => void; collapsedText: string | null; } -export const QueryEditorRowTitle: React.FC = ({ - dataSourceName, - inMixedMode, - disabled, - query, - queries, - onClick, - onChange, - onTimeRangeChange, - timeRange, - collapsedText, -}) => { - const theme = useTheme(); - const styles = getQueryEditorRowTitleStyles(theme); +export const QueryEditorRowHeader: React.FC = (props) => { + const { dataSource, onChangeDataSource, disabled, query, queries, onClick, onChange, collapsedText } = props; + + const styles = useStyles(getStyles); const [isEditing, setIsEditing] = useState(false); const [validationError, setValidationError] = useState(null); @@ -91,10 +81,6 @@ export const QueryEditorRowTitle: React.FC = ({ event.target.select(); }; - const onDataSourceChange = (dataSource: DataSourceInstanceSettings) => { - onChange({ ...query, datasource: dataSource.name }); - }; - return (
{!isEditing && ( @@ -126,17 +112,8 @@ export const QueryEditorRowTitle: React.FC = ({ {validationError && {validationError}} )} - {inMixedMode && ( -
- {query.datasource !== ExpressionDatasourceID && ( - <> - - {onTimeRangeChange && timeRange && } - - )} -
- )} - {dataSourceName && !inMixedMode && ({dataSourceName})} + + {dataSource && !onChangeDataSource && ({dataSource.name})} {disabled && Disabled} {collapsedText && ( @@ -148,7 +125,27 @@ export const QueryEditorRowTitle: React.FC = ({ ); }; -const getQueryEditorRowTitleStyles = stylesFactory((theme: GrafanaTheme) => { +const PickerRenderer: React.FC = (props) => { + const { onChangeTimeRange, timeRange, onChangeDataSource, dataSource } = props; + const styles = useStyles(getStyles); + + if (!onChangeTimeRange && !onChangeDataSource) { + return null; + } + + if (dataSource.uid === ExpressionDatasourceUID) { + return null; + } + + return ( +
+ {onChangeDataSource && } + {onChangeTimeRange && timeRange && } +
+ ); +}; + +const getStyles = (theme: GrafanaTheme) => { return { wrapper: css` display: flex; @@ -225,5 +222,9 @@ const getQueryEditorRowTitleStyles = stylesFactory((theme: GrafanaTheme) => { color: ${theme.colors.textWeak}; padding-left: 10px; `, + pickerWrapper: css` + display: flex; + margin-left: 8px; + `, }; -}); +}; diff --git a/public/app/features/query/components/QueryEditorRowTitle.test.tsx b/public/app/features/query/components/QueryEditorRowTitle.test.tsx deleted file mode 100644 index e5d61830df8..00000000000 --- a/public/app/features/query/components/QueryEditorRowTitle.test.tsx +++ /dev/null @@ -1,66 +0,0 @@ -import React from 'react'; -import { fireEvent, render, screen } from '@testing-library/react'; -import { Props, QueryEditorRowTitle } from './QueryEditorRowTitle'; - -function renderScenario(overrides: Partial) { - const props: Props = { - query: { - refId: 'A', - }, - queries: [ - { - refId: 'A', - }, - { - refId: 'B', - }, - ], - dataSourceName: 'hello', - disabled: false, - onChange: jest.fn(), - onClick: jest.fn(), - collapsedText: '', - }; - - Object.assign(props, overrides); - - return { - props, - renderResult: render(), - }; -} - -describe('QueryEditorRowTitle', () => { - it('Can edit title', () => { - const scenario = renderScenario({}); - screen.getByTestId('query-name-div').click(); - - const input = screen.getByTestId('query-name-input'); - fireEvent.change(input, { target: { value: 'new name' } }); - fireEvent.blur(input); - - expect((scenario.props.onChange as any).mock.calls[0][0].refId).toBe('new name'); - }); - - it('Show error when other query with same name exists', async () => { - renderScenario({}); - - screen.getByTestId('query-name-div').click(); - const input = screen.getByTestId('query-name-input'); - fireEvent.change(input, { target: { value: 'B' } }); - const alert = await screen.findByRole('alert'); - - expect(alert.textContent).toBe('Query name already exists'); - }); - - it('Show error when empty name is specified', async () => { - renderScenario({}); - - screen.getByTestId('query-name-div').click(); - const input = screen.getByTestId('query-name-input'); - fireEvent.change(input, { target: { value: '' } }); - const alert = await screen.findByRole('alert'); - - expect(alert.textContent).toBe('An empty query name is not allowed'); - }); -}); diff --git a/public/app/features/query/components/QueryEditorRows.tsx b/public/app/features/query/components/QueryEditorRows.tsx index 569a87d5a0b..2ca3ad21f47 100644 --- a/public/app/features/query/components/QueryEditorRows.tsx +++ b/public/app/features/query/components/QueryEditorRows.tsx @@ -5,6 +5,7 @@ import React, { PureComponent } from 'react'; import { DataQuery, DataSourceInstanceSettings, PanelData } from '@grafana/data'; import { QueryEditorRow } from './QueryEditorRow'; import { DragDropContext, Droppable, DropResult } from 'react-beautiful-dnd'; +import { getDataSourceSrv } from '@grafana/runtime'; interface Props { // The query configuration @@ -39,6 +40,35 @@ export class QueryEditorRows extends PureComponent { ); } + onDataSourceChange(dataSource: DataSourceInstanceSettings, index: number) { + const { queries, onQueriesChange } = this.props; + + onQueriesChange( + queries.map((item, itemIndex) => { + if (itemIndex !== index) { + return item; + } + + if (item.datasource) { + const previous = getDataSourceSrv().getInstanceSettings(item.datasource); + + if (previous?.type === dataSource.type) { + return { + ...item, + datasource: dataSource.name, + }; + } + } + + return { + refId: item.refId, + hide: item.hide, + datasource: dataSource.name, + }; + }) + ); + } + onDragEnd = (result: DropResult) => { const { queries, onQueriesChange } = this.props; @@ -67,21 +97,29 @@ export class QueryEditorRows extends PureComponent { {(provided) => { return (
- {queries.map((query, index) => ( - this.onChangeQuery(query, index)} - onRemoveQuery={this.onRemoveQuery} - onAddQuery={this.props.onAddQuery} - onRunQuery={this.props.onRunQueries} - queries={queries} - /> - ))} + {queries.map((query, index) => { + const dataSourceSettings = getDataSourceSettings(query, dsSettings); + const onChangeDataSourceSettings = dsSettings.meta.mixed + ? (settings: DataSourceInstanceSettings) => this.onDataSourceChange(settings, index) + : undefined; + + return ( + this.onChangeQuery(query, index)} + onRemoveQuery={this.onRemoveQuery} + onAddQuery={this.props.onAddQuery} + onRunQuery={this.props.onRunQueries} + queries={queries} + /> + ); + })} {provided.placeholder}
); @@ -91,3 +129,14 @@ export class QueryEditorRows extends PureComponent { ); } } + +const getDataSourceSettings = ( + query: DataQuery, + groupSettings: DataSourceInstanceSettings +): DataSourceInstanceSettings => { + if (!query.datasource) { + return groupSettings; + } + const querySettings = getDataSourceSrv().getInstanceSettings(query.datasource); + return querySettings || groupSettings; +}; diff --git a/public/app/features/query/components/QueryGroup.tsx b/public/app/features/query/components/QueryGroup.tsx index a9cd247f738..8d3b7fb80c7 100644 --- a/public/app/features/query/components/QueryGroup.tsx +++ b/public/app/features/query/components/QueryGroup.tsx @@ -19,7 +19,10 @@ import { import { PluginHelp } from 'app/core/components/PluginHelp/PluginHelp'; import { addQuery } from 'app/core/utils/query'; import { Unsubscribable } from 'rxjs'; -import { expressionDatasource, ExpressionDatasourceID } from 'app/features/expressions/ExpressionDatasource'; +import { + dataSource as expressionDatasource, + ExpressionDatasourceID, +} from 'app/features/expressions/ExpressionDatasource'; import { selectors } from '@grafana/e2e-selectors'; import { PanelQueryRunner } from '../state/PanelQueryRunner'; import { QueryGroupOptionsEditor } from './QueryGroupOptions'; diff --git a/public/app/features/query/state/runRequest.ts b/public/app/features/query/state/runRequest.ts index aa6f87a3902..e451a052224 100644 --- a/public/app/features/query/state/runRequest.ts +++ b/public/app/features/query/state/runRequest.ts @@ -22,7 +22,11 @@ import { } from '@grafana/data'; import { toDataQueryError } from '@grafana/runtime'; import { emitDataRequestEvent } from './queryAnalytics'; -import { expressionDatasource, ExpressionDatasourceID } from 'app/features/expressions/ExpressionDatasource'; +import { + dataSource as expressionDatasource, + ExpressionDatasourceID, + ExpressionDatasourceUID, +} from 'app/features/expressions/ExpressionDatasource'; import { ExpressionQuery } from 'app/features/expressions/types'; type MapOfResponsePackets = { [str: string]: DataQueryResponse }; @@ -175,7 +179,7 @@ export function callQueryMethod( ) { // If any query has an expression, use the expression endpoint for (const target of request.targets) { - if (target.datasource === ExpressionDatasourceID) { + if (target.datasource === ExpressionDatasourceID || target.datasource === ExpressionDatasourceUID) { return expressionDatasource.query(request as DataQueryRequest); } } diff --git a/public/app/types/unified-alerting-dto.ts b/public/app/types/unified-alerting-dto.ts index 89fdff90bae..55692e191a5 100644 --- a/public/app/types/unified-alerting-dto.ts +++ b/public/app/types/unified-alerting-dto.ts @@ -92,16 +92,12 @@ export enum GrafanaAlertState { KeepLastState = 'KeepLastState', OK = 'OK', } - -export interface GrafanaQueryModel extends DataQuery { - datasource: string; - datasourceUid: string; -} export interface GrafanaQuery { refId: string; queryType: string; relativeTimeRange: RelativeTimeRange; - model: GrafanaQueryModel; + datasourceUid: string; + model: DataQuery; } export interface PostableGrafanaRuleDefinition { diff --git a/public/app/types/unified-alerting.ts b/public/app/types/unified-alerting.ts index 5b13e136948..0033bcad14e 100644 --- a/public/app/types/unified-alerting.ts +++ b/public/app/types/unified-alerting.ts @@ -8,7 +8,6 @@ import { Labels, Annotations, RulerRuleGroupDTO, - GrafanaQueryModel, } from './unified-alerting-dto'; export type Alert = { @@ -82,7 +81,6 @@ export interface CombinedRule { rulerRule?: RulerRuleDTO; group: CombinedRuleGroup; namespace: CombinedRuleNamespace; - queries?: GrafanaQueryModel[]; } export interface CombinedRuleGroup {