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 #441

Merged
merged 9 commits into from
Feb 5, 2024

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?

@amhester amhester requested a review from a team as a code owner January 17, 2024 16:12
Copy link
Contributor

@george-e-shaw-iv george-e-shaw-iv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to still think about your questions in the PR summary, sorry for the delay.

pkg/olog/hooks/app.go Outdated Show resolved Hide resolved
@malept
Copy link
Member

malept commented Jan 29, 2024

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?

I think as long as they reside in logically distinct files in the olog package, keeping it all in the same package is fine. I would prefer the API design that makes it the easiest for the user, which at the moment to me, seems like the "keep it all in one package" option.

Should the provided hook funcs live in this hooks package, or should they simply live with their related domain

I would prefer if the hook funcs live in their related domain. I am generally worried that folks will start to just dump hooks in the hooks package.

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.

It seems fine, it's similar to the MarshalLog pattern used by the existing log module so at least we're not changing concepts on users when they migrate.

@amhester
Copy link
Contributor Author

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?

I think as long as they reside in logically distinct files in the olog package, keeping it all in the same package is fine. I would prefer the API design that makes it the easiest for the user, which at the moment to me, seems like the "keep it all in one package" option.

Should the provided hook funcs live in this hooks package, or should they simply live with their related domain

I would prefer if the hook funcs live in their related domain. I am generally worried that folks will start to just dump hooks in the hooks package.

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.

It seems fine, it's similar to the MarshalLog pattern used by the existing log module so at least we're not changing concepts on users when they migrate.

I've gone ahead and refactored per your comments. Seems reasonable to me.

@amhester amhester merged commit 5bf8066 into jaredallard/feat/olog Feb 5, 2024
1 check passed
@amhester amhester deleted the alexhester/feat/olog branch February 5, 2024 13:16
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.

4 participants