-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add initial logger integration #4265
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
Conversation
kanongil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just adding some things I noticed.
Also, I would really like to have a way to change the log level at runtime. That way I can up the log level of running servers to better diagnose intermittent issues.
| logger: Validate.object({ | ||
| additionalFields: Validate.object(), | ||
| level: Validate.valid('emergency', 'alert', 'critical', 'error', 'warning', 'notice', 'info', 'debug').default('info'), | ||
| stderr: Validate.object().optional().instance(Stream.Writable), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writable does not really support instanceof check. Eg. any stream inherited from Duplex will fail this check, as it inherits from Readable. Stream will be the safe check.
| return; | ||
| } | ||
|
|
||
| let { level, type, stdout, stderr, additionalFields } = this.settings.logger; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer const here. Can be done by assigning default values.
|
One thing on my mind is how to reconcile const server = Hapi.server({
debug: {
// This is the default value: typically users wouldn't explicitly set this to 'debug' (backwards compatible)
type: 'debug',
request: ['implementation']
}
});
const server = Hapi.server({
debug: {
// Set the type to 'std' or 'pino' to use the new logging functionality
type: 'std',
level: 'notice'
}
});Then in a major version of hapi we could (for example) rename |
|
I think integrating the log plugin might be the wrong approach. We will be forced to expose different log features through hapi and surface helper methods on the server for things like changing the log level. This is fine, but it also tightly couples the log plugin releases with hapi, which may force unnecessary upgrades on people who already have a logging solution. Also, as @devinivy points out there are some inconsistencies between this and the existing debug options. Instead, I propose that we keep log as a separate plugin that we support and encourage people to use for their logging needs. Thank you for the review on this PR, but I'm going to close this PR for now as it feels like the wrong approach. |
loggersettings.loggersettings are slightly different than @hapi/log options primarily to simplify usageloggerbeing enabled and there don't appear to be any conflicts, only that log statements can be output twice since there are 2 loggers.PR is for initial review, shouldn't be merged until we agree that @hapi/log is 1.0.0 complete.