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

chore!: adopt log/slog, drop go-kit/log #183

Merged
merged 2 commits into from
Oct 21, 2024
Merged

chore!: adopt log/slog, drop go-kit/log #183

merged 2 commits into from
Oct 21, 2024

Conversation

SuperQ
Copy link
Contributor

@SuperQ SuperQ commented Oct 20, 2024

This PR includes:

  • linter updates to enable sloglint linter
  • Go dep updates for prometheus/{client_golang,common,exporter-toolkit} libs
  • refactorings to adopt log/slog in favor of go-kit/log

The bulk of this PR was automated by the following script which is being used to aid in converting the various exporters/projects to use slog:

https://gist.github.com/tjhop/49f96fb7ebbe55b12deee0b0312d8434

This PR includes:
- linter updates to enable `sloglint` linter
- Go dep updates for prometheus/{client_golang,common,exporter-toolkit} libs
- refactorings to adopt log/slog in favor of go-kit/log

The bulk of this PR was automated by the following script which is being used to aid in converting the various exporters/projects to use slog:

https://gist.github.com/tjhop/49f96fb7ebbe55b12deee0b0312d8434

Signed-off-by: SuperQ <[email protected]>
@SuperQ
Copy link
Contributor Author

SuperQ commented Oct 20, 2024

👓 @tjhop

@SuperQ SuperQ requested a review from dswarbrick October 20, 2024 08:30
Copy link
Member

@dswarbrick dswarbrick left a comment

Choose a reason for hiding this comment

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

Overall mostly ok, just a few nits here and there. Some of the logging calls that pass an error as a KV should perhaps actually be logger.Error(...) instead of Warn or Info. I think some of the Info logging calls are also actually more like Debug.

I'm not sure how necessary it is to log an error immediately before returning a wrapped error, e.g.

		c.logger.Error("Error polling:", "err", err)
		return fmt.Errorf("error polling: %w", err)

In cases like that, it probably makes more sense that the caller chooses whether to log the returned error.

Lastly, there seems to be some inconsistency between logging errors with the key err and error. It would be good to standardize on one or the other.

cmd/client/main.go Outdated Show resolved Hide resolved
cmd/client/main.go Show resolved Hide resolved
cmd/client/main.go Show resolved Hide resolved
cmd/client/main.go Show resolved Hide resolved
cmd/client/main.go Show resolved Hide resolved
cmd/client/main.go Show resolved Hide resolved
cmd/proxy/main.go Show resolved Hide resolved
cmd/client/main.go Show resolved Hide resolved
cmd/client/main.go Show resolved Hide resolved
cmd/client/main.go Show resolved Hide resolved
@SuperQ
Copy link
Contributor Author

SuperQ commented Oct 21, 2024

Decisions about what to log when, as well as changing a lot of the logging strings, is scope creep. Additional improvements in logging are something that will be done in separate PRs.

Signed-off-by: Ben Kochie <[email protected]>

Co-authored-by: Daniel Swarbrick <[email protected]>
Signed-off-by: Ben Kochie <[email protected]>
@SuperQ SuperQ merged commit 62f3cfb into master Oct 21, 2024
5 checks passed
@SuperQ SuperQ deleted the superq/slog branch October 21, 2024 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants