Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 274 Add pino logging to backend #278

Merged
merged 5 commits into from Nov 21, 2019
Merged

Conversation

miggs125
Copy link
Contributor

Closes #274

@manekenpix
Copy link
Member

Screenshot from 2019-11-18 22-52-34

No, no, abort, abort, rebase, rebase!

@miggs125
Copy link
Contributor Author

This is what happens when I use the github gui as opposed to the command line...

@humphd
Copy link
Contributor

humphd commented Nov 20, 2019

You can fix it like this:

git checkout master
git pull upstream master
git checkout -B fix274 6b5b3c2ad3ca53fb620562bd648f7a05ea137189
git rebase master
...fix fix fix...
npm install
...add package-lock.json if changed...
git push origin fix274 -f

@humphd
Copy link
Contributor

humphd commented Nov 20, 2019

@miggs125 you're my first victim due to #295 landing! Can you please rebase on master, and get rid of the changes to package-lock.json.

If it were me, I might do something like this:

git checkout fix274
git checkout -- package*
git checkout master
git pull upstream master
git checkout -
git rebase master
npm install --save express-pino-logger pino-pretty
git add package.json
git commit ...

If it goes wrong, I'll help fix it.

@miggs125
Copy link
Contributor Author

@humphd Thanks for the help!

@humphd
Copy link
Contributor

humphd commented Nov 20, 2019

How painful was that?

Also, I notice your earlier PR set had a version bump on pino-pretty to 3.4.0, in case you want to do that in here.

@miggs125
Copy link
Contributor Author

miggs125 commented Nov 20, 2019

@humphd I actually ran into the error before I could read your message but I figured it out. Wasn't super painful but it took a couple of tries

@humphd
Copy link
Contributor

humphd commented Nov 20, 2019

Flagging @hansal7014 in on this review, since he's worked on the logging code in the past.


const log = parentLogger.child({ module: 'count-blog-domain' });
const log = logger.child({ module: 'count-blog-domain' });
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing we'll have to do in another PR is get better at naming our "modules" for the logger. Each class doesn't need its own module name. Rather, we should lump them together into logical components in the app.

prettyPrint: { translateTime: 'SYS: yyyy-mm-dd HH:MM:ss.l ' },
prettyPrint: {
translateTime: 'SYS: yyyy-mm-dd HH:MM:ss.l ',
colorize: !(os.type() === 'Windows_NT'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch on Windows and colour issues.

Copy link
Member

@manekenpix manekenpix left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@hansal7014
Copy link
Contributor

Changes look good to merge. Nice work @miggs125.

@humphd humphd merged commit c48ced6 into Seneca-CDOT:master Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Pino logging to our Express web server
4 participants