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 <erik.sundell@grafana.com>

* Handle errors with no details in toTestingStatus instead of re-throwing

* Update packages/grafana-data/src/types/datasource.ts

Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>

Co-authored-by: Erik Sundell <erik.sundell@grafana.com>
Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>

packages/grafana-data/src/types/datasource.ts

@@ -228,7 +228,11 @@ abstract class DataSourceApi<
   abstract query(request: DataQueryRequest<TQuery>): Promise<DataQueryResponse> | Observable<DataQueryResponse>;
 
   /**
-   * 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<any>;
 
@@ -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;

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();
+    });
+  });
 });

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

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,
       };
     }
 

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)]);
+    });
   });
 });

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 }));
       }

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<CloudWatchQuery,
     );
   }
 
-  testDatasource() {
+  async testDatasource() {
     // use billing metrics for test
     const region = this.defaultRegion;
     const namespace = 'AWS/Billing';
     const metricName = 'EstimatedCharges';
     const dimensions = {};
 
-    return this.getDimensionValues(region, namespace, metricName, 'ServiceName', dimensions).then(() => ({
-      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<TSDBResponse> {

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<MssqlQuery, MssqlOptions> {
   id: any;
@@ -172,12 +173,7 @@ export class MssqlDatasource extends DataSourceWithBackend<MssqlQuery, MssqlOpti
       .pipe(
         mapTo({ status: 'success', message: 'Database Connection OK' }),
         catchError((err) => {
-          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();

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<MySQLQuery, MySQLOptions> {
   id: any;
@@ -181,12 +182,7 @@ export class MysqlDatasource extends DataSourceWithBackend<MySQLQuery, MySQLOpti
       .pipe(
         mapTo({ status: 'success', message: 'Database Connection OK' }),
         catchError((err) => {
-          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();

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<PostgresQuery, PostgresOptions> {
   id: any;
@@ -174,12 +175,7 @@ export class PostgresDatasource extends DataSourceWithBackend<PostgresQuery, Pos
         return { status: 'success', message: 'Database Connection OK' };
       })
       .catch((err: any) => {
-        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);
       });
   }