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

Adds npmlog as default logger #1209

Closed
wants to merge 1 commit into from
Closed

Adds npmlog as default logger #1209

wants to merge 1 commit into from

Conversation

flovilmart
Copy link
Contributor

No description provided.

@codecov-io
Copy link

Current coverage is 93.09%

Merging #1209 into master will increase coverage by +0.07% as of 76db50a

@@            master   #1209   diff @@
======================================
  Files           83      83       
  Stmts         5278    5288    +10
  Branches       963     960     -3
  Methods          0       0       
======================================
+ Hit           4910    4923    +13
  Partial         10      10       
+ Missed         358     355     -3

Review entire Coverage Diff as of 76db50a

Powered by Codecov. Updated on successful CI builds.

@drew-gross
Copy link
Contributor

Shouldn't these be going into the LoggerAdapter?

@flovilmart
Copy link
Contributor Author

The logger adapter is not final and not used. It does 2 things now. 1 configure Winston, 2. Provide an API for the logs endpoint.

For now, we don't use Winston at all. And if we were to use it, we'd need to decide what logs go there.

console.log don't go to Winston, nor we expose the logger anywhere else.

@drew-gross
Copy link
Contributor

We'd like to expose the logger endpoint so you can see your logs on the dashboard. Obviously it's not there yet, but this seems like a step in a different direction.

@flovilmart
Copy link
Contributor Author

Yeah, I know... I'm adding winston instead now, but I'm not sure this is the right direction either...

@drew-gross
Copy link
Contributor

With the logger adapter you should be able to just plug in any logger. Having a default seems fine, as long it just works out of the box.

@flovilmart
Copy link
Contributor Author

Yeah but that's not the issue, we have server logs for debug, verbose purposes, request traces, cloud code logs etc...
The logger adapter now is not accessible from all the parts of parse-server...

@drew-gross
Copy link
Contributor

It probably should be. I think our logging infra is scattered and awful right now and I think we should be making it more unified and useful rather than adding more log systems.

@flovilmart
Copy link
Contributor Author

Yeah that's why I'm moving back to winston.

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.

4 participants