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

Convert lib/utils to slog #50340

Merged
merged 1 commit into from
Dec 19, 2024
Merged

Convert lib/utils to slog #50340

merged 1 commit into from
Dec 19, 2024

Conversation

rosstimothy
Copy link
Contributor

This only converts the places in the utils package that were logging with logrus, but it does not fully remove the logrus package as a dependency. The package is home to the logrus text formatter which needs to exist until slog is the only logging mechanism in use.

@rosstimothy rosstimothy added the no-changelog Indicates that a PR does not require a changelog entry label Dec 17, 2024
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-50340.d3pp5qlev8mo18.amplifyapp.com

@rosstimothy rosstimothy marked this pull request as ready for review December 17, 2024 18:12
Copy link
Contributor

@kopiczko kopiczko left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but those context.Backgroud() calls outside test files are a bit concerning. It may be less obvious to replace them with a proper context even if the calling function eventually takes it as an argument. But possibly that's something already discussed?

lib/utils/addr.go Show resolved Hide resolved
@@ -156,7 +155,7 @@ func (l *LoadBalancer) AddBackend(b NetAddr) {
l.Lock()
defer l.Unlock()
l.backends = append(l.backends, b)
l.Debugf("Backends %v.", l.backends)
l.logger.DebugContext(l.ctx, "Backends updated", "backends", l.backends)
Copy link
Contributor

Choose a reason for hiding this comment

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

"Backends added"?

lib/utils/loadbalancer.go Show resolved Hide resolved
lib/utils/registry/registry_windows.go Show resolved Hide resolved
@rosstimothy
Copy link
Contributor Author

Per RFD 154:

Use the Context variants of the slog.Logger API. Providing the context.Context to all log messages allows them to be enriched with additional attributes. For example, this allows log messages to automatically have a request or trace id added, which makes correlating messages that were emitted as a result of one particular action or event. If no context is readily available for the logger to consume, prefer to use a context.TODO if it is possible to get a context to the call site in the future but refactoring is to do be done in a separate unit of work. If there is likely to never be a context provided to the function producing the log output, then prefer to use a context.Background to indicate as such

@rosstimothy rosstimothy added this pull request to the merge queue Dec 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 19, 2024
@rosstimothy rosstimothy added this pull request to the merge queue Dec 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 19, 2024
This only converts the places in the utils package that were
logging with logrus, but it does not fully remove the logrus
package as a dependency. The package is home to the logrus text
formatter which needs to exist until slog is the only logging
mechanism in use.
@rosstimothy rosstimothy added this pull request to the merge queue Dec 19, 2024
Merged via the queue into master with commit 05be924 Dec 19, 2024
42 checks passed
@rosstimothy rosstimothy deleted the tross/utils_slog branch December 19, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants