diff --git a/.betterer.results b/.betterer.results index 7b66278d808..37e7e5d0902 100644 --- a/.betterer.results +++ b/.betterer.results @@ -3633,11 +3633,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"], diff --git a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md index 4a2b9ea226d..be89606d023 100644 --- a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md +++ b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md @@ -61,6 +61,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 | | `enableDatagridEditing` | Enables the edit functionality in the datagrid panel | diff --git a/packages/grafana-data/src/types/featureToggles.gen.ts b/packages/grafana-data/src/types/featureToggles.gen.ts index a9390451921..a6c0acb89d3 100644 --- a/packages/grafana-data/src/types/featureToggles.gen.ts +++ b/packages/grafana-data/src/types/featureToggles.gen.ts @@ -89,6 +89,7 @@ export interface FeatureToggles { renderAuthJWT?: boolean; pyroscopeFlameGraph?: boolean; externalServiceAuth?: boolean; + refactorVariablesTimeRange?: boolean; useCachingService?: boolean; enableElasticsearchBackendQuerying?: boolean; authenticationConfigUI?: boolean; diff --git a/pkg/services/featuremgmt/registry.go b/pkg/services/featuremgmt/registry.go index 15507576b4c..af546f0cb0e 100644 --- a/pkg/services/featuremgmt/registry.go +++ b/pkg/services/featuremgmt/registry.go @@ -479,6 +479,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", diff --git a/pkg/services/featuremgmt/toggles_gen.csv b/pkg/services/featuremgmt/toggles_gen.csv index 9b5c8207409..700e99c9e7a 100644 --- a/pkg/services/featuremgmt/toggles_gen.csv +++ b/pkg/services/featuremgmt/toggles_gen.csv @@ -70,6 +70,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 diff --git a/pkg/services/featuremgmt/toggles_gen.go b/pkg/services/featuremgmt/toggles_gen.go index 10fec8b5113..0f22332ae3e 100644 --- a/pkg/services/featuremgmt/toggles_gen.go +++ b/pkg/services/featuremgmt/toggles_gen.go @@ -291,6 +291,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" diff --git a/public/app/features/variables/state/actions.ts b/public/app/features/variables/state/actions.ts index b2ce9dafc1c..4cd626cba7d 100644 --- a/public/app/features/variables/state/actions.ts +++ b/public/app/features/variables/state/actions.ts @@ -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, it’s time range, and has no dependent nodes + * ----- it should be added to the variablesRefreshTimeRange. + * + * -- b) If a variable A is a query variable, it’s 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> => 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> => async (dispatch, getState) => { const variableInState = getVariable(identifier, getState()); diff --git a/public/app/features/variables/state/onTimeRangeUpdated.test.ts b/public/app/features/variables/state/onTimeRangeUpdated.test.ts index e7e24bac26b..8c641ce359b 100644 --- a/public/app/features/variables/state/onTimeRangeUpdated.test.ts +++ b/public/app/features/variables/state/onTimeRangeUpdated.test.ts @@ -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({ 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({ 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({ 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 {