chore(logger): streamline logging #10630

SimonSchick · 2019-03-26T22:15:47Z

Pull Request check-list

Please make sure to review and check all of these items:

Does npm run test or npm run test-DIALECT pass with this change (including linting)?
Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
Have you added new tests to prevent regressions?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Replaces debug package with debuglog native and acts as a foundation for #10284 (comment)

cc @javiertury

~~#Fixes 10661~~

eseliger · 2019-03-26T23:29:21Z

Looking good so far, but the build is failing

SimonSchick · 2019-03-26T23:54:50Z

hnnng

codecov · 2019-03-27T00:03:56Z

Codecov Report

Merging #10630 into master will increase coverage by 0.11%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master   #10630      +/-   ##
==========================================
+ Coverage   96.18%    96.3%   +0.11%     
==========================================
  Files          93       93              
  Lines        9011     8976      -35     
==========================================
- Hits         8667     8644      -23     
+ Misses        344      332      -12

Impacted Files	Coverage Δ
lib/dialects/abstract/connection-manager.js	`90.17% <100%> (ø)`	⬆️
lib/model.js	`96.67% <100%> (ø)`	⬆️
lib/dialects/mysql/connection-manager.js	`96.36% <100%> (ø)`	⬆️
lib/dialects/mysql/query.js	`98.3% <100%> (-0.07%)`	⬇️
lib/dialects/sqlite/query.js	`98.64% <100%> (-0.04%)`	⬇️
lib/dialects/abstract/query.js	`91.05% <100%> (+0.4%)`	⬆️
lib/dialects/sqlite/connection-manager.js	`100% <100%> (ø)`	⬆️
lib/hooks.js	`96.96% <100%> (ø)`	⬆️
lib/data-types.js	`91.28% <100%> (+0.03%)`	⬆️
lib/dialects/mariadb/connection-manager.js	`100% <100%> (ø)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab7a954...3a8e5b4. Read the comment docs.

eseliger

🌟

sushantdhiman

This is a breaking change I am afraid, it will break if someone is using debug specific logging features.

What are the advantages of this? With debug you can filter out different type of logging. If you want only pool logging you can set DEBUG=sequelize:pool* etc, is that possible with debuglog?

SimonSchick · 2019-03-27T11:24:17Z

Yes, see https://nodejs.org/api/util.html#util_util_debuglog_section

SimonSchick · 2019-03-27T13:43:37Z

I actually went ahead and streamlined all query logging into a single method sequelize.logQuery that handles both debug and regular logging mechanisms.
I also managed to purge a good amount of duplication while doing so.

Please note that it's relatively easy to bring back the debug package if that's highly desired.

eseliger · 2019-03-27T14:10:08Z

to me this looks very clean and like the way we want to be headed to. So if you guys feel like this is a breaking change I'd still be up for pushing this for a v6 release instead 🤔

SimonSchick · 2019-03-27T23:14:24Z

I don't think this is breaking btw, in no place sequelize states that it uses debug or that you can use DEBUG=sequelize as env do enable debugging, it's not part of any public interfaces either.

eseliger · 2019-03-27T23:15:41Z

I'm totally up for merging this, but the build is broken again 😔

SimonSchick · 2019-03-27T23:16:45Z

Heck.

lib/dialects/postgres/query.js

…gQuery

sushantdhiman · 2019-03-28T05:29:48Z

This is a breaking change and can't be merged. Even if docs are lacking that we support debug I am sure many are using this feature since we added support for debug. Also the environment variable for logs will be changed from DEBUG to NODE_DEBUG

…lize.logQuery

SimonSchick · 2019-03-28T19:51:51Z

Imo the only breaking change is the change in any public documentation or otherwise explicitly exposed format.

How would anyone know you can use debug if it's not in the docs?
They'd have to inspect the actual code to figure that out, and add that point 99% of developers would probably just add console.log at that point or use the logging option instead.

I have reverted it but please consider documenting the things you want developers to use/know about.

sushantdhiman

Unifying query logging logic is a good idea, I have left some suggestions. Other changes for logger class can be reverted

lib/utils/logger.js

lib/sequelize.js

…h sequelize.logQuery

SimonSchick · 2019-03-29T16:36:44Z

Boiled it down a little, the DebugContext class was a remnant of me trying make logging more lazy, ended up not using it.

… through sequelize.logQuery

…logging through sequelize.logQuery

SimonSchick · 2019-03-30T12:41:51Z

~~@sushantdhiman snug in a fix for #10661~~

Disregard that, needs more work, will be another PR.

…nel all logging through sequelize.logQuery" This reverts commit 5fd9078.

lib/utils/logger.js

lib/dialects/abstract/query.js

lib/dialects/mariadb/query.js

…): channel all logging through sequelize.logQuery"

…logging): channel all logging through sequelize.logQuery"

lib/utils/logger.js

…! feat(logging): channel all logging through sequelize.logQuery"

sushantdhiman

👍

sushantdhiman · 2019-04-03T11:40:56Z

🎉 This PR is included in version 5.2.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

Rename postgresql query debug namespace (mistake in #10630)

SimonSchick requested review from harshithkashyap, sushantdhiman and eseliger March 26, 2019 22:15

SimonSchick force-pushed the chore/native-log branch from 9a66975 to d14527b Compare March 26, 2019 22:26

chore(logger): use native logger

91be438

SimonSchick force-pushed the chore/native-log branch from d14527b to 91be438 Compare March 26, 2019 23:56

eseliger approved these changes Mar 27, 2019

View reviewed changes

sushantdhiman suggested changes Mar 27, 2019

View reviewed changes

javiertury mentioned this pull request Mar 27, 2019

feat(query): bind parameters #10284

Closed

5 tasks

feat(logging): channel all logging through sequelize.logQuery

d7d4c30

fixup! feat(logging): channel all logging through sequelize.logQuery

ffc538c

eseliger reviewed Mar 27, 2019

View reviewed changes

lib/dialects/postgres/query.js Show resolved Hide resolved

fixup! fixup! feat(logging): channel all logging through sequelize.lo…

c0da184

…gQuery

eseliger requested a review from sushantdhiman March 28, 2019 00:30

fixup! fixup! fixup! feat(logging): channel all logging through seque…

308b6e5

…lize.logQuery

sushantdhiman suggested changes Mar 29, 2019

View reviewed changes

lib/utils/logger.js Outdated Show resolved Hide resolved

lib/sequelize.js Outdated Show resolved Hide resolved

fixup! fixup! fixup! fixup! feat(logging): channel all logging throug…

633dd7b

…h sequelize.logQuery

fixup! fixup! fixup! fixup! fixup! feat(logging): channel all logging…

c06fb1b

… through sequelize.logQuery

SimonSchick changed the title ~~chore(logger): use native logger~~ chore(logger): streamline logging Mar 30, 2019

fixup! fixup! fixup! fixup! fixup! fixup! feat(logging): channel all …

5fd9078

…logging through sequelize.logQuery

Revert "fixup! fixup! fixup! fixup! fixup! fixup! feat(logging): chan…

30b8982

…nel all logging through sequelize.logQuery" This reverts commit 5fd9078.

sushantdhiman reviewed Apr 2, 2019

View reviewed changes

lib/utils/logger.js Outdated Show resolved Hide resolved

sushantdhiman reviewed Apr 2, 2019

View reviewed changes

lib/utils/logger.js Show resolved Hide resolved

sushantdhiman reviewed Apr 2, 2019

View reviewed changes

lib/dialects/abstract/query.js Outdated Show resolved Hide resolved

sushantdhiman reviewed Apr 2, 2019

View reviewed changes

lib/dialects/mariadb/query.js Outdated Show resolved Hide resolved

SimonSchick added 2 commits April 2, 2019 11:00

fixup! Revert "fixup! fixup! fixup! fixup! fixup! fixup! feat(logging…

d352f63

…): channel all logging through sequelize.logQuery"

fixup! fixup! Revert "fixup! fixup! fixup! fixup! fixup! fixup! feat(…

da371c0

…logging): channel all logging through sequelize.logQuery"

sushantdhiman reviewed Apr 3, 2019

View reviewed changes

lib/utils/logger.js Outdated Show resolved Hide resolved

fixup! fixup! fixup! Revert "fixup! fixup! fixup! fixup! fixup! fixup…

3a8e5b4

…! feat(logging): channel all logging through sequelize.logQuery"

sushantdhiman approved these changes Apr 3, 2019

View reviewed changes

sushantdhiman merged commit 7445423 into master Apr 3, 2019

sushantdhiman deleted the chore/native-log branch April 3, 2019 08:54

sushantdhiman added the released label Apr 3, 2019

KillWolfVlad mentioned this pull request Apr 11, 2019

fix(postgres): rename debug context #10752

Merged

5 tasks

eseliger pushed a commit that referenced this pull request Apr 11, 2019

fix(postgres): rename debug context (#10752)

728707c

Rename postgresql query debug namespace (mistake in #10630)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(logger): streamline logging #10630

chore(logger): streamline logging #10630

SimonSchick commented Mar 26, 2019 •

edited

eseliger commented Mar 26, 2019

SimonSchick commented Mar 26, 2019

codecov bot commented Mar 27, 2019 •

edited

eseliger left a comment

sushantdhiman left a comment

SimonSchick commented Mar 27, 2019

SimonSchick commented Mar 27, 2019

eseliger commented Mar 27, 2019

SimonSchick commented Mar 27, 2019

eseliger commented Mar 27, 2019

SimonSchick commented Mar 27, 2019

sushantdhiman commented Mar 28, 2019

SimonSchick commented Mar 28, 2019

sushantdhiman left a comment

SimonSchick commented Mar 29, 2019

SimonSchick commented Mar 30, 2019 •

edited

sushantdhiman left a comment

sushantdhiman commented Apr 3, 2019

chore(logger): streamline logging #10630

chore(logger): streamline logging #10630

Conversation

SimonSchick commented Mar 26, 2019 • edited

Pull Request check-list

Description of change

eseliger commented Mar 26, 2019

SimonSchick commented Mar 26, 2019

codecov bot commented Mar 27, 2019 • edited

Codecov Report

eseliger left a comment

Choose a reason for hiding this comment

sushantdhiman left a comment

Choose a reason for hiding this comment

SimonSchick commented Mar 27, 2019

SimonSchick commented Mar 27, 2019

eseliger commented Mar 27, 2019

SimonSchick commented Mar 27, 2019

eseliger commented Mar 27, 2019

SimonSchick commented Mar 27, 2019

sushantdhiman commented Mar 28, 2019

SimonSchick commented Mar 28, 2019

sushantdhiman left a comment

Choose a reason for hiding this comment

SimonSchick commented Mar 29, 2019

SimonSchick commented Mar 30, 2019 • edited

sushantdhiman left a comment

Choose a reason for hiding this comment

sushantdhiman commented Apr 3, 2019

SimonSchick commented Mar 26, 2019 •

edited

codecov bot commented Mar 27, 2019 •

edited

SimonSchick commented Mar 30, 2019 •

edited