Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Server tests et al #3363

Merged
merged 7 commits into from
Aug 17, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions server/src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,13 @@ var conf = convict({
default: 0,
env: "FORCE_DB_VERSION",
arg: "force-db-version"
},
logQueryLimit: {
doc: "Log queries that take more than time amount of time (in milliseconds)",
format: "int",
default: 50,
env: "LOG_QUERY_LIMIT",
arg: "log-query-limit"
}
},
gaId: {
Expand Down
57 changes: 57 additions & 0 deletions server/src/db.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const config = require("./config").getProperties();

const pg = require("pg");
const mozlog = require("./logging").mozlog("db");
const logQueryLimit = config.db.logQueryLimit;

let user = encodeURIComponent(config.db.user);
let dbname = encodeURIComponent(config.db.dbname || config.db.user);
Expand All @@ -28,10 +29,12 @@ function getConnection() {
}

exports.queryWithClient = function(client, ...params) {
let timer = initTiming();
return new Promise((resolve, reject) => {
// Babel only supports spreads as the final element of a list; i.e.,
// `[...params, value]` is invalid.
params.push(function afterQuery(err, result) {
timer();
if (err) {
reject(err);
} else {
Expand All @@ -47,9 +50,11 @@ exports.getConnection = getConnection;
exports.constr = constr;

exports.select = function(sql, args) {
let timer = initTiming();
return getConnection().then(function([client, done]) {
return exports.queryWithClient(client, sql, args).then(({rows}) => {
done();
timer();
return rows;
}, (error) => {
done();
Expand All @@ -59,9 +64,11 @@ exports.select = function(sql, args) {
};

exports.insert = function(sql, args) {
let timer = initTiming();
return getConnection().then(function([client, done]) {
return exports.queryWithClient(client, sql, args).then(() => {
done();
timer();
return true;
}).catch(err => {
done();
Expand All @@ -75,7 +82,9 @@ exports.insert = function(sql, args) {
};

exports.update = function(sql, args) {
let timer = initTiming();
return exports.exec(sql, args).then((result) => {
timer();
return result.rowCount;
});
};
Expand Down Expand Up @@ -115,9 +124,11 @@ exports.transaction = function(func) {
exports.del = exports.update;

exports.exec = function(sql, args) {
let timer = initTiming();
return getConnection().then(function([client, done]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be useful to track slow connection times as well? Maybe shove a second timer in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I think about it, I somehow remember that we removed connection pooling. But I'm not sure. It might be good for a followup, but I'd be surprised if we saw anything interesting.

return exports.queryWithClient(client, sql, args).then(result => {
done();
timer();
return result;
}, err => {
done();
Expand All @@ -133,3 +144,49 @@ exports.markersForArgs = function(starting, numberOfArgs) {
}
return result.join(", ");
};

function doNothing() {
}

function initTiming() {
if (!logQueryLimit) {
return doNothing;
}
let caller = getCallerPosition(2);
if (caller == "skip") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little bit confused by this getCallerPosition-related stuff, it seems a little dangerously clever.

Could we instead issue the timing calls from the files that call into db.js, like servershot.js? That might be an opportunity to pass a label into initTiming to identify the query, which would make it directly greppable. Would also get us moving in the direction of adding timing for arbitrary other events.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I was trying to make this minimal, without adding a bunch of stuff throughout the codebase. Given that, it required some cleverness to identify queries (there might have been an option to identify queries based on the SQL, but then there's a question of how to uniquely identify SQL without putting every query into the log).

But I don't think this should be forever code... like, this code isn't Mr. Right, just Mr. Right Now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol ok

// This happens when getCallerPosition detects we shouldn't time this function call
return doNothing;
}
let start = Date.now();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe process.hrtime instead, to get a high resolution timestamp?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at process.hrtime, but it splits the time into two numbers, and seemed complicated to work with. We don't really care about anything that's less than (or even close to) 1ms anyway.

return function() {
let time = Date.now() - start;
if (time >= logQueryLimit) {
mozlog.info("db-timing", {caller, time});
}
};
}

const skipCaller = /\/server\/db.js:/;
const abortTiming = /\/server\/db.js:/;

function getCallerPosition(stacklevel) {
// Obviously the caller knows its position, so we really
// want the caller of the caller of this function
let exc = new Error();
let lines = exc.stack.split("\n");
let index = stacklevel + 2;
while (lines[index] && skipCaller.test(lines[index])) {
if (abortTiming.test(lines[index])) {
// This is a case where this function is being called by another function
// inside this module; when that happens we know this call is already
// being logged, and don't need to log it
return "skip";
}
index++;
}
let caller = lines[index];
if (!caller) {
return "unknown";
}
return caller.replace(/^ */, "")
}
4 changes: 4 additions & 0 deletions server/src/servershot.js
Original file line number Diff line number Diff line change
Expand Up @@ -856,6 +856,10 @@ Shot.upgradeSearch = function() {
return resolve();
}
Shot.get("upgrade_search_only", rows[index].id).then((shot) => {
// This shouldn't really happen, but apparently can...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related to another bug, or just something unfiled that you ran into?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something weird in my local database, I don't know if it was due to testing or some latent bug I didn't notice before.

if (!shot) {
return;
}
return shot.upgradeSearch();
}).then(() => {
index++;
Expand Down