Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v10.0.x] Dashboards: Variables - Improve slow template variable loading due same variable loaded multiple times on time range change #69641

Merged
merged 1 commit into from
Jun 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 6 additions & 4 deletions .betterer.results
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 |

Expand Down
1 change: 1 addition & 0 deletions packages/grafana-data/src/types/featureToggles.gen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ export interface FeatureToggles {
renderAuthJWT?: boolean;
pyroscopeFlameGraph?: boolean;
externalServiceAuth?: boolean;
refactorVariablesTimeRange?: boolean;
useCachingService?: boolean;
enableElasticsearchBackendQuerying?: boolean;
authenticationConfigUI?: boolean;
Expand Down
6 changes: 6 additions & 0 deletions pkg/services/featuremgmt/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions pkg/services/featuremgmt/toggles_gen.csv
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions pkg/services/featuremgmt/toggles_gen.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
120 changes: 110 additions & 10 deletions public/app/features/variables/state/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -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,
Expand All @@ -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) =>
Expand All @@ -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());
Expand Down