Migrate pkg to slog #30708

benoittgt · 2025-03-25T16:44:31Z

What this PR does / why we need it:

tl;dr : migrate packages and cmd from log to slog for logs

While trying to implement #30689 but for V4. It was mentionned that I should use slog. So I tried to make a first try in
#30685, but it requires a change accross packages and cmd.

This PR migrates packages and cmd to slog.

Migrate all log to slog key-value, with everything in Debug, except logs that start with warning:
Start all logs phrase with lowercase letter like recommend in Go
Try to make logs more easily grepable by adding more often the name and the namespace of the resource
Follow same construction in struct as before, so SDK's users can pass custom slogger with a custom handler to process logs

This code still need to be fully manually tested but first I would like to get some feedbacks.

cc @robertsirc @TerryHowe

Close: #30698

Special notes for your reviewer:

If applicable:

this PR contains user facing changes (the docs needed label should be applied if so)
this PR contains unit tests
this PR has been tested for backwards compatibility

pkg/kube/logger.go

As for helm v4. We want to migrate logs to slog. Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

``` helmClient.Log = NewSlogAdapter(slog.Default()) ``` Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

See: helm#30698 (comment) Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

benoittgt · 2025-04-07T15:03:25Z

pkg/cli/logger.go

+	}
+
+	// Create a handler that removes timestamps
+	handler := slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{


We could use color for error or not https://betterstack.com/community/guides/logging/logging-in-go/

I am more curious why add the logger rather than just use slog directly? I thought I mentioned that on another PR.

I thought we wanted to chose extactly the output, like do not have the timestamp. Also to handle debug loglevel like it was done before it seems we need to have a custom slog Handler that handle the debug flag just before log is processed.

pkg/cmd/root.go

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

robertsirc

First let me thank you for all your hard work on this. Where we are calling logging looks good but how we implement the logging i think needs to change. Rather than passing around a logger Log as to just call slog directly. Would love to setup some time to disucss if you are up to it.

pkg/cmd/root.go

robertsirc · 2025-04-07T16:42:18Z

pkg/cli/logger.go

+	}
+
+	// Create a handler that removes timestamps
+	handler := slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{


I am more curious why add the logger rather than just use slog directly? I thought I mentioned that on another PR.

internal/monocular/client.go

So we should use dynamic handler to set the log level after. With this patch we can clearly see the output. Before we were always stuck in log level "info" and not seeing debug log level ``` bin/helm upgrade --install --debug --wait frontend \ --namespace test \ --set replicaCount=2 \ --set backend=http://backend-podinfo:9898/echo \ podinfo/podinfo level=DEBUG msg="getting history for release" release=frontend level=DEBUG msg="preparing upgrade" name=frontend level=DEBUG msg="performing update" name=frontend level=DEBUG msg="creating upgraded release" name=frontend level=DEBUG msg="checking resources for changes" resources=2 level=DEBUG msg="no changes detected" kind=Service name=frontend-podinfo level=DEBUG msg="patching resource" kind=Deployment name=frontend-podinfo namespace=test level=DEBUG msg="waiting for resources" count=2 timeout=5m0s level=DEBUG msg="waiting for resource" name=frontend-podinfo kind=Deployment expectedStatus=Current actualStatus=Unknown level=DEBUG msg="updating status for upgraded release" name=frontend Release "frontend" has been upgraded. Happy Helming! NAME: frontend LAST DEPLOYED: Thu Apr 10 09:56:25 2025 NAMESPACE: test STATUS: deployed REVISION: 6 DESCRIPTION: Upgrade complete ``` Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

benoittgt · 2025-04-10T12:32:09Z

Would love to setup some time to disucss if you are up to it.

I will ping you on Slack. ;)

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

benoittgt · 2025-04-10T14:04:50Z

pkg/cmd/root.go

@@ -148,6 +140,9 @@ func newRootCmdWithConfig(actionConfig *action.Configuration, out io.Writer, arg
 	settings.AddFlags(flags)
 	addKlogFlags(flags)

+	logger := logging.NewLogger(func() bool { return settings.Debug })


We need to be dynamic with the settings because it is set later. Maybe we could simplify, if you have an idea maybe.

Some ideas here: https://www.reddit.com/r/golang/comments/15nwnkl/achieve_lshortfile_with_slog/ Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

benoittgt · 2025-04-10T14:19:03Z

After a quick call with @robertsirc I removed all mention of Log and instead leveral slog directly + slog.SetDefault.

SDK developer will still be able to set a custom default logger with a dedicated handler thank to SetDefault
I choose to make a Handler for slog to do not loose the --debug behavior.
We loose the call location with the logging with slog 7f02e89
, but I think log message are pretty uniq and it's easy to find the location of the call with basic text search on Github.

Error trace are one-lined but I think it's ok.

benoit.tigeot » bin/helm uninstall --namespace test podinfo 

Error: uninstall: Release not loaded: podinfo: release: not found

benoit.tigeot » bin/helm uninstall --namespace test podinfo --debug

level=DEBUG msg="getting release history" name=podinfo
Error: uninstall: Release not loaded: podinfo: release: not found
level=DEBUG msg=error error="release: not found\nhelm.sh/helm/v4/pkg/storage/driver.init\n\t<autogenerated>:1\nruntime.doInit1\n\truntime/proc.go:7291\nruntime.doInit\n\truntime/proc.go:7258\nruntime.main\n\truntime/proc.go:254\nruntime.goexit\n\truntime/asm_arm64.s:1223\nuninstall: Release not loaded: podinfo\nhelm.sh/helm/v4/pkg/action.(*Uninstall).Run\n\thelm.sh/helm/v4/pkg/action/uninstall.go:87\nhelm.sh/helm/v4/pkg/cmd.newUninstallCmd.func2\n\thelm.sh/helm/v4/pkg/cmd/uninstall.go:60\ngithub.com/spf13/cobra.(*Command).execute\n\tgithub.com/spf13/cobra@v1.9.1/command.go:1015\ngithub.com/spf13/cobra.(*Command).ExecuteC\n\tgithub.com/spf13/cobra@v1.9.1/command.go:1148\ngithub.com/spf13/cobra.(*Command).Execute\n\tgithub.com/spf13/cobra@v1.9.1/command.go:1071\nmain.main\n\thelm.sh/helm/v4/cmd/helm/helm.go:48\nruntime.main\n\truntime/proc.go:272\nruntime.goexit\n\truntime/asm_arm64.s:1223"

Current output example:

» bin/helm upgrade --install --debug --wait frontend --namespace test --set replicaCount=2 --set backend=http://backend-podinfo:9898/echo podinfo/podinfo
level=DEBUG msg="getting history for release" release=frontend
level=DEBUG msg="getting release history" name=frontend
level=DEBUG msg="preparing upgrade" name=frontend
level=DEBUG msg="getting last revision" name=frontend
....

Sorry again for the size of the PR.

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

robertsirc

LGTM, there are some issues that we are going to want to follow up with another PR to fix but since this is part of Helm4, we have some liberties.

robertsirc · 2025-04-10T19:03:31Z

pkg/action/install.go

@@ -237,24 +238,24 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma
 	// Check reachability of cluster unless in client-only mode (e.g. `helm template` without `--validate`)
 	if !i.ClientOnly {
 		if err := i.cfg.KubeClient.IsReachable(); err != nil {
-			i.cfg.Log(fmt.Sprintf("ERROR: Cluster reachability check failed: %v", err))
+			slog.Error(fmt.Sprintf("cluster reachability check failed: %v", err))
 			return nil, errors.Wrap(err, "cluster reachability check failed")


So, we log this as an error here but then return the error below it, it will be logged again. It could make sense to change this but let me bring this back others before you change anything.

mattfarina · 2025-04-11T19:41:55Z

pkg/kube/client.go

-	if err != nil {
-		return errors.Errorf("Failed to close reader for pod: %s, container: %s", podName, containerName)
-	}


Note, this was unreachable code so it's safe to remove.

pull-request-size bot added the size/L label Mar 25, 2025

benoittgt force-pushed the migrate-kube-pkg-to-slog branch from dca38af to df264d2 Compare March 25, 2025 16:48

benoittgt commented Mar 25, 2025

View reviewed changes

pkg/kube/logger.go Outdated Show resolved Hide resolved

benoittgt marked this pull request as ready for review March 26, 2025 08:42

benoittgt changed the title ~~Migrate kube pkg to slog~~ [WIP] Migrate kube pkg to slog Mar 31, 2025

pull-request-size bot added size/XL and removed size/L labels Mar 31, 2025

benoittgt changed the title ~~[WIP] Migrate kube pkg to slog~~ [WIP] Migrate pkg to slog Mar 31, 2025

benoittgt force-pushed the migrate-kube-pkg-to-slog branch from a4e9552 to 359f585 Compare March 31, 2025 09:38

benoittgt changed the title ~~[WIP] Migrate pkg to slog~~ Migrate pkg to slog Mar 31, 2025

benoittgt mentioned this pull request Apr 7, 2025

Migrate ready checks to slog #30698

Closed

3 tasks

benoittgt added 17 commits April 7, 2025 15:35

Migrate kube package to slog

f4631bf

As for helm v4. We want to migrate logs to slog. Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Remove unreachable error

dfaf249

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Do no mask warning alerts

8d30464

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Try to make log more common and more easily grepable

d6d7cff

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Provide an adapter to easily pass a slog.Default()

b642bca

``` helmClient.Log = NewSlogAdapter(slog.Default()) ``` Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Extra comment + Default logger fallback

227d270

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Some interesting rephrasing by Terry Howe

eb2dfe7

See: helm#30698 (comment) Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Properly use DefaultLogger

394ba2d

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Deal with linting errors

3a22df9

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Fix call to kube log

ede7386

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Demonstrate the impact of having Logger defined in kube package

fae2345

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Migrate to a dedicated internal package for slog adapter + migrate more

83cdffe

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Migrate more code to log adapter

b42767b

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Fix linting issue and temporary removing logging in test acion

15de13f

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Fix missing logger to SQL in test

3db7ebc

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Explain why we ignore the timestamp

947658a

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Migrate to pure slog without a custom wrapper

b238072

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

benoittgt force-pushed the migrate-kube-pkg-to-slog branch from 0edab58 to b238072 Compare April 7, 2025 14:46

Properly discard by default logs

83a5a14

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

benoittgt added 2 commits April 7, 2025 16:50

Enforce error style with others

b6adbbb

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Do not remove the functionality to print log in test

baa597c

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

benoittgt commented Apr 7, 2025

View reviewed changes

pkg/cmd/root.go Outdated Show resolved Hide resolved

benoittgt force-pushed the migrate-kube-pkg-to-slog branch from 6e684c8 to bf3d011 Compare April 7, 2025 15:46

benoittgt added 4 commits April 7, 2025 17:48

Properly reproduce the nopLogger as before

5580f61

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Trick slog to return the full error

6ce9673

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Go the slog way

3e4e783

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Linting

710770e

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

benoittgt force-pushed the migrate-kube-pkg-to-slog branch from bf3d011 to 710770e Compare April 7, 2025 15:49

benoittgt added 2 commits April 7, 2025 18:01

Prevent redefining verbose flags

5c74603

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Leverage slog.Any for errors

0c85456

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

benoittgt force-pushed the migrate-kube-pkg-to-slog branch from 87846ec to 0c85456 Compare April 7, 2025 16:26

benoittgt requested a review from robertsirc April 7, 2025 16:45

robertsirc requested changes Apr 9, 2025

View reviewed changes

benoittgt added 2 commits April 10, 2025 15:35

Call slog directly instead of using a wrapper

cbaac76

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Use the logger with proper handling of dynamic debug on 2 locations

e7eedae

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

benoittgt commented Apr 10, 2025

View reviewed changes

No longer log call location using flag, it will need to be done in slog

7f02e89

Some ideas here: https://www.reddit.com/r/golang/comments/15nwnkl/achieve_lshortfile_with_slog/ Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

Fix nil pointer dereference in ready test

c05bcbd

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

benoittgt force-pushed the migrate-kube-pkg-to-slog branch from 7bcf82a to c05bcbd Compare April 10, 2025 14:40

Prefer using slog.Any when displaying errors

68440d7

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>

robertsirc self-requested a review April 11, 2025 19:35

robertsirc approved these changes Apr 11, 2025

View reviewed changes

mattfarina approved these changes Apr 11, 2025

View reviewed changes

mattfarina merged commit 7a1eb77 into helm:main Apr 11, 2025
5 checks passed

benoittgt deleted the migrate-kube-pkg-to-slog branch April 12, 2025 07:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate pkg to slog #30708

Migrate pkg to slog #30708

benoittgt commented Mar 25, 2025 •

edited

Loading

benoittgt Apr 7, 2025

robertsirc Apr 7, 2025

benoittgt Apr 10, 2025

robertsirc left a comment

robertsirc Apr 7, 2025

benoittgt commented Apr 10, 2025 •

edited

Loading

benoittgt Apr 10, 2025

benoittgt commented Apr 10, 2025 •

edited

Loading

robertsirc left a comment

robertsirc Apr 10, 2025

mattfarina Apr 11, 2025

Migrate pkg to slog #30708

Migrate pkg to slog #30708

Conversation

benoittgt commented Mar 25, 2025 • edited Loading

benoittgt Apr 7, 2025

Choose a reason for hiding this comment

robertsirc Apr 7, 2025

Choose a reason for hiding this comment

benoittgt Apr 10, 2025

Choose a reason for hiding this comment

robertsirc left a comment

Choose a reason for hiding this comment

robertsirc Apr 7, 2025

Choose a reason for hiding this comment

benoittgt commented Apr 10, 2025 • edited Loading

benoittgt Apr 10, 2025

Choose a reason for hiding this comment

benoittgt commented Apr 10, 2025 • edited Loading

robertsirc left a comment

Choose a reason for hiding this comment

robertsirc Apr 10, 2025

Choose a reason for hiding this comment

mattfarina Apr 11, 2025

Choose a reason for hiding this comment

benoittgt commented Mar 25, 2025 •

edited

Loading

benoittgt commented Apr 10, 2025 •

edited

Loading

benoittgt commented Apr 10, 2025 •

edited

Loading