-
Notifications
You must be signed in to change notification settings - Fork 107
feat: add configurable logLevel to install options
#283
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
12ff7a7 to
232a734
Compare
erickzhao
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.
API-wise, I think a logLevel option would make more sense and cover all our bases.
Options would be debug, info, warn, error, or none, where enabling a less important log level would make all higher-priority ones.
It feels more ergonomic that way since you don't usually want to ignore non-contiguous log levels.
|
|
||
| ```js | ||
| await devtron.install({ ignoreLogs: ['debug', '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.
Agreed with @erickzhao about a logLevel option. Then these two examples could be
devtron.install({ logLevel: [] })
devtron.install({ logLevel: ['info', 'warn'] })
I'm partial to the idea of the quiet option as a shorthand though. Open to either keeping it or removing it.
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 head it'd look more like a sliding scale:
devtron.install({ logLevel: 'quiet' }) // or `'none'`
devtron.install({ logLevel: 'error' }) // only shows `error`
devtron.install({ logLevel: 'warn' }) // shows `warn`, or `error`, but not `debug` or `info`
devtron.install({ logLevel: 'info' }) // shows `info`, `warn`, or `error`, but not `debug`since it'd be unlikely that you would want to specifically ignore non-contiguous log levels (e.g. ignoring error and debug but not info and warn).
|
@erickzhao I made the requested changes and also updated the NPM Package to |
quiet mode and ignoreLogs supportlogLevel to install options
src/utils/Logger.ts
Outdated
| private readonly logLevelMap: Record<LogLevel, number> = { | ||
| debug: 1, | ||
| info: 2, | ||
| warn: 3, | ||
| error: 4, | ||
| none: 5, | ||
| }; |
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 think we could combine this and the LogLevel type in src/types/shared.ts if we just make LogLevel into a numeric enum:
enum LogLevel {
none,
debug,
info,
warn,
error
}
Enums without assigned values in TS auto-assign to incrementing number values so you can get > comparisons out of the box without defining a separate map.
See:
- TypeScript docs: https://www.typescriptlang.org/docs/handbook/enums.html#numeric-enums
- Playground: https://www.typescriptlang.org/play/?#code/KYOwrgtgBAMg9gcxsAbsANlA3gKCvqAE2ACMwEAaPAgSxADM4qCoB3AQwCcQ7Lr9gnTnE44AvjhwBjOCADOAFyj0wIKAF4oACmAAuWImRp0ASg0A+bPygz5cdMAB06RDqiX4SVBkfEyCEwBucUkcFRAtTyMfOkYTMNVIw290X1JyEyA
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.
Since we want users to be able to pass log level as a string, like devtron.install({ logLevel: 'warn' }), I'll need to create another type with log level strings as well, right ?
|
I've added the |
|
🎉 This PR is included in version 2.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description of Change
Loggerclass withlogLeveloption.devtron.install(options)to configure the logger automatically, allowing users to set a minimum log level. Only logs at the selected level and above will be shown, preventing unwanted Devtron logs from cluttering the terminal.optionstable.Usage: