From 6da7ed970611529f30dc72adf482d10f28b071d4 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 8 Jun 2016 18:04:31 +0200 Subject: [PATCH 1/7] Add support for canceling in-flight requests Repeat requests, if they already exist in the in-flight request map, will cause the previous request to cancel. The implementation of the unique key is the responsibility of individual datasources. --- public/app/core/services/backend_srv.js | 34 ++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/public/app/core/services/backend_srv.js b/public/app/core/services/backend_srv.js index f27e427c70b..f1ab7b5824b 100644 --- a/public/app/core/services/backend_srv.js +++ b/public/app/core/services/backend_srv.js @@ -7,7 +7,7 @@ define([ function (angular, _, coreModule, config) { 'use strict'; - coreModule.default.service('backendSrv', function($http, alertSrv, $timeout) { + coreModule.default.service('backendSrv', function($http, alertSrv, $timeout, $q) { var self = this; this.get = function(url, params) { @@ -91,8 +91,25 @@ function (angular, _, coreModule, config) { }); }; + var datasourceInFlightRequests = {}; + var HTTP_REQUEST_ABORTED = -1; this.datasourceRequest = function(options) { options.retry = options.retry || 0; + + // A cacheKey is provided by the datasource as a unique identifier for a + // particular query. If the cacheKey exists, the promise it is keyed to + // is canceled, canceling the previous datasource request if it is still + // in-flight. + var canceler; + if (options.cacheKey) { + if (canceler = datasourceInFlightRequests[options.cacheKey]) { + canceler.resolve(); + } + canceler = $q.defer(); + options.timeout = canceler.promise; + datasourceInFlightRequests[options.cacheKey] = canceler; + } + var requestIsLocal = options.url.indexOf('/') === 0; var firstAttempt = options.retry === 0; @@ -102,10 +119,25 @@ function (angular, _, coreModule, config) { } return $http(options).then(null, function(err) { + if (err.status === HTTP_REQUEST_ABORTED) { + // Need to return the right data structure so it has no effect on + // iterating over returned data in datasource.ts#115 + // TODO: Hitting another refresh cancels the "loading" animation on + // panes. Figure out how to keep it going. + return { + data: { + data: { + result: [] + } + } + }; + } + // handle unauthorized for backend requests if (requestIsLocal && firstAttempt && err.status === 401) { return self.loginPing().then(function() { options.retry = 1; + canceler.resolve(); return self.datasourceRequest(options); }); } From 18f9f6c159e7d4ee23f58e7def05e6754258712c Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Wed, 8 Jun 2016 18:05:46 +0200 Subject: [PATCH 2/7] Add in-flight identifier for Prometheus requests Repeat Prometheus requests with the same query will cancel preceding requests. --- public/app/plugins/datasource/prometheus/datasource.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/public/app/plugins/datasource/prometheus/datasource.ts b/public/app/plugins/datasource/prometheus/datasource.ts index cbd7b1ba24b..d4231c4867d 100644 --- a/public/app/plugins/datasource/prometheus/datasource.ts +++ b/public/app/plugins/datasource/prometheus/datasource.ts @@ -21,10 +21,11 @@ export function PrometheusDatasource(instanceSettings, $q, backendSrv, templateS this.withCredentials = instanceSettings.withCredentials; this.lastErrors = {}; - this._request = function(method, url) { + this._request = function(method, url, cacheKey) { var options: any = { url: this.url + url, - method: method + method: method, + cacheKey: cacheKey }; if (this.basicAuth || this.withCredentials) { @@ -122,7 +123,7 @@ export function PrometheusDatasource(instanceSettings, $q, backendSrv, templateS this.performTimeSeriesQuery = function(query, start, end) { var url = '/api/v1/query_range?query=' + encodeURIComponent(query.expr) + '&start=' + start + '&end=' + end + '&step=' + query.step; - return this._request('GET', url); + return this._request('GET', url, query.expr.toString()); }; this.performSuggestQuery = function(query) { From bc7c2cd3f5ce78fa91865973f6510cbefc3fd880 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Thu, 9 Jun 2016 11:36:46 +0200 Subject: [PATCH 3/7] Create cacheKey at top-level Responsibility is now to pass the cacheKey through to `datasourceRequest` in each datasources' implementation of `query`. --- public/app/features/panel/metrics_panel_ctrl.ts | 4 ++++ public/app/plugins/datasource/prometheus/datasource.ts | 5 +++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/public/app/features/panel/metrics_panel_ctrl.ts b/public/app/features/panel/metrics_panel_ctrl.ts index d3805d5f44f..e20df095d22 100644 --- a/public/app/features/panel/metrics_panel_ctrl.ts +++ b/public/app/features/panel/metrics_panel_ctrl.ts @@ -182,6 +182,10 @@ class MetricsPanelCtrl extends PanelCtrl { cacheTimeout: this.panel.cacheTimeout }; + metricsQuery.targets.forEach(function(target) { + target.cacheKey = target.expr + target.refId + metricsQuery.panelId; + }); + return datasource.query(metricsQuery); } diff --git a/public/app/plugins/datasource/prometheus/datasource.ts b/public/app/plugins/datasource/prometheus/datasource.ts index d4231c4867d..295ea05bb8f 100644 --- a/public/app/plugins/datasource/prometheus/datasource.ts +++ b/public/app/plugins/datasource/prometheus/datasource.ts @@ -25,7 +25,7 @@ export function PrometheusDatasource(instanceSettings, $q, backendSrv, templateS var options: any = { url: this.url + url, method: method, - cacheKey: cacheKey + cacheKey: cacheKey, }; if (this.basicAuth || this.withCredentials) { @@ -76,6 +76,7 @@ export function PrometheusDatasource(instanceSettings, $q, backendSrv, templateS var query: any = {}; query.expr = templateSrv.replace(target.expr, options.scopedVars, self.interpolateQueryExpr); + query.cacheKey = target.cacheKey; var interval = target.interval || options.interval; var intervalFactor = target.intervalFactor || 1; @@ -123,7 +124,7 @@ export function PrometheusDatasource(instanceSettings, $q, backendSrv, templateS this.performTimeSeriesQuery = function(query, start, end) { var url = '/api/v1/query_range?query=' + encodeURIComponent(query.expr) + '&start=' + start + '&end=' + end + '&step=' + query.step; - return this._request('GET', url, query.expr.toString()); + return this._request('GET', url, query.cacheKey); }; this.performSuggestQuery = function(query) { From efdb990e56e218fe45e9dec82e5ea916e3615c02 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Thu, 9 Jun 2016 12:13:17 +0200 Subject: [PATCH 4/7] Return an error for a canceled request. Allow the datasource to decide how to handle a canceled request. --- public/app/core/services/backend_srv.js | 18 ++++++------------ .../datasource/prometheus/datasource.ts | 4 ++++ 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/public/app/core/services/backend_srv.js b/public/app/core/services/backend_srv.js index f1ab7b5824b..71e4b50d51e 100644 --- a/public/app/core/services/backend_srv.js +++ b/public/app/core/services/backend_srv.js @@ -120,21 +120,15 @@ function (angular, _, coreModule, config) { return $http(options).then(null, function(err) { if (err.status === HTTP_REQUEST_ABORTED) { - // Need to return the right data structure so it has no effect on - // iterating over returned data in datasource.ts#115 - // TODO: Hitting another refresh cancels the "loading" animation on - // panes. Figure out how to keep it going. - return { - data: { - data: { - result: [] - } - } - }; + // TODO: Hitting refresh before the original request returns cancels + // the "loading" animation on the panes, but it should continue to be + // visible. + err.statusText = "request aborted"; + return err; } // handle unauthorized for backend requests - if (requestIsLocal && firstAttempt && err.status === 401) { + if (requestIsLocal && firstAttempt && err.status === 401) { return self.loginPing().then(function() { options.retry = 1; canceler.resolve(); diff --git a/public/app/plugins/datasource/prometheus/datasource.ts b/public/app/plugins/datasource/prometheus/datasource.ts index 295ea05bb8f..0e65ba6fb22 100644 --- a/public/app/plugins/datasource/prometheus/datasource.ts +++ b/public/app/plugins/datasource/prometheus/datasource.ts @@ -58,6 +58,7 @@ export function PrometheusDatasource(instanceSettings, $q, backendSrv, templateS return escapedValues.join('|'); }; + var HTTP_REQUEST_ABORTED = -1; // Called once per panel (graph) this.query = function(options) { var self = this; @@ -107,6 +108,9 @@ export function PrometheusDatasource(instanceSettings, $q, backendSrv, templateS var result = []; _.each(allResponse, function(response, index) { + if (response.status === HTTP_REQUEST_ABORTED) { + return; + } if (response.status === 'error') { self.lastErrors.query = response.error; throw response.error; From cc64d65c2f13fa28cea03c47630e07415f7ea0b5 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Thu, 9 Jun 2016 12:57:55 +0200 Subject: [PATCH 5/7] Rename cacheKey to exprID/requestID Depending on context in the code, rename cacheKey to exprID when it identifies a unique expression, and rename cacheKey to requestID when it is identifying a request. --- public/app/core/services/backend_srv.js | 10 +++++----- public/app/features/panel/metrics_panel_ctrl.ts | 2 +- public/app/plugins/datasource/prometheus/datasource.ts | 8 ++++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/public/app/core/services/backend_srv.js b/public/app/core/services/backend_srv.js index 71e4b50d51e..5ba40dfc86b 100644 --- a/public/app/core/services/backend_srv.js +++ b/public/app/core/services/backend_srv.js @@ -96,18 +96,18 @@ function (angular, _, coreModule, config) { this.datasourceRequest = function(options) { options.retry = options.retry || 0; - // A cacheKey is provided by the datasource as a unique identifier for a - // particular query. If the cacheKey exists, the promise it is keyed to + // A requestID is provided by the datasource as a unique identifier for a + // particular query. If the requestID exists, the promise it is keyed to // is canceled, canceling the previous datasource request if it is still // in-flight. var canceler; - if (options.cacheKey) { - if (canceler = datasourceInFlightRequests[options.cacheKey]) { + if (options.requestID) { + if (canceler = datasourceInFlightRequests[options.requestID]) { canceler.resolve(); } canceler = $q.defer(); options.timeout = canceler.promise; - datasourceInFlightRequests[options.cacheKey] = canceler; + datasourceInFlightRequests[options.requestID] = canceler; } var requestIsLocal = options.url.indexOf('/') === 0; diff --git a/public/app/features/panel/metrics_panel_ctrl.ts b/public/app/features/panel/metrics_panel_ctrl.ts index e20df095d22..2a72ef4b24e 100644 --- a/public/app/features/panel/metrics_panel_ctrl.ts +++ b/public/app/features/panel/metrics_panel_ctrl.ts @@ -183,7 +183,7 @@ class MetricsPanelCtrl extends PanelCtrl { }; metricsQuery.targets.forEach(function(target) { - target.cacheKey = target.expr + target.refId + metricsQuery.panelId; + target.exprID = target.expr + target.refId + metricsQuery.panelId; }); return datasource.query(metricsQuery); diff --git a/public/app/plugins/datasource/prometheus/datasource.ts b/public/app/plugins/datasource/prometheus/datasource.ts index 0e65ba6fb22..3548b4fedb2 100644 --- a/public/app/plugins/datasource/prometheus/datasource.ts +++ b/public/app/plugins/datasource/prometheus/datasource.ts @@ -21,11 +21,11 @@ export function PrometheusDatasource(instanceSettings, $q, backendSrv, templateS this.withCredentials = instanceSettings.withCredentials; this.lastErrors = {}; - this._request = function(method, url, cacheKey) { + this._request = function(method, url, requestID) { var options: any = { url: this.url + url, method: method, - cacheKey: cacheKey, + requestID: requestID, }; if (this.basicAuth || this.withCredentials) { @@ -77,7 +77,7 @@ export function PrometheusDatasource(instanceSettings, $q, backendSrv, templateS var query: any = {}; query.expr = templateSrv.replace(target.expr, options.scopedVars, self.interpolateQueryExpr); - query.cacheKey = target.cacheKey; + query.requestID = target.exprID; var interval = target.interval || options.interval; var intervalFactor = target.intervalFactor || 1; @@ -128,7 +128,7 @@ export function PrometheusDatasource(instanceSettings, $q, backendSrv, templateS this.performTimeSeriesQuery = function(query, start, end) { var url = '/api/v1/query_range?query=' + encodeURIComponent(query.expr) + '&start=' + start + '&end=' + end + '&step=' + query.step; - return this._request('GET', url, query.cacheKey); + return this._request('GET', url, query.requestID); }; this.performSuggestQuery = function(query) { From 8797be9f896c73bf94ad9ca51043568f1fad17f8 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Thu, 9 Jun 2016 15:16:53 +0200 Subject: [PATCH 6/7] Add existence check for canceler. --- public/app/core/services/backend_srv.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/public/app/core/services/backend_srv.js b/public/app/core/services/backend_srv.js index 5ba40dfc86b..3c96e3c69b4 100644 --- a/public/app/core/services/backend_srv.js +++ b/public/app/core/services/backend_srv.js @@ -131,7 +131,9 @@ function (angular, _, coreModule, config) { if (requestIsLocal && firstAttempt && err.status === 401) { return self.loginPing().then(function() { options.retry = 1; - canceler.resolve(); + if (canceler) { + canceler.resolve(); + } return self.datasourceRequest(options); }); } From 048bcf19f662de737dcd4041870248f7a78afbb1 Mon Sep 17 00:00:00 2001 From: stuart nelson Date: Thu, 9 Jun 2016 15:17:17 +0200 Subject: [PATCH 7/7] Use panel+ref for unique id Previously, if a user changed the query between requests, the previous query would not be canceled. This handles that edge-case. --- public/app/features/panel/metrics_panel_ctrl.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/public/app/features/panel/metrics_panel_ctrl.ts b/public/app/features/panel/metrics_panel_ctrl.ts index 2a72ef4b24e..4d0fddf58ec 100644 --- a/public/app/features/panel/metrics_panel_ctrl.ts +++ b/public/app/features/panel/metrics_panel_ctrl.ts @@ -183,7 +183,7 @@ class MetricsPanelCtrl extends PanelCtrl { }; metricsQuery.targets.forEach(function(target) { - target.exprID = target.expr + target.refId + metricsQuery.panelId; + target.exprID = target.refId + metricsQuery.panelId; }); return datasource.query(metricsQuery);