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

Adding Pino logs #46

Merged
merged 8 commits into from
Oct 28, 2020
Merged

Adding Pino logs #46

merged 8 commits into from
Oct 28, 2020

Conversation

resurl
Copy link
Contributor

@resurl resurl commented Oct 27, 2020

Changes

  • logger pretty prints when process.env.NODE_ENV === "development"

under /server

  • set ESLint no-console rule to true
  • added RUN field in Dockerfile to install Chromium. this was a bug i didn't catch when we first started using Docker
  • ignored scraper output files because running nodemon on the scraper would cause infinite restarts
  • added Pino logger, replaced all console statements to use the logger instead

under /client

  • set ESLint no-console rule to true
  • add Pino logger to be imported, in case logging is needed

Motivation and Context

having logs instead of using console.log allows us to debug with richer information. with pino, we can potentially filter logs by log level (for example, only seeing ERROR messages if any, or taking out DEBUG messages when needed). it also allows us to keep track of metrics down the road.

How Has This Been Tested?

  • tested on both Docker and local setup + ran a scraper test to test log levels, both work as expected

Screenshots (if appropriate):

local logs
docker logs

Related Issues

@resurl resurl requested review from a team, yehee and johnagl October 27, 2020 17:52
@resurl
Copy link
Contributor Author

resurl commented Oct 27, 2020

@yehee i added something to the Dockerfile if you want to take a look, in case you were wondering why i requested you LOL.

@resurl resurl changed the title Logger Adding Pino logs Oct 27, 2020
server/utils/logger.ts Show resolved Hide resolved
server/.eslintrc.js Show resolved Hide resolved
server/src/index.ts Outdated Show resolved Hide resolved
@johnagl
Copy link
Collaborator

johnagl commented Oct 28, 2020

pinologgererror
I'm getting this error for some reason. Package.json has the correct dependency. Looking into it..

Looks like the node_modules line in volumes of server was causing it. I'm not sure why. Have to look into it more.

Copy link
Collaborator

@johnagl johnagl left a comment

Choose a reason for hiding this comment

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

Looks good overall!

@resurl resurl merged commit b4d748c into main Oct 28, 2020
@yehee
Copy link
Contributor

yehee commented Oct 31, 2020

pinologgererror
I'm getting this error for some reason. Package.json has the correct dependency. Looking into it..

Looks like the node_modules line in volumes of server was causing it. I'm not sure why. Have to look into it more.

@johnagl Bumped into this issue just now, has this been resolved?

yehee added a commit that referenced this pull request Oct 31, 2020
yehee added a commit that referenced this pull request Nov 3, 2020
@resurl resurl deleted the logger branch November 6, 2020 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants