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

Refactor loggers with Slog #1642

Merged
merged 14 commits into from
Oct 23, 2023
Merged

Refactor loggers with Slog #1642

merged 14 commits into from
Oct 23, 2023

Conversation

jan-law
Copy link
Contributor

@jan-law jan-law commented Aug 10, 2023

Related #1170
Depends on dbason/opentelemetry-collector-contrib#1
Depends on kralicky/totem#5

Replaces zap with slog. According to docs, this should eliminate any allocations for logging strings and numbers. Benchmark test results for slog handlers: https://gist.github.com/jan-law/bfb4db82ba05dbcb551e0de1ed36f8c1

How to rebase branches ahead of main

In general, logging calls are identical with a few exceptions:

  • Format string logs eg Infof, Errorf -> replace with Info(fmt.Sprintf(...)), Error(fmt.Sprintf(...))
  • logger.New().Named("some-name") -> replace with logger.New().WithGroup("some-name")
  • When you need to create a logr.Logr, you can use logger.NewLogr(options...)
  • To create a new logger, see pkg/logger/logger.go for available options

This script will help you refactor existing zap logs with the slog equivalents.

Disclaimer: gofmt can only refactor logging calls that have a fixed number of arguments and is also case sensitive. If you created a log message with a different number of key/val pairs than what already exists in the codebase, the script may not format that log message.

Notes

  • log messages of the form logger.With(args...).Info("blah") are still valid, but may cost extra memory allocations. You can replace them with logger.Info("blah", args...)
  • Slog doesn't enforce an even number of args when you're logging loosely typed key/value pairs. It will log the last odd-numbered arg as !BADKEY=<value>. Docs recommend using https://pkg.go.dev/cmd/vet to check your code

@jan-law jan-law added enhancement New feature or request tech debt labels Aug 10, 2023
@jan-law jan-law force-pushed the slog branch 8 times, most recently from 6991582 to 6ad9c63 Compare August 14, 2023 16:33
@jan-law
Copy link
Contributor Author

jan-law commented Aug 14, 2023

Note: branches rancher/opni:slog and jan-law:slog are the same. Lmk if you'd like me to make a PR with the upstream branch instead of my fork

@jan-law
Copy link
Contributor Author

jan-law commented Oct 17, 2023

Note: I still need to update go.mod with the updated commit hash once dbason/opentelemetry-collector-contrib#1 is merged

@kralicky
Copy link
Member

Some of the log messages that look like this:

a.logger.With(
	zap.Error(err),
	zap.String("group", ruleGroup.Name),
).Error("failed to marshal rule group")

seem to have been rewritten as

a.logger.Error("failed to marshal rule group", logger.Err(err),
	"group", ruleGroup.Name)

Is there a way we can have the format match? i.e.

a.logger.With(
	logger.Err(err),
	"group", ruleGroup.Name,
).Error("failed to marshal rule group")

@jan-law
Copy link
Contributor Author

jan-law commented Oct 19, 2023

Is there a way we can have the format match?

Yes, your last example is valid slog syntax. The reason why I changed the format is because the original format causes an extra allocation per line, and you mentioned you wanted to reduce allocations by moving to slog.

I can revert the format of the logger.With calls so that they match if you'd prefer the same format?

@kralicky
Copy link
Member

Ah, yeah I think for most cases the extra memory allocation isn't a big deal, but in performance oriented code that is good to keep in mind. we should keep the old format in places where it's originally written like that.

kralicky
kralicky previously approved these changes Oct 23, 2023
Copy link
Member

@kralicky kralicky left a comment

Choose a reason for hiding this comment

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

LGTM

@jan-law
Copy link
Contributor Author

jan-law commented Oct 23, 2023

Rebased onto main, no other changes

@jan-law
Copy link
Contributor Author

jan-law commented Oct 23, 2023

@kralicky This PR is using my fork of Dan's copy of opentelemetry-collector-contrib. Dan mentioned his copy is outdated and needed time to think about how to update the rancher sandbox fork here.

Should we merge this PR and then update the fork later? How do you want to handle this?

@kralicky
Copy link
Member

Yeah that sounds fine to me, we can merge this PR first.

@jan-law jan-law merged commit e5b62ae into rancher:main Oct 23, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request tech debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants