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: bugs related to logging with type json #893

Merged
merged 5 commits into from
Aug 24, 2018

Conversation

mlucool
Copy link
Contributor

@mlucool mlucool commented Aug 2, 2018

This commit cleans up some logging infra and ensures that the msg
in the json log are the same as what you get when you use pretty.
It also removes some confusing nulls that appear in the logs.

Type:

The following has been addressed in the PR: bug

Description:
When using the log format json the msg would be untemplated version (e.g.)

"msg":"@{status}, user: @{user}(@{remoteIP}), req: '@{request.method} @{request.url}'

This fixes that by refactoring and sharing the same template logic in all cases.

Copy link
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

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

Since there is no unit test for this, testing manually and comenting out the file log it fails on init.

# log settings
logs:
  - {type: stdout, format: pretty, level: http}
  - {type: file, path: /Users/user/verdaccio.log, level: info}

The error

/Users/user/.nvm/versions/node/v9.9.0/bin/node /Users/user/projects/verdaccio.master.git/debug/bootstrap.js
 warn --- config file  - /Users/user/.config/verdaccio/config.yaml
 warn --- Plugin successfully loaded: htpasswd
(node:74012) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'write' of undefined
    at Stream.stream.write.obj [as write] (/Users/user/projects/verdaccio.master.git/src/lib/logger.js:74:14)
    at Logger._emit (/Users/user/projects/verdaccio.master.git/node_modules/bunyan/lib/bunyan.js:923:22)
    at Logger.warn (/Users/user/projects/verdaccio.master.git/node_modules/bunyan/lib/bunyan.js:1045:24)
    at Object.keys.map.pluginId (/Users/user/projects/verdaccio.master.git/src/lib/plugin-loader.js:112:19)
    at Array.map (<anonymous>)
    at loadPlugin (/Users/user/projects/verdaccio.master.git/src/lib/plugin-loader.js:54:37)
    at Auth._loadPlugin (/Users/user/projects/verdaccio.master.git/src/lib/auth.js:42:12)
    at new Auth (/Users/user/projects/verdaccio.master.git/src/lib/auth.js:32:25)
    at defineAPI (/Users/user/projects/verdaccio.master.git/src/api/index.js:33:23)
    at /Users/user/projects/verdaccio.master.git/src/api/index.js:118:10
(node:74012) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:74012) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Copy link
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

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

It should not fails

This commit cleans up some logging infra and ensures that the msg
in the json log are the same as what you get when you use pretty.
It also removes some confusing nulls that appear in the logs.
@mlucool
Copy link
Contributor Author

mlucool commented Aug 13, 2018

This was due to an extra const. It could have been caught by a lint rule no-shadow. Maybe that should be added to rules too?

@mlucool
Copy link
Contributor Author

mlucool commented Aug 20, 2018

@juanpicado Is there still something you are waiting for? The broken tests seem unrelated to this commit.

@juanpicado
Copy link
Member

sorry @mlucool I totally forgot this. I'll review today.

@juanpicado juanpicado added this to the 3.x.x milestone Aug 21, 2018
@juanpicado juanpicado added this to To do in Roadmap via automation Aug 21, 2018
Copy link
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

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

@mlucool perfect !! I tested 👍

@juanpicado juanpicado merged commit cd231ba into verdaccio:master Aug 24, 2018
Roadmap automation moved this from To do to Done Aug 24, 2018
priscilawebdev pushed a commit that referenced this pull request Sep 3, 2018
This commit cleans up some logging infra and ensures that the msg
in the json log are the same as what you get when you use pretty.
It also removes some confusing nulls that appear in the logs.
@lock
Copy link

lock bot commented Jan 18, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Roadmap
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants