-
Notifications
You must be signed in to change notification settings - Fork 4
Update logging in Mira #174
Conversation
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.
LGTM! A few comments
@@ -36,7 +35,6 @@ process.on('unhandledRejection', onUnhandledError); | |||
|
|||
app | |||
.use(swagger2koa.ui(document, '/openapi')) | |||
.use(koaLoggerWinston(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.
Was there no way to set the koa logger to log on debug level? It could be useful on debug level
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.
Not that I could find. If we want to log the /engines
endpoint on info anyway, this was the easiest way I found. Are there other benefits of using koa-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.
Seems to be hard-coded to info
for non-error codes. Let's remove then.
@@ -16,7 +16,7 @@ class Logger { | |||
timestamp: true, | |||
json: true, | |||
stringify: true, | |||
level: process.env.LOG_LEVEL || 'silly', | |||
level: process.env.MIRA_LOG_LEVEL || 'info', |
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.
Is this "standard"? Or, should it be MIRA_MINIMUM_LOG_LEVEL
or similar?
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.
In my opinion MIRA_LOG_LEVEL
should be sufficient. Haven't seen any standard regarding this, but didn't dig to deep :)
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.
👍
This PR contains the following:
info
This closes #168