Skip to content
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

unified logging pattern & facilities #53

Open
spy16 opened this issue Sep 14, 2022 · 1 comment
Open

unified logging pattern & facilities #53

spy16 opened this issue Sep 14, 2022 · 1 comment

Comments

@spy16
Copy link
Contributor

spy16 commented Sep 14, 2022

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

  1. 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.

@krtkvrm
Copy link
Member

krtkvrm commented Sep 19, 2022

+1, we can just have helpers over uber/zap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants