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.

Sign up for GitHub

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

Jump to bottom

[release/1.7 backport] log: cleanups and improvements to decouple more from logrus #9001

Merged
merged 12 commits into from
Aug 24, 2023
Merged

[release/1.7 backport] log: cleanups and improvements to decouple more from logrus #9001

merged 12 commits into from
Aug 24, 2023

Conversation

thaJeztah
Copy link
Member

@thaJeztah
removes/docker: remove unnecessary conversion (unconvert)
df76aae 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit e2ad5a9)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
log: remove testify dependency
6621e08 
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 <github@gone.nl>
(cherry picked from commit 6fe7e03)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
log: SetFormat: include returns in switch
bfdce4c 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 40ee5fb)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
log: WithLogger: remove redundant intermediate var
7aa4f8f 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 4a36022)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
log: group "enum" consts and touch-up docs
e8e086e 
Also updated the level descriptions with their documentation from
logrus.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 0b6333a)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
log: add all log-levels that are accepted
93b6cb7 
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 <github@gone.nl>
(cherry picked from commit 81ac648)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
log: define G() as a function instead of a variable
dbbe28b 
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/containerd@v1.6.22/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 <github@gone.nl>
(cherry picked from commit 778ac30)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
log: define OutputFormat type
0a79e67 
Strong-type the format.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit dd67240)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
log: add log.Entry type
707ca94 
Don't return logrus types from exported functions.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 634a4a1)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
log: make Fields type a generic map[string]any
932795f 
Decouple it from logrus, but with the same type.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 238da2c)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
log: add package documentation and summary of package's purpose
01445bb 
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 <github@gone.nl>
(cherry picked from commit 6baff16)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
log: swap logrus functions with their equivalent on default logger
2a9ae3c 
[`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 <github@gone.nl>
(cherry picked from commit 85a2c9a)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
dmcgowan
dmcgowan approved these changes Aug 24, 2023
View reviewed changes
Copy link
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

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

Hold until after 1.7.4

@dmcgowan dmcgowan merged commit ff0cd27 into containerd:release/1.7 Aug 24, 2023
47 checks passed
@thaJeztah thaJeztah deleted the 1.7_backport_log_improve branch August 25, 2023 15:23
Mengkzhaoyun pushed a commit to open-beagle/containerd that referenced this pull request Aug 30, 2023
Merge tag 'v1.7.5'
39ebfea 
containerd 1.7.5

Welcome to the v1.7.5 release of containerd!

The fifth patch release for containerd 1.7 fixes a versioning issue from
the previous release and includes some internal logging API changes.

See the changelog for complete list of changes

Please try out the release binaries and report any issues at
https://github.com/containerd/containerd/issues.

* Sebastiaan van Stijn
* Derek McGowan
* Akihiro Suda
* Antonio Huete Jimenez
* Phil Estes
* Samuel Karp
<details><summary>18 commits</summary>
<p>

* [release/1.7] Prepare release notes for 1.7.5 ([#9010](containerd/containerd#9010))
  * [`93b23eb10`](containerd/containerd@93b23eb) Prepare release notes for v1.7.5
  * [`fb1292c8d`](containerd/containerd@fb1292c) Bump version to v1.7.4
* [release/1.7 backport] go.mod: github.com/containerd/continuity v0.4.2 ([#9012](containerd/containerd#9012))
  * [`503ab21bf`](containerd/containerd@503ab21) go.mod: github.com/containerd/continuity v0.4.2
* [release/1.7 backport] log: cleanups and improvements to decouple more from logrus ([#9001](containerd/containerd#9001))
  * [`2a9ae3c51`](containerd/containerd@2a9ae3c) log: swap logrus functions with their equivalent on default logger
  * [`01445bb73`](containerd/containerd@01445bb) log: add package documentation and summary of package's purpose
  * [`932795f45`](containerd/containerd@932795f) log: make Fields type a generic map[string]any
  * [`707ca94d8`](containerd/containerd@707ca94) log: add log.Entry type
  * [`0a79e67e4`](containerd/containerd@0a79e67) log: define OutputFormat type
  * [`dbbe28b7d`](containerd/containerd@dbbe28b) log: define G() as a function instead of a variable
  * [`93b6cb784`](containerd/containerd@93b6cb7) log: add all log-levels that are accepted
  * [`e8e086e02`](containerd/containerd@e8e086e) log: group "enum" consts and touch-up docs
  * [`7aa4f8fdc`](containerd/containerd@7aa4f8f) log: WithLogger: remove redundant intermediate var
  * [`bfdce4ce4`](containerd/containerd@bfdce4c) log: SetFormat: include returns in switch
  * [`6621e0888`](containerd/containerd@6621e08) log: remove testify dependency
  * [`df76aaede`](containerd/containerd@df76aae) removes/docker: remove unnecessary conversion (unconvert)
</p>
</details>
<details><summary>2 commits</summary>
<p>

* Add initial DragonFly BSD support ([#230](containerd/continuity#230))
  * [`bcc6e25`](containerd/continuity@bcc6e25) dragonfly: Initial porting work
</p>
</details>

* **github.com/containerd/continuity**  1e0d26eb2381 -> v0.4.2

Previous release can be found at [v1.7.4](https://github.com/containerd/containerd/releases/tag/v1.7.4)
@thaJeztah thaJeztah mentioned this pull request Aug 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcantah dcantah dcantah approved these changes

@estesp estesp estesp approved these changes

@dmcgowan dmcgowan dmcgowan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@thaJeztah @dmcgowan @estesp @dcantah