From 8ef260972d2f0197e2b72689c62e0f49b242498b Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Wed, 29 Aug 2018 13:50:21 +0200 Subject: [PATCH 1/5] add to log when overviews are being used in dataviews --- lib/cartodb/backends/dataview.js | 8 +++++++- lib/cartodb/models/dataview/base.js | 5 +++-- lib/cartodb/models/dataview/overviews/aggregation.js | 2 +- lib/cartodb/models/dataview/overviews/formula.js | 2 +- lib/cartodb/models/dataview/overviews/histogram.js | 2 +- 5 files changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index a2f5327a..4f50121f 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -64,7 +64,13 @@ DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, param return callback(err); } - return callback(null, dataviewResult); + const stats = {}; + + if (dataviewResult && dataviewResult.usesOverviews) { + stats.usesOverviews = dataviewResult.usesOverviews; + } + + return callback(null, dataviewResult, stats); }); }); }; diff --git a/lib/cartodb/models/dataview/base.js b/lib/cartodb/models/dataview/base.js index 80e021b1..13743f5b 100644 --- a/lib/cartodb/models/dataview/base.js +++ b/lib/cartodb/models/dataview/base.js @@ -21,7 +21,7 @@ function getPGTypeName (pgType) { module.exports = class BaseDataview { getResult (psql, override, callback) { - this.sql(psql, override, (err, query) => { + this.sql(psql, override, (err, query, flags = { usesOverviews : false }) => { if (err) { return callback(err); } @@ -33,7 +33,8 @@ module.exports = class BaseDataview { result = this.format(result, override); result.type = this.getType(); - + result.usesOverviews = flags.usesOverviews; + return callback(null, result); }, true); // use read-only transaction diff --git a/lib/cartodb/models/dataview/overviews/aggregation.js b/lib/cartodb/models/dataview/overviews/aggregation.js index 5df092f4..312a398e 100644 --- a/lib/cartodb/models/dataview/overviews/aggregation.js +++ b/lib/cartodb/models/dataview/overviews/aggregation.js @@ -209,7 +209,7 @@ Aggregation.prototype.sql = function(psql, override, callback) { debug(aggregationSql); - return callback(null, aggregationSql); + return callback(null, aggregationSql, { usesOverviews: true }); }; var aggregationFnQueryTpl = { diff --git a/lib/cartodb/models/dataview/overviews/formula.js b/lib/cartodb/models/dataview/overviews/formula.js index 2a97061e..dd487ccb 100644 --- a/lib/cartodb/models/dataview/overviews/formula.js +++ b/lib/cartodb/models/dataview/overviews/formula.js @@ -74,5 +74,5 @@ Formula.prototype.sql = function (psql, override, callback) { debug(formulaSql); - return callback(null, formulaSql); + return callback(null, formulaSql, { usesOverviews: true }); }; diff --git a/lib/cartodb/models/dataview/overviews/histogram.js b/lib/cartodb/models/dataview/overviews/histogram.js index 6674f6a0..cc7e7342 100644 --- a/lib/cartodb/models/dataview/overviews/histogram.js +++ b/lib/cartodb/models/dataview/overviews/histogram.js @@ -178,7 +178,7 @@ Histogram.prototype.sql = function(psql, override, callback) { var histogramSql = this._buildQuery(override); - return callback(null, histogramSql); + return callback(null, histogramSql, { usesOverviews: true }); }; Histogram.prototype._buildQuery = function (override) { From 732f891850de07538b97490f3411d136d989de59 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Wed, 29 Aug 2018 15:06:48 +0200 Subject: [PATCH 2/5] refactor in order not to tamper the dataview results data --- lib/cartodb/backends/dataview.js | 8 +------- lib/cartodb/models/dataview/base.js | 13 +++++++++---- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index 4f50121f..99a115e6 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -59,17 +59,11 @@ DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, param return callback(error); } - dataview.getResult(pg, overrideParams, function (err, dataviewResult) { + dataview.getResult(pg, overrideParams, function (err, dataviewResult, stats = {}) { if (err) { return callback(err); } - const stats = {}; - - if (dataviewResult && dataviewResult.usesOverviews) { - stats.usesOverviews = dataviewResult.usesOverviews; - } - return callback(null, dataviewResult, stats); }); }); diff --git a/lib/cartodb/models/dataview/base.js b/lib/cartodb/models/dataview/base.js index 13743f5b..366a458c 100644 --- a/lib/cartodb/models/dataview/base.js +++ b/lib/cartodb/models/dataview/base.js @@ -21,7 +21,7 @@ function getPGTypeName (pgType) { module.exports = class BaseDataview { getResult (psql, override, callback) { - this.sql(psql, override, (err, query, flags = { usesOverviews : false }) => { + this.sql(psql, override, (err, query, flags = null) => { if (err) { return callback(err); } @@ -33,10 +33,15 @@ module.exports = class BaseDataview { result = this.format(result, override); result.type = this.getType(); - result.usesOverviews = flags.usesOverviews; - - return callback(null, result); + //Overviews looging + const stats = {}; + + if (flags && flags.usesOverviews) { + stats.usesOverviews = true; + } + + return callback(null, result, stats); }, true); // use read-only transaction }); } From c9d50c412d907f44bfeb7e4f77801d319410d92a Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Wed, 29 Aug 2018 18:00:13 +0200 Subject: [PATCH 3/5] add tests for dataviews --- lib/cartodb/models/dataview/base.js | 2 +- test/acceptance/dataviews/overviews.js | 29 +++++++++++++++++++------- test/support/test-client.js | 7 +++---- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/lib/cartodb/models/dataview/base.js b/lib/cartodb/models/dataview/base.js index 366a458c..40f6c612 100644 --- a/lib/cartodb/models/dataview/base.js +++ b/lib/cartodb/models/dataview/base.js @@ -34,7 +34,7 @@ module.exports = class BaseDataview { result = this.format(result, override); result.type = this.getType(); - //Overviews looging + //Overviews logging const stats = {}; if (flags && flags.usesOverviews) { diff --git a/test/acceptance/dataviews/overviews.js b/test/acceptance/dataviews/overviews.js index 552b9ac6..9cf7bf0f 100644 --- a/test/acceptance/dataviews/overviews.js +++ b/test/acceptance/dataviews/overviews.js @@ -48,11 +48,12 @@ describe('dataviews using tables without overviews', function() { it("should expose a formula", function(done) { var testClient = new TestClient(nonOverviewsMapConfig); - testClient.getDataview('country_places_count', { own_filter: 0 }, function(err, formula_result) { + testClient.getDataview('country_places_count', { own_filter: 0 }, function(err, formula_result, headers) { if (err) { return done(err); } assert.deepEqual(formula_result, { operation: 'count', result: 7313, nulls: 0, type: 'formula' }); + assert(getUsesOverviewsFromHeaders(headers) === undefined); //Overviews logging testClient.drain(done); }); @@ -257,7 +258,7 @@ describe('dataviews using tables with overviews', function() { it("should expose a sum formula", function(done) { var testClient = new TestClient(overviewsMapConfig); - testClient.getDataview('test_sum', { own_filter: 0 }, function(err, formula_result) { + testClient.getDataview('test_sum', { own_filter: 0 }, function(err, formula_result, headers) { if (err) { return done(err); } @@ -269,6 +270,7 @@ describe('dataviews using tables with overviews', function() { "nulls":0, "type":"formula" }); + assert.ok(getUsesOverviewsFromHeaders(headers)); //Overviews logging testClient.drain(done); }); @@ -276,7 +278,7 @@ describe('dataviews using tables with overviews', function() { it("should expose an avg formula", function(done) { var testClient = new TestClient(overviewsMapConfig); - testClient.getDataview('test_avg', { own_filter: 0 }, function(err, formula_result) { + testClient.getDataview('test_avg', { own_filter: 0 }, function(err, formula_result, headers) { if (err) { return done(err); } @@ -288,6 +290,7 @@ describe('dataviews using tables with overviews', function() { "infinities": 0, "nans": 0 }); + assert.ok(getUsesOverviewsFromHeaders(headers)); //Overviews logging testClient.drain(done); }); @@ -295,7 +298,7 @@ describe('dataviews using tables with overviews', function() { it("should expose a count formula", function(done) { var testClient = new TestClient(overviewsMapConfig); - testClient.getDataview('test_count', { own_filter: 0 }, function(err, formula_result) { + testClient.getDataview('test_count', { own_filter: 0 }, function(err, formula_result, headers) { if (err) { return done(err); } @@ -307,6 +310,7 @@ describe('dataviews using tables with overviews', function() { "infinities": 0, "nans": 0 }); + assert.ok(getUsesOverviewsFromHeaders(headers)); //Overviews logging testClient.drain(done); }); @@ -374,13 +378,14 @@ describe('dataviews using tables with overviews', function() { it("should expose a histogram", function (done) { var testClient = new TestClient(overviewsMapConfig); - testClient.getDataview('test_histogram', function (err, histogram) { + testClient.getDataview('test_histogram', function (err, histogram, headers) { if (err) { return done(err); } assert.ok(histogram); assert.equal(histogram.type, 'histogram'); assert.ok(Array.isArray(histogram.bins)); + assert.ok(getUsesOverviewsFromHeaders(headers)); //Overviews logging testClient.drain(done); }); }); @@ -462,7 +467,7 @@ describe('dataviews using tables with overviews', function() { it("should expose a filtered sum formula", function (done) { var testClient = new TestClient(overviewsMapConfig); - testClient.getDataview('test_sum', params, function (err, formula_result) { + testClient.getDataview('test_sum', params, function (err, formula_result, headers) { if (err) { return done(err); } @@ -474,13 +479,14 @@ describe('dataviews using tables with overviews', function() { "nans": 0, "type":"formula" }); + assert.ok(getUsesOverviewsFromHeaders(headers)); //Overviews logging testClient.drain(done); }); }); it("should expose a filtered avg formula", function(done) { var testClient = new TestClient(overviewsMapConfig); - testClient.getDataview('test_avg', params, function(err, formula_result) { + testClient.getDataview('test_avg', params, function(err, formula_result, headers) { if (err) { return done(err); } @@ -492,6 +498,7 @@ describe('dataviews using tables with overviews', function() { "nans": 0, "type":"formula" }); + assert.ok(getUsesOverviewsFromHeaders(headers)); //Overviews logging testClient.drain(done); }); @@ -499,7 +506,7 @@ describe('dataviews using tables with overviews', function() { it("should expose a filtered count formula", function(done) { var testClient = new TestClient(overviewsMapConfig); - testClient.getDataview('test_count', params, function(err, formula_result) { + testClient.getDataview('test_count', params, function(err, formula_result, headers) { if (err) { return done(err); } @@ -511,6 +518,7 @@ describe('dataviews using tables with overviews', function() { "nulls":0, "type":"formula" }); + assert.ok(getUsesOverviewsFromHeaders(headers)); //Overviews logging testClient.drain(done); }); @@ -647,3 +655,8 @@ describe('dataviews using tables with overviews', function() { }); }); }); + + +function getUsesOverviewsFromHeaders(headers) { + return headers && headers['x-tiler-profiler'] && JSON.parse(headers['x-tiler-profiler']).usesOverviews; +} diff --git a/test/support/test-client.js b/test/support/test-client.js index db014c9a..e3172cdf 100644 --- a/test/support/test-client.js +++ b/test/support/test-client.js @@ -509,16 +509,15 @@ TestClient.prototype.getDataview = function(dataviewName, params, callback) { if (err) { return next(err); } - - next(null, JSON.parse(res.body)); + next(null, JSON.parse(res.body), res.headers); } ); }, - function finish(err, dataview) { + function finish(err, dataview, headers = null) { if (err) { return callback(err); } - return callback(null, dataview); + return callback(null, dataview, headers); } ); }; From 7c52f504e59aa75a2e43e87e7764b5f110b20768 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Thu, 30 Aug 2018 14:30:03 +0200 Subject: [PATCH 4/5] add dataview type to overviews logs --- lib/cartodb/models/dataview/base.js | 3 +++ test/acceptance/dataviews/overviews.js | 17 +++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/lib/cartodb/models/dataview/base.js b/lib/cartodb/models/dataview/base.js index 40f6c612..22cdc23b 100644 --- a/lib/cartodb/models/dataview/base.js +++ b/lib/cartodb/models/dataview/base.js @@ -39,6 +39,9 @@ module.exports = class BaseDataview { if (flags && flags.usesOverviews) { stats.usesOverviews = true; + if (this.getType) { + stats.dataviewType = this.getType(); + } } return callback(null, result, stats); diff --git a/test/acceptance/dataviews/overviews.js b/test/acceptance/dataviews/overviews.js index 9cf7bf0f..cfe5a7fc 100644 --- a/test/acceptance/dataviews/overviews.js +++ b/test/acceptance/dataviews/overviews.js @@ -271,6 +271,7 @@ describe('dataviews using tables with overviews', function() { "type":"formula" }); assert.ok(getUsesOverviewsFromHeaders(headers)); //Overviews logging + assert(getDataviewTypeFromHeaders(headers) === 'formula'); //Overviews logging testClient.drain(done); }); @@ -291,6 +292,7 @@ describe('dataviews using tables with overviews', function() { "nans": 0 }); assert.ok(getUsesOverviewsFromHeaders(headers)); //Overviews logging + assert(getDataviewTypeFromHeaders(headers) === 'formula'); //Overviews logging testClient.drain(done); }); @@ -311,6 +313,7 @@ describe('dataviews using tables with overviews', function() { "nans": 0 }); assert.ok(getUsesOverviewsFromHeaders(headers)); //Overviews logging + assert(getDataviewTypeFromHeaders(headers) === 'formula'); //Overviews logging testClient.drain(done); }); @@ -386,6 +389,8 @@ describe('dataviews using tables with overviews', function() { assert.equal(histogram.type, 'histogram'); assert.ok(Array.isArray(histogram.bins)); assert.ok(getUsesOverviewsFromHeaders(headers)); //Overviews logging + assert(getDataviewTypeFromHeaders(headers) === 'histogram'); //Overviews logging + testClient.drain(done); }); }); @@ -594,10 +599,11 @@ describe('dataviews using tables with overviews', function() { it("should expose an aggregation dataview filtering special float values out", function (done) { var testClient = new TestClient(overviewsMapConfig); - testClient.getDataview('test_categories_special_values', params, function (err, dataview) { + testClient.getDataview('test_categories_special_values', params, function (err, dataview, headers) { if (err) { return done(err); } + assert.deepEqual(dataview, { aggregation: 'sum', count: 5, @@ -610,6 +616,10 @@ describe('dataviews using tables with overviews', function() { categories: [ { category: 'Hawai', value: 6, agg: false } ], type: 'aggregation' }); + + assert.ok(getUsesOverviewsFromHeaders(headers)); //Overviews logging + assert(getDataviewTypeFromHeaders(headers) === 'aggregation'); //Overviews logging + testClient.drain(done); }); }); @@ -656,7 +666,10 @@ describe('dataviews using tables with overviews', function() { }); }); - function getUsesOverviewsFromHeaders(headers) { return headers && headers['x-tiler-profiler'] && JSON.parse(headers['x-tiler-profiler']).usesOverviews; } + +function getDataviewTypeFromHeaders(headers) { + return headers && headers['x-tiler-profiler'] && JSON.parse(headers['x-tiler-profiler']).dataviewType; +} From 95d179835c48d4233b49b13c385f89df1c5b777f Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Thu, 30 Aug 2018 14:52:37 +0200 Subject: [PATCH 5/5] add to logs even if no overviews tables were used. {usesOverviews:false} --- lib/cartodb/models/dataview/base.js | 15 +++++++++------ test/acceptance/dataviews/overviews.js | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/cartodb/models/dataview/base.js b/lib/cartodb/models/dataview/base.js index 22cdc23b..455461a2 100644 --- a/lib/cartodb/models/dataview/base.js +++ b/lib/cartodb/models/dataview/base.js @@ -37,13 +37,16 @@ module.exports = class BaseDataview { //Overviews logging const stats = {}; - if (flags && flags.usesOverviews) { - stats.usesOverviews = true; - if (this.getType) { - stats.dataviewType = this.getType(); - } + if (flags && flags.usesOverviews !== undefined) { + stats.usesOverviews = flags.usesOverviews; + } else { + stats.usesOverviews = false; } - + + if (this.getType) { + stats.dataviewType = this.getType(); + } + return callback(null, result, stats); }, true); // use read-only transaction }); diff --git a/test/acceptance/dataviews/overviews.js b/test/acceptance/dataviews/overviews.js index cfe5a7fc..3487a08b 100644 --- a/test/acceptance/dataviews/overviews.js +++ b/test/acceptance/dataviews/overviews.js @@ -53,7 +53,7 @@ describe('dataviews using tables without overviews', function() { return done(err); } assert.deepEqual(formula_result, { operation: 'count', result: 7313, nulls: 0, type: 'formula' }); - assert(getUsesOverviewsFromHeaders(headers) === undefined); //Overviews logging + assert(getUsesOverviewsFromHeaders(headers) === false); //Overviews logging testClient.drain(done); });