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 #6179 Initializing default logger #6186

Merged
merged 3 commits into from
Nov 7, 2019
Merged

Fix #6179 Initializing default logger #6186

merged 3 commits into from
Nov 7, 2019

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Nov 5, 2019

Fixes: #6179

Removes unnessary logs.

@kevflynn This PR will allow you to run Parse Server with your configs.

Fixes: #6179

Removes unnessary logs
src/logger.js Outdated

export function setLogger(aLogger) {
logger = aLogger;
}

export function getLogger() {
if (!logger) {
return defaultLogger();
Copy link
Contributor

Choose a reason for hiding this comment

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

do you just want to instantiate the default logger once instead of instantiating a new instance for earch get logger call?

i.e.

if (!logger) {
  logger = defaultLogger();
}
return logger;

Copy link
Member Author

Choose a reason for hiding this comment

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

I just noticed that

@acinader
Copy link
Contributor

acinader commented Nov 5, 2019

once you remove the log calls from the adapters, is the change in logger.js necessary?

not clear to me what the change in logger.js is doing, or how it addresses the issue.

@codecov
Copy link

codecov bot commented Nov 5, 2019

Codecov Report

Merging #6186 into master will increase coverage by 10.38%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #6186       +/-   ##
===========================================
+ Coverage   83.42%   93.81%   +10.38%     
===========================================
  Files         167      167               
  Lines       11298    11293        -5     
===========================================
+ Hits         9425    10594     +1169     
+ Misses       1873      699     -1174
Impacted Files Coverage Δ
src/logger.js 100% <ø> (ø) ⬆️
src/Adapters/Auth/twitter.js 96% <ø> (-0.43%) ⬇️
src/Adapters/Auth/vkontakte.js 88.23% <ø> (+3.23%) ⬆️
src/Adapters/Logger/WinstonLogger.js 100% <100%> (ø) ⬆️
src/Routers/UsersRouter.js 94.19% <0%> (+0.64%) ⬆️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 92.97% <0%> (+0.7%) ⬆️
src/Controllers/UserController.js 94.39% <0%> (+0.93%) ⬆️
src/Controllers/FilesController.js 93.02% <0%> (+2.32%) ⬆️
src/Routers/PushRouter.js 96.55% <0%> (+3.44%) ⬆️
src/Adapters/Storage/Postgres/PostgresClient.js 85.71% <0%> (+7.14%) ⬆️
... and 1 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 1e788a7...44f2478. Read the comment docs.

@dplewis
Copy link
Member Author

dplewis commented Nov 6, 2019

@acinader This one will need some work. If want to reproduce it see my #6179 (comment)

At least we should allow the configurations to be set before crashing. That way silencing the logs can be done.

If you can think of a better fix in the meantime let me know.

@dplewis
Copy link
Member Author

dplewis commented Nov 7, 2019

@acinader I think this is good to go. If there is an error with winston just use console transport.

@acinader
Copy link
Contributor

acinader commented Nov 7, 2019

so even without the logging in the adapters, this still throws?

is there a legitimate case where we could now be 'swallowing' an error? i.e. you want to supress the error when the logger is first referenced, but you will also supress an legitimate error.

is there any way to defer instantiating the logger instead?

@dplewis
Copy link
Member Author

dplewis commented Nov 7, 2019

@acinader I added a commit with a fix. It no longer throws an error and will use the console as backup.

@acinader
Copy link
Contributor

acinader commented Nov 7, 2019

yes, I see the current diff. I don't see where you're adding the console, or how that'll work.

@dplewis
Copy link
Member Author

dplewis commented Nov 7, 2019

Its already in there right after the try catch

@dplewis dplewis requested a review from acinader November 7, 2019 22:48
@dplewis
Copy link
Member Author

dplewis commented Nov 7, 2019

@kevflynn Can you see if this fixes your issue?

@dplewis dplewis merged commit a2d332f into master Nov 7, 2019
@dplewis dplewis deleted the logger-default branch November 7, 2019 23:41
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
…#6186)

* Fix parse-community#6179 Initializing default logger

Fixes: parse-community#6179

Removes unnessary logs

* fix typo

* if error just write to console
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants