From 0cb57f52de3f81894cc8eebe6953e94df3dfd296 Mon Sep 17 00:00:00 2001 From: woodsaj Date: Tue, 8 Mar 2016 15:03:34 +0800 Subject: [PATCH 1/4] refactor how template vars are updated. fixes #4283 Use promises to order the updates of variable options so that parents are always updated before children. This ensures that we only need to query the datasource once per variable as variables that depend on other variables will only be processed once their parent has been. This commit also ensures that variable options are refreshed if "refresh_on_load" is true even when query params are used to set the variable seltion. --- .../features/templating/templateValuesSrv.js | 75 +++++++++++++++---- 1 file changed, 61 insertions(+), 14 deletions(-) diff --git a/public/app/features/templating/templateValuesSrv.js b/public/app/features/templating/templateValuesSrv.js index 46d2f90bc03..5256c3e59b5 100644 --- a/public/app/features/templating/templateValuesSrv.js +++ b/public/app/features/templating/templateValuesSrv.js @@ -27,29 +27,72 @@ function (angular, _, kbn) { var queryParams = $location.search(); var promises = []; + //use promises to delay processing variables that + //depend on other variables. + this.variableLock = {}; + var self = this; + _.forEach(this.variables, function(variable) { + self.variableLock[variable.name] = $q.defer(); + }); + for (var i = 0; i < this.variables.length; i++) { var variable = this.variables[i]; - var urlValue = queryParams['var-' + variable.name]; - if (urlValue !== void 0) { - promises.push(this.setVariableFromUrl(variable, urlValue)); - } - else if (variable.refresh) { - promises.push(this.updateOptions(variable)); - } - else if (variable.type === 'interval') { - this.updateAutoInterval(variable); - } + promises.push(this.processVariable(variable, queryParams)); } return $q.all(promises); }; + this.processVariable = function(variable, queryParams) { + var dependencies = []; + var self = this; + // determine our dependencies. + if (variable.type === "query") { + _.forEach(this.variables, function(v) { + if (templateSrv.containsVariable(variable.query, v.name)) { + dependencies.push(self.variableLock[v.name].promise); + } + }); + } + return $q.all(dependencies).then(function() { + var variableName = variable.name; + var urlValue = queryParams['var-' + variable.name]; + if (urlValue !== void 0) { + return self.setVariableFromUrl(variable, urlValue).then(function() { + self.variableLock[variableName].resolve(); + }); + } + else if (variable.refresh) { + return self.updateOptions(variable).then(function() { + self.variableLock[variableName].resolve(); + }); + } + else if (variable.type === 'interval') { + self.updateAutoInterval(variable); + self.variableLock[variableName].resolve(); + } else { + self.variableLock[variableName].resolve(); + } + }); + }; + this.setVariableFromUrl = function(variable, urlValue) { + if (variable.refresh) { + var self = this; + //refresh the list of options before setting the value + return this.updateOptions(variable).then(function() { + var option = _.findWhere(variable.options, { text: urlValue }); + option = option || { text: urlValue, value: urlValue }; + + self.updateAutoInterval(variable); + return self.setVariableValue(variable, option, true); + }); + } var option = _.findWhere(variable.options, { text: urlValue }); option = option || { text: urlValue, value: urlValue }; this.updateAutoInterval(variable); - return this.setVariableValue(variable, option); + return this.setVariableValue(variable, option, true); }; this.updateAutoInterval = function(variable) { @@ -64,7 +107,7 @@ function (angular, _, kbn) { templateSrv.setGrafanaVariable('$__auto_interval', interval); }; - this.setVariableValue = function(variable, option) { + this.setVariableValue = function(variable, option, firstLoad) { variable.current = angular.copy(option); if (_.isArray(variable.current.value)) { @@ -74,6 +117,11 @@ function (angular, _, kbn) { self.selectOptionsForCurrentValue(variable); templateSrv.updateTemplateData(); + // on first load, variable loading is ordered to ensure + // that parents are updated before children. + if (firstLoad) { + return $q.when(); + } return self.updateOptionsInChildVariables(variable); }; @@ -119,8 +167,7 @@ function (angular, _, kbn) { return datasourceSrv.get(variable.datasource) .then(_.partial(this.updateOptionsFromMetricFindQuery, variable)) - .then(_.partial(this.updateTags, variable)) - .then(_.partial(this.validateVariableSelectionState, variable)); + .then(_.partial(this.updateTags, variable)); }; this.selectOptionsForCurrentValue = function(variable) { From dc4743a392e4a4494f967b8d27f758db7917309c Mon Sep 17 00:00:00 2001 From: woodsaj Date: Tue, 8 Mar 2016 15:41:24 +0800 Subject: [PATCH 2/4] ensure current template variable is set. - When distributing a dashboard it is often not practical to have "current" template variable option set. This commit ensures that for dashboards with no current, it is assigned when the dashboard loads (assuming refresh on load is set.) --- public/app/features/templating/templateValuesSrv.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/public/app/features/templating/templateValuesSrv.js b/public/app/features/templating/templateValuesSrv.js index 5256c3e59b5..7750bb34a19 100644 --- a/public/app/features/templating/templateValuesSrv.js +++ b/public/app/features/templating/templateValuesSrv.js @@ -64,6 +64,10 @@ function (angular, _, kbn) { } else if (variable.refresh) { return self.updateOptions(variable).then(function() { + if (_.isEmpty(variable.current) && variable.options.length) { + console.log("setting current for %s", variable.name); + self.setVariableValue(variable, variable.options[0], true); + } self.variableLock[variableName].resolve(); }); } From 2475ca8f9a5f23263315dd3181b74c30f5b17050 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Tue, 8 Mar 2016 09:32:54 +0100 Subject: [PATCH 3/4] fix(templaing): refactoring PR #4283 --- .../features/templating/templateValuesSrv.js | 59 +++++++++---------- 1 file changed, 28 insertions(+), 31 deletions(-) diff --git a/public/app/features/templating/templateValuesSrv.js b/public/app/features/templating/templateValuesSrv.js index 7750bb34a19..51895ff1ebf 100644 --- a/public/app/features/templating/templateValuesSrv.js +++ b/public/app/features/templating/templateValuesSrv.js @@ -27,10 +27,9 @@ function (angular, _, kbn) { var queryParams = $location.search(); var promises = []; - //use promises to delay processing variables that - //depend on other variables. + // use promises to delay processing variables that + // depend on other variables. this.variableLock = {}; - var self = this; _.forEach(this.variables, function(variable) { self.variableLock[variable.name] = $q.defer(); }); @@ -45,7 +44,8 @@ function (angular, _, kbn) { this.processVariable = function(variable, queryParams) { var dependencies = []; - var self = this; + var lock = self.variableLock[variable.name]; + // determine our dependencies. if (variable.type === "query") { _.forEach(this.variables, function(v) { @@ -54,49 +54,44 @@ function (angular, _, kbn) { } }); } + return $q.all(dependencies).then(function() { - var variableName = variable.name; var urlValue = queryParams['var-' + variable.name]; if (urlValue !== void 0) { - return self.setVariableFromUrl(variable, urlValue).then(function() { - self.variableLock[variableName].resolve(); - }); + return self.setVariableFromUrl(variable, urlValue).then(lock.resolve); } else if (variable.refresh) { return self.updateOptions(variable).then(function() { if (_.isEmpty(variable.current) && variable.options.length) { console.log("setting current for %s", variable.name); - self.setVariableValue(variable, variable.options[0], true); + self.setVariableValue(variable, variable.options[0]); } - self.variableLock[variableName].resolve(); + lock.resolve(); }); } else if (variable.type === 'interval') { self.updateAutoInterval(variable); - self.variableLock[variableName].resolve(); + lock.resolve(); } else { - self.variableLock[variableName].resolve(); + lock.resolve(); } }); }; this.setVariableFromUrl = function(variable, urlValue) { + var promise = $q.when(true); + if (variable.refresh) { - var self = this; - //refresh the list of options before setting the value - return this.updateOptions(variable).then(function() { - var option = _.findWhere(variable.options, { text: urlValue }); - option = option || { text: urlValue, value: urlValue }; - - self.updateAutoInterval(variable); - return self.setVariableValue(variable, option, true); - }); + promise = this.updateOptions(variable); } - var option = _.findWhere(variable.options, { text: urlValue }); - option = option || { text: urlValue, value: urlValue }; - this.updateAutoInterval(variable); - return this.setVariableValue(variable, option, true); + return promise.then(function() { + var option = _.findWhere(variable.options, { text: urlValue }); + option = option || { text: urlValue, value: urlValue }; + + self.updateAutoInterval(variable); + return self.setVariableValue(variable, option, true); + }); }; this.updateAutoInterval = function(variable) { @@ -111,7 +106,7 @@ function (angular, _, kbn) { templateSrv.setGrafanaVariable('$__auto_interval', interval); }; - this.setVariableValue = function(variable, option, firstLoad) { + this.setVariableValue = function(variable, option, initPhase) { variable.current = angular.copy(option); if (_.isArray(variable.current.value)) { @@ -119,13 +114,14 @@ function (angular, _, kbn) { } self.selectOptionsForCurrentValue(variable); - templateSrv.updateTemplateData(); + // on first load, variable loading is ordered to ensure // that parents are updated before children. - if (firstLoad) { + if (initPhase) { return $q.when(); } + return self.updateOptionsInChildVariables(variable); }; @@ -171,7 +167,8 @@ function (angular, _, kbn) { return datasourceSrv.get(variable.datasource) .then(_.partial(this.updateOptionsFromMetricFindQuery, variable)) - .then(_.partial(this.updateTags, variable)); + .then(_.partial(this.updateTags, variable)) + .then(_.partial(this.validateVariableSelectionState, variable)); }; this.selectOptionsForCurrentValue = function(variable) { @@ -196,7 +193,7 @@ function (angular, _, kbn) { this.validateVariableSelectionState = function(variable) { if (!variable.current) { if (!variable.options.length) { return; } - return self.setVariableValue(variable, variable.options[0]); + return self.setVariableValue(variable, variable.options[0], true); } if (_.isArray(variable.current.value)) { @@ -204,7 +201,7 @@ function (angular, _, kbn) { } else { var currentOption = _.findWhere(variable.options, { text: variable.current.text }); if (currentOption) { - return self.setVariableValue(variable, currentOption); + return self.setVariableValue(variable, currentOption, true); } else { if (!variable.options.length) { return; } return self.setVariableValue(variable, variable.options[0]); From 6fac471415c15c5a19dbc06df2e1c91d8b73d6f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Tue, 8 Mar 2016 10:41:20 +0100 Subject: [PATCH 4/4] feat(templating): fixed failing unit tests in PR #4287 --- public/test/specs/templateValuesSrv-specs.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/public/test/specs/templateValuesSrv-specs.js b/public/test/specs/templateValuesSrv-specs.js index 0eaa9d1d99f..d81ce4fd059 100644 --- a/public/test/specs/templateValuesSrv-specs.js +++ b/public/test/specs/templateValuesSrv-specs.js @@ -34,12 +34,13 @@ define([ options: [{text: "test", value: "test"}] }; - beforeEach(function() { + beforeEach(function(done) { var dashboard = { templating: { list: [variable] } }; var urlParams = {}; urlParams["var-apps"] = "new"; ctx.$location.search = sinon.stub().returns(urlParams); - ctx.service.init(dashboard); + ctx.service.init(dashboard).then(function() { done(); }); + ctx.$rootScope.$digest(); }); it('should update current value', function() { @@ -56,12 +57,13 @@ define([ options: [{text: "val1", value: "val1"}, {text: 'val2', value: 'val2'}, {text: 'val3', value: 'val3', selected: true}] }; - beforeEach(function() { + beforeEach(function(done) { var dashboard = { templating: { list: [variable] } }; var urlParams = {}; urlParams["var-apps"] = ["val2", "val1"]; ctx.$location.search = sinon.stub().returns(urlParams); - ctx.service.init(dashboard); + ctx.service.init(dashboard).then(function() { done(); }); + ctx.$rootScope.$digest(); }); it('should update current value', function() {