From 9ace8686a1abffe21d2b3b115db795900b67590c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Jamr=C3=B3z?= Date: Thu, 8 Jul 2021 14:32:27 +0200 Subject: [PATCH] Datasources: Improve error handling for testing data sources (#35120) * Improve error handling for error messages The error message will be read from error object from the following properties in the following order: - message - data.message - statusText * Convert api/ds/query errors to TestingStatus SQL datasources (mysql, mssql, postgres) and CloudWatch use api/ds/query to test the data source, but previously didn't handle errors returned by this endpoint. If the error cannot be handled it's re-thrown to be handled in public/app/features/datasources/state/actions.ts * Use async/await instead of Promises * Remove incorrect type import TestingStatus is in app/types. Should be pulled down to grafana/data but it depends on HealthCheckResultDetails that is public and lives in grafana/runtime. Ideally TestingStatus should live in grafana/data but I'm not sure if HealthCheckResultDetails can be move there too (?) * Update packages/grafana-data/src/types/datasource.ts Co-authored-by: Erik Sundell * Handle errors with no details in toTestingStatus instead of re-throwing * Update packages/grafana-data/src/types/datasource.ts Co-authored-by: Marcus Efraimsson Co-authored-by: Erik Sundell Co-authored-by: Marcus Efraimsson --- packages/grafana-data/src/types/datasource.ts | 12 +++- .../src/utils/queryResponse.test.ts | 44 ++++++++++++++- .../src/utils/queryResponse.ts | 34 ++++++++++- public/app/core/services/backend_srv.ts | 9 ++- .../datasources/state/actions.test.ts | 56 +++++++++++++++++++ .../app/features/datasources/state/actions.ts | 5 +- .../datasource/cloudwatch/datasource.ts | 16 ++++-- .../plugins/datasource/mssql/datasource.ts | 8 +-- .../plugins/datasource/mysql/datasource.ts | 10 +--- .../plugins/datasource/postgres/datasource.ts | 8 +-- 10 files changed, 170 insertions(+), 32 deletions(-) diff --git a/packages/grafana-data/src/types/datasource.ts b/packages/grafana-data/src/types/datasource.ts index 63159188674..8d8b099234f 100644 --- a/packages/grafana-data/src/types/datasource.ts +++ b/packages/grafana-data/src/types/datasource.ts @@ -228,7 +228,11 @@ abstract class DataSourceApi< abstract query(request: DataQueryRequest): Promise | Observable; /** - * Test & verify datasource settings & connection details + * Test & verify datasource settings & connection details (returning TestingStatus) + * + * When verification fails - errors specific to the data source should be handled here and converted to + * a TestingStatus object. Unknown errors and HTTP errors can be re-thrown and will be handled here: + * public/app/features/datasources/state/actions.ts */ abstract testDatasource(): Promise; @@ -472,7 +476,13 @@ export enum DataQueryErrorType { export interface DataQueryError { data?: { + /** + * Short information about the error + */ message?: string; + /** + * Detailed information about the error. Only returned when app_mode is development. + */ error?: string; }; message?: string; diff --git a/packages/grafana-runtime/src/utils/queryResponse.test.ts b/packages/grafana-runtime/src/utils/queryResponse.test.ts index f04647fccb4..eb8bb907abc 100644 --- a/packages/grafana-runtime/src/utils/queryResponse.test.ts +++ b/packages/grafana-runtime/src/utils/queryResponse.test.ts @@ -1,6 +1,6 @@ import { DataQuery, toDataFrameDTO, DataFrame } from '@grafana/data'; -import { FetchResponse } from 'src/services'; -import { BackendDataSourceResponse, toDataQueryResponse } from './queryResponse'; +import { FetchError, FetchResponse } from 'src/services'; +import { BackendDataSourceResponse, toDataQueryResponse, toTestingStatus } from './queryResponse'; const resp = ({ data: { @@ -333,4 +333,44 @@ describe('Query Response parser', () => { ] `); }); + + describe('should convert to TestingStatus', () => { + test('from api/ds/query generic errors', () => { + const result = toTestingStatus({ status: 500, data: { message: 'message', error: 'error' } } as FetchError); + expect(result).toMatchObject({ + status: 'error', + message: 'message', + details: { message: 'error' }, + }); + }); + test('from api/ds/query result errors', () => { + const result = toTestingStatus({ + status: 400, + data: { + results: { + A: { + error: 'error', + }, + }, + }, + } as FetchError); + expect(result).toMatchObject({ + status: 'error', + message: 'error', + }); + }); + test('unknown errors', () => { + expect(() => { + toTestingStatus({ status: 503, data: 'Fatal Error' } as FetchError); + }).toThrow(); + + expect(() => { + toTestingStatus({ status: 503, data: {} } as FetchError); + }).toThrow(); + + expect(() => { + toTestingStatus({ status: 503 } as FetchError); + }).toThrow(); + }); + }); }); diff --git a/packages/grafana-runtime/src/utils/queryResponse.ts b/packages/grafana-runtime/src/utils/queryResponse.ts index 499de5df7e1..1acf15de5a8 100644 --- a/packages/grafana-runtime/src/utils/queryResponse.ts +++ b/packages/grafana-runtime/src/utils/queryResponse.ts @@ -13,7 +13,7 @@ import { DataFrameJSON, dataFrameFromJSON, } from '@grafana/data'; -import { FetchResponse } from '../services'; +import { FetchError, FetchResponse } from '../services'; /** * Single response object from a backend data source. Properties are optional but response should contain at least @@ -127,6 +127,38 @@ export function toDataQueryResponse( return rsp; } +/** + * Data sources using api/ds/query to test data sources can use this function to + * handle errors and convert them to TestingStatus object. + * + * If possible, this should be avoided in favor of implementing /health endpoint + * and testing data source with DataSourceWithBackend.testDataSource() + * + * Re-thrown errors are handled by testDataSource() in public/app/features/datasources/state/actions.ts + * + * @returns {TestingStatus} + */ +export function toTestingStatus(err: FetchError): any { + const queryResponse = toDataQueryResponse(err); + // POST api/ds/query errors returned as { message: string, error: string } objects + if (queryResponse.error?.data?.message) { + return { + status: 'error', + message: queryResponse.error.data.message, + details: queryResponse.error?.data?.error ? { message: queryResponse.error.data.error } : undefined, + }; + } + // POST api/ds/query errors returned in results object + else if (queryResponse.error?.refId && queryResponse.error?.message) { + return { + status: 'error', + message: queryResponse.error.message, + }; + } + + throw err; +} + /** * Convert an object into a DataQueryError -- if this is an HTTP response, * it will put the correct values in the error field diff --git a/public/app/core/services/backend_srv.ts b/public/app/core/services/backend_srv.ts index f57ee1b3d2a..5e0fdd36603 100644 --- a/public/app/core/services/backend_srv.ts +++ b/public/app/core/services/backend_srv.ts @@ -299,14 +299,19 @@ export class BackendSrv implements BackendService { ]); } - processRequestError(options: BackendSrvRequest, err: FetchError): FetchError { + /** + * Processes FetchError to ensure "data" property is an object. + * + * @see DataQueryError.data + */ + processRequestError(options: BackendSrvRequest, err: FetchError): FetchError<{ message: string; error?: string }> { err.data = err.data ?? { message: 'Unexpected error' }; if (typeof err.data === 'string') { err.data = { + message: err.data, error: err.statusText, response: err.data, - message: err.data, }; } diff --git a/public/app/features/datasources/state/actions.test.ts b/public/app/features/datasources/state/actions.test.ts index 6ed49eed537..6a007639234 100644 --- a/public/app/features/datasources/state/actions.test.ts +++ b/public/app/features/datasources/state/actions.test.ts @@ -29,6 +29,31 @@ const getBackendSrvMock = () => withNoBackendCache: jest.fn().mockImplementationOnce((cb) => cb()), } as any); +const failDataSourceTest = async (error: object) => { + const dependencies: TestDataSourceDependencies = { + getDatasourceSrv: () => + ({ + get: jest.fn().mockReturnValue({ + testDatasource: jest.fn().mockImplementation(() => { + throw error; + }), + }), + } as any), + getBackendSrv: getBackendSrvMock, + }; + const state = { + testingStatus: { + message: '', + status: '', + }, + }; + const dispatchedActions = await thunkTester(state) + .givenThunk(testDataSource) + .whenThunkIsDispatched('Azure Monitor', dependencies); + + return dispatchedActions; +}; + describe('Name exists', () => { const plugins = getMockPlugins(5); @@ -194,5 +219,36 @@ describe('testDataSource', () => { expect(dispatchedActions).toEqual([testDataSourceStarting(), testDataSourceFailed(result)]); }); + + it('then testDataSourceFailed should be dispatched with response error message', async () => { + const result = { + message: 'Error testing datasource', + }; + const dispatchedActions = await failDataSourceTest({ + message: 'Error testing datasource', + data: { message: 'Response error message' }, + statusText: 'Bad Request', + }); + expect(dispatchedActions).toEqual([testDataSourceStarting(), testDataSourceFailed(result)]); + }); + + it('then testDataSourceFailed should be dispatched with response data message', async () => { + const result = { + message: 'Response error message', + }; + const dispatchedActions = await failDataSourceTest({ + data: { message: 'Response error message' }, + statusText: 'Bad Request', + }); + expect(dispatchedActions).toEqual([testDataSourceStarting(), testDataSourceFailed(result)]); + }); + + it('then testDataSourceFailed should be dispatched with response statusText', async () => { + const result = { + message: 'HTTP error Bad Request', + }; + const dispatchedActions = await failDataSourceTest({ data: {}, statusText: 'Bad Request' }); + expect(dispatchedActions).toEqual([testDataSourceStarting(), testDataSourceFailed(result)]); + }); }); }); diff --git a/public/app/features/datasources/state/actions.ts b/public/app/features/datasources/state/actions.ts index 65ff5767e27..79228d37cd7 100644 --- a/public/app/features/datasources/state/actions.ts +++ b/public/app/features/datasources/state/actions.ts @@ -99,8 +99,9 @@ export const testDataSource = ( dispatch(testDataSourceSucceeded(result)); } catch (err) { - const { statusText, message: errMessage, details } = err; - const message = statusText ? 'HTTP error ' + statusText : errMessage; + const { statusText, message: errMessage, details, data } = err; + + const message = errMessage || data?.message || 'HTTP error ' + statusText; dispatch(testDataSourceFailed({ message, details })); } diff --git a/public/app/plugins/datasource/cloudwatch/datasource.ts b/public/app/plugins/datasource/cloudwatch/datasource.ts index 68a226df349..a6f7d69aa37 100644 --- a/public/app/plugins/datasource/cloudwatch/datasource.ts +++ b/public/app/plugins/datasource/cloudwatch/datasource.ts @@ -64,6 +64,7 @@ import { CloudWatchLanguageProvider } from './language_provider'; import { VariableWithMultiSupport } from 'app/features/variables/types'; import { AwsUrl, encodeUrl } from './aws_url'; import { increasingInterval } from './utils/rxjs/increasingInterval'; +import { toTestingStatus } from '@grafana/runtime/src/utils/queryResponse'; import config from 'app/core/config'; const DS_QUERY_ENDPOINT = '/api/ds/query'; @@ -878,17 +879,22 @@ export class CloudWatchDatasource extends DataSourceWithBackend ({ - status: 'success', - message: 'Data source is working', - })); + try { + await this.getDimensionValues(region, namespace, metricName, 'ServiceName', dimensions); + return { + status: 'success', + message: 'Data source is working', + }; + } catch (error) { + return toTestingStatus(error); + } } awsRequest(url: string, data: MetricRequest): Observable { diff --git a/public/app/plugins/datasource/mssql/datasource.ts b/public/app/plugins/datasource/mssql/datasource.ts index 3e08278a104..8224d496db9 100644 --- a/public/app/plugins/datasource/mssql/datasource.ts +++ b/public/app/plugins/datasource/mssql/datasource.ts @@ -8,6 +8,7 @@ import ResponseParser from './response_parser'; import { getTemplateSrv, TemplateSrv } from 'app/features/templating/template_srv'; import { MssqlQueryForInterpolation, MssqlQuery, MssqlOptions } from './types'; import { getTimeSrv, TimeSrv } from 'app/features/dashboard/services/TimeSrv'; +import { toTestingStatus } from '@grafana/runtime/src/utils/queryResponse'; export class MssqlDatasource extends DataSourceWithBackend { id: any; @@ -172,12 +173,7 @@ export class MssqlDatasource extends DataSourceWithBackend { - console.error(err); - if (err.data && err.data.message) { - return of({ status: 'error', message: err.data.message }); - } - - return of({ status: 'error', message: err.status }); + return of(toTestingStatus(err)); }) ) .toPromise(); diff --git a/public/app/plugins/datasource/mysql/datasource.ts b/public/app/plugins/datasource/mysql/datasource.ts index 4a0349744c9..b93860550ef 100644 --- a/public/app/plugins/datasource/mysql/datasource.ts +++ b/public/app/plugins/datasource/mysql/datasource.ts @@ -1,5 +1,4 @@ import { map as _map } from 'lodash'; -import { of } from 'rxjs'; import { catchError, map, mapTo } from 'rxjs/operators'; import { getBackendSrv, DataSourceWithBackend, FetchResponse, BackendDataSourceResponse } from '@grafana/runtime'; import { DataSourceInstanceSettings, ScopedVars, MetricFindValue, AnnotationEvent } from '@grafana/data'; @@ -9,6 +8,8 @@ import { MysqlQueryForInterpolation, MySQLOptions, MySQLQuery } from './types'; import { getTemplateSrv, TemplateSrv } from 'app/features/templating/template_srv'; import { getSearchFilterScopedVar } from '../../../features/variables/utils'; import { getTimeSrv, TimeSrv } from 'app/features/dashboard/services/TimeSrv'; +import { of } from 'rxjs'; +import { toTestingStatus } from '@grafana/runtime/src/utils/queryResponse'; export class MysqlDatasource extends DataSourceWithBackend { id: any; @@ -181,12 +182,7 @@ export class MysqlDatasource extends DataSourceWithBackend { - console.error(err); - if (err.data && err.data.message) { - return of({ status: 'error', message: err.data.message }); - } else { - return of({ status: 'error', message: err.status }); - } + return of(toTestingStatus(err)); }) ) .toPromise(); diff --git a/public/app/plugins/datasource/postgres/datasource.ts b/public/app/plugins/datasource/postgres/datasource.ts index 3c551023e63..34cc2d5f7a8 100644 --- a/public/app/plugins/datasource/postgres/datasource.ts +++ b/public/app/plugins/datasource/postgres/datasource.ts @@ -10,6 +10,7 @@ import { getTimeSrv, TimeSrv } from 'app/features/dashboard/services/TimeSrv'; //Types import { PostgresOptions, PostgresQuery, PostgresQueryForInterpolation } from './types'; import { getSearchFilterScopedVar } from '../../../features/variables/utils'; +import { toTestingStatus } from '@grafana/runtime/src/utils/queryResponse'; export class PostgresDatasource extends DataSourceWithBackend { id: any; @@ -174,12 +175,7 @@ export class PostgresDatasource extends DataSourceWithBackend { - console.error(err); - if (err.data && err.data.message) { - return { status: 'error', message: err.data.message }; - } else { - return { status: 'error', message: err.status }; - } + return toTestingStatus(err); }); }