Skip to content

🎨 logging improvements#7597

Merged
ErisDS merged 18 commits intoTryGhost:masterfrom
kirrg001:1.0.0-dev/logging-improvements
Oct 25, 2016
Merged

🎨 logging improvements#7597
ErisDS merged 18 commits intoTryGhost:masterfrom
kirrg001:1.0.0-dev/logging-improvements

Conversation

@kirrg001
Copy link
Copy Markdown
Contributor

@kirrg001 kirrg001 commented Oct 19, 2016

refs #7116

This PR contains small improvements to our logging component.
I already marked the TODOS in the issue #7116 !

If you go step by step through the commits, every single change is hopefully clear. Please read the commit description.

My favourite way of developing with node:
LEVEL=error MODE=long node index.js
It only prints errors and all request/response information.

@kirrg001 kirrg001 force-pushed the 1.0.0-dev/logging-improvements branch 4 times, most recently from 608906d to b18c418 Compare October 19, 2016 16:25
@kirrg001 kirrg001 changed the title [WIP] 🎨 logging improvements 🎨 logging improvements Oct 19, 2016
@kirrg001
Copy link
Copy Markdown
Contributor Author

ready for review

Copy link
Copy Markdown
Member

@ErisDS ErisDS left a comment

Choose a reason for hiding this comment

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

This is looking tonnes better! I have only one main piece of feedback here about the middleware for logging, ask me if you don't remember what I'm talking about.

Comment thread core/server/app.js Outdated

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

Comment thread core/server/logging/index.js Outdated

This comment was marked as abuse.

Comment thread core/server/auth/passport.js Outdated

This comment was marked as abuse.

Comment thread core/server/logging/PrettyStream.js Outdated

This comment was marked as abuse.

Comment thread core/server/logging/PrettyStream.js Outdated

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@kirrg001
Copy link
Copy Markdown
Contributor Author

kirrg001 commented Oct 20, 2016

74a8e29

Removed req.ip from log output.
Removed response-time dependency. (looks like this now: 90ms)

@kirrg001
Copy link
Copy Markdown
Contributor Author

pushed moving request logging into a middleware

82f9265

@kevinansfield kevinansfield added this to the 1.0.0-alpha.6 milestone Oct 21, 2016
@kirrg001
Copy link
Copy Markdown
Contributor Author

I have added a new commit: d7389e4

See commit description. We add a requestId and userId to each log entry if available.
If there is no userId available, it will show null right now.

- every parameter is confiureable
- increase default number of files to 100

[ci skip]
- example: content/logs/http___my_ghost_blog_com_ghost.log
- user can change the path to something custom by setting logging.path

[ci skip]

change
- tidy up
- generic handling (was important to support more use cases, for example: logging.info({ anyKey: anyValue }))
- common log format
- less code 🕵🏻

[ci skip]
- remove setLoggers -> this function had too much of redundant code
- instead: add smart this.log function
- remove logging.request (---> GhostLogger just forwards the values, it doesn't matter if that is a request or not a request)
- make .warn .debug .info .error small and smart

[ci skip]
- redesign GhostLogger to add CustomLoggers very easily

----> Example CustomLogger

function CustomLogger(options) {
  // Base iterates over defined transports
  // EXAMPLE: ['stdout', 'elasticsearch']
  Base.call(this, options);
}
util.inherits(...);

// OVERRIDE default stdout stream and your own!!!
CustomLogger.prototype.setStdoutStream = function() {}

// add a new stream
// get's called automatically when transport elasticsearch is defined
CustomLogger.prototype.setElasticsearchStream = function() {}

[ci skip]
- content/logs/domain.error.log --> contains only the errors
- content/logs/domain.log --> contains everything
- rotation for both files

[ci skip]
- one true way
- lots of npm modules use debug and it offers a nice set of features

[ci skip]
- added TODO to add unique request id
- added TODO to move logging middleware to ignition
- see TryGhost#7116
- log requestId and userId as meta information
- add cuid as dependency
- there was a new usage because we have merged a new PR using logging.debug
@kirrg001 kirrg001 force-pushed the 1.0.0-dev/logging-improvements branch from d7389e4 to 2b04496 Compare October 24, 2016 11:40
- use uuid.v1
@kirrg001
Copy link
Copy Markdown
Contributor Author

Removed cuid for requestId as suggested. Using uuid now instead 👍 Thanks

@ErisDS ErisDS merged commit 0e13ef8 into TryGhost:master Oct 25, 2016
mixonic pushed a commit to mixonic/Ghost that referenced this pull request Oct 28, 2016
* 🎨  rotation config
  - every parameter is configureable
  - increase default number of files to 100
* 🎨  ghost.log location
  - example: content/logs/http___my_ghost_blog_com_ghost.log
  - user can change the path to something custom by setting logging.path
* 🛠   add response-time as dependency
* 🎨  readable PrettyStream
  - tidy up
  - generic handling (was important to support more use cases, for example: logging.info({ anyKey: anyValue }))
  - common log format
  - less code 🕵🏻
* 🎨  GhostLogger cleanup
  - remove setLoggers -> this function had too much of redundant code
  - instead: add smart this.log function
  - remove logging.request (---> GhostLogger just forwards the values, it doesn't matter if that is a request or not a request)
  - make .warn .debug .info .error small and smart
* 🎨  app.js: add response time as middleware and remove logging.request
* 🎨  setStdoutStream and setFileStream
  - redesign GhostLogger to add CustomLoggers very easily

----> Example CustomLogger

function CustomLogger(options) {
  // Base iterates over defined transports
  // EXAMPLE: ['stdout', 'elasticsearch']
  Base.call(this, options);
}
util.inherits(...);

// OVERRIDE default stdout stream and your own!!!
CustomLogger.prototype.setStdoutStream = function() {}

// add a new stream
// get's called automatically when transport elasticsearch is defined
CustomLogger.prototype.setElasticsearchStream = function() {}

* 🎨  log into multiple file by default
  - content/logs/domain.error.log --> contains only the errors
  - content/logs/domain.log --> contains everything
  - rotation for both files
* 🔥  remove logging.debug and use npm debug only
* ✨  shortcuts for mode and level
* 🎨  jshint/jscs
* 🎨  stdout as much as possible for an error
* 🎨  fix tests
* 🎨  remove req.ip from log output, remove response-time dependency
* 🎨  create middleware for logging
  - added TODO to move logging middleware to ignition
geekhuyang pushed a commit to geekhuyang/Ghost that referenced this pull request Nov 20, 2016
* 🎨  rotation config
  - every parameter is configureable
  - increase default number of files to 100
* 🎨  ghost.log location
  - example: content/logs/http___my_ghost_blog_com_ghost.log
  - user can change the path to something custom by setting logging.path
* 🛠   add response-time as dependency
* 🎨  readable PrettyStream
  - tidy up
  - generic handling (was important to support more use cases, for example: logging.info({ anyKey: anyValue }))
  - common log format
  - less code 🕵🏻
* 🎨  GhostLogger cleanup
  - remove setLoggers -> this function had too much of redundant code
  - instead: add smart this.log function
  - remove logging.request (---> GhostLogger just forwards the values, it doesn't matter if that is a request or not a request)
  - make .warn .debug .info .error small and smart
* 🎨  app.js: add response time as middleware and remove logging.request
* 🎨  setStdoutStream and setFileStream
  - redesign GhostLogger to add CustomLoggers very easily

----> Example CustomLogger

function CustomLogger(options) {
  // Base iterates over defined transports
  // EXAMPLE: ['stdout', 'elasticsearch']
  Base.call(this, options);
}
util.inherits(...);

// OVERRIDE default stdout stream and your own!!!
CustomLogger.prototype.setStdoutStream = function() {}

// add a new stream
// get's called automatically when transport elasticsearch is defined
CustomLogger.prototype.setElasticsearchStream = function() {}

* 🎨  log into multiple file by default
  - content/logs/domain.error.log --> contains only the errors
  - content/logs/domain.log --> contains everything
  - rotation for both files
* 🔥  remove logging.debug and use npm debug only
* ✨  shortcuts for mode and level
* 🎨  jshint/jscs
* 🎨  stdout as much as possible for an error
* 🎨  fix tests
* 🎨  remove req.ip from log output, remove response-time dependency
* 🎨  create middleware for logging
  - added TODO to move logging middleware to ignition
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.

3 participants