You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Currently salt/log package tries to abstract Logrus and Zap into a common formatted-logger interface (i.e., Infof(msg string, args ...any), etc. ). While logrus is designed as a formatted-logger, uber/zap is specifically designed for efficient structured logging and this kind of abstraction nullifies the major benefit of it.
I propose we remove this abstraction altogether1 and assume direct usage of zap within salt and in ODPF applications that use salt. Benefits of doing this:
All the benefits of structured logging (easy to parse logs, easy to search/filter by field values, easy to attach request context with each log, etc.)
Not giving an abstracted formatted-logger will force us to always stick to structured logging.
Assuming zap as the logger of choice allows us to provide certain useful utility abstractions. Few examples:
* A request-logging middleware in mux package that automatically logs request info (method, path, client-ip, etc.) and response info (status, response time, etc.)
* A middleware for injecting request related context (req-id, current user id, the route info, etc.) into req.Context() so that every log in all the subsequent layers automatically add this to every log entry.
Footnotes
We can still have some utility functions if we need to (e.g., a helper to inject log context into ctx). But attempting to abstract over logging functionality will not have justifiable benefits. ↩
The text was updated successfully, but these errors were encountered:
Currently
salt/log
package tries to abstract Logrus and Zap into a common formatted-logger interface (i.e.,Infof(msg string, args ...any)
, etc. ). Whilelogrus
is designed as a formatted-logger,uber/zap
is specifically designed for efficient structured logging and this kind of abstraction nullifies the major benefit of it.I propose we remove this abstraction altogether1 and assume direct usage of zap within salt and in ODPF applications that use salt. Benefits of doing this:
* A request-logging middleware in
mux
package that automatically logs request info (method, path, client-ip, etc.) and response info (status, response time, etc.)* A middleware for injecting request related context (req-id, current user id, the route info, etc.) into
req.Context()
so that every log in all the subsequent layers automatically add this to every log entry.Footnotes
We can still have some utility functions if we need to (e.g., a helper to inject log context into
ctx
). But attempting to abstract over logging functionality will not have justifiable benefits. ↩The text was updated successfully, but these errors were encountered: