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

feat: added slog adapter #1173

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Conversation

robbert229
Copy link
Contributor

Unsure if this is even wanted however, this PR introduces an adapter (originally from here) to more easily use slog as the underlying logger implementation instead of zap.

This covers: #1170

@CLAassistant
Copy link

CLAassistant commented Mar 10, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@JacobOaks JacobOaks left a comment

Choose a reason for hiding this comment

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

Left a couple comments on the details, but thank you for this PR! I think this addition makes sense. Since slog is in the standard library, I think this can be generally used by anybody who wants to. cc: @tchung1118, if you think this is good too we can merge and include in the next Fx release.

fxevent/slog.go Outdated Show resolved Hide resolved
fxevent/slog.go Outdated Show resolved Hide resolved
fxevent/slog.go Outdated Show resolved Hide resolved
fxevent/slog.go Outdated Show resolved Hide resolved
fxevent/slog.go Outdated Show resolved Hide resolved
@robbert229 robbert229 force-pushed the feature/fx-slog branch 2 times, most recently from fdf7380 to 48825e6 Compare March 12, 2024 07:45
Copy link
Contributor

@tchung1118 tchung1118 left a comment

Choose a reason for hiding this comment

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

Left a few more comments. I think it looks good otherwise. Thank you again for your contribution.

fxevent/slog.go Outdated
"strings"
)

var _ Logger = &SlogLogger{}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
var _ Logger = &SlogLogger{}
var _ Logger = (*SlogLogger)(nil)

fxevent/slog.go Outdated

var _ Logger = &SlogLogger{}

// SlogLogger is a shim that allows uber/fx to use slog for logging.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
// SlogLogger is a shim that allows uber/fx to use slog for logging.
// SlogLogger is an Fx event logger that logs events using a slog logger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

fxevent/slog.go Outdated
}

func slogStrings(key string, str []string) slog.Attr {
var attrs []any
Copy link
Contributor

Choose a reason for hiding this comment

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

can we pre-allocate here?

Suggested change
var attrs []any
attrs := make([]slog.Attr, 0, len(str))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup! resolved

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 99.01478% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 98.73%. Comparing base (bb632be) to head (9475fc1).

Files Patch % Lines
fxevent/slog.go 99.01% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1173      +/-   ##
==========================================
+ Coverage   98.71%   98.73%   +0.02%     
==========================================
  Files          30       31       +1     
  Lines        2648     2851     +203     
==========================================
+ Hits         2614     2815     +201     
- Misses         27       29       +2     
  Partials        7        7              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@JacobOaks JacobOaks left a comment

Choose a reason for hiding this comment

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

LGTM!

@JacobOaks JacobOaks merged commit 340d5ac into uber-go:master Mar 13, 2024
12 checks passed
@robbert229 robbert229 deleted the feature/fx-slog branch March 18, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants