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

[shell-operator] Replace Logrus with slog #671

Merged
merged 21 commits into from
Nov 5, 2024
Merged

[shell-operator] Replace Logrus with slog #671

merged 21 commits into from
Nov 5, 2024

Conversation

ldmonster
Copy link
Contributor

@ldmonster ldmonster commented Oct 18, 2024

Overview

Replace Logrus to Slog implementation

What this PR does / why we need it

We can freely remove Logrus. That's the point!

Special notes for your reviewer

For testing purpose deckhouse/deckhouse#10209

@ldmonster ldmonster added the dependencies Pull requests that update a dependency file label Oct 18, 2024
@ldmonster ldmonster marked this pull request as ready for review October 21, 2024 13:06
pkg/app/log.go Outdated Show resolved Hide resolved
pkg/app/log.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/hook/hook_manager.go Outdated Show resolved Hide resolved
Copy link
Contributor

@libmonsoon-dev libmonsoon-dev left a comment

Choose a reason for hiding this comment

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

I propose to define an interface at the shell-operator level (currently it is here https://github.com/flant/addon-operator/blob/main/pkg/utils/logger/logger.go) but with the following changes:

  1. mark all *f and *ln methods as deprecated. *ln because Println in the logger does nothing different from Print and simply inflates the interface for no reason, and *f methods should not be called in the structured logger, all arguments should be passed as slog.Attr. (or key-value pairs, but we are not using this now, see below). Otherwise, we will have a situation like now, when all non-primitive types are first converted to a string by the fmt package, and then also encoded with json, the consequences of which we see every day on environments.

  2. Considering that slog.Info perceives arguments as key-value pairs, we cannot use it directly in this interface, so at this stage I propose to create only With, WithGroup, FatalAttrs, PanicAttrs, DebugAttrs, ErrorAttrs, InfoAttrs, WarnAttrs, leaving the slog.Logger methods that accept key-value pairs in the form of []any until better times.

@ldmonster
Copy link
Contributor Author

ldmonster commented Oct 22, 2024

I propose to define an interface at the shell-operator level (currently it is here https://github.com/flant/addon-operator/blob/main/pkg/utils/logger/logger.go) but with the following changes:

  1. mark all *f and *ln methods as deprecated. *ln because Println in the logger does nothing different from Print and simply inflates the interface for no reason, and *f methods should not be called in the structured logger, all arguments should be passed as slog.Attr. (or key-value pairs, but we are not using this now, see below). Otherwise, we will have a situation like now, when all non-primitive types are first converted to a string by the fmt package, and then also encoded with json, the consequences of which we see every day on environments.
  2. Considering that slog.Info perceives arguments as key-value pairs, we cannot use it directly in this interface, so at this stage I propose to create only With, WithGroup, FatalAttrs, PanicAttrs, DebugAttrs, ErrorAttrs, InfoAttrs, WarnAttrs, leaving the slog.Logger methods that accept key-value pairs in the form of []any until better times.
  1. I did not find *ln methods. About marking *f methods - good. I made it. Global logger must be deprecated at all, so i mark it in head of package. About slog.Attrs. We block one way of using attrs after with linter.
  2. No reason, if we have a linter.

@yalosev yalosev changed the title [shell-operator] Replace Logrus with flant logger [shell-operator] Replace Logrus with slog Oct 23, 2024
pkg/config/config.go Outdated Show resolved Hide resolved
libmonsoon-dev
libmonsoon-dev previously approved these changes Oct 24, 2024
@ldmonster ldmonster force-pushed the feature/slog branch 2 times, most recently from 9577036 to e37695d Compare October 28, 2024 15:53
@ldmonster ldmonster requested a review from ipaqsa October 29, 2024 15:39
@yalosev yalosev added the run/tests Run tests on full matrix of k8s versions label Nov 1, 2024
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Pavel Okhlopkov added 2 commits November 2, 2024 15:47
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Pavel Okhlopkov added 5 commits November 2, 2024 20:31
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
@ldmonster ldmonster requested a review from yalosev November 2, 2024 19:41
Pavel Okhlopkov added 4 commits November 2, 2024 22:47
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Pavel Okhlopkov added 8 commits November 5, 2024 10:24
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
@yalosev yalosev merged commit 7eb16e6 into main Nov 5, 2024
8 checks passed
@yalosev yalosev deleted the feature/slog branch November 5, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file run/tests Run tests on full matrix of k8s versions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants