Skip to content

operator: Convert logrus to slog #35567

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
Merged
merged 1 commit into from
Oct 29, 2024
Merged

operator: Convert logrus to slog #35567

merged 1 commit into from
Oct 29, 2024

Conversation

sayboras
Copy link
Member

No description provided.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 26, 2024
@sayboras sayboras added the release-note/misc This PR makes changes that have no direct user impact. label Oct 26, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 26, 2024
@sayboras
Copy link
Member Author

/test

@sayboras sayboras marked this pull request as ready for review October 27, 2024 02:23
@sayboras sayboras requested review from a team as code owners October 27, 2024 02:23
@sayboras sayboras force-pushed the pr/tammach/operator-slog branch from aceb33d to 6ca7d7f Compare October 27, 2024 02:55
@sayboras
Copy link
Member Author

/test

bimmlerd
bimmlerd approved these changes Oct 28, 2024
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment •
edited
Loading

Choose a reason for hiding this comment

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

contributing change lgtm; minor nit when glancing through code

doniacld
doniacld reviewed Oct 28, 2024
View reviewed changes
@sayboras
operator: Convert logrus to slog
9fc8e03 
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras sayboras force-pushed the pr/tammach/operator-slog branch from 6ca7d7f to 9fc8e03 Compare October 28, 2024 14:10
@sayboras
Copy link
Member Author

/test

dylandreimerink
dylandreimerink approved these changes Oct 29, 2024
View reviewed changes
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

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

Seems good for my CODEOWNERS

pippolo84
pippolo84 approved these changes Oct 29, 2024
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

Nice, thanks! 🚀

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 29, 2024
@sayboras sayboras added this pull request to the merge queue Oct 29, 2024
Merged via the queue into main with commit 0bc32ae Oct 29, 2024
266 checks passed
@sayboras sayboras deleted the pr/tammach/operator-slog branch October 29, 2024 22:16
@giorio94 giorio94 mentioned this pull request Oct 30, 2024
Merged
giorio94
giorio94 reviewed Oct 30, 2024
View reviewed changes
operator/watchers/logfields.go
@@ -8,4 +8,4 @@ import (
"github.com/cilium/cilium/pkg/logging/logfields"
)

var log = logging.DefaultLogger.WithField(logfields.LogSubsys, "watchers")
var log = logging.DefaultSlogLogger.With(logfields.LogSubsys, "watchers")
Copy link
Member

Choose a reason for hiding this comment

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

@sayboras This appears not to correctly respect the settings (e.g., debug log), because the default logger is completely replaced (rather than being mutated) during the configuration phase:

cilium/pkg/logging/slog.go

Lines 77 to 88 in 91a844c

switch logFormat {
case LogFormatJSON, LogFormatJSONTimestamp:
DefaultSlogLogger = slog.New(slog.NewJSONHandler(
writer,
&opts,
))
case LogFormatText, LogFormatTextTimestamp:
DefaultSlogLogger = slog.New(slog.NewTextHandler(
writer,
&opts,
))
}

Could you please take a look?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah thanks, let me double check

Copy link
Member

@bimmlerd bimmlerd Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Argh, yeah, I committed that crime originally; I should have caught this. IIRC the idea was that you're supposed to use the slog.Logger provided by hive, which would then be a stable reference

(the problem is that slog doesn't allow you to mutate the logger)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I guess the tricky part is for the modules that have not yet been converted to hive yet. I wonder whether it could make sense to maybe unexport that variable (and provide a safer getter if necessary) to prevent potentially storing a reference to the uninitialized version.

sayboras added a commit that referenced this pull request Nov 2, 2024
@sayboras
watcher: Avoid using global default slog
54166e1 
As highlighted in below comment, the default logger is completely
replaced (rather than being mutated) during the configuration phase, so
we should avoid using global slog instance.

Relates: #35567

Reported-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras sayboras mentioned this pull request Nov 2, 2024
Merged
sayboras added a commit that referenced this pull request Nov 2, 2024
@sayboras
watcher: Avoid using global default slog
6031f83 
As highlighted in below comment, the default logger is completely
replaced (rather than being mutated) during the configuration phase, so
we should avoid using global slog instance.

Relates: #35567

Reported-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit that referenced this pull request Nov 4, 2024
@sayboras
watcher: Avoid using global default slog
6be5b67 
As highlighted in below comment, the default logger is completely
replaced (rather than being mutated) during the configuration phase, so
we should avoid using global slog instance.

Relates: #35567

Reported-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
github-merge-queue bot pushed a commit that referenced this pull request Nov 4, 2024
@sayboras
watcher: Avoid using global default slog
0e4bc5e 
As highlighted in below comment, the default logger is completely
replaced (rather than being mutated) during the configuration phase, so
we should avoid using global slog instance.

Relates: #35567

Reported-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bimmlerd bimmlerd bimmlerd approved these changes

@giorio94 giorio94 giorio94 left review comments

@pippolo84 pippolo84 pippolo84 approved these changes

@dylandreimerink dylandreimerink dylandreimerink approved these changes

@thorn3r thorn3r thorn3r approved these changes

@harsimran-pabla harsimran-pabla harsimran-pabla approved these changes

+1 more reviewer

@doniacld doniacld doniacld approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sayboras @pippolo84 @dylandreimerink @bimmlerd @doniacld @thorn3r @giorio94 @harsimran-pabla