Skip to content

log: cleanups and improvements to decouple more from logrus#8894

Merged
dmcgowan merged 12 commits intocontainerd:mainfrom
thaJeztah:log_improve
Aug 21, 2023
Merged

log: cleanups and improvements to decouple more from logrus#8894
dmcgowan merged 12 commits intocontainerd:mainfrom
thaJeztah:log_improve

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Jul 30, 2023

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

log: define G() as a function instead of a variable

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.

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.

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.

log: swap logrus functions with their equivalent on default logger

logrus.SetLevel(), logrus.GetLevel() and logrus.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:

  • 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).

With this PR:

docs

Comment thread log/context.go
// 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
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@thaJeztah thaJeztah requested review from cpuguy83 and mxpv July 30, 2023 20:33
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]>
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]>
Copy link
Copy Markdown
Member

@mxpv mxpv left a comment

Choose a reason for hiding this comment

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

Nice!
A non-blocking nit below.

Comment thread log/context.go
// - "error" ([ErrorLevel])
// - "fatal" ([FatalLevel])
// - "panic" ([PanicLevel])
func SetLevel(level string) error {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we've introduced Level type above, should we accept it here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So this is the reverse; the Level above is the numeric (integer) value after parsing the string value 😞.

@thaJeztah
Copy link
Copy Markdown
Member Author

FWIW; I had some conversations with @AkihiroSuda and @cpuguy83 about this package; both mentioned "shouldn't we use to slog instead"?

And I agree that we (likely) want to ultimately end up using Golang's slog, however, due to containerd's circular dependency on itself, and a lot of code still assuming either "containerd/log" or "logrus" semantics in various repositories, it may take a couple of iterations before we'd be able to get there.

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, SetFormat("json") could be translated into something like;

L = slog.New(slog.NewJSONHandler(os.Stdout, nil))

(potentially with some wrapper so that we can convert L.WithField("count", 3).Info("hello") into slog.Info("hello", "count", 3), etc.

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.

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.

3 participants