fix(log): Missing failed query log on async queries (#33024)

superset-frontend/spec/fixtures/mockDatabases.ts

@@ -0,0 +1,48 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export default {
+  1: {
+    allow_ctas: false,
+    allow_cvas: false,
+    allow_dml: false,
+    allow_file_upload: false,
+    allow_run_async: true,
+    backend: 'postgresql',
+    database_name: 'examples',
+    expose_in_sqllab: true,
+    force_ctas_schema: null,
+    id: 1,
+  },
+};
+
+export const disabledAsyncDb = {
+  21: {
+    allow_ctas: false,
+    allow_cvas: false,
+    allow_dml: false,
+    allow_file_upload: false,
+    allow_run_async: false,
+    backend: 'postgresql',
+    database_name: 'examples',
+    expose_in_sqllab: true,
+    force_ctas_schema: null,
+    id: 21,
+  },
+};

superset-frontend/src/SqlLab/actions/sqlLab.js

@@ -252,28 +252,30 @@ export function querySuccess(query, results) {
   return { type: QUERY_SUCCESS, query, results };
 }
 
-export function queryFailed(query, msg, link, errors) {
+export function logFailedQuery(query, errors) {
   return function (dispatch) {
     const eventData = {
       has_err: true,
       start_offset: query.startDttm,
       ts: new Date().getTime(),
     };
-    errors?.forEach(({ error_type: errorType, extra }) => {
-      const messages = extra?.issue_codes?.map(({ message }) => message) || [
-        errorType,
-      ];
-      messages.forEach(message => {
-        dispatch(
-          logEvent(LOG_ACTIONS_SQLLAB_FETCH_FAILED_QUERY, {
-            ...eventData,
-            error_type: errorType,
-            error_details: message,
-          }),
-        );
-      });
+    errors?.forEach(({ error_type: errorType, message, extra }) => {
+      const issueCodes = extra?.issue_codes?.map(({ code }) => code) || [-1];
+      dispatch(
+        logEvent(LOG_ACTIONS_SQLLAB_FETCH_FAILED_QUERY, {
+          ...eventData,
+          error_type: errorType,
+          issue_codes: issueCodes,
+          error_details: message,
+        }),
+      );
     });
+  };
+}
 
+export function queryFailed(query, msg, link, errors) {
+  return function (dispatch) {
+    dispatch(logFailedQuery(query, errors));
     dispatch({ type: QUERY_FAILED, query, msg, link, errors });
   };
 }

superset-frontend/src/SqlLab/actions/sqlLab.test.js

@@ -294,7 +294,7 @@ describe('async actions', () => {
     });
 
     it('calls queryFailed on fetch error and logs the error details', () => {
-      expect.assertions(3);
+      expect.assertions(2);
 
       fetchMock.post(
         runQueryEndpoint,
@@ -312,7 +312,6 @@ describe('async actions', () => {
       const expectedActionTypes = [
         actions.START_QUERY,
         LOG_EVENT,
-        LOG_EVENT,
         actions.QUERY_FAILED,
       ];
       const { dispatch } = store;
@@ -320,12 +319,7 @@ describe('async actions', () => {
       return request(dispatch, () => initialState).then(() => {
         const actions = store.getActions();
         expect(actions.map(a => a.type)).toEqual(expectedActionTypes);
-        expect(actions[1].payload.eventData.error_details).toContain(
-          'Issue 1000',
-        );
-        expect(actions[2].payload.eventData.error_details).toContain(
-          'Issue 1001',
-        );
+        expect(actions[1].payload.eventData.issue_codes).toEqual([1000, 1001]);
       });
     });
   });

superset-frontend/src/SqlLab/components/QueryAutoRefresh/QueryAutoRefresh.test.tsx

@@ -16,10 +16,12 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+import { QueryState } from '@superset-ui/core';
 import fetchMock from 'fetch-mock';
 import configureStore from 'redux-mock-store';
 import thunk from 'redux-thunk';
 import { render, waitFor } from 'spec/helpers/testing-library';
+import { LOG_ACTIONS_SQLLAB_FETCH_FAILED_QUERY } from 'src/logger/LogUtils';
 import {
   CLEAR_INACTIVE_QUERIES,
   REFRESH_QUERIES,
@@ -31,9 +33,13 @@ import QueryAutoRefresh, {
 } from 'src/SqlLab/components/QueryAutoRefresh';
 import { successfulQuery, runningQuery } from 'src/SqlLab/fixtures';
 import { QueryDictionary } from 'src/SqlLab/types';
+import mockDatabases from 'spec/fixtures/mockDatabases';
 
 const middlewares = [thunk];
 const mockStore = configureStore(middlewares);
+const mockState = {
+  databases: mockDatabases,
+};
 
 // NOTE: The uses of @ts-ignore in this file is to enable testing of bad inputs to verify the
 // function / component handles bad data elegantly
@@ -106,7 +112,9 @@ describe('QueryAutoRefresh', () => {
   });
 
   it('Attempts to refresh when given pending query', async () => {
-    const store = mockStore();
+    const store = mockStore({
+      sqlLab: { ...mockState },
+    });
     fetchMock.get(refreshApi, {
       result: [
         {
@@ -135,7 +143,7 @@ describe('QueryAutoRefresh', () => {
   });
 
   it('Attempts to clear inactive queries when updated queries are empty', async () => {
-    const store = mockStore();
+    const store = mockStore({ sqlLab: { ...mockState } });
     fetchMock.get(refreshApi, {
       result: [],
     });
@@ -163,7 +171,7 @@ describe('QueryAutoRefresh', () => {
   });
 
   it('Does not fail and attempts to refresh when given pending query and invalid query', async () => {
-    const store = mockStore();
+    const store = mockStore({ sqlLab: { ...mockState } });
     fetchMock.get(refreshApi, {
       result: [
         {
@@ -193,7 +201,7 @@ describe('QueryAutoRefresh', () => {
   });
 
   it('Does NOT Attempt to refresh when given only completed queries', async () => {
-    const store = mockStore();
+    const store = mockStore({ sqlLab: { ...mockState } });
     fetchMock.get(refreshApi, {
       result: [
         {
@@ -220,4 +228,57 @@ describe('QueryAutoRefresh', () => {
     );
     expect(fetchMock.calls(refreshApi)).toHaveLength(0);
   });
+
+  it('logs the failed error for async queries', async () => {
+    const store = mockStore({ sqlLab: { ...mockState } });
+    fetchMock.get(refreshApi, {
+      result: [
+        {
+          id: runningQuery.id,
+          dbId: 1,
+          state: QueryState.Failed,
+          extra: {
+            errors: [
+              {
+                error_type: 'TEST_ERROR',
+                level: 'error',
+                message: 'Syntax invalid',
+                extra: {
+                  issue_codes: [
+                    {
+                      code: 102,
+                      message: 'DB failed',
+                    },
+                  ],
+                },
+              },
+            ],
+          },
+        },
+      ],
+    });
+    render(
+      <QueryAutoRefresh
+        queries={runningQueries}
+        queriesLastUpdate={queriesLastUpdate}
+      />,
+      { useRedux: true, store },
+    );
+    await waitFor(
+      () =>
+        expect(store.getActions()).toContainEqual(
+          expect.objectContaining({
+            payload: expect.objectContaining({
+              eventName: LOG_ACTIONS_SQLLAB_FETCH_FAILED_QUERY,
+              eventData: expect.objectContaining({
+                error_type: 'TEST_ERROR',
+                error_details: 'Syntax invalid',
+                issue_codes: [102],
+              }),
+            }),
+          }),
+        ),
+      { timeout: QUERY_UPDATE_FREQ + 100 },
+    );
+  });
 });

superset-frontend/src/SqlLab/components/QueryAutoRefresh/index.tsx

@@ -17,7 +17,7 @@
  * under the License.
  */
 import { useRef } from 'react';
-import { useDispatch } from 'react-redux';
+import { useSelector, useDispatch } from 'react-redux';
 import { isObject } from 'lodash';
 import rison from 'rison';
 import {
@@ -25,13 +25,17 @@ import {
   Query,
   runningQueryStateList,
   QueryResponse,
+  QueryState,
+  lruCache,
 } from '@superset-ui/core';
-import { QueryDictionary } from 'src/SqlLab/types';
+import { QueryDictionary, SqlLabRootState } from 'src/SqlLab/types';
 import useInterval from 'src/SqlLab/utils/useInterval';
 import {
   refreshQueries,
   clearInactiveQueries,
+  logFailedQuery,
 } from 'src/SqlLab/actions/sqlLab';
+import type { DatabaseObject } from 'src/features/databases/types';
 
 export const QUERY_UPDATE_FREQ = 2000;
 const QUERY_UPDATE_BUFFER_MS = 5000;
@@ -67,6 +71,17 @@ function QueryAutoRefresh({
   // pendingRequest check ensures we only have one active http call to check for query statuses
   const pendingRequestRef = useRef(false);
   const cleanInactiveRequestRef = useRef(false);
+  const failedQueries = useRef(lruCache(1000));
+  const databases = useSelector<SqlLabRootState>(
+    ({ sqlLab }) => sqlLab.databases,
+  ) as Record<string, DatabaseObject>;
+  const asyncFetchDbs = useRef(
+    new Set(
+      Object.values(databases)
+        .filter(({ allow_run_async }) => Boolean(allow_run_async))
+        .map(({ id }) => id),
+    ),
+  );
   const dispatch = useDispatch();
 
   const checkForRefresh = () => {
@@ -97,6 +112,17 @@ function QueryAutoRefresh({
                   {},
                 ) ?? {};
               dispatch(refreshQueries(queries));
+              jsonPayload.result.forEach(query => {
+                const { id, dbId, state } = query;
+                if (
+                  asyncFetchDbs.current.has(dbId) &&
+                  !failedQueries.current.has(id) &&
+                  state === QueryState.Failed
+                ) {
+                  dispatch(logFailedQuery(query, query.extra?.errors));
+                  failedQueries.current.set(id, true);
+                }
+              });
             } else {
               dispatch(clearInactiveQueries(QUERY_UPDATE_FREQ));
             }