Skip to content

Conversation

@ldmonster
Copy link
Collaborator

@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
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
Collaborator 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
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 <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
@ldmonster ldmonster requested a review from yalosev November 2, 2024 19:41
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
@yalosev yalosev merged commit 7eb16e6 into main Nov 5, 2024
@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.

5 participants