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

feat: Add hooks log handler wrapper for olog package #440

Closed
wants to merge 23 commits into from

Conversation

amhester
Copy link
Contributor

What this PR does / why we need it

This PR attempts to address the issues/omissions noted here. This PR provides an example implementation of a log handler wrapper which provides callers with the ability to automatically add further attributes to all logs written to that logger instance. This helps solve for callers being able to easily and consistently add standard information to all logs, especially with information that is usually on the context or controlled by other tools (such as tracing information). This package provides both a handler and Logger function by which to provide these hooks, as well as an out-of-the-box hook for adding AppInfo (which also required an addition to the app package to implement the slog.LogValuer interface).

Ideally, this package would be a place where we could provide other out-of-the-box hooks for other information such as tracing, actor, and whatever else makes sense. However, there is also an argument that these hooks could simply live in the packages which govern that data (i.e. the AppInfo hook could live in the app package).

TODO: Expose global registry level setting. More info below:

In addition to this hooks package, I also added a potential way to actually get/set the levels for different modules/packages using the global level registry (currently not used or accessible from what I can tell). Not sure if this is something we want to support, but I feel could be nice to adjust required logging levels for different packages for a variety of purposes (debugging, cost cutting, etc...).

Notes for your reviewers

This is just an example implementation and is written primarily to start the discussion around how we want to handle this workflow, as the base olog implementation doesn't easily allow for adding data to logs, especially from context (the olog log handler is not exposed in any way). While a lot of this functionality could be provided by developers in each of their funcs which are called per request/transaction, it would be cumbersome and easily missed. That being said, there are a number of points I think are worth discussing regarding this implementation and I'd like thoughts on:

  1. Should the hooks implementation (the handler which wraps the default global handler and allows for hooks) reside in the olog package directly instead of this hooks sub-package? The more I think about this, the more I'm inclined to think it should, and then be exposed as options on the New func. The options would basically just allow for hook funcs to be provided similar to the Logger func provided by this hooks package. However, I wanted to keep these concepts isolated for now, which is why I chose to implement it in a separate package. Thoughts?
  2. Should the provided hook funcs live in this hooks package, or should they simply live with their related domain. For example, this PR provides an AppInfo hook for extracting and adding AppInfo data to the logs, but should this hook just live in the app package?
  3. Is this hooks pattern something we're comfortable with? It seems a natural way to me to augment the log records automatically, but am open to other patterns to accomplish this.

Global registry level setting

Not sure if this is something we necessarily want/need to support, especially in the near-term, but since it was noted as a goal (Ability to change log level at a module and package level.), I felt it should probably be addressed or at least discussed. However, I can't quite determine a great solution for this as of yet. I don't think wholly exposing the entire global level registry necessarily makes sense, and I personally think having users set levels by key where the key is either some package or module name is also not that great of an experience (what specifically is a module or package name key? You can only really know for sure when looking under the hood at the implementation). 2 possible ways to help ease the usage of the global registry could be (but I'm not sold on either tbh):

  1. Providing an iterator func to allow users to search all registered packages/modules and determine what they want to do from there. This iterator could even probably pass a level setting func to provide a controlled and succinct way to set those levels per package. However, this would be cumbersome and doesn't really help the problem of making the registry and its keys both accessible and understandable. Also, as is, the global registry isn't actually getting set when a package creates a new logger. This is probably just not a good way to go.
  2. Have each package which plans on using olog/slog to expose their module/package names (as determined by the olog package) as package vars. Then any called could simply use these vars when accessing (set/get'ing) the global registry and not need to care about the actual format of the keys for those packages/modules. However, this then puts the burden on those packages to always expose this information, which also isn't necessarily great.

Also, I could just be over-thinking this and it could be entirely reasonable for people to know what the key for any given package/module will be, and use that to directly access to registry. May not be too bad if given proper documentation for expected format. Thoughts?

Jared Allard and others added 23 commits December 6, 2023 15:01
PoC implementation of a logger using `slog.Logger` as the primary type
with wrappers around it. Adds a global registry used for determining the
log level of a logger based on the module. Eventually, `New()` will
associate loggers with the current package and module name.

LogLevels can be set based on module or package name using the
`(*levelRegistry).Set` function.

While it would be more performant to store the `leveler` instances in
the registry and update those on `Set`, I feel that would complicate the
code. I also feel that a should a logger become a bottleneck in the
execution of an application, it should be removed in favor of tracing or
metrics gathering.
@amhester amhester requested a review from a team as a code owner January 17, 2024 16:04
@amhester amhester closed this Jan 17, 2024
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

Successfully merging this pull request may close these issues.

2 participants