From 9400ccf47820be2ad3e481ea1c5b9fc0dc7db3f4 Mon Sep 17 00:00:00 2001 From: Laura Benz <48948963+L-M-K-B@users.noreply.github.com> Date: Thu, 12 Jan 2023 11:43:40 +0100 Subject: [PATCH] Glue: Validate target query in correlations page (#57245) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: add draft version of validate button * feat: add some styling and basics * temp: intermediate result * refactor: solve TODOs * refactor: replace string in state * refactor: replace error message style * refactor: set validate state on change in ds * refactor: add QueryRunner * refactor: add QueryRunner * temp: temporary status * Emit PanelData to check if the query is valid * refactor: clean up * refactor: improve a11y of error message and adjust test * Remove deprecated property call, change equality * refactor: add changes from code review * refactor: remove memory leak * refactor: replace query runner * refactor: adjust error handling * refactor: move testing to related unit test * refactor: clean up test for QueryEditorField * refactor: clean up test for CorrelationsPage * refactor: repair test * refactor: clean up * refactor: add refId in order avoid errors when running Loki queries * refactor: replace buildQueryTransaction + set query to invalid if query is empty * refactor: add empty query value to test cases * refactor: end handleValidation after setIsValidQuery() * refactor: refactor test * refactor: fix last two tests * refactor: modify validation * refactor: add happy path * refactor: clean up * refactor: clean up tests (not final) * refactor: further clean up * refactor: add condition for failing * refactor: finish clean up * refactor: changes from code review * refactor: add response state to condition Co-authored-by: Piotr Jamróz * refactor: fix prettier issue * refactor: remove unused return * refactor: replace change in queryAnalytics.ts * refactor: remove correlations from query analytics * refactor: remove unnecessary test preparation * refactor: revert changes from commit 4997327 Co-authored-by: Piotr Jamróz Co-authored-by: Kristina Durivage --- packages/grafana-data/src/types/app.ts | 1 + .../correlations/CorrelationsPage.test.tsx | 4 +- .../Forms/QueryEditorField.test.tsx | 110 +++++++++++++++++- .../correlations/Forms/QueryEditorField.tsx | 106 ++++++++++++++++- .../query/state/queryAnalytics.test.ts | 15 +-- .../features/query/state/queryAnalytics.ts | 6 +- public/test/mocks/datasource_srv.ts | 2 +- 7 files changed, 222 insertions(+), 22 deletions(-) diff --git a/packages/grafana-data/src/types/app.ts b/packages/grafana-data/src/types/app.ts index 47d75b91d98..40da580ae5b 100644 --- a/packages/grafana-data/src/types/app.ts +++ b/packages/grafana-data/src/types/app.ts @@ -16,6 +16,7 @@ export enum CoreApp { Unknown = 'unknown', PanelEditor = 'panel-editor', PanelViewer = 'panel-viewer', + Correlations = 'correlations', } export interface AppRootProps { diff --git a/public/app/features/correlations/CorrelationsPage.test.tsx b/public/app/features/correlations/CorrelationsPage.test.tsx index d891c39bee5..8ad54c9bfae 100644 --- a/public/app/features/correlations/CorrelationsPage.test.tsx +++ b/public/app/features/correlations/CorrelationsPage.test.tsx @@ -304,7 +304,7 @@ describe('CorrelationsPage', () => { expect(screen.getByRole('button', { name: /add$/i })).toBeInTheDocument(); }); - it('correctly adds correlations', async () => { + it('correctly adds first correlation', async () => { const CTAButton = screen.getByRole('button', { name: /add correlation/i }); expect(CTAButton).toBeInTheDocument(); @@ -440,7 +440,7 @@ describe('CorrelationsPage', () => { }); }); - it('correctly adds correlations', async () => { + it('correctly adds new correlation', async () => { const addNewButton = screen.getByRole('button', { name: /add new/i }); expect(addNewButton).toBeInTheDocument(); fireEvent.click(addNewButton); diff --git a/public/app/features/correlations/Forms/QueryEditorField.test.tsx b/public/app/features/correlations/Forms/QueryEditorField.test.tsx index fdae41700ec..e2f8ec081c5 100644 --- a/public/app/features/correlations/Forms/QueryEditorField.test.tsx +++ b/public/app/features/correlations/Forms/QueryEditorField.test.tsx @@ -1,15 +1,16 @@ -import { render, screen } from '@testing-library/react'; +import { fireEvent, render, screen, waitFor, waitForElementToBeRemoved } from '@testing-library/react'; import React, { ReactNode } from 'react'; import { FormProvider, useForm } from 'react-hook-form'; import { MockDataSourceApi } from 'test/mocks/datasource_srv'; +import { LoadingState } from '@grafana/data'; import { setDataSourceSrv } from '@grafana/runtime'; import { MockDataSourceSrv } from 'app/features/alerting/unified/mocks'; import { QueryEditorField } from './QueryEditorField'; const Wrapper = ({ children }: { children: ReactNode }) => { - const methods = useForm(); + const methods = useForm({ defaultValues: { query: {} } }); return {children}; }; @@ -21,7 +22,7 @@ const defaultGetHandler = async (name: string) => { return dsApi; }; -const renderWithContext = async ( +const renderWithContext = ( children: ReactNode, getHandler: (name: string) => Promise = defaultGetHandler ) => { @@ -33,7 +34,24 @@ const renderWithContext = async ( render({children}); }; +const initiateDsApi = () => { + const dsApi = new MockDataSourceApi('dsApiMock'); + dsApi.components = { + QueryEditor: () => <>query editor, + }; + + renderWithContext(, async () => { + return dsApi; + }); + + return dsApi; +}; + describe('QueryEditorField', () => { + afterAll(() => { + jest.restoreAllMocks(); + }); + it('should render the query editor', async () => { renderWithContext(); @@ -63,4 +81,90 @@ describe('QueryEditorField', () => { await screen.findByRole('alert', { name: 'Data source does not export a query editor.' }) ).toBeInTheDocument(); }); + + describe('Query validation', () => { + it('should result in succeeded validation if LoadingState.Done and data is available', async () => { + const dsApi = initiateDsApi(); + + await waitForElementToBeRemoved(() => screen.queryByText(/loading query editor/i)); + + dsApi.result = { + data: [ + { + name: 'test', + fields: [], + length: 1, + }, + ], + state: LoadingState.Done, + }; + + fireEvent.click(screen.getByRole('button', { name: /Validate query$/i })); + + await waitFor(() => { + expect(screen.getByText('This query is valid.')).toBeInTheDocument(); + }); + }); + + it('should result in failed validation if LoadingState.Error and data is not available', async () => { + const dsApi = initiateDsApi(); + + await waitForElementToBeRemoved(() => screen.queryByText(/loading query editor/i)); + + dsApi.result = { + data: [], + state: LoadingState.Error, + }; + + fireEvent.click(screen.getByRole('button', { name: /Validate query$/i })); + + await waitFor(() => { + const alertEl = screen.getByRole('alert'); + expect(alertEl).toBeInTheDocument(); + expect(alertEl).toHaveTextContent(/this query is not valid/i); + }); + }); + + it('should result in failed validation if LoadingState.Error and data is available', async () => { + const dsApi = initiateDsApi(); + + await waitForElementToBeRemoved(() => screen.queryByText(/loading query editor/i)); + + dsApi.result = { + data: [ + { + name: 'test', + fields: [], + length: 1, + }, + ], + state: LoadingState.Error, + }; + + fireEvent.click(screen.getByRole('button', { name: /Validate query$/i })); + + await waitFor(() => { + const alertEl = screen.getByRole('alert'); + expect(alertEl).toBeInTheDocument(); + expect(alertEl).toHaveTextContent(/this query is not valid/i); + }); + }); + + it('should result in failed validation if result with LoadingState.Done and data is not available', async () => { + const dsApi = initiateDsApi(); + + await waitForElementToBeRemoved(() => screen.queryByText(/loading query editor/i)); + + dsApi.result = { + data: [], + state: LoadingState.Done, + }; + + fireEvent.click(screen.getByRole('button', { name: /Validate query$/i })); + + await waitFor(() => { + expect(screen.getByText('This query is not valid.')).toBeInTheDocument(); + }); + }); + }); }); diff --git a/public/app/features/correlations/Forms/QueryEditorField.tsx b/public/app/features/correlations/Forms/QueryEditorField.tsx index ffcc21f7c77..c1e039f7b9c 100644 --- a/public/app/features/correlations/Forms/QueryEditorField.tsx +++ b/public/app/features/correlations/Forms/QueryEditorField.tsx @@ -1,9 +1,24 @@ -import React from 'react'; +import { css } from '@emotion/css'; +import React, { useState } from 'react'; import { Controller } from 'react-hook-form'; import { useAsync } from 'react-use'; +import { CoreApp, DataQuery, getDefaultTimeRange, GrafanaTheme2 } from '@grafana/data'; import { getDataSourceSrv } from '@grafana/runtime'; -import { Field, LoadingPlaceholder, Alert } from '@grafana/ui'; +import { + Field, + LoadingPlaceholder, + Alert, + Button, + HorizontalGroup, + Icon, + FieldValidationMessage, + useStyles2, +} from '@grafana/ui'; + +import { generateKey } from '../../../core/utils/explore'; +import { QueryTransaction } from '../../../types'; +import { runRequest } from '../../query/state/runRequest'; interface Props { dsUid?: string; @@ -12,7 +27,19 @@ interface Props { error?: string; } +function getStyle(theme: GrafanaTheme2) { + return { + valid: css` + color: ${theme.colors.success.text}; + `, + }; +} + export const QueryEditorField = ({ dsUid, invalid, error, name }: Props) => { + const [isValidQuery, setIsValidQuery] = useState(undefined); + + const style = useStyles2(getStyle); + const { value: datasource, loading: dsLoading, @@ -23,8 +50,56 @@ export const QueryEditorField = ({ dsUid, invalid, error, name }: Props) => { } return getDataSourceSrv().get(dsUid); }, [dsUid]); + const QueryEditor = datasource?.components?.QueryEditor; + const handleValidation = (value: DataQuery) => { + const interval = '1s'; + const intervalMs = 1000; + const id = generateKey(); + const queries = [{ ...value, refId: 'A' }]; + + const transaction: QueryTransaction = { + queries, + request: { + app: CoreApp.Correlations, + timezone: 'utc', + startTime: Date.now(), + interval, + intervalMs, + targets: queries, + range: getDefaultTimeRange(), + requestId: 'correlations_' + id, + scopedVars: { + __interval: { text: interval, value: interval }, + __interval_ms: { text: intervalMs, value: intervalMs }, + }, + }, + id, + done: false, + }; + + if (datasource) { + runRequest(datasource, transaction.request).subscribe((panelData) => { + if ( + !panelData || + panelData.state === 'Error' || + (panelData.state === 'Done' && panelData.series.length === 0) + ) { + setIsValidQuery(false); + } else if ( + panelData.state === 'Done' && + panelData.series.length > 0 && + Boolean(panelData.series.find((element) => element.length > 0)) + ) { + setIsValidQuery(true); + } else { + setIsValidQuery(undefined); + } + }); + } + }; + return ( { if (!QueryEditor) { return ; } - - return {}} onChange={onChange} datasource={datasource} query={value} />; + return ( + <> + handleValidation(value)} + onChange={(value) => { + setIsValidQuery(undefined); + onChange(value); + }} + datasource={datasource} + query={value} + /> + + {isValidQuery ? ( +
+ This query is valid. +
+ ) : isValidQuery === false ? ( + This query is not valid. + ) : null} + + + + ); }} /> diff --git a/public/app/features/query/state/queryAnalytics.test.ts b/public/app/features/query/state/queryAnalytics.test.ts index 839df12bf4c..7399a41c4e0 100644 --- a/public/app/features/query/state/queryAnalytics.test.ts +++ b/public/app/features/query/state/queryAnalytics.test.ts @@ -138,11 +138,10 @@ describe('emitDataRequestEvent - from a dashboard panel', () => { datasourceName: datasource.name, datasourceId: datasource.id, datasourceUid: datasource.uid, + datasourceType: datasource.type, + source: 'dashboard', panelId: 2, dashboardId: 1, - dashboardName: 'Test Dashboard', - dashboardUid: 'test', - folderName: 'Test Folder', dataSize: 0, duration: 1, totalQueries: 0, @@ -162,11 +161,10 @@ describe('emitDataRequestEvent - from a dashboard panel', () => { datasourceName: datasource.name, datasourceId: datasource.id, datasourceUid: datasource.uid, + datasourceType: datasource.type, + source: 'dashboard', panelId: 2, dashboardId: 1, - dashboardName: 'Test Dashboard', - dashboardUid: 'test', - folderName: 'Test Folder', dataSize: 2, duration: 1, totalQueries: 2, @@ -186,11 +184,10 @@ describe('emitDataRequestEvent - from a dashboard panel', () => { datasourceName: datasource.name, datasourceId: datasource.id, datasourceUid: datasource.uid, + datasourceType: datasource.type, + source: 'dashboard', panelId: 2, dashboardId: 1, - dashboardName: 'Test Dashboard', - dashboardUid: 'test', - folderName: 'Test Folder', dataSize: 2, duration: 1, totalQueries: 1, diff --git a/public/app/features/query/state/queryAnalytics.ts b/public/app/features/query/state/queryAnalytics.ts index 23d5ded86a3..102cb954567 100644 --- a/public/app/features/query/state/queryAnalytics.ts +++ b/public/app/features/query/state/queryAnalytics.ts @@ -31,8 +31,8 @@ export function emitDataRequestEvent(datasource: DataSourceApi) { duration: data.request.endTime! - data.request.startTime, }; - if (data.request.app === CoreApp.Explore) { - enrichWithExploreInfo(eventData, data); + if (data.request.app === CoreApp.Explore || data.request.app === CoreApp.Correlations) { + enrichWithInfo(eventData, data); } else { enrichWithDashboardInfo(eventData, data); } @@ -49,7 +49,7 @@ export function emitDataRequestEvent(datasource: DataSourceApi) { done = true; }; - function enrichWithExploreInfo(eventData: DataRequestEventPayload, data: PanelData) { + function enrichWithInfo(eventData: DataRequestEventPayload, data: PanelData) { const totalQueries = Object.keys(data.series).length; eventData.totalQueries = totalQueries; } diff --git a/public/test/mocks/datasource_srv.ts b/public/test/mocks/datasource_srv.ts index e6caeda6d0c..d7c9844158f 100644 --- a/public/test/mocks/datasource_srv.ts +++ b/public/test/mocks/datasource_srv.ts @@ -31,7 +31,7 @@ export class DatasourceSrvMock { export class MockDataSourceApi extends DataSourceApi { result: DataQueryResponse = { data: [] }; - constructor(name?: string, result?: DataQueryResponse, meta?: any, private error: string | null = null) { + constructor(name?: string, result?: DataQueryResponse, meta?: any, public error: string | null = null) { super({ name: name ? name : 'MockDataSourceApi' } as DataSourceInstanceSettings); if (result) { this.result = result;