From 62d66f2dbc3b741846153d4429177cc8e473621f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 2 Jun 2020 09:00:45 +0200 Subject: [PATCH 01/18] Do not use global logger in middlewares, use the one initialized in res.locals instead --- lib/api/api-router.js | 3 +-- lib/api/map/anonymous-map-controller.js | 1 - lib/api/map/preview-template-controller.js | 11 +++-------- lib/api/middlewares/cache-channel-header.js | 4 ++-- lib/api/middlewares/cache-control-header.js | 4 ++-- lib/api/middlewares/increment-map-view-count.js | 4 ++-- lib/api/middlewares/last-modified-header.js | 4 ++-- lib/api/middlewares/logger.js | 6 +++--- lib/api/middlewares/metrics.js | 4 +++- lib/api/middlewares/surrogate-key-header.js | 4 ++-- lib/api/template/named-template-controller.js | 1 - 11 files changed, 20 insertions(+), 26 deletions(-) diff --git a/lib/api/api-router.js b/lib/api/api-router.js index 9ce4fa2d..4356df5d 100644 --- a/lib/api/api-router.js +++ b/lib/api/api-router.js @@ -86,8 +86,7 @@ module.exports = class ApiRouter { const { rendererCache, tileBackend, attributesBackend, previewBackend, mapBackend, mapStore } = windshaftFactory({ rendererOptions: serverOptions, redisPool, - onTileErrorStrategy: getOnTileErrorStrategy({ enabled: environmentOptions.enabledFeatures.onTileErrorStrategy }), - logger: global.logger + onTileErrorStrategy: getOnTileErrorStrategy({ enabled: environmentOptions.enabledFeatures.onTileErrorStrategy }) }); const rendererStatsReporter = new RendererStatsReporter(rendererCache, serverOptions.renderCache.statsInterval); diff --git a/lib/api/map/anonymous-map-controller.js b/lib/api/map/anonymous-map-controller.js index 46ed876a..85e9860a 100644 --- a/lib/api/map/anonymous-map-controller.js +++ b/lib/api/map/anonymous-map-controller.js @@ -95,7 +95,6 @@ module.exports = class AnonymousMapController { metrics({ enabled: this.config.pubSubMetrics.enabled, metricsBackend: this.metricsBackend, - logger: global.logger, tags: metricsTags }), credentials(), diff --git a/lib/api/map/preview-template-controller.js b/lib/api/map/preview-template-controller.js index e9dc547d..92d4380e 100644 --- a/lib/api/map/preview-template-controller.js +++ b/lib/api/map/preview-template-controller.js @@ -70,7 +70,6 @@ module.exports = class PreviewTemplateController { metrics({ enabled: this.config.pubSubMetrics.enabled, metricsBackend: this.metricsBackend, - logger: global.logger, tags: metricsTags }), credentials(), @@ -337,17 +336,13 @@ function setContentTypeHeader () { }; } -function incrementMapViewsError (ctx) { - return `ERROR: failed to increment mapview count for user '${ctx.user}': ${ctx.err}`; -} - function incrementMapViews ({ metadataBackend }) { return function incrementMapViewsMiddleware (req, res, next) { - const { user, mapConfigProvider } = res.locals; + const { user, mapConfigProvider, logger } = res.locals; mapConfigProvider.getMapConfig((err, mapConfig) => { if (err) { - global.logger.info(incrementMapViewsError({ user, err })); + logger.warn(err, `ERROR: failed to increment mapview count for user '${user}'`); return next(); } @@ -359,7 +354,7 @@ function incrementMapViews ({ metadataBackend }) { metadataBackend.incMapviewCount(user, statTag, (err) => { if (err) { - global.logger.info(incrementMapViewsError({ user, err })); + logger.warn(err, `ERROR: failed to increment mapview count for user '${user}'`); } next(); diff --git a/lib/api/middlewares/cache-channel-header.js b/lib/api/middlewares/cache-channel-header.js index c93cce57..66486392 100644 --- a/lib/api/middlewares/cache-channel-header.js +++ b/lib/api/middlewares/cache-channel-header.js @@ -6,11 +6,11 @@ module.exports = function setCacheChannelHeader () { return next(); } - const { mapConfigProvider } = res.locals; + const { mapConfigProvider, logger } = res.locals; mapConfigProvider.getAffectedTables((err, affectedTables) => { if (err) { - global.logger.warn(err, 'ERROR generating Cache Channel Header'); + logger.warn(err, 'ERROR generating Cache Channel Header'); return next(); } diff --git a/lib/api/middlewares/cache-control-header.js b/lib/api/middlewares/cache-control-header.js index 7afed19f..18ec7728 100644 --- a/lib/api/middlewares/cache-control-header.js +++ b/lib/api/middlewares/cache-control-header.js @@ -40,11 +40,11 @@ module.exports = function setCacheControlHeader ({ return next(); } - const { mapConfigProvider = { getAffectedTables: callback => callback() } } = res.locals; + const { mapConfigProvider = { getAffectedTables: callback => callback() }, logger } = res.locals; mapConfigProvider.getAffectedTables((err, affectedTables) => { if (err) { - global.logger.warn(err, 'ERROR generating Cache Control Header'); + logger.warn(err, 'ERROR generating Cache Control Header'); return next(); } diff --git a/lib/api/middlewares/increment-map-view-count.js b/lib/api/middlewares/increment-map-view-count.js index 7617375f..1b71e8e3 100644 --- a/lib/api/middlewares/increment-map-view-count.js +++ b/lib/api/middlewares/increment-map-view-count.js @@ -2,7 +2,7 @@ module.exports = function incrementMapViewCount (metadataBackend) { return function incrementMapViewCountMiddleware (req, res, next) { - const { mapConfig, user } = res.locals; + const { mapConfig, user, logger } = res.locals; const statTag = mapConfig.obj().stat_tag; if (statTag) { @@ -14,7 +14,7 @@ module.exports = function incrementMapViewCount (metadataBackend) { req.profiler.done('incMapviewCount'); if (err) { - global.logger.warn(err, `ERROR: failed to increment mapview count for user '${user}'`); + logger.warn(err, `ERROR: failed to increment mapview count for user '${user}'`); } next(); diff --git a/lib/api/middlewares/last-modified-header.js b/lib/api/middlewares/last-modified-header.js index 8f1dd296..b68d7c31 100644 --- a/lib/api/middlewares/last-modified-header.js +++ b/lib/api/middlewares/last-modified-header.js @@ -6,7 +6,7 @@ module.exports = function setLastModifiedHeader () { return next(); } - const { mapConfigProvider, cache_buster: cacheBuster } = res.locals; + const { mapConfigProvider, cache_buster: cacheBuster, logger } = res.locals; if (cacheBuster) { const cacheBusterTimestamp = parseInt(cacheBuster, 10); @@ -21,7 +21,7 @@ module.exports = function setLastModifiedHeader () { mapConfigProvider.getAffectedTables((err, affectedTables) => { if (err) { - global.logger.warn(err, 'ERROR generating Last Modified Header'); + logger.warn(err, 'ERROR generating Last Modified Header'); return next(); } diff --git a/lib/api/middlewares/logger.js b/lib/api/middlewares/logger.js index acebdb88..f709383a 100644 --- a/lib/api/middlewares/logger.js +++ b/lib/api/middlewares/logger.js @@ -5,10 +5,10 @@ const uuid = require('uuid'); module.exports = function logger () { return function loggerMiddleware (req, res, next) { const id = req.get('X-Request-Id') || uuid.v4(); - res.locals.logger = global.logger.child({ id }); + const logger = res.locals.logger = global.logger.child({ id }); - res.locals.logger.info(req); - res.on('finish', () => res.locals.logger.info(res)); + logger.info(req); + res.on('finish', () => logger.info(res)); next(); }; diff --git a/lib/api/middlewares/metrics.js b/lib/api/middlewares/metrics.js index 4ac0ebf6..60478557 100644 --- a/lib/api/middlewares/metrics.js +++ b/lib/api/middlewares/metrics.js @@ -3,7 +3,7 @@ const EVENT_VERSION = '1'; const MAX_LENGTH = 100; -module.exports = function metrics ({ enabled, tags, metricsBackend, logger }) { +module.exports = function metrics ({ enabled, tags, metricsBackend }) { if (!enabled) { return function metricsDisabledMiddleware (req, res, next) { next(); @@ -15,6 +15,8 @@ module.exports = function metrics ({ enabled, tags, metricsBackend, logger }) { } return function metricsMiddleware (req, res, next) { + const { logger } = res.locals; + res.on('finish', () => { const { event, attributes } = getEventData(req, res, tags); diff --git a/lib/api/middlewares/surrogate-key-header.js b/lib/api/middlewares/surrogate-key-header.js index a3b1e039..c1415038 100644 --- a/lib/api/middlewares/surrogate-key-header.js +++ b/lib/api/middlewares/surrogate-key-header.js @@ -5,7 +5,7 @@ const NamedMapMapConfigProvider = require('../../models/mapconfig/provider/named module.exports = function setSurrogateKeyHeader ({ surrogateKeysCache }) { return function setSurrogateKeyHeaderMiddleware (req, res, next) { - const { user, mapConfigProvider } = res.locals; + const { user, mapConfigProvider, logger } = res.locals; if (mapConfigProvider instanceof NamedMapMapConfigProvider) { surrogateKeysCache.tag(res, new NamedMapsCacheEntry(user, mapConfigProvider.getTemplateName())); @@ -17,7 +17,7 @@ module.exports = function setSurrogateKeyHeader ({ surrogateKeysCache }) { mapConfigProvider.getAffectedTables((err, affectedTables) => { if (err) { - global.logger.warn(err, 'ERROR generating Surrogate Key Header'); + logger.warn(err, 'ERROR generating Surrogate Key Header'); return next(); } diff --git a/lib/api/template/named-template-controller.js b/lib/api/template/named-template-controller.js index 88270b89..79b39d7c 100644 --- a/lib/api/template/named-template-controller.js +++ b/lib/api/template/named-template-controller.js @@ -93,7 +93,6 @@ module.exports = class NamedMapController { metrics({ enabled: this.config.pubSubMetrics.enabled, metricsBackend: this.metricsBackend, - logger: global.logger, tags: metricsTags }), credentials(), From 48c28aea0b9c56d0820782882fd7df37bdbedd27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 2 Jun 2020 11:49:54 +0200 Subject: [PATCH 02/18] Do not bind logger to global object, now it's a part of serverOptions --- app.js | 72 ++++++++----------- lib/api/api-router.js | 12 ++-- lib/api/middlewares/logger.js | 10 +-- lib/server-options.js | 5 +- lib/utils/logger.js | 48 +++++++++++++ .../dynamic-styling-named-maps-test.js | 4 +- test/acceptance/errors-with-context-test.js | 4 +- test/support/test-helper.js | 10 --- 8 files changed, 97 insertions(+), 68 deletions(-) create mode 100644 lib/utils/logger.js diff --git a/app.js b/app.js index 7cbb6a74..84bc3e3b 100755 --- a/app.js +++ b/app.js @@ -3,21 +3,11 @@ const http = require('http'); const https = require('https'); const path = require('path'); -const fs = require('fs'); const semver = require('semver'); -const pino = require('pino'); // TODO: research it it's still needed const setICUEnvVariable = require('./lib/utils/icu-data-env-setter'); -global.logger = pino({ base: null, level: process.env.NODE_ENV === 'test' ? 'fatal' : 'info' }, pino.destination({ sync: false })); - -const { engines } = require('./package.json'); -if (!semver.satisfies(process.versions.node, engines.node)) { - global.logger.fatal(new Error(`Node version ${process.versions.node} is not supported, please use Node.js ${engines.node}.`)); - process.exit(1); -} - // This function should be called before the require('yargs'). setICUEnvVariable(); @@ -37,27 +27,9 @@ const argv = require('yargs') const environmentArg = argv._[0] || process.env.NODE_ENV || 'development'; const configurationFile = path.resolve(argv.config || `./config/environments/${environmentArg}.js`); -if (!fs.existsSync(configurationFile)) { - global.logger.fatal(new Error(`Configuration file ${configurationFile} does not exist`)); - process.exit(1); -} - global.environment = require(configurationFile); -const ENVIRONMENT = argv._[0] || process.env.NODE_ENV || global.environment.environment; -process.env.NODE_ENV = ENVIRONMENT; +process.env.NODE_ENV = argv._[0] || process.env.NODE_ENV || global.environment.environment; -const availableEnvironments = { - production: true, - staging: true, - development: true -}; - -if (!availableEnvironments[ENVIRONMENT]) { - global.logger.fatal(new Error(`Invalid environment argument, valid ones: ${Object.keys(availableEnvironments).join(', ')}`)); - process.exit(1); -} - -process.env.NODE_ENV = ENVIRONMENT; if (global.environment.uv_threadpool_size) { process.env.UV_THREADPOOL_SIZE = global.environment.uv_threadpool_size; } @@ -76,10 +48,28 @@ https.globalAgent = new https.Agent(agentOptions); // Include cartodb_windshaft only _after_ the "global" variable is set // See https://github.com/Vizzuality/Windshaft-cartodb/issues/28 -const cartodbWindshaft = require('./lib/server'); +const createServer = require('./lib/server'); const serverOptions = require('./lib/server-options'); +const { logger } = serverOptions; -const server = cartodbWindshaft(serverOptions); +const availableEnvironments = { + production: true, + staging: true, + development: true +}; + +if (!availableEnvironments[process.env.NODE_ENV]) { + logger.fatal(new Error(`Invalid environment argument, valid ones: ${Object.keys(availableEnvironments).join(', ')}`)); + process.exit(1); +} + +const { engines } = require('./package.json'); +if (!semver.satisfies(process.versions.node, engines.node)) { + logger.fatal(new Error(`Node version ${process.versions.node} is not supported, please use Node.js ${engines.node}.`)); + process.exit(1); +} + +const server = createServer(serverOptions); // Specify the maximum length of the queue of pending connections for the HTTP server. // The actual length will be determined by the OS through sysctl settings such as tcp_max_syn_backlog and somaxconn on Linux. @@ -91,10 +81,10 @@ const listener = server.listen(serverOptions.bind.port, serverOptions.bind.host, const version = require('./package').version; listener.on('listening', function () { - global.logger.info(`Using Node.js ${process.version}`); - global.logger.info(`Using configuration file ${configurationFile}`); + logger.info(`Using Node.js ${process.version}`); + logger.info(`Using configuration file ${configurationFile}`); const { address, port } = listener.address(); - global.logger.info(`Windshaft tileserver ${version} started on ${address}:${port} PID=${process.pid} (${ENVIRONMENT})`); + logger.info(`Windshaft tileserver ${version} started on ${address}:${port} PID=${process.pid} (${process.env.NODE_ENV})`); }); function getCPUUsage (oldUsage) { @@ -189,19 +179,19 @@ function getGCTypeValue (type) { return value; } -const exitProcess = pino.final(global.logger, (err, logger, listener, signal, killTimeout) => { - scheduleForcedExit(killTimeout, logger); +const exitProcess = logger.finish((err, finalLogger, listener, signal, killTimeout) => { + scheduleForcedExit(killTimeout, finalLogger); - logger.info(`Process has received signal: ${signal}`); + finalLogger.info(`Process has received signal: ${signal}`); let code = 0; if (err) { code = 1; - logger.fatal(err); + finalLogger.fatal(err); } - logger.info(`Process is going to exit with code: ${code}`); + finalLogger.info(`Process is going to exit with code: ${code}`); listener.close(() => process.exit(code)); }); @@ -215,10 +205,10 @@ function addHandlers (listener, killTimeout) { addHandlers(listener, 45000); -function scheduleForcedExit (killTimeout, logger) { +function scheduleForcedExit (killTimeout, finalLogger) { // Schedule exit if there is still ongoing work to deal with const killTimer = setTimeout(() => { - global.logger.info('Process didn\'t close on time. Force exit'); + finalLogger.info('Process didn\'t close on time. Force exit'); process.exit(1); }, killTimeout); diff --git a/lib/api/api-router.js b/lib/api/api-router.js index 4356df5d..bee3ef90 100644 --- a/lib/api/api-router.js +++ b/lib/api/api-router.js @@ -47,7 +47,7 @@ const LayergroupMetadata = require('../utils/layergroup-metadata'); const RendererStatsReporter = require('../stats/reporter/renderer'); const initializeStatusCode = require('./middlewares/initialize-status-code'); -const logger = require('./middlewares/logger'); +const initLogger = require('./middlewares/logger'); const bodyParser = require('body-parser'); const servedByHostHeader = require('./middlewares/served-by-host-header'); const stats = require('./middlewares/stats'); @@ -97,7 +97,7 @@ module.exports = class ApiRouter { const surrogateKeysCacheBackends = createSurrogateKeysCacheBackends(serverOptions); const surrogateKeysCache = new SurrogateKeysCache(surrogateKeysCacheBackends); - const templateMaps = createTemplateMaps({ redisPool, surrogateKeysCache }); + const templateMaps = createTemplateMaps({ redisPool, surrogateKeysCache, logger: this.serverOptions.logger }); const analysisStatusBackend = new AnalysisStatusBackend(); const analysisBackend = new AnalysisBackend(metadataBackend, serverOptions.analysis); @@ -200,7 +200,7 @@ module.exports = class ApiRouter { middlewares.forEach(middleware => apiRouter.use(middleware())); - apiRouter.use(logger()); + apiRouter.use(initLogger({ logger: this.serverOptions.logger })); apiRouter.use(initializeStatusCode()); apiRouter.use(bodyParser.json()); apiRouter.use(servedByHostHeader()); @@ -225,7 +225,7 @@ module.exports = class ApiRouter { } }; -function createTemplateMaps ({ redisPool, surrogateKeysCache }) { +function createTemplateMaps ({ redisPool, surrogateKeysCache, logger }) { const templateMaps = new TemplateMaps(redisPool, { max_user_templates: global.environment.maxUserTemplates }); @@ -234,10 +234,10 @@ function createTemplateMaps ({ redisPool, surrogateKeysCache }) { const startTime = Date.now(); surrogateKeysCache.invalidate(new NamedMapsCacheEntry(user, templateName), (err) => { if (err) { - return global.logger.error(err); + return logger.error(err); } - global.logger.info({ user, type: 'named_map_invalidation', elapsed: Date.now() - startTime }); + logger.info({ user, type: 'named_map_invalidation', elapsed: Date.now() - startTime }); }); } diff --git a/lib/api/middlewares/logger.js b/lib/api/middlewares/logger.js index f709383a..93a02a03 100644 --- a/lib/api/middlewares/logger.js +++ b/lib/api/middlewares/logger.js @@ -2,13 +2,13 @@ const uuid = require('uuid'); -module.exports = function logger () { - return function loggerMiddleware (req, res, next) { +module.exports = function initLogger ({ logger }) { + return function initLoggerMiddleware (req, res, next) { const id = req.get('X-Request-Id') || uuid.v4(); - const logger = res.locals.logger = global.logger.child({ id }); + res.locals.logger = logger.child({ id }); - logger.info(req); - res.on('finish', () => logger.info(res)); + res.locals.logger.info(req); + res.on('finish', () => res.locals.logger.info(res)); next(); }; diff --git a/lib/server-options.js b/lib/server-options.js index 26b14714..40306e67 100644 --- a/lib/server-options.js +++ b/lib/server-options.js @@ -3,6 +3,7 @@ const fqdn = require('@carto/fqdn-sync'); var _ = require('underscore'); var OverviewsQueryRewriter = require('./utils/overviews-query-rewriter'); +const Logger = require('./utils/logger'); var rendererConfig = _.defaults(global.environment.renderer || {}, { cache_ttl: 60000, // milliseconds @@ -127,7 +128,7 @@ module.exports = { varnish_purge_enabled: global.environment.varnish.purge_enabled, fastly: global.environment.fastly || {}, cache_enabled: global.environment.cache_enabled, - log_format: global.environment.log_format, useProfiler: global.environment.useProfiler, - pubSubMetrics: Object.assign({ enabled: false }, global.environment.pubSubMetrics) + pubSubMetrics: Object.assign({ enabled: false }, global.environment.pubSubMetrics), + logger: new Logger() }; diff --git a/lib/utils/logger.js b/lib/utils/logger.js new file mode 100644 index 00000000..4d1340a7 --- /dev/null +++ b/lib/utils/logger.js @@ -0,0 +1,48 @@ +'use strict'; + +const pino = require('pino'); + +module.exports = class Logger { + constructor () { + const { LOG_LEVEL, NODE_ENV } = process.env; + const options = { + base: null, // Do not bind hostname, pid and friends by default + level: LOG_LEVEL || NODE_ENV === 'test' ? 'fatal' : 'info' + }; + const dest = pino.destination({ sync: false }); + + this._logger = pino(options, dest); + } + + trace (...args) { + this._logger.trace(...args); + } + + debug (...args) { + this._logger.debug(...args); + } + + info (...args) { + this._logger.info(...args); + } + + warn (...args) { + this._logger.warn(...args); + } + + error (...args) { + this._logger.error(...args); + } + + fatal (...args) { + this._logger.fatal(...args); + } + + child (...args) { + return this._logger.child(...args); + } + + finish (callback) { + return pino.final(this._logger, callback); + } +}; diff --git a/test/acceptance/dynamic-styling-named-maps-test.js b/test/acceptance/dynamic-styling-named-maps-test.js index 48391640..881aaaaa 100644 --- a/test/acceptance/dynamic-styling-named-maps-test.js +++ b/test/acceptance/dynamic-styling-named-maps-test.js @@ -4,14 +4,14 @@ var assert = require('../support/assert'); var step = require('step'); var LayergroupToken = require('../../lib/models/layergroup-token'); var testHelper = require('../support/test-helper'); -var CartodbWindshaft = require('../../lib/server'); +var createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); describe('dynamic styling for named maps', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); }); var keysToDelete; diff --git a/test/acceptance/errors-with-context-test.js b/test/acceptance/errors-with-context-test.js index 23e09e7a..5765c4b1 100644 --- a/test/acceptance/errors-with-context-test.js +++ b/test/acceptance/errors-with-context-test.js @@ -1,14 +1,14 @@ 'use strict'; var assert = require('../support/assert'); -var CartodbWindshaft = require('../../lib/server'); +var createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); describe('error with context', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); }); var layerOK = { diff --git a/test/support/test-helper.js b/test/support/test-helper.js index 94f9dffa..29692cdd 100644 --- a/test/support/test-helper.js +++ b/test/support/test-helper.js @@ -1,12 +1,5 @@ 'use strict'; -/** - * User: simon - * Date: 30/08/2011 - * Time: 13:52 - * Desc: Loads test specific variables - */ - var assert = require('assert'); var fs = require('fs'); var LZMA = require('lzma').LZMA; @@ -14,7 +7,6 @@ var LZMA = require('lzma').LZMA; var lzmaWorker = new LZMA(); var redis = require('redis'); -const pino = require('pino'); const setICUEnvVariable = require('../../lib/utils/icu-data-env-setter'); // set environment specific variables @@ -24,8 +16,6 @@ process.env.NODE_ENV = 'test'; setICUEnvVariable(); -global.logger = pino({ base: null, level: process.env.NODE_ENV === 'test' ? 'fatal' : 'info' }, pino.destination({ sync: false })); - // Utility function to compress & encode LZMA function lzmaCompressToBase64 (payload, mode, callback) { lzmaWorker.compress(payload, mode, From ffe19827fd85fd0e9e6708ec0c4f0696ffa0e129 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 2 Jun 2020 11:57:11 +0200 Subject: [PATCH 03/18] Rename factory and don't use the keyword 'new' to create server while testing --- test/acceptance/analysis/named-maps-test.js | 4 ++-- .../auth/authorization-basic-use-cases-test.js | 4 ++-- test/acceptance/cache/cache-headers-test.js | 4 ++-- .../cache/surrogate-keys-invalidation-test.js | 4 ++-- test/acceptance/health-check-test.js | 10 +++++----- test/acceptance/multilayer-server-test.js | 4 ++-- test/acceptance/multilayer-test.js | 10 +++++----- test/acceptance/named-layers-test.js | 4 ++-- test/acceptance/named-layers-visibility-test.js | 4 ++-- test/acceptance/named-maps-authentication-test.js | 4 ++-- test/acceptance/named-maps-cache-test.js | 4 ++-- test/acceptance/named-maps-static-view-test.js | 6 +++--- test/acceptance/named-maps-stats-test.js | 4 ++-- test/acceptance/overviews-metadata-named-maps-test.js | 4 ++-- test/acceptance/overviews-metadata-test.js | 6 +++--- test/acceptance/regressions-test.js | 4 ++-- test/acceptance/server-test.js | 6 +++--- test/acceptance/templates-test.js | 4 ++-- test/acceptance/turbo-carto/named-maps-test.js | 4 ++-- test/acceptance/widgets/named-maps-test.js | 4 ++-- test/support/test-client.js | 8 ++++---- 21 files changed, 53 insertions(+), 53 deletions(-) diff --git a/test/acceptance/analysis/named-maps-test.js b/test/acceptance/analysis/named-maps-test.js index d04beadd..a0181ffe 100644 --- a/test/acceptance/analysis/named-maps-test.js +++ b/test/acceptance/analysis/named-maps-test.js @@ -4,7 +4,7 @@ var assert = require('../../support/assert'); var helper = require('../../support/test-helper'); -var CartodbWindshaft = require('../../../lib/server'); +const createServer = require('../../../lib/server'); var serverOptions = require('../../../lib/server-options'); var TestClient = require('../../support/test-client'); @@ -14,7 +14,7 @@ describe('named-maps analysis', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); }); var IMAGE_TOLERANCE_PER_MIL = 20; diff --git a/test/acceptance/auth/authorization-basic-use-cases-test.js b/test/acceptance/auth/authorization-basic-use-cases-test.js index 9e48077b..a58ad9ad 100644 --- a/test/acceptance/auth/authorization-basic-use-cases-test.js +++ b/test/acceptance/auth/authorization-basic-use-cases-test.js @@ -2,7 +2,7 @@ const assert = require('../../support/assert'); const testHelper = require('../../support/test-helper'); -const CartodbWindshaft = require('../../../lib/server'); +const createServer = require('../../../lib/server'); const serverOptions = require('../../../lib/server-options'); var LayergroupToken = require('../../../lib/models/layergroup-token'); @@ -47,7 +47,7 @@ describe('Basic authorization use cases', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); }); beforeEach(function () { diff --git a/test/acceptance/cache/cache-headers-test.js b/test/acceptance/cache/cache-headers-test.js index d96bada7..6a00a7eb 100644 --- a/test/acceptance/cache/cache-headers-test.js +++ b/test/acceptance/cache/cache-headers-test.js @@ -5,7 +5,7 @@ var testHelper = require('../../support/test-helper'); var assert = require('../../support/assert'); var qs = require('querystring'); -var CartodbWindshaft = require('../../../lib/server'); +const createServer = require('../../../lib/server'); var serverOptions = require('../../../lib/server-options'); var LayergroupToken = require('../../../lib/models/layergroup-token'); @@ -14,7 +14,7 @@ describe('get requests with cache headers', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); server.setMaxListeners(0); }); diff --git a/test/acceptance/cache/surrogate-keys-invalidation-test.js b/test/acceptance/cache/surrogate-keys-invalidation-test.js index 4e33947b..675dab1a 100644 --- a/test/acceptance/cache/surrogate-keys-invalidation-test.js +++ b/test/acceptance/cache/surrogate-keys-invalidation-test.js @@ -7,7 +7,7 @@ var step = require('step'); var FastlyPurge = require('fastly-purge'); var _ = require('underscore'); var NamedMapsCacheEntry = require('../../../lib/cache/model/named-maps-entry'); -var CartodbWindshaft = require('../../../lib/server'); +const createServer = require('../../../lib/server'); var nock = require('nock'); describe('templates surrogate keys', function () { @@ -33,7 +33,7 @@ describe('templates surrogate keys', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); nock.disableNetConnect(); nock.enableNetConnect(/(127.0.0.1|cartocdn.com)/); }); diff --git a/test/acceptance/health-check-test.js b/test/acceptance/health-check-test.js index 53ac76f7..c08c626e 100644 --- a/test/acceptance/health-check-test.js +++ b/test/acceptance/health-check-test.js @@ -5,7 +5,7 @@ require('../support/test-helper'); var fs = require('fs'); var assert = require('../support/assert'); -var CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); describe('health checks', function () { @@ -41,7 +41,7 @@ describe('health checks', function () { }; it('returns 200 and ok=true with enabled configuration', function (done) { - var server = new CartodbWindshaft(serverOptions); + var server = createServer(serverOptions); assert.response(server, healthCheckRequest, RESPONSE_OK, function (res, err) { assert.ok(!err); @@ -62,7 +62,7 @@ describe('health checks', function () { fs.readFile = function (filename, callback) { callback(null, errorMessage); }; - var server = new CartodbWindshaft(serverOptions); + var server = createServer(serverOptions); assert.response(server, healthCheckRequest, RESPONSE_FAIL, function (res, err) { fs.readFile = readFileFn; @@ -82,7 +82,7 @@ describe('health checks', function () { fs.readFile = function (filename, callback) { callback(null, ''); }; - var server = new CartodbWindshaft(serverOptions); + var server = createServer(serverOptions); assert.response(server, healthCheckRequest, RESPONSE_FAIL, function (res, err) { fs.readFile = readFileFn; @@ -100,7 +100,7 @@ describe('health checks', function () { it('not err if disabled file does not exist', function (done) { global.environment.disabled_file = '/tmp/ftreftrgtrccre'; - var server = new CartodbWindshaft(serverOptions); + var server = createServer(serverOptions); assert.response(server, healthCheckRequest, RESPONSE_OK, function (res, err) { assert.ok(!err); diff --git a/test/acceptance/multilayer-server-test.js b/test/acceptance/multilayer-server-test.js index 04a281e1..5e621aa7 100644 --- a/test/acceptance/multilayer-server-test.js +++ b/test/acceptance/multilayer-server-test.js @@ -10,14 +10,14 @@ var LayergroupToken = require('../../lib/models/layergroup-token'); var PgQueryRunner = require('../../lib/backends/pg-query-runner'); var QueryTables = require('cartodb-query-tables').queryTables; -var CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); describe('tests from old api translated to multilayer', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); server.setMaxListeners(0); }); diff --git a/test/acceptance/multilayer-test.js b/test/acceptance/multilayer-test.js index 91868751..d291d86e 100644 --- a/test/acceptance/multilayer-test.js +++ b/test/acceptance/multilayer-test.js @@ -19,7 +19,7 @@ var windshaftFixtures = path.join(__dirname, '/../../node_modules/windshaft/test var IMAGE_EQUALS_TOLERANCE_PER_MIL = 20; var IMAGE_EQUALS_HIGHER_TOLERANCE_PER_MIL = 25; -var CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); var QueryTables = require('cartodb-query-tables').queryTables; @@ -30,7 +30,7 @@ var QueryTables = require('cartodb-query-tables').queryTables; var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); server.setMaxListeners(0); }); @@ -851,7 +851,7 @@ var QueryTables = require('cartodb-query-tables').queryTables; function doRestartServer (err/*, res */) { assert.ifError(err); // hack simulating restart... - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); return null; }, function doGet1 (err) { @@ -1274,7 +1274,7 @@ var QueryTables = require('cartodb-query-tables').queryTables; it('cache control for layergroup default value', function (done) { global.environment.varnish.layergroupTtl = null; - var server = new CartodbWindshaft(serverOptions); + var server = createServer(serverOptions); assert.response(server, layergroupTtlRequest, layergroupTtlResponseExpectation, function (res) { @@ -1291,7 +1291,7 @@ var QueryTables = require('cartodb-query-tables').queryTables; var layergroupTtl = 300; global.environment.varnish.layergroupTtl = layergroupTtl; - var server = new CartodbWindshaft(serverOptions); + var server = createServer(serverOptions); assert.response(server, layergroupTtlRequest, layergroupTtlResponseExpectation, function (res) { diff --git a/test/acceptance/named-layers-test.js b/test/acceptance/named-layers-test.js index 1ea33e31..edfc8067 100644 --- a/test/acceptance/named-layers-test.js +++ b/test/acceptance/named-layers-test.js @@ -3,7 +3,7 @@ var testHelper = require('../support/test-helper'); var assert = require('../support/assert'); -var CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); var LayergroupToken = require('../../lib/models/layergroup-token'); @@ -17,7 +17,7 @@ describe('named_layers', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); }); // configure redis pool instance to use in tests diff --git a/test/acceptance/named-layers-visibility-test.js b/test/acceptance/named-layers-visibility-test.js index a9b0624c..51ad979e 100644 --- a/test/acceptance/named-layers-visibility-test.js +++ b/test/acceptance/named-layers-visibility-test.js @@ -4,7 +4,7 @@ var step = require('step'); var testHelper = require('../support/test-helper'); var assert = require('../support/assert'); -var CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); var RedisPool = require('redis-mpool'); @@ -17,7 +17,7 @@ describe('layers visibility for previews', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); }); // configure redis pool instance to use in tests diff --git a/test/acceptance/named-maps-authentication-test.js b/test/acceptance/named-maps-authentication-test.js index cd1e20eb..f559bbbd 100644 --- a/test/acceptance/named-maps-authentication-test.js +++ b/test/acceptance/named-maps-authentication-test.js @@ -6,7 +6,7 @@ var querystring = require('querystring'); var assert = require('../support/assert'); const mapnik = require('@carto/mapnik'); -var CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); var TemplateMaps = require('../../lib/backends/template-maps'); var NamedMapsCacheEntry = require('../../lib/cache/model/named-maps-entry'); @@ -15,7 +15,7 @@ describe('named maps authentication', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); }); // configure redis pool instance to use in tests diff --git a/test/acceptance/named-maps-cache-test.js b/test/acceptance/named-maps-cache-test.js index 4c036858..712d859e 100644 --- a/test/acceptance/named-maps-cache-test.js +++ b/test/acceptance/named-maps-cache-test.js @@ -5,14 +5,14 @@ require('../support/test-helper'); const helper = require('../support/test-helper'); var assert = require('../support/assert'); const mapnik = require('@carto/mapnik'); -var CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); describe('named maps provider cache', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); }); var username = 'localhost'; diff --git a/test/acceptance/named-maps-static-view-test.js b/test/acceptance/named-maps-static-view-test.js index 08d5ac82..8ffad2e9 100644 --- a/test/acceptance/named-maps-static-view-test.js +++ b/test/acceptance/named-maps-static-view-test.js @@ -6,7 +6,7 @@ var RedisPool = require('redis-mpool'); var assert = require('../support/assert'); const mapnik = require('@carto/mapnik'); -var CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); var TemplateMaps = require('../../lib/backends/template-maps'); @@ -86,7 +86,7 @@ describe('named maps static view', function () { }; // this could be removed once named maps are invalidated, otherwise you hits the cache - var server = new CartodbWindshaft(serverOptions); + var server = createServer(serverOptions); assert.response(server, requestOptions, expectedResponse, function (res, err) { testHelper.deleteRedisKeys({ 'user:localhost:mapviews:global': 5 }, function () { @@ -323,7 +323,7 @@ describe('named maps static view', function () { }; // this could be removed once named maps are invalidated, otherwise you hits the cache - var server = new CartodbWindshaft(serverOptions); + var server = createServer(serverOptions); assert.response(server, requestOptions, expectedResponse, function (res, err) { assert.ifError(err); diff --git a/test/acceptance/named-maps-stats-test.js b/test/acceptance/named-maps-stats-test.js index c7a1ded7..bd6b474c 100644 --- a/test/acceptance/named-maps-stats-test.js +++ b/test/acceptance/named-maps-stats-test.js @@ -6,7 +6,7 @@ var querystring = require('querystring'); var assert = require('../support/assert'); const mapnik = require('@carto/mapnik'); -var CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); var TemplateMaps = require('../../lib/backends/template-maps'); var NamedMapsCacheEntry = require('../../lib/cache/model/named-maps-entry'); @@ -15,7 +15,7 @@ describe('named maps preview stats', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); }); var redisPool = new RedisPool(global.environment.redis); diff --git a/test/acceptance/overviews-metadata-named-maps-test.js b/test/acceptance/overviews-metadata-named-maps-test.js index 60712ec6..10dc2458 100644 --- a/test/acceptance/overviews-metadata-named-maps-test.js +++ b/test/acceptance/overviews-metadata-named-maps-test.js @@ -3,7 +3,7 @@ var testHelper = require('../support/test-helper'); var assert = require('../support/assert'); -var CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); var LayergroupToken = require('../../lib/models/layergroup-token'); @@ -18,7 +18,7 @@ describe('overviews metadata for named maps', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); }); // configure redis pool instance to use in tests diff --git a/test/acceptance/overviews-metadata-test.js b/test/acceptance/overviews-metadata-test.js index d06b8cdf..828c34e3 100644 --- a/test/acceptance/overviews-metadata-test.js +++ b/test/acceptance/overviews-metadata-test.js @@ -3,7 +3,7 @@ var testHelper = require('../support/test-helper'); var assert = require('../support/assert'); -var CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); var LayergroupToken = require('../../lib/models/layergroup-token'); @@ -18,7 +18,7 @@ describe('overviews metadata', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); }); // configure redis pool instance to use in tests @@ -193,7 +193,7 @@ describe('overviews metadata with filters', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); }); // configure redis pool instance to use in tests diff --git a/test/acceptance/regressions-test.js b/test/acceptance/regressions-test.js index b777d729..a4c8672b 100644 --- a/test/acceptance/regressions-test.js +++ b/test/acceptance/regressions-test.js @@ -5,7 +5,7 @@ var assert = require('../support/assert'); const helper = require('../support/test-helper'); var TestClient = require('../support/test-client'); const LayergroupToken = require('../../lib/models/layergroup-token'); -const CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); const serverOptions = require('../../lib/server-options'); describe('regressions', function () { @@ -44,7 +44,7 @@ describe('regressions', function () { // See: https://github.com/CartoDB/Windshaft-cartodb/pull/956 it('"/user/localhost/api/v1/map" should create an anonymous map', function (done) { - const server = new CartodbWindshaft(serverOptions); + const server = createServer(serverOptions); const layergroup = { version: '1.7.0', layers: [ diff --git a/test/acceptance/server-test.js b/test/acceptance/server-test.js index 7fb16e4d..245529aa 100644 --- a/test/acceptance/server-test.js +++ b/test/acceptance/server-test.js @@ -6,14 +6,14 @@ var assert = require('../support/assert'); var querystring = require('querystring'); var step = require('step'); -var CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); describe('server', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); server.setMaxListeners(0); }); @@ -45,7 +45,7 @@ describe('server old_api', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); server.setMaxListeners(0); }); diff --git a/test/acceptance/templates-test.js b/test/acceptance/templates-test.js index c7926fe3..fb4dca0b 100644 --- a/test/acceptance/templates-test.js +++ b/test/acceptance/templates-test.js @@ -21,7 +21,7 @@ var http = require('http'); var helper = require('../support/test-helper'); -var CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); var LayergroupToken = require('../../lib/models/layergroup-token'); @@ -30,7 +30,7 @@ describe('template_api', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); server.setMaxListeners(0); // FIXME: we need a better way to reset cache while running tests server.layergroupAffectedTablesCache.cache.reset(); diff --git a/test/acceptance/turbo-carto/named-maps-test.js b/test/acceptance/turbo-carto/named-maps-test.js index 5076b66f..67d40a54 100644 --- a/test/acceptance/turbo-carto/named-maps-test.js +++ b/test/acceptance/turbo-carto/named-maps-test.js @@ -4,7 +4,7 @@ var assert = require('../../support/assert'); var step = require('step'); var LayergroupToken = require('../../../lib/models/layergroup-token'); var testHelper = require('../../support/test-helper'); -var CartodbWindshaft = require('../../../lib/server'); +const createServer = require('../../../lib/server'); var serverOptions = require('../../../lib/server-options'); const mapnik = require('@carto/mapnik'); var IMAGE_TOLERANCE_PER_MIL = 10; @@ -13,7 +13,7 @@ describe('turbo-carto for named maps', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); }); var keysToDelete; diff --git a/test/acceptance/widgets/named-maps-test.js b/test/acceptance/widgets/named-maps-test.js index b93f26eb..e91d71a4 100644 --- a/test/acceptance/widgets/named-maps-test.js +++ b/test/acceptance/widgets/named-maps-test.js @@ -8,7 +8,7 @@ var queue = require('queue-async'); var helper = require('../../support/test-helper'); -var CartodbWindshaft = require('../../../lib/server'); +const createServer = require('../../../lib/server'); var serverOptions = require('../../../lib/server-options'); var LayergroupToken = require('../../../lib/models/layergroup-token'); @@ -17,7 +17,7 @@ describe('named-maps widgets', function () { var server; before(function () { - server = new CartodbWindshaft(serverOptions); + server = createServer(serverOptions); }); var username = 'localhost'; diff --git a/test/support/test-client.js b/test/support/test-client.js index 45fb198e..ffe71ef3 100644 --- a/test/support/test-client.js +++ b/test/support/test-client.js @@ -12,7 +12,7 @@ var LayergroupToken = require('../../lib/models/layergroup-token'); var assert = require('./assert'); var helper = require('./test-helper'); -var CartodbWindshaft = require('../../lib/server'); +const createServer = require('../../lib/server'); var serverOptions = require('../../lib/server-options'); serverOptions.analysis.batch.inlineExecution = true; @@ -30,7 +30,7 @@ function TestClient (config, apiKey, extraHeaders = {}, overrideServerOptions = this.extraHeaders = extraHeaders; this.keysToDelete = {}; this.serverOptions = Object.assign({}, serverOptions, overrideServerOptions); - this.server = new CartodbWindshaft(this.serverOptions); + this.server = createServer(this.serverOptions); } module.exports = TestClient; @@ -1348,7 +1348,7 @@ TestClient.prototype.drain = function (callback) { module.exports.getStaticMap = function getStaticMap (templateName, params, callback) { var self = this; - self.server = new CartodbWindshaft(serverOptions); + self.server = createServer(serverOptions); if (!callback) { callback = params; @@ -1378,7 +1378,7 @@ module.exports.getStaticMap = function getStaticMap (templateName, params, callb }; // this could be removed once named maps are invalidated, otherwise you hits the cache - var server = new CartodbWindshaft(serverOptions); + var server = createServer(serverOptions); assert.response(server, requestOptions, expectedResponse, function (res, err) { helper.deleteRedisKeys({ 'user:localhost:mapviews:global': 5 }, function () { From b60116410affe559fa9204395e09e3d64089faab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 2 Jun 2020 12:31:18 +0200 Subject: [PATCH 04/18] Use req/res logger instead of the one bound to global object --- lib/api/map/anonymous-map-controller.js | 2 ++ lib/api/middlewares/user.js | 5 +-- lib/backends/analysis.js | 2 -- lib/models/cdb-request.js | 44 ++++++++++++------------- test/unit/cdb-request-test.js | 13 ++++---- 5 files changed, 34 insertions(+), 32 deletions(-) diff --git a/lib/api/map/anonymous-map-controller.js b/lib/api/map/anonymous-map-controller.js index 85e9860a..7236935b 100644 --- a/lib/api/map/anonymous-map-controller.js +++ b/lib/api/map/anonymous-map-controller.js @@ -152,6 +152,7 @@ function prepareAdapterMapConfig (mapConfigAdapter) { return function prepareAdapterMapConfigMiddleware (req, res, next) { const requestMapConfig = req.body; + const { logger } = res.locals; const { user, api_key: apiKey } = res.locals; const { dbuser, dbname, dbpassword, dbhost, dbport } = res.locals; const params = Object.assign({ dbuser, dbname, dbpassword, dbhost, dbport }, req.query); @@ -159,6 +160,7 @@ function prepareAdapterMapConfig (mapConfigAdapter) { const context = { analysisConfiguration: { user, + logger, db: { host: dbhost, port: dbport, diff --git a/lib/api/middlewares/user.js b/lib/api/middlewares/user.js index ce8c73bf..31e7f909 100644 --- a/lib/api/middlewares/user.js +++ b/lib/api/middlewares/user.js @@ -3,9 +3,10 @@ const CdbRequest = require('../../models/cdb-request'); module.exports = function user (metadataBackend) { - const cdbRequest = new CdbRequest(); - return function userMiddleware (req, res, next) { + const { logger } = res.locals; + const cdbRequest = new CdbRequest({ logger }); + res.locals.user = getUserNameFromRequest(req, cdbRequest); metadataBackend.getUserId(res.locals.user, (err, userId) => { diff --git a/lib/backends/analysis.js b/lib/backends/analysis.js index 2844a028..376e12bb 100644 --- a/lib/backends/analysis.js +++ b/lib/backends/analysis.js @@ -30,8 +30,6 @@ AnalysisBackend.prototype.create = function (analysisConfiguration, analysisDefi analysisConfiguration.batch.inlineExecution = this.batchConfig.inlineExecution; analysisConfiguration.batch.hostHeaderTemplate = this.batchConfig.hostHeaderTemplate; - analysisConfiguration.logger = global.logger; - this.getAnalysesLimits(analysisConfiguration.user, function (err, limits) { if (err) {} analysisConfiguration.limits = limits || {}; diff --git a/lib/models/cdb-request.js b/lib/models/cdb-request.js index 54b3db5c..a962b558 100644 --- a/lib/models/cdb-request.js +++ b/lib/models/cdb-request.js @@ -1,29 +1,29 @@ 'use strict'; -function CdbRequest () { - this.RE_USER_FROM_HOST = new RegExp(global.environment.user_from_host || - '^([^\\.]+)\\.' // would extract "strk" from "strk.cartodb.com" - ); -} - -module.exports = CdbRequest; - -CdbRequest.prototype.userByReq = function (req) { - var host = req.headers.host || ''; - - if (req.params.user) { - return req.params.user; +module.exports = class CdbRequest { + constructor ({ logger }) { + this.logger = logger; + // would extract "strk" from "strk.cartodb.com" + this.RE_USER_FROM_HOST = new RegExp(global.environment.user_from_host || '^([^\\.]+)\\.'); } - var mat = host.match(this.RE_USER_FROM_HOST); + userByReq (req) { + const host = req.headers.host || ''; - if (!mat) { - return global.logger.error(new Error(`Pattern '${this.RE_USER_FROM_HOST}' does not match hostname '${host}'`)); + if (req.params.user) { + return req.params.user; + } + + const mat = host.match(this.RE_USER_FROM_HOST); + + if (!mat) { + return this.logger.error(new Error(`Pattern '${this.RE_USER_FROM_HOST}' does not match hostname '${host}'`)); + } + + if (mat.length !== 2) { + return this.logger.error(new Error(`Pattern '${this.RE_USER_FROM_HOST}' gave unexpected matches against '${host}': ${mat}`)); + } + + return mat[1]; } - - if (mat.length !== 2) { - return global.logger.error(new Error(`Pattern '${this.RE_USER_FROM_HOST}' gave unexpected matches against '${host}': ${mat}`)); - } - - return mat[1]; }; diff --git a/test/unit/cdb-request-test.js b/test/unit/cdb-request-test.js index 3e1880f7..9db38488 100644 --- a/test/unit/cdb-request-test.js +++ b/test/unit/cdb-request-test.js @@ -4,6 +4,7 @@ require('../support/test-helper'); var assert = require('assert'); var CdbRequest = require('../../lib/models/cdb-request'); +const { logger } = require('../../lib/server-options'); describe('req2params', function () { function createRequest (host, userParam) { @@ -22,7 +23,7 @@ describe('req2params', function () { } it('extracts name from host header', function () { - var cdbRequest = new CdbRequest(); + var cdbRequest = new CdbRequest({ logger }); var user = cdbRequest.userByReq(createRequest('localhost')); assert.strictEqual(user, 'localhost'); @@ -32,7 +33,7 @@ describe('req2params', function () { var userFromHostConfig = global.environment.user_from_host; global.environment.user_from_host = null; - var cdbRequest = new CdbRequest(); + var cdbRequest = new CdbRequest({ logger }); var user = cdbRequest.userByReq(createRequest('development.localhost.lan')); global.environment.user_from_host = userFromHostConfig; @@ -41,7 +42,7 @@ describe('req2params', function () { }); it('considers user param before headers', function () { - var cdbRequest = new CdbRequest(); + var cdbRequest = new CdbRequest({ logger }); var user = cdbRequest.userByReq(createRequest('localhost', 'development')); assert.strictEqual(user, 'development'); @@ -51,7 +52,7 @@ describe('req2params', function () { var userFromHostConfig = global.environment.user_from_host; global.environment.user_from_host = null; - var cdbRequest = new CdbRequest(); + var cdbRequest = new CdbRequest({ logger }); var user = cdbRequest.userByReq(createRequest('localhost')); global.environment.user_from_host = userFromHostConfig; @@ -63,7 +64,7 @@ describe('req2params', function () { var userFromHostConfig = global.environment.user_from_host; global.environment.user_from_host = null; - var cdbRequest = new CdbRequest(); + var cdbRequest = new CdbRequest({ logger }); var user = cdbRequest.userByReq(createRequest(undefined)); global.environment.user_from_host = userFromHostConfig; @@ -75,7 +76,7 @@ describe('req2params', function () { var userFromHostConfig = global.environment.user_from_host; global.environment.user_from_host = null; - var cdbRequest = new CdbRequest(); + var cdbRequest = new CdbRequest({ logger }); var user = cdbRequest.userByReq(createRequest(null)); global.environment.user_from_host = userFromHostConfig; From b7b3392bdd6784896315d752f061feb39915e45b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 2 Jun 2020 13:16:26 +0200 Subject: [PATCH 05/18] Be able to set log level from env variable LOG_LEVEL --- lib/utils/logger.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/utils/logger.js b/lib/utils/logger.js index 4d1340a7..33646049 100644 --- a/lib/utils/logger.js +++ b/lib/utils/logger.js @@ -5,11 +5,12 @@ const pino = require('pino'); module.exports = class Logger { constructor () { const { LOG_LEVEL, NODE_ENV } = process.env; + const logLevelFromNodeEnv = NODE_ENV === 'test' ? 'fatal' : 'info'; const options = { base: null, // Do not bind hostname, pid and friends by default - level: LOG_LEVEL || NODE_ENV === 'test' ? 'fatal' : 'info' + level: LOG_LEVEL || logLevelFromNodeEnv }; - const dest = pino.destination({ sync: false }); + const dest = pino.destination({ sync: false }); // stdout this._logger = pino(options, dest); } From afeb91dc8605d96b1ab603ebef17c0c813863d33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 2 Jun 2020 13:20:57 +0200 Subject: [PATCH 06/18] Bring back logger for windshaft --- lib/api/api-router.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/api/api-router.js b/lib/api/api-router.js index bee3ef90..62d318d9 100644 --- a/lib/api/api-router.js +++ b/lib/api/api-router.js @@ -86,7 +86,8 @@ module.exports = class ApiRouter { const { rendererCache, tileBackend, attributesBackend, previewBackend, mapBackend, mapStore } = windshaftFactory({ rendererOptions: serverOptions, redisPool, - onTileErrorStrategy: getOnTileErrorStrategy({ enabled: environmentOptions.enabledFeatures.onTileErrorStrategy }) + onTileErrorStrategy: getOnTileErrorStrategy({ enabled: environmentOptions.enabledFeatures.onTileErrorStrategy }), + logger: this.serverOptions.logger }); const rendererStatsReporter = new RendererStatsReporter(rendererCache, serverOptions.renderCache.statsInterval); From 7d8d05b865ab7a7efbc81c9aa4021f2b32c14928 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 2 Jun 2020 16:11:39 +0200 Subject: [PATCH 07/18] Log errors and do not send 'X-Tiler-Errors' header --- lib/api/middlewares/error-middleware.js | 62 +-------- test/acceptance/error-middleware-test.js | 44 ------ test/unit/error-middleware-test.js | 167 ----------------------- 3 files changed, 6 insertions(+), 267 deletions(-) delete mode 100644 test/acceptance/error-middleware-test.js diff --git a/lib/api/middlewares/error-middleware.js b/lib/api/middlewares/error-middleware.js index 05d1bd49..0822c144 100644 --- a/lib/api/middlewares/error-middleware.js +++ b/lib/api/middlewares/error-middleware.js @@ -1,10 +1,10 @@ 'use strict'; -const _ = require('underscore'); const debug = require('debug')('windshaft:cartodb:error-middleware'); module.exports = function errorMiddleware (/* options */) { return function error (err, req, res, next) { + const { logger } = res.locals; var allErrors = Array.isArray(err) ? err : [err]; allErrors = populateLimitErrors(allErrors); @@ -15,14 +15,14 @@ module.exports = function errorMiddleware (/* options */) { var statusCode = findStatusCode(err); - setErrorHeader(allErrors, statusCode, res); - debug('[%s ERROR] -- %d: %s, %s', label, statusCode, err, err.stack); - // If a callback was requested, force status to 200 if (req.query && req.query.callback) { statusCode = 200; } + allErrors.forEach((err) => debug('[%s ERROR] -- %d: %s, %s', label, statusCode, err, err.stack)); + allErrors.forEach((err) => logger.error(err)); + var errorResponseBody = { errors: allErrors.map(errorMessage), errors_with_context: allErrors.map(errorMessageWithContext) @@ -135,7 +135,7 @@ function statusFromErrorMessage (errMsg) { function errorMessage (err) { // See https://github.com/Vizzuality/Windshaft-cartodb/issues/68 - var message = (_.isString(err) ? err : err.message) || 'Unknown error'; + var message = (typeof err === 'string' ? err : err.message) || 'Unknown error'; return stripConnectionInfo(message); } @@ -165,7 +165,7 @@ function shouldBeExposed (prop) { function errorMessageWithContext (err) { // See https://github.com/Vizzuality/Windshaft-cartodb/issues/68 - var message = (_.isString(err) ? err : err.message) || 'Unknown error'; + var message = (typeof err === 'string' ? err : err.message) || 'Unknown error'; var error = { type: err.type || 'unknown', @@ -181,53 +181,3 @@ function errorMessageWithContext (err) { return error; } - -function setErrorHeader (errors, statusCode, res) { - const errorsCopy = errors.slice(0); - const mainError = errorsCopy.shift(); - - const errorsLog = { - mainError: { - statusCode: statusCode || 200, - message: mainError.message, - name: mainError.name, - label: mainError.label, - type: mainError.type, - subtype: mainError.subtype - } - }; - - errorsLog.moreErrors = errorsCopy.map(error => { - return { - message: error.message, - name: error.name, - label: error.label, - type: error.type, - subtype: error.subtype - }; - }); - - res.set('X-Tiler-Errors', stringifyForLogs(errorsLog)); -} - -/** - * Remove problematic nested characters - * from object for logs RegEx - * - * @param {Object} object - */ -function stringifyForLogs (object) { - Object.keys(object).map(key => { - if (typeof object[key] === 'string') { - object[key] = object[key].replace(/[^a-zA-Z0-9]/g, ' '); - } else if (typeof object[key] === 'object') { - stringifyForLogs(object[key]); - } else if (object[key] instanceof Array) { - for (const element of object[key]) { - stringifyForLogs(element); - } - } - }); - - return JSON.stringify(object); -} diff --git a/test/acceptance/error-middleware-test.js b/test/acceptance/error-middleware-test.js deleted file mode 100644 index 936c957f..00000000 --- a/test/acceptance/error-middleware-test.js +++ /dev/null @@ -1,44 +0,0 @@ -'use strict'; - -const assert = require('../support/assert'); -const TestClient = require('../support/test-client'); - -describe('error middleware', function () { - it('should returns a errors header', function (done) { - const mapConfig = { - version: '1.6.0', - layers: [{ - type: 'mapnik', - options: {} - }] - }; - - const errorHeader = { - mainError: { - statusCode: 400, - message: 'Missing cartocss for layer 0 options', - name: 'Error', - label: 'ANONYMOUS LAYERGROUP', - type: 'layer' - }, - moreErrors: [] - }; - - this.testClient = new TestClient(mapConfig, 1234); - - const params = { - response: { - status: 400, - headers: { - 'Content-Type': 'application/json; charset=utf-8', - 'X-Tiler-Errors': JSON.stringify(errorHeader) - } - } - }; - - this.testClient.getLayergroup(params, (err) => { - assert.ifError(err); - done(); - }); - }); -}); diff --git a/test/unit/error-middleware-test.js b/test/unit/error-middleware-test.js index b973d49a..8b845de5 100644 --- a/test/unit/error-middleware-test.js +++ b/test/unit/error-middleware-test.js @@ -20,171 +20,4 @@ describe('error-middleware', function () { 'Error status code for multiline/PSQL does not match' ); }); - - it('should return a header with errors', function (done) { - const error = new Error('error test'); - error.label = 'test label'; - error.type = 'test type'; - error.subtype = 'test subtype'; - - const errors = [error, error]; - - const req = {}; - const res = { - headers: {}, - set (key, value) { - this.headers[key] = value; - }, - statusCode: 0, - status (status) { - this.statusCode = status; - }, - json () {}, - send () {} - }; - - const errorHeader = { - mainError: { - statusCode: 400, - message: error.message, - name: error.name, - label: error.label, - type: error.type, - subtype: error.subtype - }, - moreErrors: [{ - message: error.message, - name: error.name, - label: error.label, - type: error.type, - subtype: error.subtype - }] - }; - - const errorFn = errorMiddleware(); - errorFn(errors, req, res, (err) => { - if (err) { - return done(err); - } - - assert.deepStrictEqual(res.headers, { - 'X-Tiler-Errors': JSON.stringify(errorHeader) - }); - - return done(); - }); - }); - - it('JSONP should return a header with error status code', function (done) { - const error = new Error('error test'); - error.label = 'test label'; - error.type = 'test type'; - error.subtype = 'test subtype'; - - const errors = [error, error]; - - const req = { - query: { callback: true } - }; - const res = { - headers: {}, - set (key, value) { - this.headers[key] = value; - }, - statusCode: 0, - status (status) { - this.statusCode = status; - }, - jsonp () {}, - send () {} - }; - - const errorHeader = { - mainError: { - statusCode: 400, - message: error.message, - name: error.name, - label: error.label, - type: error.type, - subtype: error.subtype - }, - moreErrors: [{ - message: error.message, - name: error.name, - label: error.label, - type: error.type, - subtype: error.subtype - }] - }; - - const errorFn = errorMiddleware(); - errorFn(errors, req, res, (err) => { - if (err) { - return done(err); - } - - assert.deepStrictEqual(res.headers, { - 'X-Tiler-Errors': JSON.stringify(errorHeader) - }); - - return done(); - }); - }); - - it('should escape chars that broke logs regex', function (done) { - const badString = 'error: ( ) = " \" \' * $ & |'; // eslint-disable-line no-useless-escape - const escapedString = 'error '; - - const error = new Error(badString); - error.label = badString; - error.type = badString; - error.subtype = badString; - - const errors = [error, error]; - - const req = {}; - const res = { - headers: {}, - set (key, value) { - this.headers[key] = value; - }, - statusCode: 0, - status (status) { - this.statusCode = status; - }, - json () {}, - send () {} - }; - - const errorHeader = { - mainError: { - statusCode: 400, - message: escapedString, - name: error.name, - label: escapedString, - type: escapedString, - subtype: escapedString - }, - moreErrors: [{ - message: escapedString, - name: error.name, - label: escapedString, - type: escapedString, - subtype: escapedString - }] - }; - - const errorFn = errorMiddleware(); - errorFn(errors, req, res, (err) => { - if (err) { - return done(err); - } - - assert.deepStrictEqual(res.headers, { - 'X-Tiler-Errors': JSON.stringify(errorHeader) - }); - - return done(); - }); - }); }); From 29c6505252413edda9929c97d6c29c84acd85703 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Tue, 2 Jun 2020 17:09:06 +0200 Subject: [PATCH 08/18] Do not set header 'x-tiler-profiler' and log it instead --- lib/api/api-router.js | 4 +- lib/api/middlewares/metrics.js | 9 +- lib/api/middlewares/{stats.js => profiler.js} | 11 +- lib/stats/profiler-proxy.js | 4 + package-lock.json | 5 - package.json | 1 - test/acceptance/dataviews/overviews-test.js | 24 +--- .../overviews-metadata-named-maps-test.js | 122 ------------------ test/acceptance/overviews-metadata-test.js | 77 ----------- 9 files changed, 17 insertions(+), 240 deletions(-) rename lib/api/middlewares/{stats.js => profiler.js} (73%) diff --git a/lib/api/api-router.js b/lib/api/api-router.js index 62d318d9..13a4fdd6 100644 --- a/lib/api/api-router.js +++ b/lib/api/api-router.js @@ -50,7 +50,7 @@ const initializeStatusCode = require('./middlewares/initialize-status-code'); const initLogger = require('./middlewares/logger'); const bodyParser = require('body-parser'); const servedByHostHeader = require('./middlewares/served-by-host-header'); -const stats = require('./middlewares/stats'); +const profiler = require('./middlewares/profiler'); const lzmaMiddleware = require('./middlewares/lzma'); const cors = require('./middlewares/cors'); const user = require('./middlewares/user'); @@ -206,7 +206,7 @@ module.exports = class ApiRouter { apiRouter.use(bodyParser.json()); apiRouter.use(servedByHostHeader()); apiRouter.use(clientHeader()); - apiRouter.use(stats({ + apiRouter.use(profiler({ enabled: this.serverOptions.useProfiler, statsClient: global.statsClient })); diff --git a/lib/api/middlewares/metrics.js b/lib/api/middlewares/metrics.js index 60478557..dc69b373 100644 --- a/lib/api/middlewares/metrics.js +++ b/lib/api/middlewares/metrics.js @@ -53,7 +53,7 @@ function getEventData (req, res, tags) { template_hash: getTemplateHash({ res }), stat_tag: getStatTag({ res }), response_code: res.statusCode.toString(), - response_time: getResponseTime(res), + response_time: getResponseTime(req), source_domain: req.hostname, event_version: EVENT_VERSION }, tags.attributes, extra); @@ -123,13 +123,12 @@ function getStatTag ({ res }) { } } -// FIXME: 'X-Tiler-Profiler' might not be accurate enough -function getResponseTime (res) { - const profiler = res.get('X-Tiler-Profiler'); +// FIXME: 'Profiler' might not be accurate enough +function getResponseTime (req) { let stats; try { - stats = JSON.parse(profiler); + stats = req.profiler.toJSON(); } catch (e) { return undefined; } diff --git a/lib/api/middlewares/stats.js b/lib/api/middlewares/profiler.js similarity index 73% rename from lib/api/middlewares/stats.js rename to lib/api/middlewares/profiler.js index 9fe212ac..9ed3eb7b 100644 --- a/lib/api/middlewares/stats.js +++ b/lib/api/middlewares/profiler.js @@ -2,20 +2,21 @@ const Profiler = require('../../stats/profiler-proxy'); const debug = require('debug')('windshaft:cartodb:stats'); -const onHeaders = require('on-headers'); -module.exports = function stats (options) { +module.exports = function profiler (options) { const { enabled = true, statsClient } = options; - return function statsMiddleware (req, res, next) { + return function profilerMiddleware (req, res, next) { + const { logger } = res.locals; + req.profiler = new Profiler({ statsd_client: statsClient, profile: enabled }); - onHeaders(res, () => res.set('X-Tiler-Profiler', req.profiler.toJSONString())); - res.on('finish', () => { + logger.info({ stats: req.profiler.toJSON() }); + try { // May throw due to dns, see: http://github.com/CartoDB/Windshaft/issues/166 req.profiler.sendStats(); diff --git a/lib/stats/profiler-proxy.js b/lib/stats/profiler-proxy.js index 9dad81f5..dc2de378 100644 --- a/lib/stats/profiler-proxy.js +++ b/lib/stats/profiler-proxy.js @@ -52,4 +52,8 @@ ProfilerProxy.prototype.toJSONString = function () { return this.profile ? this.profiler.toJSONString() : '{}'; }; +ProfilerProxy.prototype.toJSON = function () { + return this.profile ? JSON.parse(this.profiler.toJSONString()) : {}; +}; + module.exports = ProfilerProxy; diff --git a/package-lock.json b/package-lock.json index 4eb1f3c8..fe1e59de 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4920,11 +4920,6 @@ "ee-first": "1.1.1" } }, - "on-headers": { - "version": "1.0.1", - "resolved": "https://registry.npmjs.org/on-headers/-/on-headers-1.0.1.tgz", - "integrity": "sha1-ko9dD0cNSTQmUepnlLCFfBAGk/c=" - }, "once": { "version": "1.4.0", "resolved": "https://registry.npmjs.org/once/-/once-1.4.0.tgz", diff --git a/package.json b/package.json index 4e656ff0..af37467f 100644 --- a/package.json +++ b/package.json @@ -51,7 +51,6 @@ "lru-cache": "4.1.3", "lzma": "2.3.2", "node-statsd": "0.1.1", - "on-headers": "1.0.1", "pino": "^6.3.1", "queue-async": "1.1.0", "redis-mpool": "^0.8.0", diff --git a/test/acceptance/dataviews/overviews-test.js b/test/acceptance/dataviews/overviews-test.js index f46226a8..722f486c 100644 --- a/test/acceptance/dataviews/overviews-test.js +++ b/test/acceptance/dataviews/overviews-test.js @@ -55,7 +55,6 @@ describe('dataviews using tables without overviews', function () { return done(err); } assert.deepStrictEqual(formulaResult, { operation: 'count', result: 7313, nulls: 0, type: 'formula' }); - assert(getUsesOverviewsFromHeaders(headers) === false); // Overviews logging testClient.drain(done); }); @@ -269,8 +268,6 @@ describe('dataviews using tables with overviews', function () { nulls: 0, type: 'formula' }); - assert.ok(getUsesOverviewsFromHeaders(headers)); // Overviews logging - assert(getDataviewTypeFromHeaders(headers) === 'formula'); // Overviews logging testClient.drain(done); }); @@ -290,8 +287,6 @@ describe('dataviews using tables with overviews', function () { infinities: 0, nans: 0 }); - assert.ok(getUsesOverviewsFromHeaders(headers)); // Overviews logging - assert(getDataviewTypeFromHeaders(headers) === 'formula'); // Overviews logging testClient.drain(done); }); @@ -311,8 +306,6 @@ describe('dataviews using tables with overviews', function () { infinities: 0, nans: 0 }); - assert.ok(getUsesOverviewsFromHeaders(headers)); // Overviews logging - assert(getDataviewTypeFromHeaders(headers) === 'formula'); // Overviews logging testClient.drain(done); }); @@ -387,8 +380,6 @@ describe('dataviews using tables with overviews', function () { assert.ok(histogram); assert.strictEqual(histogram.type, 'histogram'); assert.ok(Array.isArray(histogram.bins)); - assert.ok(getUsesOverviewsFromHeaders(headers)); // Overviews logging - assert(getDataviewTypeFromHeaders(headers) === 'histogram'); // Overviews logging testClient.drain(done); }); @@ -480,7 +471,7 @@ describe('dataviews using tables with overviews', function () { nans: 0, type: 'formula' }); - assert.ok(getUsesOverviewsFromHeaders(headers)); // Overviews logging + testClient.drain(done); }); }); @@ -499,7 +490,6 @@ describe('dataviews using tables with overviews', function () { nans: 0, type: 'formula' }); - assert.ok(getUsesOverviewsFromHeaders(headers)); // Overviews logging testClient.drain(done); }); @@ -519,7 +509,6 @@ describe('dataviews using tables with overviews', function () { nulls: 0, type: 'formula' }); - assert.ok(getUsesOverviewsFromHeaders(headers)); // Overviews logging testClient.drain(done); }); @@ -611,9 +600,6 @@ describe('dataviews using tables with overviews', function () { type: 'aggregation' }); - assert.ok(getUsesOverviewsFromHeaders(headers)); // Overviews logging - assert(getDataviewTypeFromHeaders(headers) === 'aggregation'); // Overviews logging - testClient.drain(done); }); }); @@ -830,11 +816,3 @@ 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; -} diff --git a/test/acceptance/overviews-metadata-named-maps-test.js b/test/acceptance/overviews-metadata-named-maps-test.js index 10dc2458..be41488b 100644 --- a/test/acceptance/overviews-metadata-named-maps-test.js +++ b/test/acceptance/overviews-metadata-named-maps-test.js @@ -172,126 +172,4 @@ describe('overviews metadata for named maps', function () { } ); }); - - describe('Overviews Flags', function () { - it('Overviews used', function (done) { - step( - function postTemplate () { - var next = this; - - assert.response(server, { - url: '/api/v1/map/named?api_key=1234', - method: 'POST', - headers: { host: 'localhost', 'Content-Type': 'application/json' }, - data: JSON.stringify(template) - }, {}, function (res, err) { - next(err, res); - }); - }, - function instantiateTemplate (err) { - assert.ifError(err); - - var next = this; - assert.response(server, { - url: '/api/v1/map/named/' + templateId, - method: 'POST', - headers: { - host: 'localhost', - 'Content-Type': 'application/json' - } - }, {}, - function (res, err) { - return next(err, res); - }); - }, - function checkFlags (err, res) { - assert.ifError(err); - - var next = this; - - var parsedBody = JSON.parse(res.body); - - keysToDelete['map_cfg|' + LayergroupToken.parse(parsedBody.layergroupid).token] = 0; - keysToDelete['user:localhost:mapviews:global'] = 5; - - const headers = JSON.parse(res.headers['x-tiler-profiler']); - - assert.ok(headers.overviewsAddedToMapconfig); - assert.strictEqual(headers.mapType, 'named'); - - next(); - }, - - function finish (err) { - done(err); - } - ); - }); - - it('Overviews NOT used', function (done) { - const nonOverviewsTemplateId = 'non-overviews-template'; - - var nonOverviewsTemplate = { - version: '0.0.1', - name: nonOverviewsTemplateId, - auth: { method: 'open' }, - layergroup: { - version: '1.0.0', - layers: [nonOverviewsLayer] - } - }; - - step( - function postTemplate () { - var next = this; - - assert.response(server, { - url: '/api/v1/map/named?api_key=1234', - method: 'POST', - headers: { host: 'localhost', 'Content-Type': 'application/json' }, - data: JSON.stringify(nonOverviewsTemplate) - }, {}, function (res, err) { - next(err, res); - }); - }, - function instantiateTemplate (err) { - assert.ifError(err); - - var next = this; - assert.response(server, { - url: '/api/v1/map/named/' + nonOverviewsTemplateId, - method: 'POST', - headers: { - host: 'localhost', - 'Content-Type': 'application/json' - } - }, {}, - function (res, err) { - return next(err, res); - }); - }, - function checkFlags (err, res) { - assert.ifError(err); - - var next = this; - - var parsedBody = JSON.parse(res.body); - - keysToDelete['map_cfg|' + LayergroupToken.parse(parsedBody.layergroupid).token] = 0; - keysToDelete['user:localhost:mapviews:global'] = 5; - - const headers = JSON.parse(res.headers['x-tiler-profiler']); - - assert.strictEqual(headers.overviewsAddedToMapconfig, false); - assert.strictEqual(headers.mapType, 'named'); - - next(); - }, - - function finish (err) { - done(err); - } - ); - }); - }); }); diff --git a/test/acceptance/overviews-metadata-test.js b/test/acceptance/overviews-metadata-test.js index 828c34e3..e2ca3be3 100644 --- a/test/acceptance/overviews-metadata-test.js +++ b/test/acceptance/overviews-metadata-test.js @@ -110,83 +110,6 @@ describe('overviews metadata', function () { } ); }); - - describe('Overviews Flags', function () { - it('Overviews used', function (done) { - var layergroup = { - version: '1.0.0', - layers: [overviewsLayer, nonOverviewsLayer] - }; - - var layergroupUrl = '/api/v1/map'; - - var expectedToken; - step( - function doPost () { - var next = this; - assert.response(server, { - url: layergroupUrl, - method: 'POST', - headers: { host: 'localhost', 'Content-Type': 'application/json' }, - data: JSON.stringify(layergroup) - }, {}, function (res) { - assert.strictEqual(res.statusCode, 200, res.body); - - const headers = JSON.parse(res.headers['x-tiler-profiler']); - - assert.ok(headers.overviewsAddedToMapconfig); - assert.strictEqual(headers.mapType, 'anonymous'); - - const parsedBody = JSON.parse(res.body); - expectedToken = parsedBody.layergroupid; - next(); - }); - }, - function finish (err) { - keysToDelete['map_cfg|' + LayergroupToken.parse(expectedToken).token] = 0; - keysToDelete['user:localhost:mapviews:global'] = 5; - done(err); - } - ); - }); - it('Overviews NOT used', function (done) { - var layergroup = { - version: '1.0.0', - layers: [nonOverviewsLayer] - }; - - var layergroupUrl = '/api/v1/map'; - - var expectedToken; - step( - function doPost () { - var next = this; - assert.response(server, { - url: layergroupUrl, - method: 'POST', - headers: { host: 'localhost', 'Content-Type': 'application/json' }, - data: JSON.stringify(layergroup) - }, {}, function (res) { - assert.strictEqual(res.statusCode, 200, res.body); - - const headers = JSON.parse(res.headers['x-tiler-profiler']); - - assert.strictEqual(headers.overviewsAddedToMapconfig, false); - assert.strictEqual(headers.mapType, 'anonymous'); - - const parsedBody = JSON.parse(res.body); - expectedToken = parsedBody.layergroupid; - next(); - }); - }, - function finish (err) { - keysToDelete['map_cfg|' + LayergroupToken.parse(expectedToken).token] = 0; - keysToDelete['user:localhost:mapviews:global'] = 5; - done(err); - } - ); - }); - }); }); describe('overviews metadata with filters', function () { From 1e89821d976070101115287201eba78eda10db97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 3 Jun 2020 14:28:35 +0200 Subject: [PATCH 09/18] Use standard serializers for error, request, and response --- lib/api/middlewares/error-middleware.js | 5 +++-- lib/api/middlewares/logger.js | 4 ++-- lib/utils/logger.js | 7 ++++++- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/api/middlewares/error-middleware.js b/lib/api/middlewares/error-middleware.js index 0822c144..583b54f5 100644 --- a/lib/api/middlewares/error-middleware.js +++ b/lib/api/middlewares/error-middleware.js @@ -9,6 +9,7 @@ module.exports = function errorMiddleware (/* options */) { allErrors = populateLimitErrors(allErrors); + // TODO REMOVE THIS THREE LINES PLEASE const label = err.label || 'UNKNOWN'; err = allErrors[0] || new Error(label); allErrors[0] = err; @@ -20,8 +21,8 @@ module.exports = function errorMiddleware (/* options */) { statusCode = 200; } - allErrors.forEach((err) => debug('[%s ERROR] -- %d: %s, %s', label, statusCode, err, err.stack)); - allErrors.forEach((err) => logger.error(err)); + allErrors.forEach((err) => debug('[%s ERROR] -- %d: %s, %s', err.label || 'UNKNOWN', statusCode, err, err.stack)); + logger.error({ errors: allErrors }); var errorResponseBody = { errors: allErrors.map(errorMessage), diff --git a/lib/api/middlewares/logger.js b/lib/api/middlewares/logger.js index 93a02a03..09b92ef0 100644 --- a/lib/api/middlewares/logger.js +++ b/lib/api/middlewares/logger.js @@ -7,8 +7,8 @@ module.exports = function initLogger ({ logger }) { const id = req.get('X-Request-Id') || uuid.v4(); res.locals.logger = logger.child({ id }); - res.locals.logger.info(req); - res.on('finish', () => res.locals.logger.info(res)); + res.locals.logger.info({ request: req }); + res.on('finish', () => res.locals.logger.info({ response: res })); next(); }; diff --git a/lib/utils/logger.js b/lib/utils/logger.js index 33646049..d9c9b85c 100644 --- a/lib/utils/logger.js +++ b/lib/utils/logger.js @@ -8,7 +8,12 @@ module.exports = class Logger { const logLevelFromNodeEnv = NODE_ENV === 'test' ? 'fatal' : 'info'; const options = { base: null, // Do not bind hostname, pid and friends by default - level: LOG_LEVEL || logLevelFromNodeEnv + level: LOG_LEVEL || logLevelFromNodeEnv, + serializers: { + request: pino.stdSerializers.req, + response: pino.stdSerializers.res, + errors: (errors) => errors.map((err) => pino.stdSerializers.err(err)) + } }; const dest = pino.destination({ sync: false }); // stdout From 219d2c9044f80ef884cd563f05b4e8b43ccbb010 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 3 Jun 2020 15:10:31 +0200 Subject: [PATCH 10/18] Shortcuts for serializers --- lib/utils/logger.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/utils/logger.js b/lib/utils/logger.js index d9c9b85c..0a4b3df0 100644 --- a/lib/utils/logger.js +++ b/lib/utils/logger.js @@ -1,6 +1,7 @@ 'use strict'; const pino = require('pino'); +const { req: requestSerializer, res: responseSerializer, err: errorSerializer } = pino.stdSerializers; module.exports = class Logger { constructor () { @@ -10,9 +11,9 @@ module.exports = class Logger { base: null, // Do not bind hostname, pid and friends by default level: LOG_LEVEL || logLevelFromNodeEnv, serializers: { - request: pino.stdSerializers.req, - response: pino.stdSerializers.res, - errors: (errors) => errors.map((err) => pino.stdSerializers.err(err)) + request: requestSerializer, + response: responseSerializer, + errors: (errors) => errors.map((err) => errorSerializer(err)) } }; const dest = pino.destination({ sync: false }); // stdout From 107a97aa9eb705198f225c7966855722275db661 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 3 Jun 2020 15:11:08 +0200 Subject: [PATCH 11/18] Honor @oleurud's comment --- lib/api/api-router.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/api/api-router.js b/lib/api/api-router.js index 13a4fdd6..9491b506 100644 --- a/lib/api/api-router.js +++ b/lib/api/api-router.js @@ -235,10 +235,10 @@ function createTemplateMaps ({ redisPool, surrogateKeysCache, logger }) { const startTime = Date.now(); surrogateKeysCache.invalidate(new NamedMapsCacheEntry(user, templateName), (err) => { if (err) { - return logger.error(err); + return logger.error(err, `Named map (${templateName}) invalidation failed, user: ${user}`); } - logger.info({ user, type: 'named_map_invalidation', elapsed: Date.now() - startTime }); + logger.info({ user, type: 'named_map_invalidation', elapsed: Date.now() - startTime }, `Named map (${templateName}) invalidation success, user: ${user}`); }); } From c37e3f173d0a06aa0e0cff46d4b546569019d32f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 3 Jun 2020 15:39:02 +0200 Subject: [PATCH 12/18] Handle error properly in user middleware, it will logged in error middleware --- lib/api/middlewares/user.js | 11 +++++++---- lib/models/cdb-request.js | 11 +++-------- test/unit/cdb-request-test.js | 33 +++++++++++++-------------------- 3 files changed, 23 insertions(+), 32 deletions(-) diff --git a/lib/api/middlewares/user.js b/lib/api/middlewares/user.js index 31e7f909..814b62e6 100644 --- a/lib/api/middlewares/user.js +++ b/lib/api/middlewares/user.js @@ -3,11 +3,14 @@ const CdbRequest = require('../../models/cdb-request'); module.exports = function user (metadataBackend) { - return function userMiddleware (req, res, next) { - const { logger } = res.locals; - const cdbRequest = new CdbRequest({ logger }); + const cdbRequest = new CdbRequest(); - res.locals.user = getUserNameFromRequest(req, cdbRequest); + return function userMiddleware (req, res, next) { + try { + res.locals.user = getUserNameFromRequest(req, cdbRequest); + } catch (err) { + return next(err); + } metadataBackend.getUserId(res.locals.user, (err, userId) => { if (err || !userId) { diff --git a/lib/models/cdb-request.js b/lib/models/cdb-request.js index a962b558..ca160649 100644 --- a/lib/models/cdb-request.js +++ b/lib/models/cdb-request.js @@ -1,8 +1,7 @@ 'use strict'; module.exports = class CdbRequest { - constructor ({ logger }) { - this.logger = logger; + constructor () { // would extract "strk" from "strk.cartodb.com" this.RE_USER_FROM_HOST = new RegExp(global.environment.user_from_host || '^([^\\.]+)\\.'); } @@ -16,12 +15,8 @@ module.exports = class CdbRequest { const mat = host.match(this.RE_USER_FROM_HOST); - if (!mat) { - return this.logger.error(new Error(`Pattern '${this.RE_USER_FROM_HOST}' does not match hostname '${host}'`)); - } - - if (mat.length !== 2) { - return this.logger.error(new Error(`Pattern '${this.RE_USER_FROM_HOST}' gave unexpected matches against '${host}': ${mat}`)); + if (!mat || mat.length !== 2) { + throw new Error(`No username found in hostname '${host}'`); } return mat[1]; diff --git a/test/unit/cdb-request-test.js b/test/unit/cdb-request-test.js index 9db38488..93d1894a 100644 --- a/test/unit/cdb-request-test.js +++ b/test/unit/cdb-request-test.js @@ -4,9 +4,8 @@ require('../support/test-helper'); var assert = require('assert'); var CdbRequest = require('../../lib/models/cdb-request'); -const { logger } = require('../../lib/server-options'); -describe('req2params', function () { +describe('username in host header (CdbRequest)', function () { function createRequest (host, userParam) { var req = { params: {}, @@ -23,7 +22,7 @@ describe('req2params', function () { } it('extracts name from host header', function () { - var cdbRequest = new CdbRequest({ logger }); + var cdbRequest = new CdbRequest(); var user = cdbRequest.userByReq(createRequest('localhost')); assert.strictEqual(user, 'localhost'); @@ -33,7 +32,7 @@ describe('req2params', function () { var userFromHostConfig = global.environment.user_from_host; global.environment.user_from_host = null; - var cdbRequest = new CdbRequest({ logger }); + var cdbRequest = new CdbRequest(); var user = cdbRequest.userByReq(createRequest('development.localhost.lan')); global.environment.user_from_host = userFromHostConfig; @@ -42,45 +41,39 @@ describe('req2params', function () { }); it('considers user param before headers', function () { - var cdbRequest = new CdbRequest({ logger }); + var cdbRequest = new CdbRequest(); var user = cdbRequest.userByReq(createRequest('localhost', 'development')); assert.strictEqual(user, 'development'); }); - it('returns undefined when it cannot extract username', function () { + it('returns throw when it cannot extract username', function () { var userFromHostConfig = global.environment.user_from_host; global.environment.user_from_host = null; - var cdbRequest = new CdbRequest({ logger }); - var user = cdbRequest.userByReq(createRequest('localhost')); + var cdbRequest = new CdbRequest(); + assert.throws(() => cdbRequest.userByReq(createRequest('localhost'))); global.environment.user_from_host = userFromHostConfig; - - assert.strictEqual(user, undefined); }); - it('should not fail for undefined host header', function () { + it('should throw for undefined host header', function () { var userFromHostConfig = global.environment.user_from_host; global.environment.user_from_host = null; - var cdbRequest = new CdbRequest({ logger }); - var user = cdbRequest.userByReq(createRequest(undefined)); + var cdbRequest = new CdbRequest(); + assert.throws(() => cdbRequest.userByReq(createRequest(undefined))); global.environment.user_from_host = userFromHostConfig; - - assert.strictEqual(user, undefined); }); - it('should not fail for null host header', function () { + it('should throw for null host header', function () { var userFromHostConfig = global.environment.user_from_host; global.environment.user_from_host = null; - var cdbRequest = new CdbRequest({ logger }); - var user = cdbRequest.userByReq(createRequest(null)); + var cdbRequest = new CdbRequest(); + assert.throws(() => cdbRequest.userByReq(createRequest(null))); global.environment.user_from_host = userFromHostConfig; - - assert.strictEqual(user, undefined); }); }); From 0eadfe6ee9c0aceb9ea7fe3ca61a603ea29b3a5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 3 Jun 2020 15:51:36 +0200 Subject: [PATCH 13/18] Simpligy error middleware --- lib/api/middlewares/error-middleware.js | 31 +++++++------------------ 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/lib/api/middlewares/error-middleware.js b/lib/api/middlewares/error-middleware.js index 583b54f5..f4258383 100644 --- a/lib/api/middlewares/error-middleware.js +++ b/lib/api/middlewares/error-middleware.js @@ -1,35 +1,20 @@ 'use strict'; -const debug = require('debug')('windshaft:cartodb:error-middleware'); - module.exports = function errorMiddleware (/* options */) { return function error (err, req, res, next) { const { logger } = res.locals; - var allErrors = Array.isArray(err) ? err : [err]; + const errors = populateLimitErrors(Array.isArray(err) ? err : [err]); + let statusCode = findStatusCode(errors[0]); - allErrors = populateLimitErrors(allErrors); + logger.error({ errors }); - // TODO REMOVE THIS THREE LINES PLEASE - const label = err.label || 'UNKNOWN'; - err = allErrors[0] || new Error(label); - allErrors[0] = err; - - var statusCode = findStatusCode(err); - - // If a callback was requested, force status to 200 - if (req.query && req.query.callback) { - statusCode = 200; - } - - allErrors.forEach((err) => debug('[%s ERROR] -- %d: %s, %s', err.label || 'UNKNOWN', statusCode, err, err.stack)); - logger.error({ errors: allErrors }); - - var errorResponseBody = { - errors: allErrors.map(errorMessage), - errors_with_context: allErrors.map(errorMessageWithContext) + const errorResponseBody = { + errors: errors.map(errorMessage), + errors_with_context: errors.map(errorMessageWithContext) }; - res.status(statusCode); + // If a callback was requested, force status to 200 + res.status(req.query.callback ? 200 : statusCode); if (req.query && req.query.callback) { res.jsonp(errorResponseBody); From 1dda183a31b41cf193fae7557f9d9193688e82de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 3 Jun 2020 16:19:42 +0200 Subject: [PATCH 14/18] typo --- lib/api/middlewares/error-middleware.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/api/middlewares/error-middleware.js b/lib/api/middlewares/error-middleware.js index f4258383..645a8992 100644 --- a/lib/api/middlewares/error-middleware.js +++ b/lib/api/middlewares/error-middleware.js @@ -4,7 +4,6 @@ module.exports = function errorMiddleware (/* options */) { return function error (err, req, res, next) { const { logger } = res.locals; const errors = populateLimitErrors(Array.isArray(err) ? err : [err]); - let statusCode = findStatusCode(errors[0]); logger.error({ errors }); @@ -14,7 +13,7 @@ module.exports = function errorMiddleware (/* options */) { }; // If a callback was requested, force status to 200 - res.status(req.query.callback ? 200 : statusCode); + res.status(req.query.callback ? 200 : findStatusCode(errors[0])); if (req.query && req.query.callback) { res.jsonp(errorResponseBody); From 210f5b01ec3fce48d5075fb9c100789de14b953d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 3 Jun 2020 17:32:16 +0200 Subject: [PATCH 15/18] Make sure all errors use the serializer set for the logger --- lib/api/map/preview-template-controller.js | 6 ++++-- lib/api/middlewares/cache-channel-header.js | 3 ++- lib/api/middlewares/cache-control-header.js | 3 ++- lib/api/middlewares/error-middleware.js | 2 +- lib/api/middlewares/increment-map-view-count.js | 3 ++- lib/api/middlewares/last-modified-header.js | 3 ++- lib/api/middlewares/metrics.js | 2 ++ lib/api/middlewares/surrogate-key-header.js | 3 ++- lib/utils/logger.js | 2 +- test/acceptance/turbo-carto/regressions-test.js | 2 +- 10 files changed, 19 insertions(+), 10 deletions(-) diff --git a/lib/api/map/preview-template-controller.js b/lib/api/map/preview-template-controller.js index 92d4380e..49ee18c7 100644 --- a/lib/api/map/preview-template-controller.js +++ b/lib/api/map/preview-template-controller.js @@ -342,7 +342,8 @@ function incrementMapViews ({ metadataBackend }) { mapConfigProvider.getMapConfig((err, mapConfig) => { if (err) { - logger.warn(err, `ERROR: failed to increment mapview count for user '${user}'`); + err.message = `Failed to increment mapview count for user '${user}'. ${err.message}`; + logger.warn({ error: err }); return next(); } @@ -354,7 +355,8 @@ function incrementMapViews ({ metadataBackend }) { metadataBackend.incMapviewCount(user, statTag, (err) => { if (err) { - logger.warn(err, `ERROR: failed to increment mapview count for user '${user}'`); + err.message = `Failed to increment mapview count for user '${user}'. ${err.message}`; + logger.warn({ error: err }); } next(); diff --git a/lib/api/middlewares/cache-channel-header.js b/lib/api/middlewares/cache-channel-header.js index 66486392..622aa053 100644 --- a/lib/api/middlewares/cache-channel-header.js +++ b/lib/api/middlewares/cache-channel-header.js @@ -10,7 +10,8 @@ module.exports = function setCacheChannelHeader () { mapConfigProvider.getAffectedTables((err, affectedTables) => { if (err) { - logger.warn(err, 'ERROR generating Cache Channel Header'); + err.message = `Error generating Cache Channel Header. ${err.message}`; + logger.warn({ error: err }); return next(); } diff --git a/lib/api/middlewares/cache-control-header.js b/lib/api/middlewares/cache-control-header.js index 18ec7728..5ae0d481 100644 --- a/lib/api/middlewares/cache-control-header.js +++ b/lib/api/middlewares/cache-control-header.js @@ -44,7 +44,8 @@ module.exports = function setCacheControlHeader ({ mapConfigProvider.getAffectedTables((err, affectedTables) => { if (err) { - logger.warn(err, 'ERROR generating Cache Control Header'); + err.message = `Error generating Cache Control Header. ${err.message}`; + logger.warn({ error: err }); return next(); } diff --git a/lib/api/middlewares/error-middleware.js b/lib/api/middlewares/error-middleware.js index 645a8992..faca863f 100644 --- a/lib/api/middlewares/error-middleware.js +++ b/lib/api/middlewares/error-middleware.js @@ -5,7 +5,7 @@ module.exports = function errorMiddleware (/* options */) { const { logger } = res.locals; const errors = populateLimitErrors(Array.isArray(err) ? err : [err]); - logger.error({ errors }); + logger.error({ error: errors }); const errorResponseBody = { errors: errors.map(errorMessage), diff --git a/lib/api/middlewares/increment-map-view-count.js b/lib/api/middlewares/increment-map-view-count.js index 1b71e8e3..ea4fc285 100644 --- a/lib/api/middlewares/increment-map-view-count.js +++ b/lib/api/middlewares/increment-map-view-count.js @@ -14,7 +14,8 @@ module.exports = function incrementMapViewCount (metadataBackend) { req.profiler.done('incMapviewCount'); if (err) { - logger.warn(err, `ERROR: failed to increment mapview count for user '${user}'`); + err.message = `Failed to increment mapview count for user '${user}'. ${err.message}`; + logger.warn({ error: err }); } next(); diff --git a/lib/api/middlewares/last-modified-header.js b/lib/api/middlewares/last-modified-header.js index b68d7c31..f2d41292 100644 --- a/lib/api/middlewares/last-modified-header.js +++ b/lib/api/middlewares/last-modified-header.js @@ -21,7 +21,8 @@ module.exports = function setLastModifiedHeader () { mapConfigProvider.getAffectedTables((err, affectedTables) => { if (err) { - logger.warn(err, 'ERROR generating Last Modified Header'); + err.message = `Error generating Last Modified Header. ${err.message}`; + logger.warn({ error: err }); return next(); } diff --git a/lib/api/middlewares/metrics.js b/lib/api/middlewares/metrics.js index dc69b373..6b7a9ff8 100644 --- a/lib/api/middlewares/metrics.js +++ b/lib/api/middlewares/metrics.js @@ -15,6 +15,8 @@ module.exports = function metrics ({ enabled, tags, metricsBackend }) { } return function metricsMiddleware (req, res, next) { + // FIXME: use parent logger as we don't want bind the error to the request + // but we still want to know if an error is thrown const { logger } = res.locals; res.on('finish', () => { diff --git a/lib/api/middlewares/surrogate-key-header.js b/lib/api/middlewares/surrogate-key-header.js index c1415038..c8fd550d 100644 --- a/lib/api/middlewares/surrogate-key-header.js +++ b/lib/api/middlewares/surrogate-key-header.js @@ -17,7 +17,8 @@ module.exports = function setSurrogateKeyHeader ({ surrogateKeysCache }) { mapConfigProvider.getAffectedTables((err, affectedTables) => { if (err) { - logger.warn(err, 'ERROR generating Surrogate Key Header'); + err.message = `Erros generating Surrogate Key Header. ${err.message}`; + logger.warn({ error: err }); return next(); } diff --git a/lib/utils/logger.js b/lib/utils/logger.js index 0a4b3df0..54beee2e 100644 --- a/lib/utils/logger.js +++ b/lib/utils/logger.js @@ -13,7 +13,7 @@ module.exports = class Logger { serializers: { request: requestSerializer, response: responseSerializer, - errors: (errors) => errors.map((err) => errorSerializer(err)) + error: (error) => Array.isArray(error) ? error.map((err) => errorSerializer(err)) : [errorSerializer(error)] } }; const dest = pino.destination({ sync: false }); // stdout diff --git a/test/acceptance/turbo-carto/regressions-test.js b/test/acceptance/turbo-carto/regressions-test.js index e36dc2e1..d0ba7d89 100644 --- a/test/acceptance/turbo-carto/regressions-test.js +++ b/test/acceptance/turbo-carto/regressions-test.js @@ -67,7 +67,7 @@ describe('turbo-carto regressions', function () { }); }); - it('should fail for private tables', function (done) { + it.only('should fail for private tables', function (done) { var cartocss = [ '#private_table {', ' marker-placement: point;', From d073f7e3dd8cb19d48233a3805e002d3d9fa26c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 3 Jun 2020 17:34:30 +0200 Subject: [PATCH 16/18] typo --- test/acceptance/turbo-carto/regressions-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/acceptance/turbo-carto/regressions-test.js b/test/acceptance/turbo-carto/regressions-test.js index d0ba7d89..e36dc2e1 100644 --- a/test/acceptance/turbo-carto/regressions-test.js +++ b/test/acceptance/turbo-carto/regressions-test.js @@ -67,7 +67,7 @@ describe('turbo-carto regressions', function () { }); }); - it.only('should fail for private tables', function (done) { + it('should fail for private tables', function (done) { var cartocss = [ '#private_table {', ' marker-placement: point;', From 7b53b7c30a7a57f800d639273aaa87c85e294b2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 3 Jun 2020 19:51:56 +0200 Subject: [PATCH 17/18] Stop using profiling wrongly. Now it only saves custom events from backends (tile, map, attributes, etc..) and calculates the response time. Besides, removed tags to know whether overviews are being used. --- lib/api/map/anonymous-map-controller.js | 11 +-------- .../map/attributes-layergroup-controller.js | 2 -- ...lustered-features-layergroup-controller.js | 2 -- lib/api/map/preview-layergroup-controller.js | 2 -- lib/api/map/preview-template-controller.js | 5 ++-- lib/api/map/tile-layergroup-controller.js | 2 -- lib/api/middlewares/authorize.js | 2 -- .../middlewares/check-json-content-type.js | 2 -- lib/api/middlewares/db-conn-setup.js | 2 -- .../middlewares/increment-map-view-count.js | 2 -- lib/api/middlewares/init-profiler.js | 11 --------- lib/api/middlewares/lzma.js | 2 -- lib/api/middlewares/map-error.js | 1 - lib/api/middlewares/profiler.js | 4 ++++ lib/api/middlewares/send-response.js | 2 -- lib/api/template/admin-template-controller.js | 6 ----- lib/api/template/named-template-controller.js | 8 ------- lib/api/template/tile-template-controller.js | 3 +-- lib/backends/auth.js | 6 ----- lib/models/dataview/base.js | 17 ++----------- lib/models/dataview/overviews/aggregation.js | 2 +- lib/models/dataview/overviews/formula.js | 2 +- lib/models/dataview/overviews/histogram.js | 2 +- .../adapter/mapconfig-overviews-adapter.js | 24 +++++++------------ 24 files changed, 21 insertions(+), 101 deletions(-) delete mode 100644 lib/api/middlewares/init-profiler.js diff --git a/lib/api/map/anonymous-map-controller.js b/lib/api/map/anonymous-map-controller.js index 7236935b..ad0f4324 100644 --- a/lib/api/map/anonymous-map-controller.js +++ b/lib/api/map/anonymous-map-controller.js @@ -7,7 +7,6 @@ const cleanUpQueryParams = require('../middlewares/clean-up-query-params'); const credentials = require('../middlewares/credentials'); const dbConnSetup = require('../middlewares/db-conn-setup'); const authorize = require('../middlewares/authorize'); -const initProfiler = require('../middlewares/init-profiler'); const checkJsonContentType = require('../middlewares/check-json-content-type'); const incrementMapViewCount = require('../middlewares/increment-map-view-count'); const augmentLayergroupData = require('../middlewares/augment-layergroup-data'); @@ -76,7 +75,6 @@ module.exports = class AnonymousMapController { } middlewares () { - const isTemplateInstantiation = false; const useTemplateHash = false; const includeQuery = true; const label = 'ANONYMOUS LAYERGROUP'; @@ -102,7 +100,6 @@ module.exports = class AnonymousMapController { dbConnSetup(this.pgConnection), rateLimit(this.userLimitsBackend, RATE_LIMIT_ENDPOINTS_GROUPS.ANONYMOUS), cleanUpQueryParams(['aggregation']), - initProfiler(isTemplateInstantiation), checkJsonContentType(), checkCreateLayergroup(), prepareAdapterMapConfig(this.mapConfigAdapter), @@ -143,7 +140,6 @@ function checkCreateLayergroup () { } } - req.profiler.done('checkCreateLayergroup'); return next(); }; } @@ -179,12 +175,7 @@ function prepareAdapterMapConfig (mapConfigAdapter) { requestMapConfig, params, context, - (err, requestMapConfig, stats = { overviewsAddedToMapconfig: false }) => { - req.profiler.done('anonymous.getMapConfig'); - - stats.mapType = 'anonymous'; - req.profiler.add(stats); - + (err, requestMapConfig) => { if (err) { return next(err); } diff --git a/lib/api/map/attributes-layergroup-controller.js b/lib/api/map/attributes-layergroup-controller.js index f734d3d2..ad0f1bc8 100644 --- a/lib/api/map/attributes-layergroup-controller.js +++ b/lib/api/map/attributes-layergroup-controller.js @@ -61,8 +61,6 @@ module.exports = class AttributesLayergroupController { function getFeatureAttributes (attributesBackend) { return function getFeatureAttributesMiddleware (req, res, next) { - req.profiler.start('windshaft.maplayer_attribute'); - const { mapConfigProvider } = res.locals; const { token } = res.locals; const { dbuser, dbname, dbpassword, dbhost, dbport } = res.locals; diff --git a/lib/api/map/clustered-features-layergroup-controller.js b/lib/api/map/clustered-features-layergroup-controller.js index 7b300ac7..611813c4 100644 --- a/lib/api/map/clustered-features-layergroup-controller.js +++ b/lib/api/map/clustered-features-layergroup-controller.js @@ -62,8 +62,6 @@ module.exports = class AggregatedFeaturesLayergroupController { function getClusteredFeatures (clusterBackend) { return function getFeatureAttributesMiddleware (req, res, next) { - req.profiler.start('windshaft.maplayer_cluster_features'); - const { mapConfigProvider } = res.locals; const { user, token } = res.locals; const { dbuser, dbname, dbpassword, dbhost, dbport } = res.locals; diff --git a/lib/api/map/preview-layergroup-controller.js b/lib/api/map/preview-layergroup-controller.js index 698d94ff..0d8edf9e 100644 --- a/lib/api/map/preview-layergroup-controller.js +++ b/lib/api/map/preview-layergroup-controller.js @@ -100,7 +100,6 @@ function getPreviewImageByCenter (previewBackend) { const options = { mapConfigProvider, format, width, height, zoom, center }; previewBackend.getImage(options, (err, image, stats = {}) => { - req.profiler.done(`render-${format}`); req.profiler.add(stats); if (err) { @@ -133,7 +132,6 @@ function getPreviewImageByBoundingBox (previewBackend) { const options = { mapConfigProvider, format, width, height, bbox }; previewBackend.getImage(options, (err, image, stats = {}) => { - req.profiler.done(`render-${format}`); req.profiler.add(stats); if (err) { diff --git a/lib/api/map/preview-template-controller.js b/lib/api/map/preview-template-controller.js index 49ee18c7..287c0122 100644 --- a/lib/api/map/preview-template-controller.js +++ b/lib/api/map/preview-template-controller.js @@ -292,7 +292,7 @@ function getImage ({ previewBackend, label }) { if (zoom !== undefined && center) { const options = { mapConfigProvider, format, width, height, zoom, center }; - return previewBackend.getImage(options, (err, image, stats) => { + return previewBackend.getImage(options, (err, image, stats = {}) => { req.profiler.add(stats); if (err) { @@ -309,9 +309,8 @@ function getImage ({ previewBackend, label }) { const options = { mapConfigProvider, format, width, height, bbox }; - previewBackend.getImage(options, (err, image, stats) => { + previewBackend.getImage(options, (err, image, stats = {}) => { req.profiler.add(stats); - req.profiler.done('render-' + format); if (err) { err.label = label; diff --git a/lib/api/map/tile-layergroup-controller.js b/lib/api/map/tile-layergroup-controller.js index a067c2e6..5c5c468f 100644 --- a/lib/api/map/tile-layergroup-controller.js +++ b/lib/api/map/tile-layergroup-controller.js @@ -96,8 +96,6 @@ function getStatusCode (tile, format) { function getTile (tileBackend) { return function getTileMiddleware (req, res, next) { - req.profiler.start(`windshaft.${req.params.layer ? 'maplayer_tile' : 'map_tile'}`); - const { mapConfigProvider } = res.locals; const { token } = res.locals; const { layer, z, x, y, format } = req.params; diff --git a/lib/api/middlewares/authorize.js b/lib/api/middlewares/authorize.js index 506f0b77..82f20a12 100644 --- a/lib/api/middlewares/authorize.js +++ b/lib/api/middlewares/authorize.js @@ -3,8 +3,6 @@ module.exports = function authorize (authBackend) { return function authorizeMiddleware (req, res, next) { authBackend.authorize(req, res, (err, authorized) => { - req.profiler.done('authorize'); - if (err) { return next(err); } diff --git a/lib/api/middlewares/check-json-content-type.js b/lib/api/middlewares/check-json-content-type.js index 57d7b5a8..e894809c 100644 --- a/lib/api/middlewares/check-json-content-type.js +++ b/lib/api/middlewares/check-json-content-type.js @@ -6,8 +6,6 @@ module.exports = function checkJsonContentType () { return next(new Error('POST data must be of type application/json')); } - req.profiler.done('checkJsonContentTypeMiddleware'); - next(); }; }; diff --git a/lib/api/middlewares/db-conn-setup.js b/lib/api/middlewares/db-conn-setup.js index 7a7e76a8..a81e8ba0 100644 --- a/lib/api/middlewares/db-conn-setup.js +++ b/lib/api/middlewares/db-conn-setup.js @@ -7,8 +7,6 @@ module.exports = function dbConnSetup (pgConnection) { const { user } = res.locals; pgConnection.setDBConn(user, res.locals, (err) => { - req.profiler.done('dbConnSetup'); - if (err) { if (err.message && err.message.indexOf('name not found') !== -1) { err.http_status = 404; diff --git a/lib/api/middlewares/increment-map-view-count.js b/lib/api/middlewares/increment-map-view-count.js index ea4fc285..c226d1d6 100644 --- a/lib/api/middlewares/increment-map-view-count.js +++ b/lib/api/middlewares/increment-map-view-count.js @@ -11,8 +11,6 @@ module.exports = function incrementMapViewCount (metadataBackend) { // Error won't blow up, just be logged. metadataBackend.incMapviewCount(user, statTag, (err) => { - req.profiler.done('incMapviewCount'); - if (err) { err.message = `Failed to increment mapview count for user '${user}'. ${err.message}`; logger.warn({ error: err }); diff --git a/lib/api/middlewares/init-profiler.js b/lib/api/middlewares/init-profiler.js deleted file mode 100644 index 86916822..00000000 --- a/lib/api/middlewares/init-profiler.js +++ /dev/null @@ -1,11 +0,0 @@ -'use strict'; - -module.exports = function initProfiler (isTemplateInstantiation) { - const operation = isTemplateInstantiation ? 'instance_template' : 'createmap'; - - return function initProfilerMiddleware (req, res, next) { - req.profiler.start(`windshaft-cartodb.${operation}_${req.method.toLowerCase()}`); - req.profiler.done(`${operation}.initProfilerMiddleware`); - next(); - }; -}; diff --git a/lib/api/middlewares/lzma.js b/lib/api/middlewares/lzma.js index b47c3b28..7e8d8051 100644 --- a/lib/api/middlewares/lzma.js +++ b/lib/api/middlewares/lzma.js @@ -24,8 +24,6 @@ module.exports = function lzma () { delete req.query.lzma; Object.assign(req.query, JSON.parse(result)); - req.profiler.done('lzma'); - next(); } catch (err) { next(new Error('Error parsing lzma as JSON: ' + err)); diff --git a/lib/api/middlewares/map-error.js b/lib/api/middlewares/map-error.js index fcc5ef2a..b3c915ee 100644 --- a/lib/api/middlewares/map-error.js +++ b/lib/api/middlewares/map-error.js @@ -4,7 +4,6 @@ module.exports = function mapError (options) { const { addContext = false, label = 'MAPS CONTROLLER' } = options; return function mapErrorMiddleware (err, req, res, next) { - req.profiler.done('error'); const { mapConfig } = res.locals; if (addContext) { diff --git a/lib/api/middlewares/profiler.js b/lib/api/middlewares/profiler.js index 9ed3eb7b..2cf0736f 100644 --- a/lib/api/middlewares/profiler.js +++ b/lib/api/middlewares/profiler.js @@ -8,13 +8,17 @@ module.exports = function profiler (options) { return function profilerMiddleware (req, res, next) { const { logger } = res.locals; + const { id } = logger.bindings(); req.profiler = new Profiler({ statsd_client: statsClient, profile: enabled }); + req.profiler.start(id); + res.on('finish', () => { + req.profiler.done('response'); logger.info({ stats: req.profiler.toJSON() }); try { diff --git a/lib/api/middlewares/send-response.js b/lib/api/middlewares/send-response.js index f37457d6..d65d7597 100644 --- a/lib/api/middlewares/send-response.js +++ b/lib/api/middlewares/send-response.js @@ -2,8 +2,6 @@ module.exports = function sendResponse () { return function sendResponseMiddleware (req, res, next) { - req.profiler.done('res'); - res.status(res.statusCode); if (Buffer.isBuffer(res.body)) { diff --git a/lib/api/template/admin-template-controller.js b/lib/api/template/admin-template-controller.js index 456f6bc0..1a9e90e2 100644 --- a/lib/api/template/admin-template-controller.js +++ b/lib/api/template/admin-template-controller.js @@ -166,8 +166,6 @@ function updateTemplate ({ templateMaps }) { function retrieveTemplate ({ templateMaps }) { return function retrieveTemplateMiddleware (req, res, next) { - req.profiler.start('windshaft-cartodb.get_template'); - const { user } = res.locals; const templateId = templateName(req.params.template_id); @@ -195,8 +193,6 @@ function retrieveTemplate ({ templateMaps }) { function destroyTemplate ({ templateMaps }) { return function destroyTemplateMiddleware (req, res, next) { - req.profiler.start('windshaft-cartodb.delete_template'); - const { user } = res.locals; const templateId = templateName(req.params.template_id); @@ -215,8 +211,6 @@ function destroyTemplate ({ templateMaps }) { function listTemplates ({ templateMaps }) { return function listTemplatesMiddleware (req, res, next) { - req.profiler.start('windshaft-cartodb.get_template_list'); - const { user } = res.locals; templateMaps.listTemplates(user, (err, templateIds) => { diff --git a/lib/api/template/named-template-controller.js b/lib/api/template/named-template-controller.js index 79b39d7c..41246d65 100644 --- a/lib/api/template/named-template-controller.js +++ b/lib/api/template/named-template-controller.js @@ -4,7 +4,6 @@ const cleanUpQueryParams = require('../middlewares/clean-up-query-params'); const credentials = require('../middlewares/credentials'); const dbConnSetup = require('../middlewares/db-conn-setup'); const authorize = require('../middlewares/authorize'); -const initProfiler = require('../middlewares/init-profiler'); const checkJsonContentType = require('../middlewares/check-json-content-type'); const incrementMapViewCount = require('../middlewares/increment-map-view-count'); const augmentLayergroupData = require('../middlewares/augment-layergroup-data'); @@ -74,7 +73,6 @@ module.exports = class NamedMapController { } middlewares () { - const isTemplateInstantiation = true; const useTemplateHash = true; const includeQuery = false; const label = 'NAMED MAP LAYERGROUP'; @@ -100,7 +98,6 @@ module.exports = class NamedMapController { dbConnSetup(this.pgConnection), rateLimit(this.userLimitsBackend, RATE_LIMIT_ENDPOINTS_GROUPS.NAMED), cleanUpQueryParams(['aggregation']), - initProfiler(isTemplateInstantiation), checkJsonContentType(), checkInstantiteLayergroup(), getTemplate( @@ -150,8 +147,6 @@ function checkInstantiteLayergroup () { } } - req.profiler.done('checkInstantiteLayergroup'); - return next(); }; } @@ -187,9 +182,6 @@ function getTemplate ( ); mapConfigProvider.getMapConfig((err, mapConfig, rendererParams, context, stats = {}) => { - req.profiler.done('named.getMapConfig'); - - stats.mapType = 'named'; req.profiler.add(stats); if (err) { diff --git a/lib/api/template/tile-template-controller.js b/lib/api/template/tile-template-controller.js index 1112678e..bb2b0aa7 100644 --- a/lib/api/template/tile-template-controller.js +++ b/lib/api/template/tile-template-controller.js @@ -67,9 +67,8 @@ function getTile ({ tileBackend, label }) { const { layer, z, x, y, format } = req.params; const params = { layer, z, x, y, format }; - tileBackend.getTile(mapConfigProvider, params, (err, tile, headers, stats) => { + tileBackend.getTile(mapConfigProvider, params, (err, tile, headers, stats = {}) => { req.profiler.add(stats); - req.profiler.done('render-' + format); if (err) { err.label = label; diff --git a/lib/backends/auth.js b/lib/backends/auth.js index 2c9dd3ee..298b1d64 100644 --- a/lib/backends/auth.js +++ b/lib/backends/auth.js @@ -133,8 +133,6 @@ AuthBackend.prototype.authorize = function (req, res, callback) { if (isAuthorizedByApikey) { return this.pgConnection.setDBAuth(user, res.locals, 'regular', function (err) { - req.profiler.done('setDBAuth'); - if (err) { return callback(err); } @@ -150,8 +148,6 @@ AuthBackend.prototype.authorize = function (req, res, callback) { if (isAuthorizedBySigner) { return this.pgConnection.setDBAuth(user, res.locals, 'master', function (err) { - req.profiler.done('setDBAuth'); - if (err) { return callback(err); } @@ -163,8 +159,6 @@ AuthBackend.prototype.authorize = function (req, res, callback) { // if no signer name was given, use default api key if (!res.locals.signer) { return this.pgConnection.setDBAuth(user, res.locals, 'default', function (err) { - req.profiler.done('setDBAuth'); - if (err) { return callback(err); } diff --git a/lib/models/dataview/base.js b/lib/models/dataview/base.js index 0813e3b1..18897635 100644 --- a/lib/models/dataview/base.js +++ b/lib/models/dataview/base.js @@ -23,7 +23,7 @@ function getPGTypeName (pgType) { module.exports = class BaseDataview { getResult (psql, override, callback) { - this.sql(psql, override, (err, query, flags = null) => { + this.sql(psql, override, (err, query) => { if (err) { return callback(err); } @@ -36,20 +36,7 @@ module.exports = class BaseDataview { result = this.format(result, override); result.type = this.getType(); - // Overviews logging - const stats = {}; - - 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); + return callback(null, result); }, true); // use read-only transaction }); } diff --git a/lib/models/dataview/overviews/aggregation.js b/lib/models/dataview/overviews/aggregation.js index da988731..2ab6add2 100644 --- a/lib/models/dataview/overviews/aggregation.js +++ b/lib/models/dataview/overviews/aggregation.js @@ -213,7 +213,7 @@ Aggregation.prototype.sql = function (psql, override, callback) { debug(aggregationSql); - return callback(null, aggregationSql, { usesOverviews: true }); + return callback(null, aggregationSql); }; var aggregationFnQueryTpl = { diff --git a/lib/models/dataview/overviews/formula.js b/lib/models/dataview/overviews/formula.js index 35d52008..e860f8ec 100644 --- a/lib/models/dataview/overviews/formula.js +++ b/lib/models/dataview/overviews/formula.js @@ -76,5 +76,5 @@ Formula.prototype.sql = function (psql, override, callback) { debug(formulaSql); - return callback(null, formulaSql, { usesOverviews: true }); + return callback(null, formulaSql); }; diff --git a/lib/models/dataview/overviews/histogram.js b/lib/models/dataview/overviews/histogram.js index 4df5bd01..d10c1ae7 100644 --- a/lib/models/dataview/overviews/histogram.js +++ b/lib/models/dataview/overviews/histogram.js @@ -179,7 +179,7 @@ Histogram.prototype.sql = function (psql, override, callback) { var histogramSql = this._buildQuery(override); - return callback(null, histogramSql, { usesOverviews: true }); + return callback(null, histogramSql); }; Histogram.prototype._buildQuery = function (override) { diff --git a/lib/models/mapconfig/adapter/mapconfig-overviews-adapter.js b/lib/models/mapconfig/adapter/mapconfig-overviews-adapter.js index 3cb39a50..6473c162 100644 --- a/lib/models/mapconfig/adapter/mapconfig-overviews-adapter.js +++ b/lib/models/mapconfig/adapter/mapconfig-overviews-adapter.js @@ -25,58 +25,50 @@ MapConfigOverviewsAdapter.prototype.getMapConfig = function (user, requestMapCon layers.forEach(layer => augmentLayersQueue.defer(this._augmentLayer.bind(this), user, layer, analysesResults)); - augmentLayersQueue.awaitAll(function layersAugmentQueueFinish (err, results) { + augmentLayersQueue.awaitAll(function layersAugmentQueueFinish (err, layers) { if (err) { return callback(err); } - const layers = results.map(result => result.layer); - const overviewsAddedToMapconfig = results.some(result => result.overviewsAddedToMapconfig); - if (!layers || layers.length === 0) { return callback(new Error('Missing layers array from layergroup config')); } requestMapConfig.layers = layers; - const stats = { overviewsAddedToMapconfig }; - - return callback(null, requestMapConfig, stats); + return callback(null, requestMapConfig); }); }; MapConfigOverviewsAdapter.prototype._augmentLayer = function (user, layer, analysesResults, callback) { - let overviewsAddedToMapconfig = false; if (layer.type !== 'mapnik' && layer.type !== 'cartodb') { - return callback(null, { layer, overviewsAddedToMapconfig }); + return callback(null, layer); } this.overviewsMetadataBackend.getOverviewsMetadata(user, layer.options.sql, (err, metadata) => { if (err) { - return callback(err, { layer, overviewsAddedToMapconfig }); + return callback(err); } if (_.isEmpty(metadata)) { - return callback(null, { layer, overviewsAddedToMapconfig }); + return callback(null, layer); } var filters = getFilters(analysesResults, layer); - overviewsAddedToMapconfig = true; - if (!filters) { layer.options = Object.assign({}, layer.options, getQueryRewriteData(layer, analysesResults, { overviews: metadata })); - return callback(null, { layer, overviewsAddedToMapconfig }); + return callback(null, layer); } var unfilteredQuery = getUnfilteredQuery(analysesResults, layer); this.filterStatsBackend.getFilterStats(user, unfilteredQuery, filters, function (err, stats) { if (err) { - return callback(null, { layer, overviewsAddedToMapconfig }); + return callback(null, layer); } layer.options = Object.assign({}, layer.options, getQueryRewriteData(layer, analysesResults, { @@ -84,7 +76,7 @@ MapConfigOverviewsAdapter.prototype._augmentLayer = function (user, layer, analy filter_stats: stats })); - return callback(null, { layer, overviewsAddedToMapconfig }); + return callback(null, layer); }); }); }; From 6945cfc93c8974283a1c357610a5d1ec855d09e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 4 Jun 2020 12:10:15 +0200 Subject: [PATCH 18/18] Add TODO --- lib/api/middlewares/profiler.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/api/middlewares/profiler.js b/lib/api/middlewares/profiler.js index 2cf0736f..a3cb584f 100644 --- a/lib/api/middlewares/profiler.js +++ b/lib/api/middlewares/profiler.js @@ -10,6 +10,7 @@ module.exports = function profiler (options) { const { logger } = res.locals; const { id } = logger.bindings(); + // TODO: stop using profiler and log stats instead of adding them to the profiler req.profiler = new Profiler({ statsd_client: statsClient, profile: enabled