-
-
Notifications
You must be signed in to change notification settings - Fork 714
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
feat: Add functionality to customize log layout and introduce JSON logger #5732
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Someone is attempting to deploy a commit to the unleash-team Team on Vercel. A member of the Team first needs to authorize it. |
49ddad0
to
c30e28c
Compare
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.
Hi. Thank you for contributing. I'm not convinced of value of adding this configuration versus maintenance overhead it creates, especially with new niche dependency. For me custom logger is the way to go in this case. If there is more interest from the community or other team members find it useful I'll change my mind.
CC @chriswk
Thanks for taking the time to pull this together. I think it makes sense for Unleash to support json logging natively, as it is becoming in standard these days. I do share @Tymek concern on taking on a niche dependency for this, and want to take the opportunity to assess our options. |
@ivarconr I can definitely see the point about the dependency. It has not been updated in a while, however, it's a very lightweight library. The work to convert a log object to a JSON string is fairly trivial and that dependency can certainly be removed. I agree that it makes sense to include easily-configured JSON logging support, as structured logs are a fairly well-established best practice with logging aggregation services. The only tricky thing, I think, is providing the flexibility to configure the names of certain important keys in the resulting object (e.g. the key that contains the log message, the key for the timestamp, etc.). DataDog, for example, wants the log body to be contained in the The underlying goal of this MR is to allow the ability to configure a JSON logger through environment variables and without having to maintain a custom container image with that support added in. Though this was admittedly a while ago, there seemed to be support for the idea on the Slack channels. I've finally gotten around to sending in the contribution :D If the idea of adding first-class JSON logging support is agreeable, I can move forward with an implementation of it without adding the external dependency. Let me know your thoughts. |
@@ -109,6 +110,7 @@ export interface IUnleashOptions { | |||
session?: Partial<ISessionOption>; | |||
getLogger?: LogProvider; | |||
logLevel?: LogLevel; | |||
logLayout?: string | Layout; |
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 change ends up exposing which internal logging framework Unleash uses, which should be an implementation detail, and not exposed to the end users. By exposing it we cannot change it without doing a major release.
I am in favor of supporting json logs. But I am not fully convinced we want to commit ourself to use log4js in the future. In fact, if you do not provide a custom logger, I think the logger utilized by Unleash should be an internal concern of Unleash. For Unleash Cloud we actually use Winston, which have served our json needs well for a few years. I am considering that we might want to base a json logger on Winston instead and consider migrating over to Winston for the plain logger in the next major of Unleash (I suspect it would be to hard to keep the logs exactly the same between to two different providers, and any change to the log output should be considered a breaking change). One need we have for Unleash Cloud is to enrich the log with static fields (clientId, plan, region etc), named defaultMeta in Winston. This might be an internal need of our Cloud offering. Another thing we do is to enrich the Prometheus metrics with counters from log levels. I believe this can be useful for everyone hosting Unleash. I would love more pls opinions here. I still believe the logger used by Unleash should be an implementation detail, but we should consider how we can allow a few more options in a structured way to provide new formats and some more customization of the Unleash logs. Those options could be encapsulated in a log container in the option object. |
About the changes
This MR introduces the ability to customize the log layout of the default logger configuration without having to provide an entirely custom logger configuration. It also adds JSON structured logs (under the
json
name) using thelog4js-json-layout
library.Important files
This is a relatively small MR but most of the changes are related to the
logger.ts
andcreate-config.ts
files.Discussion points
N/A