Server tests et al #3363
Server tests et al #3363
Conversation
Make the tests no longer be executable, since the test runner runs
Note: test succeeds (despite what I expected from #3290)
Timing is logged when it goes over a configurable limit. Locations are logged based on caller filename/position
|
This turned into a bucket of things, but I'll cut it off here for review |
|
just curious, what is the purpose of tracking image sizes in the database? |
|
For future reporting: during some performance discussions we realized we didn't know how big (in bytes) images were, and certainly couldn't see if some users had a particularly large set of images. |
| // This happens when getCallerPosition detects we shouldn't time this function call | ||
| return doNothing; | ||
| } | ||
| let start = Date.now(); |
There was a problem hiding this comment.
Maybe process.hrtime instead, to get a high resolution timestamp?
There was a problem hiding this comment.
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.
| @@ -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]) { | |||
There was a problem hiding this comment.
Would it be useful to track slow connection times as well? Maybe shove a second timer in here?
There was a problem hiding this comment.
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 doNothing; | ||
| } | ||
| let caller = getCallerPosition(2); | ||
| if (caller == "skip") { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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... | |||
There was a problem hiding this comment.
Is this related to another bug, or just something unfiled that you ran into?
There was a problem hiding this comment.
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.
|
It also has occurred to me that I added |
No description provided.