Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

Update logging in Mira #174

Merged
merged 4 commits into from
Oct 27, 2017
Merged

Update logging in Mira #174

merged 4 commits into from
Oct 27, 2017

Conversation

wennmo
Copy link
Member

@wennmo wennmo commented Oct 27, 2017

This PR contains the following:

  • Renaming env variable for toggling log level to be consistent with the rest of Miras env variables.
  • Setting default log level to info
  • Removing koa-logger since we want different log levels depending on endpoint. Health and metrics endpoint might be too verbose in production.
  • Remove unused env variable DEV_MODE, and obsolete minikube tutorial

This closes #168

Copy link
Contributor

@janmiderback janmiderback left a 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))
Copy link
Contributor

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

Copy link
Member Author

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?

Copy link
Contributor

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',
Copy link
Contributor

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?

Copy link
Member Author

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@wennmo wennmo merged commit a28be64 into master Oct 27, 2017
@wennmo wennmo deleted the log_env branch October 27, 2017 11:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make sure Mira follows log format specified in G3 Service contract
3 participants