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: use writer directly in stdLogger. #3121

Merged
merged 5 commits into from
Mar 16, 2024
Merged

Conversation

kvii
Copy link
Contributor

@kvii kvii commented Dec 15, 2023

Description (what this PR does / why we need it):

log.stdLogger use log.Logger internally, and disable all options such as prefix or log flags. log.stdLogger use l.log like a io.Writer. So I removed l.log, use io.Writer directly.

Code "stdLogger use log.Logger with no options":

kratos/log/std.go

Lines 20 to 21 in 08300d8

return &stdLogger{
log: log.New(w, "", 0),

Code "stdLogger not use any format features of log.Logger":

kratos/log/std.go

Lines 39 to 43 in 08300d8

buf.WriteString(level.String())
for i := 0; i < len(keyvals); i += 2 {
_, _ = fmt.Fprintf(buf, " %s=%v", keyvals[i], keyvals[i+1])
}
_ = l.log.Output(4, buf.String()) //nolint:gomnd

Which issue(s) this PR fixes (resolves / be part of):

Other special notes for the reviewers:

I did not add test for stdLogger.Close, because there is no way to call Close outside.

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Dec 15, 2023
@kvii
Copy link
Contributor Author

kvii commented Dec 15, 2023

A benchmark for it.

  • new code
goos: darwin
goarch: arm64
pkg: github.com/go-kratos/kratos/v2/log
BenchmarkNew-10    	13301796	        75.79 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/go-kratos/kratos/v2/log	1.344s
  • old code
goos: darwin
goarch: arm64
pkg: github.com/go-kratos/kratos/v2/log
BenchmarkOld-10    	 6942644	       160.6 ns/op	      32 B/op	       1 allocs/op
PASS
ok  	github.com/go-kratos/kratos/v2/log	1.525s

@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.64%. Comparing base (05b23fb) to head (116b43d).

❗ Current head 116b43d differs from pull request most recent head 6c6a554. Consider uploading reports for the commit 6c6a554 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3121      +/-   ##
==========================================
+ Coverage   81.62%   81.64%   +0.01%     
==========================================
  Files          91       91              
  Lines        4163     4167       +4     
==========================================
+ Hits         3398     3402       +4     
  Misses        587      587              
  Partials      178      178              

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

@dosubot dosubot bot added the LGTM label Dec 18, 2023
@kvii kvii requested a review from shenqidebaozi February 2, 2024 02:43
@kvii
Copy link
Contributor Author

kvii commented Feb 2, 2024

@hawkingrei And could you help me to review this code too? This code removed unnecessary "log.Output" so it doubles the performance of stdLogger.

@shenqidebaozi shenqidebaozi merged commit 3110168 into go-kratos:main Mar 16, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants