log: cleanups and improvements to decouple more from logrus#8894
log: cleanups and improvements to decouple more from logrus#8894dmcgowan merged 12 commits intocontainerd:mainfrom
Conversation
| // passed around as much as you wish to avoid field duplication. | ||
| // | ||
| // Entry is a transitional type, and currently an alias for [logrus.Entry]. | ||
| type Entry = logrus.Entry |
There was a problem hiding this comment.
Entry is the tricky one; I tried if I could replace it with an interface, but unfortunately that's not (directly) possible, as some code depends on exported fields in the type.
We may be able to swap out things once all code (including plugins etc) have transitioned and directly depend on this package.
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Testify was only used for a basic assertion. Remove the dependency, in preparation of (potentially) moving this package to a separate module. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Also updated the level descriptions with their documentation from logrus. Signed-off-by: Sebastiaan van Stijn <[email protected]>
While other log-levels are not currently used in containerd itself, they can be returned by `GetLevel()`, and are accepted (no error) by `SetLevel()`. We should either accept those values, or produce an error (in `SetLevel()`), but given that there's other ways to set the log-level, we should probably acknowledge that this package is a transitional package, and still closely tied to logrus (for the time being). Signed-off-by: Sebastiaan van Stijn <[email protected]>
The `G` variable is exported, and not expected to be overwritten externally. Defining it as a function also documents it as a function on https://pkg.go.dev, instead of a variable; https://pkg.go.dev/github.com/containerd/[email protected]/log#pkg-variables Note that (while the godoc suggests otherwise) I made `GetLogger` an alias for `G`, as `G` is the most commonly used function (not the other way round), although I don't think there's a performance gain in doing so. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Strong-type the format. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Don't return logrus types from exported functions. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Decouple it from logrus, but with the same type. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Add a package doc to (try to) describe the purpose of this package, and to describe the purpose (and expectations) of aliases provided by the package. > Package log provides types and functions related to logging, passing > loggers through a context, and attaching context to the logger. > > # Transitional types > > This package contains various types that are aliases for types in [logrus]. > These aliases are intended for transitioning away from hard-coding logrus > as logging implementation. Consumers of this package are encouraged to use > the type-aliases from this package instead of directly using their logrus > equivalent. > > The intent is to replace these aliases with locally defined types and > interfaces once all consumers are no longer directly importing logrus > types. > > IMPORTANT: due to the transitional purpose of this package, it is not > guaranteed for the full logrus API to be provided in the future. As > outlined, these aliases are provided as a step to transition away from > a specific implementation which, as a result, exposes the full logrus API. > While no decisions have been made on the ultimate design and interface > provided by this package, we do not expect carrying "less common" features. Signed-off-by: Sebastiaan van Stijn <[email protected]>
[`logrus.SetLevel()`][1], [`logrus.GetLevel()`][2] and [`logrus.SetFormatter()`][3] are all convenience functions to configure logrus' standardlogger, which is the logger to which we hold a reference in the Entry configured on [`log.L`][4]. This patch: - swaps calls to `logrus.SetLevel`, `logrus.GetLevel` and `logrus.SetFormatter` for their equivalents on `log.L`. This makes it clearer what `SetLevel` does, and makes sure that we set the log-level of the logger / entry we define in our package (even if that would be swapped with a different instance). - removes the use of `logrus.NewEntry` with directly constructing a `Entry`, using the local `Entry` alias (anticipating we can swap that type in future). [1]: https://github.com/sirupsen/logrus/blob/dd1b4c2e81afc5c255f216a722b012ed26be57df/exported.go#L34C1-L37 [2]: https://github.com/sirupsen/logrus/blob/dd1b4c2e81afc5c255f216a722b012ed26be57df/exported.go#L39-L42 [3]: https://github.com/sirupsen/logrus/blob/dd1b4c2e81afc5c255f216a722b012ed26be57df/exported.go#L23-L26 [4]: https://github.com/sirupsen/logrus/blob/dd1b4c2e81afc5c255f216a722b012ed26be57df/exported.go#L9-L16 Signed-off-by: Sebastiaan van Stijn <[email protected]>
| // - "error" ([ErrorLevel]) | ||
| // - "fatal" ([FatalLevel]) | ||
| // - "panic" ([PanicLevel]) | ||
| func SetLevel(level string) error { |
There was a problem hiding this comment.
Since we've introduced Level type above, should we accept it here?
There was a problem hiding this comment.
So this is the reverse; the Level above is the numeric (integer) value after parsing the string value 😞.
|
FWIW; I had some conversations with @AkihiroSuda and @cpuguy83 about this package; both mentioned "shouldn't we use to And I agree that we (likely) want to ultimately end up using Golang's I think we can use this package as an intermediate solution to get there; removing the direct dependency / imports of logrus in the codebase means that we can use this package as "glue" / "adapter" code to migrate the codebase. It may not be "perfect", but could help during the transition. For example, L = slog.New(slog.NewJSONHandler(os.Stdout, nil))(potentially with some wrapper so that we can convert The above is why I tried to add some disclaimers to the package with "expectations" of backward compatibility; we can look for what parts of logrus's features we want to provide adaptor code (and what parts we don't want to support), and I think it would be beneficial to move this package to its own module, so that it can be updated separate from containerd itself, to (somewhat) prevent circular-dependency hell. |
We'll probably still have some way to go, but this PR attempts to make consumers of this package less coupled to logrus.
These are still aliases, but allowing consumers to use the types defined by this package allows us to switch definitions at some point.
log: remove testify dependency
Testify was only used for a basic assertion. Remove the dependency,
in preparation of (potentially) moving this package to a separate
module.
log: SetFormat: include returns in switch
log: WithLogger: remove redundant intermediate var
log: group "enum" consts and touch-up docs
Also updated the level descriptions with their documentation from
logrus.
log: add all log-levels that are accepted
While other log-levels are not currently used in containerd itself,
they can be returned by
GetLevel(), and are accepted (no error) bySetLevel(). We should either accept those values, or produce anerror (in
SetLevel()), but given that there's other ways to set thelog-level, we should probably acknowledge that this package is a transitional
package, and still closely tied to logrus (for the time being).
log: define G() as a function instead of a variable
The
Gvariable is exported, and not expected to be overwrittenexternally. Defining it as a function also documents it as a function
on https://pkg.go.dev, instead of a variable; https://pkg.go.dev/github.com/containerd/[email protected]/log#pkg-variables
Note that (while the godoc suggests otherwise) I made
GetLoggeran aliasfor
G, asGis the most commonly used function (not the other way round),although I don't think there's a performance gain in doing so.
log: define OutputFormat type
Strong-type the format.
log: add log.Entry type
Don't return logrus types from exported functions.
log: make Fields type a generic map[string]any
Decouple it from logrus, but with the same type.
log: add package documentation and summary of package's purpose
Add a package doc to (try to) describe the purpose of this package, and
to describe the purpose (and expectations) of aliases provided by the
package.
log: swap logrus functions with their equivalent on default logger
logrus.SetLevel(),logrus.GetLevel()andlogrus.SetFormatter()are all convenience functions to configure logrus' standardlogger, which is the
logger to which we hold a reference in the Entry configured on
log.L.This patch:
logrus.SetLevel,logrus.GetLevelandlogrus.SetFormatterfor their equivalents on
log.L. This makes it clearer whatSetLeveldoes,and makes sure that we set the log-level of the logger / entry we define in
our package (even if that would be swapped with a different instance).
logrus.NewEntrywith directly constructing aEntry,using the local
Entryalias (anticipating we can swap that type in future).With this PR: