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

pretty default logger #244

Merged
merged 3 commits into from Oct 30, 2019
Merged

pretty default logger #244

merged 3 commits into from Oct 30, 2019

Conversation

Justin-ZS
Copy link
Contributor

Before:
Screen Shot 2019-10-28 at 11 41 52 AM
After:
Screen Shot 2019-10-28 at 11 39 44 AM

@paveltiunov
Copy link
Member

paveltiunov commented Oct 28, 2019

@Justin-ZS Hey Jin! Thanks for contributing this! Logging is definitely a place that requires some improvements. There're log levels that have been requested some time ago. Would you like to implement those? It would help not to overwhelm console output. Something like INFO (default), TRACE and ERROR:

  • ERROR: Output event data only for errors
  • INFO: Output event data only for Performing query, Performing query completed and errors
  • TRACE: Output event data for all events

By event data I mean JSON.stringify(restParams, null, 2)

What do you think?

@Justin-ZS
Copy link
Contributor Author

@paveltiunov Fine, I will try to implement it after work.

Copy link
Member

@paveltiunov paveltiunov left a comment

Choose a reason for hiding this comment

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

@Justin-ZS Overall looks good! Could you please take a look at comments and fix linter and we're good to merge!

packages/cubejs-server-core/core/index.js Outdated Show resolved Hide resolved
packages/cubejs-server-core/core/index.js Outdated Show resolved Hide resolved
packages/cubejs-server-core/core/index.js Show resolved Hide resolved
@paveltiunov
Copy link
Member

@Justin-ZS Hey Jin! Looks good! Thanks again for contributing this!

@paveltiunov paveltiunov merged commit b1302d2 into cube-js:master Oct 30, 2019
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.

None yet

2 participants