Feature #488 : Added benchmarking support in query logs #5116
Conversation
|
Only problem i see here is you would no longer get the SQL on failure with benchmark turned on. Not sure that's a major issue since |
|
|
||
| if(self.sequelize.options.benchmark){ | ||
| var queryTime = microtime.now() - queryBegin; | ||
| self.sequelize.log('Executed (' + (self.connection.uuid || 'default') + '): ' + self.sql + ' in ' + queryTime + ' μs', self.options); |
There was a problem hiding this comment.
No space between time and us is common i believe.
No reason to save a queryTime variable is there?
Please add a space after if before (
|
Yes I thought that too @mickhansen . When error is thrown we actually don't need the benchmarks? Also |
| @@ -62,7 +63,11 @@ Query.prototype.run = function(sql, parameters) { | |||
| , query = ((parameters && parameters.length) ? this.client.query(this.sql, parameters) : this.client.query(this.sql)) | |||
| , rows = []; | |||
|
|
|||
| this.sequelize.log('Executing (' + (this.client.uuid || 'default') + '): ' + this.sql, this.options); | |||
| if (!this.sequelize.options.benchmark) { | |||
There was a problem hiding this comment.
We should probably also allow per call benchmarking ala:
Model.findAll({
logging: console.log,
benchmark. true
})
|
@mickhansen Done, for all model methods |
|
CI Clear, Let me know if PR need other changes |
| @@ -39,6 +39,7 @@ | |||
| "inflection": "^1.6.0", | |||
| "lodash": "^3.9.3", | |||
| "moment": "^2.11.0", | |||
| "microtime": "^2.0.0", | |||
There was a problem hiding this comment.
microtime requires gyp and uses gettimeofday. It won't work on windows, question is whether or not it will install.
There was a problem hiding this comment.
They have a CI build for windows. I think it will install and work for 0.10+
|
All done @mickhansen |
|
@mickhansen Any changes in this one. |
|
Looks good, do you mind squashing the commits to a single descriptive one though? |
|
@mickhansen Can you please restart the CI or accept it :) |
|
You've tested this locally i assume? |
|
Yes I have ran full tests with benchmark and single |
Feature #488 : Added benchmarking support in query logs
|
Thanks |
|
https://travis-ci.org/mickhansen/graphql-sequelize/builds/100633104 microtime has issues on Node v4.0 it looks like, will have to revert this PR. |
|
Ok, I will replace with |
|
Are there any plans incorporating this in the near future? If not, what can I do to help? |
Closes #488
Added an new option
benchmarkandnode-microtimelibrary to log time in microseconds it took to execute a SQL statement.