Dashboards: Variables - Improve slow template variable loading due same variable loaded multiple times on time range change (#66965)

This commit is contained in:
Alexa V 2023-06-06 16:12:09 +03:00 committed by GitHub
parent e770bd6cd1
commit 07dd90b5a8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 366 additions and 20 deletions

View File

@ -3555,11 +3555,13 @@ exports[`better eslint`] = {
[0, 0, 0, "Do not use any type assertions.", "6"],
[0, 0, 0, "Do not use any type assertions.", "7"],
[0, 0, 0, "Do not use any type assertions.", "8"],
[0, 0, 0, "Unexpected any. Specify a different type.", "9"],
[0, 0, 0, "Unexpected any. Specify a different type.", "10"],
[0, 0, 0, "Do not use any type assertions.", "11"],
[0, 0, 0, "Do not use any type assertions.", "9"],
[0, 0, 0, "Do not use any type assertions.", "10"],
[0, 0, 0, "Unexpected any. Specify a different type.", "11"],
[0, 0, 0, "Unexpected any. Specify a different type.", "12"],
[0, 0, 0, "Unexpected any. Specify a different type.", "13"]
[0, 0, 0, "Do not use any type assertions.", "13"],
[0, 0, 0, "Unexpected any. Specify a different type.", "14"],
[0, 0, 0, "Unexpected any. Specify a different type.", "15"]
],
"public/app/features/variables/state/keyedVariablesReducer.ts:5381": [
[0, 0, 0, "Unexpected any. Specify a different type.", "0"],

View File

@ -59,6 +59,7 @@ Some stable features are enabled by default. You can disable a stable feature by
| `nestedFolders` | Enable folder nesting |
| `alertingNoNormalState` | Stop maintaining state of alerts that are not firing |
| `renderAuthJWT` | Uses JWT-based auth for rendering instead of relying on remote cache |
| `refactorVariablesTimeRange` | Refactor time range variables flow to reduce number of API calls made when query variables are chained |
| `enableElasticsearchBackendQuerying` | Enable the processing of queries and responses in the Elasticsearch data source through backend |
| `faroDatasourceSelector` | Enable the data source selector within the Frontend Apps section of the Frontend Observability |
| `enableDatagridEditing` | Enables the edit functionality in the datagrid panel |

View File

@ -88,6 +88,7 @@ export interface FeatureToggles {
renderAuthJWT?: boolean;
pyroscopeFlameGraph?: boolean;
externalServiceAuth?: boolean;
refactorVariablesTimeRange?: boolean;
useCachingService?: boolean;
enableElasticsearchBackendQuerying?: boolean;
authenticationConfigUI?: boolean;

View File

@ -474,6 +474,12 @@ var (
RequiresDevMode: true,
Owner: grafanaAuthnzSquad,
},
{
Name: "refactorVariablesTimeRange",
Description: "Refactor time range variables flow to reduce number of API calls made when query variables are chained",
State: FeatureStateBeta,
Owner: grafanaDashboardsSquad,
},
{
Name: "useCachingService",
Description: "When turned on, the new query and resource caching implementation using a wire service inject will be used in place of the previous middleware implementation",

View File

@ -69,6 +69,7 @@ unifiedRequestLog,alpha,@grafana/backend-platform,false,false,false,false
renderAuthJWT,beta,@grafana/grafana-as-code,false,false,false,false
pyroscopeFlameGraph,alpha,@grafana/observability-traces-and-profiling,false,false,false,false
externalServiceAuth,alpha,@grafana/grafana-authnz-team,true,false,false,false
refactorVariablesTimeRange,beta,@grafana/dashboards-squad,false,false,false,false
useCachingService,stable,@grafana/grafana-operator-experience-squad,false,false,true,false
enableElasticsearchBackendQuerying,beta,@grafana/observability-logs,false,false,false,false
authenticationConfigUI,alpha,@grafana/grafana-authnz-team,false,false,false,false

1 Name State Owner requiresDevMode RequiresLicense RequiresRestart FrontendOnly
69 renderAuthJWT beta @grafana/grafana-as-code false false false false
70 pyroscopeFlameGraph alpha @grafana/observability-traces-and-profiling false false false false
71 externalServiceAuth alpha @grafana/grafana-authnz-team true false false false
72 refactorVariablesTimeRange beta @grafana/dashboards-squad false false false false
73 useCachingService stable @grafana/grafana-operator-experience-squad false false true false
74 enableElasticsearchBackendQuerying beta @grafana/observability-logs false false false false
75 authenticationConfigUI alpha @grafana/grafana-authnz-team false false false false

View File

@ -287,6 +287,10 @@ const (
// Starts an OAuth2 authentication provider for external services
FlagExternalServiceAuth = "externalServiceAuth"
// FlagRefactorVariablesTimeRange
// Refactor time range variables flow to reduce number of API calls made when query variables are chained
FlagRefactorVariablesTimeRange = "refactorVariablesTimeRange"
// FlagUseCachingService
// When turned on, the new query and resource caching implementation using a wire service inject will be used in place of the previous middleware implementation
FlagUseCachingService = "useCachingService"

View File

@ -9,7 +9,7 @@ import {
UrlQueryMap,
UrlQueryValue,
} from '@grafana/data';
import { locationService } from '@grafana/runtime';
import { config, locationService } from '@grafana/runtime';
import { notifyApp } from 'app/core/actions';
import { contextSrv } from 'app/core/services/context_srv';
import { getTimeSrv } from 'app/features/dashboard/services/TimeSrv';
@ -19,7 +19,7 @@ import { store } from 'app/store/store';
import { createErrorNotification } from '../../../core/copy/appNotification';
import { appEvents } from '../../../core/core';
import { getBackendSrv } from '../../../core/services/backend_srv';
import { Graph } from '../../../core/utils/dag';
import { Graph, Node } from '../../../core/utils/dag';
import { AppNotification, StoreState, ThunkResult } from '../../../types';
import { getDatasourceSrv } from '../../plugins/datasource_srv';
import { getTemplateSrv, TemplateSrv } from '../../templating/template_srv';
@ -641,6 +641,106 @@ export interface OnTimeRangeUpdatedDependencies {
events: typeof appEvents;
}
const dfs = (node: Node, visited: string[], variables: VariableModel[], variablesRefreshTimeRange: VariableModel[]) => {
if (!visited.includes(node.name)) {
visited.push(node.name);
}
node.outputEdges.forEach((e) => {
const child = e.outputNode;
if (child && !visited.includes(child.name)) {
const childVariable = variables.find((v) => v.name === child.name) as QueryVariableModel;
// when a variable is refreshed on time range change, we need to add that variable to be refreshed and mark its children as visited
if (
childVariable &&
childVariable.refresh === VariableRefresh.onTimeRangeChanged &&
variablesRefreshTimeRange.indexOf(childVariable) === -1
) {
variablesRefreshTimeRange.push(childVariable);
visited.push(child.name);
} else {
dfs(child, visited, variables, variablesRefreshTimeRange);
}
}
});
return variablesRefreshTimeRange;
};
/**
* This function returns a list of variables that need to be refreshed when the time range changes
* It follows this logic
* Create a graph based on all template variables.
* Loop through all the variables and perform the following checks for each variable:
*
* -- a) If a variable A is a query variable, its time range, and has no dependent nodes
* ----- it should be added to the variablesRefreshTimeRange.
*
* -- b) If a variable A is a query variable, its time range, and has dependent nodes (B, C)
* ----- 1. add the variable A to variablesRefreshTimeRange
* ----- 2. skip all the dependent nodes (B, C).
* Here, we should traverse the tree using DFS (Depth First Search), as the dependent nodes will be updated in cascade when the parent variable is updated.
*/
export const getVariablesThatNeedRefreshNew = (key: string, state: StoreState): VariableWithOptions[] => {
const allVariables = getVariablesByKey(key, state);
//create dependency graph
const g = createGraph(allVariables);
// create a list of nodes that were visited
const visitedDfs: string[] = [];
const variablesRefreshTimeRange: VariableWithOptions[] = [];
allVariables.forEach((v) => {
const node = g.getNode(v.name);
if (visitedDfs.includes(v.name)) {
return;
}
if (node) {
const parentVariableNode = allVariables.find((v) => v.name === node.name) as QueryVariableModel;
const isVariableTimeRange =
parentVariableNode && parentVariableNode.refresh === VariableRefresh.onTimeRangeChanged;
//
if (isVariableTimeRange && node.outputEdges.length === 0) {
variablesRefreshTimeRange.push(parentVariableNode);
}
// if variable is time range and other variables depend on it (output edges) add it to the list of variables that need refresh and dont visit its dependents
if (
isVariableTimeRange &&
variablesRefreshTimeRange.includes(parentVariableNode) &&
node.outputEdges.length > 0
) {
variablesRefreshTimeRange.push(parentVariableNode);
dfs(node, visitedDfs, allVariables, variablesRefreshTimeRange);
}
// if variable is not time range but has dependents (output edges) visit its dependants and repeat the process
if (
parentVariableNode &&
parentVariableNode.refresh &&
parentVariableNode.refresh !== VariableRefresh.onTimeRangeChanged
) {
dfs(node, visitedDfs, allVariables, variablesRefreshTimeRange);
}
}
});
return variablesRefreshTimeRange;
};
// old approach of refreshing variables that need refresh
const getVariablesThatNeedRefreshOld = (key: string, state: StoreState): VariableWithOptions[] => {
const allVariables = getVariablesByKey(key, state);
const variablesThatNeedRefresh = allVariables.filter((variable) => {
if (variable.hasOwnProperty('refresh') && variable.hasOwnProperty('options')) {
const variableWithRefresh = variable as unknown as QueryVariableModel;
return variableWithRefresh.refresh === VariableRefresh.onTimeRangeChanged;
}
return false;
}) as VariableWithOptions[];
return variablesThatNeedRefresh;
};
export const onTimeRangeUpdated =
(
key: string,
@ -649,14 +749,14 @@ export const onTimeRangeUpdated =
): ThunkResult<Promise<void>> =>
async (dispatch, getState) => {
dependencies.templateSrv.updateTimeRange(timeRange);
const variablesThatNeedRefresh = getVariablesByKey(key, getState()).filter((variable) => {
if (variable.hasOwnProperty('refresh') && variable.hasOwnProperty('options')) {
const variableWithRefresh = variable as unknown as QueryVariableModel;
return variableWithRefresh.refresh === VariableRefresh.onTimeRangeChanged;
}
return false;
}) as VariableWithOptions[];
// approach # 2, get variables that need refresh but use the dependency graph to only update the ones that are affected
let variablesThatNeedRefresh: VariableWithOptions[] = [];
if (config.featureToggles.refactorVariablesTimeRange) {
variablesThatNeedRefresh = getVariablesThatNeedRefreshNew(key, getState());
} else {
variablesThatNeedRefresh = getVariablesThatNeedRefreshOld(key, getState());
}
const variableIds = variablesThatNeedRefresh.map((variable) => variable.id);
const promises = variablesThatNeedRefresh.map((variable) =>
@ -672,7 +772,7 @@ export const onTimeRangeUpdated =
}
};
const timeRangeUpdated =
export const timeRangeUpdated =
(identifier: KeyedVariableIdentifier): ThunkResult<Promise<void>> =>
async (dispatch, getState) => {
const variableInState = getVariable(identifier, getState());

View File

@ -1,4 +1,6 @@
import { dateTime, TimeRange } from '@grafana/data';
import { config, DataSourceSrv } from '@grafana/runtime';
import * as runtime from '@grafana/runtime';
import { reduxTester } from '../../../../test/core/redux/reduxTester';
import { silenceConsoleOutput } from '../../../../test/core/utils/silenceConsoleOutput';
@ -12,11 +14,13 @@ import { variableAdapters } from '../adapters';
import { createConstantVariableAdapter } from '../constant/adapter';
import { createIntervalVariableAdapter } from '../interval/adapter';
import { createIntervalOptions } from '../interval/reducer';
import { constantBuilder, intervalBuilder } from '../shared/testing/builders';
import { createQueryVariableAdapter } from '../query/adapter';
import { constantBuilder, intervalBuilder, queryBuilder } from '../shared/testing/builders';
import { VariableRefresh } from '../types';
import { toKeyedVariableIdentifier, toVariablePayload } from '../utils';
import { onTimeRangeUpdated, OnTimeRangeUpdatedDependencies, setOptionAsCurrent } from './actions';
import * as actions from './actions';
import { getPreloadedState, getRootReducer, RootReducerType } from './helpers';
import { toKeyedAction } from './keyedVariablesReducer';
import {
@ -27,7 +31,29 @@ import {
} from './sharedReducer';
import { variablesInitTransaction } from './transactionReducer';
variableAdapters.setInit(() => [createIntervalVariableAdapter(), createConstantVariableAdapter()]);
variableAdapters.setInit(() => [
createIntervalVariableAdapter(),
createConstantVariableAdapter(),
createQueryVariableAdapter(),
]);
const metricFindQuery = jest
.fn()
.mockResolvedValueOnce([{ text: 'responses' }, { text: 'timers' }])
.mockResolvedValue([{ text: '200' }, { text: '500' }]);
const getMetricSources = jest.fn().mockReturnValue([]);
const getDatasource = jest.fn().mockResolvedValue({ metricFindQuery });
jest.mock('app/features/dashboard/services/TimeSrv', () => ({
getTimeSrv: () => ({
timeRange: jest.fn().mockReturnValue(undefined),
}),
}));
runtime.setDataSourceSrv({
get: getDatasource,
getList: getMetricSources,
} as unknown as DataSourceSrv);
const getTestContext = (dashboard: DashboardModel) => {
jest.clearAllMocks();
@ -68,7 +94,8 @@ const getTestContext = (dashboard: DashboardModel) => {
const dashboardState = {
getModel: () => dashboard,
} as unknown as DashboardState;
const adapter = variableAdapters.get('interval');
const adapterInterval = variableAdapters.get('interval');
const adapterQuery = variableAdapters.get('query');
const templatingState = {
variables: {
'interval-0': { ...interval },
@ -85,7 +112,100 @@ const getTestContext = (dashboard: DashboardModel) => {
interval,
range,
dependencies,
adapter,
adapterInterval,
adapterQuery,
preloadedState,
updateTimeRangeMock,
templateVariableValueUpdatedMock,
startRefreshMock,
};
};
const getTestContextVariables = (dashboard: DashboardModel) => {
jest.clearAllMocks();
const key = 'key';
const interval = intervalBuilder()
.withId('interval-0')
.withRootStateKey(key)
.withName('interval-0')
.withOptions('1m', '10m', '30m', '1h', '6h', '12h', '1d', '7d', '14d', '30d')
.withCurrent('1m')
.build();
const constant = constantBuilder()
.withId('constant-1')
.withRootStateKey(key)
.withName('constant-1')
.withOptions('a constant')
.withCurrent('a constant')
.build();
const queryA = queryBuilder()
.withId('a')
.withRootStateKey(key)
.withName('a')
.withQuery('query')
.withRefresh(VariableRefresh.onTimeRangeChanged)
.build();
const queryB = queryBuilder()
.withId('b')
.withRootStateKey(key)
.withName('b')
.withQuery('$a')
.withRefresh(VariableRefresh.onTimeRangeChanged)
.build();
const queryC = queryBuilder()
.withId('c')
.withRootStateKey(key)
.withName('c')
.withQuery('$a')
.withRefresh(VariableRefresh.onTimeRangeChanged)
.build();
const range: TimeRange = {
from: dateTime(new Date().getTime()).subtract(1, 'minutes'),
to: dateTime(new Date().getTime()),
raw: {
from: 'now-1m',
to: 'now',
},
};
const updateTimeRangeMock = jest.fn();
const templateSrvMock = { updateTimeRange: updateTimeRangeMock } as unknown as TemplateSrv;
const dependencies: OnTimeRangeUpdatedDependencies = { templateSrv: templateSrvMock, events: appEvents };
const templateVariableValueUpdatedMock = jest.fn();
const startRefreshMock = jest.fn();
dashboard.templateVariableValueUpdated = templateVariableValueUpdatedMock;
dashboard.startRefresh = startRefreshMock;
const dashboardState = {
getModel: () => dashboard,
} as unknown as DashboardState;
const adapterInterval = variableAdapters.get('interval');
const adapterQuery = variableAdapters.get('query');
const templatingState = {
variables: {
'interval-0': { ...interval },
'constant-1': { ...constant },
a: { ...queryA },
b: { ...queryB },
c: { ...queryC },
},
};
const preloadedState = {
dashboard: dashboardState,
...getPreloadedState(key, templatingState),
} as unknown as RootReducerType;
return {
key,
interval,
range,
dependencies,
adapterInterval,
adapterQuery,
preloadedState,
updateTimeRangeMock,
templateVariableValueUpdatedMock,
@ -180,10 +300,11 @@ describe('when onTimeRangeUpdated is dispatched', () => {
describe('and updateOptions throws', () => {
silenceConsoleOutput();
it('then correct actions are dispatched and correct dependencies are called', async () => {
const {
key,
adapter,
adapterInterval,
preloadedState,
range,
dependencies,
@ -192,7 +313,7 @@ describe('when onTimeRangeUpdated is dispatched', () => {
startRefreshMock,
} = getTestContext(getDashboardModel());
adapter.updateOptions = jest.fn().mockRejectedValue(new Error('Something broke'));
adapterInterval.updateOptions = jest.fn().mockRejectedValue(new Error('Something broke'));
const tester = await reduxTester<RootReducerType>({ preloadedState, debug: true })
.givenRootReducer(getRootReducer())
@ -224,6 +345,116 @@ describe('when onTimeRangeUpdated is dispatched', () => {
expect(startRefreshMock).toHaveBeenCalledTimes(0);
});
});
describe('When onTimeRangeUpdated is dispatched without refactorVariablesTimeRange feature flag ', () => {
silenceConsoleOutput();
it('then with old getVariablesThatNeedRefresh we call all of them in pararell', async () => {
const { key, preloadedState, range, dependencies } = getTestContextVariables(getDashboardModel());
// Spying on timeRangeUpdated action
const spyTimeRangeUpdated = jest.spyOn(actions, 'timeRangeUpdated');
// Initiating the Redux testing environment
await reduxTester<RootReducerType>({ preloadedState, debug: true })
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(toKeyedAction(key, variablesInitTransaction({ uid: key })))
.whenAsyncActionIsDispatched(onTimeRangeUpdated(key, range, dependencies), true);
// Using the old algorithm, all variables configured to refresh on time range are expected to be refreshed,
// irrespective of their dependencies. Thus, 'a', 'b', 'c', and 'interval' are expected to be refreshed.
// - 'a', 'b', 'c' are query variables, and 'interval' is an interval variable.
// - 'b' and 'c' depend on 'a', but this does not matter in the old algorithm, as all are refreshed in parallel.
const expectedVariables = {
queryA: {
id: 'a',
type: 'query',
rootStateKey: 'key',
},
queryB: {
id: 'b',
type: 'query',
rootStateKey: 'key',
},
queryC: {
id: 'c',
type: 'query',
rootStateKey: 'key',
},
interval: {
id: 'interval-0',
type: 'interval',
rootStateKey: 'key',
},
};
// Asserting that timeRangeUpdated action has been called with the correct arguments
expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.queryA);
expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.queryB);
expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.queryC);
expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.interval);
expect(spyTimeRangeUpdated).toHaveBeenCalledTimes(4);
spyTimeRangeUpdated.mockRestore();
});
});
describe('On dispatch of onTimeRangeUpdated with refactorVariablesTimeRange feature flag', () => {
silenceConsoleOutput();
beforeAll(() => {
config.featureToggles.refactorVariablesTimeRange = true;
});
afterAll(() => {
config.featureToggles.refactorVariablesTimeRange = false;
});
it('should refresh only the independent query variables and those with dependents, optimising the number of calls when working with chained query variables', async () => {
const { key, preloadedState, range, dependencies } = getTestContextVariables(getDashboardModel());
// Spying on timeRangeUpdated action
const spyTimeRangeUpdated = jest.spyOn(actions, 'timeRangeUpdated');
// Initiating the Redux testing environment
await reduxTester<RootReducerType>({ preloadedState, debug: true })
.givenRootReducer(getRootReducer())
.whenActionIsDispatched(toKeyedAction(key, variablesInitTransaction({ uid: key })))
.whenAsyncActionIsDispatched(onTimeRangeUpdated(key, range, dependencies), true);
// Defining the variables that are expected to be refreshed
// Based on the algorithm of getVariablesThatNeedRefreshNew, we have the following variables:
// - queryA: This is a Query variable and has no dependents. According to the algorithm, Query variables without dependents
// should be refreshed. Hence, it's expected to be in the refreshed variables list.
// - interval: This is an interval variable without any dependents. Similar to queryA, the algorithm specifies that
// variables without dependents should be refreshed when the time range changes. Hence, interval is also expected
// to be refreshed.
// Note: Variables 'b' and 'c' are not expected to be in the refreshed variables list even though they're set to
// refresh on time range change. This is because they have dependencies ('b' and 'c' depend on 'a'). The algorithm
// optimizes the refreshing process by refreshing only independent variables and those that have dependents.
// Once 'a' is refreshed, it will trigger a cascading refresh of 'b' and 'c'.
const expectedVariables = {
queryA: {
id: 'a',
type: 'query',
rootStateKey: 'key',
},
interval: {
id: 'interval-0',
type: 'interval',
rootStateKey: 'key',
},
};
// Asserting that timeRangeUpdated action has been called with the correct arguments
expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.queryA);
expect(spyTimeRangeUpdated).toHaveBeenCalledWith(expectedVariables.interval);
expect(spyTimeRangeUpdated).toHaveBeenCalledTimes(2);
spyTimeRangeUpdated.mockRestore();
});
});
});
function getDashboardModel(): DashboardModel {