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

A basic one line format #1

Merged
merged 42 commits into from
Aug 17, 2022
Merged

Conversation

TommyDew42
Copy link
Contributor

@TommyDew42 TommyDew42 commented Aug 11, 2022

closes fastify/fastify#4027

How does the log look like

YYYY-MM-dd HH:mm:ss.SSSTZ - <level> - <method> <route path> - <message>

The logs look like this:
Screenshot 2022-08-14 at 2 50 57 AM

Unit tests

The following unit tests are included:

  • fastify log
    • log of server being started
    • log of incoming request and response
    • user custom log
  • main function messageFormat
    • a basic logDescriptor without the req object
    • a basic logDescriptor with the req object

I believe without doing anything, we have already allowed the following usage:

fastify({
  logger: {
    transport: 'one-line-logger'
  }
})

It's because pino uses require.resolve('one-line-logger') to look for the path to main file of this package.

Checklist

.gitignore Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Good work! I've left some notes.

One question: should we add some color to each line?

README.md Outdated
const server = fastify({
logger: {
transport: "one-line-logger",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
transport: "one-line-logger",
transport: "@fastify/one-line-logger",

@@ -0,0 +1,54 @@
const pretty = require('pino-pretty')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const pretty = require('pino-pretty')
'use strict'
const pretty = require('pino-pretty')

test/helpers.js Show resolved Hide resolved
@@ -0,0 +1,90 @@
const { serverFactory, EPOCH, TIME } = require('./helpers')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const { serverFactory, EPOCH, TIME } = require('./helpers')
'use strict'
const { serverFactory, EPOCH, TIME } = require('./helpers')

@@ -0,0 +1,31 @@
const { EPOCH, TIME, MESSAGE_KEY } = require('./helpers')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const { EPOCH, TIME, MESSAGE_KEY } = require('./helpers')
'use strict'
const { EPOCH, TIME, MESSAGE_KEY } = require('./helpers')

src/index.js Outdated
const second = dateObject.getSeconds()
const milliSecond = dateObject.getMilliseconds()

const time = `${year}-${month}-${date} ${hour}:${minute}:${second}.${milliSecond}`
Copy link
Member

Choose a reason for hiding this comment

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

I think the timezone could be useful here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about changing it to this then?

Z YYYY-MM-dd HH:mm:ss.SSS - <level> - <method> <route path> - <message>

It will be like this

GMT+0800 2017-02-14 20:51:48.000 - info - request completed

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I mean the timezone offset:

2017-02-14 20:51:48.000+08:00 - info - request completed

This format should be compliant to the ISO 8601
https://www.w3.org/TR/NOTE-datetime

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/index.js Show resolved Hide resolved
level: number;
req?: Request;
};
export declare const messageFormatFactory: (colorize: boolean) => (log: LogDescriptor, messageKey: string) => string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed colorizerFactory is exported in the pino-pretty js file but not in the ts definition file.

Do we not want to expose the messageFormatFactory, Request & LogDescriptor too? But only export target.

Copy link
Member

Choose a reason for hiding this comment

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

I noticed colorizerFactory is exported in the pino-pretty js file but not in the ts definition file.

PR to pino-pretty?

@TommyDew42
Copy link
Contributor Author

Also updated the PR description for the timezone and color!

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

last two comment, then LGTM

Nice work!

src/index.js Show resolved Hide resolved
src/index.js Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@TommyDew42
Copy link
Contributor Author

@mcollina ops sorry pushed a commit about readme. 😓 can you help run check again?

@mcollina mcollina merged commit bfba816 into fastify:master Aug 17, 2022
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.

Provide a one-line log format as a pino transport
5 participants