jenkins-bot has submitted this change and it was merged.
Change subject: Log the remaining 4xx API responses
......................................................................
Log the remaining 4xx API responses
* Currently only "invalid data-parsoid" made it to the logger.
* Since the logger is only available after the environment is setup, a
hacky use of the processLogger is needed for the middleware that come
before it.
Change-Id: Id3238f5155d7168f5dc5ab9f4c512757d5d12a35
---
M api/ParsoidService.js
M api/routes.js
M api/server.js
M api/utils.js
4 files changed, 53 insertions(+), 28 deletions(-)
Approvals:
Subramanya Sastry: Looks good to me, approved
jenkins-bot: Verified
diff --git a/api/ParsoidService.js b/api/ParsoidService.js
index a49bef7..0961a18 100644
--- a/api/ParsoidService.js
+++ b/api/ParsoidService.js
@@ -38,7 +38,7 @@
var host = parsoidConfig.serverInterface || process.env.INTERFACE;
// Load routes
- var routes = require('./routes')(parsoidConfig);
+ var routes = require('./routes')(parsoidConfig, processLogger);
var app = express();
diff --git a/api/routes.js b/api/routes.js
index a0a4e30..30a5606 100644
--- a/api/routes.js
+++ b/api/routes.js
@@ -18,10 +18,21 @@
var TemplateRequest = ApiRequest.TemplateRequest;
-module.exports = function(parsoidConfig) {
+module.exports = function(parsoidConfig, processLogger) {
var routes = {};
var REQ_TIMEOUT = parsoidConfig.timeouts.request;
+
+ // This helper is only to be used in middleware, before an environment
+ // is setup. The logger doesn't emit the expected location info.
+ // You probably want `apiUtils.fatalRequest` instead.
+ var errOut = function(res, text, code) {
+ var err = new Error(text);
+ err.code = code || 404;
+ err.stack = null;
+ processLogger.log('fatal/request', err);
+ apiUtils.sendResponse(res, {}, text, err.code);
+ };
// Middlewares
@@ -42,13 +53,9 @@
var v3SupportedFormats = new Set(['pagebundle', 'html', 'wikitext']);
routes.v23Middle = function(version, req, res, next) {
- function errOut(err, code) {
- apiUtils.sendResponse(res, {}, err, code || 404);
- }
-
var iwp = parsoidConfig.reverseMwApiMap.get(req.params.domain);
if (!iwp) {
- return errOut('Invalid domain: ' + req.params.domain);
+ return errOut(res, 'Invalid domain: ' +
req.params.domain);
}
res.locals.apiVersion = version;
@@ -70,7 +77,7 @@
if (!supportedFormats.has(opts.format) ||
(req.method === 'GET' &&
!wt2htmlFormats.has(opts.format))) {
- return errOut('Invalid format: ' + opts.format);
+ return errOut(res, 'Invalid format: ' + opts.format);
}
// In v2 the "wikitext" format was named "wt"
@@ -82,7 +89,7 @@
res.locals.subst = !!(req.query.subst || req.body.subst);
// This is only supported for the html format
if (res.locals.subst && opts.format !== 'html') {
- return errOut('Substitution is only supported for the
HTML format.', 501);
+ return errOut(res, 'Substitution is only supported for
the HTML format.', 501);
}
if (req.method === 'POST') {
@@ -512,17 +519,13 @@
var opts = res.locals.opts;
var env = res.locals.env;
- function errOut(err, code) {
- apiUtils.sendResponse(res, env, err, code || 404);
- }
-
if (wt2htmlFormats.has(opts.format)) {
// Accept wikitext as a string or object{body,headers}
var wikitext = (opts.wikitext && typeof opts.wikitext
!== 'string') ?
opts.wikitext.body : opts.wikitext;
if (typeof wikitext !== 'string') {
if (!res.locals.pageName) {
- return errOut('No title or wikitext was
provided.', 400);
+ return apiUtils.fatalRequest(env, 'No
title or wikitext was provided.', 400);
}
// We've been given source for this page
if (opts.original && opts.original.wikitext) {
@@ -533,7 +536,7 @@
} else {
// html is required for serialization
if (opts.html === undefined) {
- return errOut('No html was supplied.', 400);
+ return apiUtils.fatalRequest(env, 'No html was
supplied.', 400);
}
// Accept html as a string or object{body,headers}
var html = (typeof opts.html === 'string') ?
diff --git a/api/server.js b/api/server.js
index 245c502..61bcb78 100755
--- a/api/server.js
+++ b/api/server.js
@@ -93,17 +93,17 @@
};
// Setup process logger
-var logger = new Logger();
-logger._createLogData = function(logType, logObject) {
+var processLogger = new Logger();
+processLogger._createLogData = function(logType, logObject) {
return new ParsoidLogData(logType, logObject, locationData);
};
-logger._defaultBackend = ParsoidLogger.prototype._defaultBackend;
+processLogger._defaultBackend = ParsoidLogger.prototype._defaultBackend;
ParsoidLogger.prototype.registerLoggingBackends.call(
- logger, [ "fatal", "error", "warning", "info" ], parsoidConfig
+ processLogger, [ "fatal", "error", "warning", "info" ], parsoidConfig
);
process.on('uncaughtException', function(err) {
- logger.log('fatal', 'uncaught exception', err);
+ processLogger.log('fatal', 'uncaught exception', err);
});
if (cluster.isMaster && argv.n > 0) {
@@ -131,7 +131,7 @@
timeouts.set(msg.timeoutId, setTimeout(function() {
timeouts.delete(msg.timeoutId);
if (worker.id in cluster.workers) {
- logger.log("warning", util.format(
+ processLogger.log("warning",
util.format(
"Cpu timeout fetching: %s;
killing worker %s.",
msg.location, pid
));
@@ -144,7 +144,7 @@
// Fork workers
var worker;
- logger.log("info", util.format("initializing %s workers", argv.n));
+ processLogger.log("info", util.format("initializing %s workers",
argv.n));
for (var i = 0; i < argv.n; i++) {
spawn();
}
@@ -152,15 +152,15 @@
cluster.on('exit', function(worker, code, signal) {
if (!worker.suicide) {
var pid = worker.process.pid;
- logger.log("warning", util.format("worker %s died (%s),
restarting.", pid, signal || code));
+ processLogger.log("warning", util.format("worker %s
died (%s), restarting.", pid, signal || code));
spawn();
}
});
var shutdownMaster = function() {
- logger.log("info", "shutting down, killing workers");
+ processLogger.log("info", "shutting down, killing workers");
cluster.disconnect(function() {
- logger.log("info", "exiting");
+ processLogger.log("info", "exiting");
process.exit(0);
});
};
@@ -172,7 +172,7 @@
// Worker
var shutdownWorker = function() {
- logger.log("warning", "shutting down");
+ processLogger.log("warning", "shutting down");
process.exit(0);
};
@@ -185,7 +185,7 @@
// For 0.10: npm install heapdump
process.on('SIGUSR2', function() {
var heapdump = require('heapdump');
- logger.log("warning", "SIGUSR2 received! Writing snapshot.");
+ processLogger.log("warning", "SIGUSR2 received! Writing
snapshot.");
process.chdir('/tmp');
heapdump.writeSnapshot();
});
@@ -200,6 +200,6 @@
}, parsoidConfig.heapUsageSampleInterval);
}
- var app = new ParsoidService(parsoidConfig, logger);
+ var app = new ParsoidService(parsoidConfig, processLogger);
}
diff --git a/api/utils.js b/api/utils.js
index 09a2b3e..2968be3 100644
--- a/api/utils.js
+++ b/api/utils.js
@@ -541,11 +541,33 @@
}
};
+/**
+ * Validates that data-parsoid was provided in the expected format.
+ *
+ * @method
+ * @param {Object} obj
+ */
apiUtils.validateDp = function(obj) {
var dp = obj['data-parsoid'];
if (!dp || !dp.body || dp.body.constructor !== Object || !dp.body.ids) {
var err = new Error('Invalid data-parsoid was provided.');
err.code = 400;
+ err.stack = null;
throw err;
}
};
+
+/**
+ * Log a fatal/request.
+ *
+ * @method
+ * @param {MWParserEnvironment} env
+ * @param {String} text
+ * @param {Number} [code]
+ */
+apiUtils.fatalRequest = function(env, text, code) {
+ var err = new Error(text);
+ err.code = code || 404;
+ err.stack = null;
+ env.log('fatal/request', err);
+};
--
To view, visit https://gerrit.wikimedia.org/r/240315
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id3238f5155d7168f5dc5ab9f4c512757d5d12a35
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/services/parsoid
Gerrit-Branch: master
Gerrit-Owner: Arlolra <[email protected]>
Gerrit-Reviewer: Arlolra <[email protected]>
Gerrit-Reviewer: Cscott <[email protected]>
Gerrit-Reviewer: Subramanya Sastry <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
