-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
🚀 Add Logger interface and fiberlog #2499
Conversation
e0eaa3b
to
044bf4d
Compare
@Skyenought Can you check the issues in the CI |
i don't know what's happening, How did ci go wrong when I just added extra code? |
@Skyenought Let me check... It's related to go 1.20.5 so it's not this PR. |
@gaby thx |
@Skyenought The setup-go action hasnt updated their manifest to support 1.20.5, thus why it fails https://github.com/actions/go-versions/blob/main/versions-manifest.json |
|
@Skyenought Can you merge main into your branch. |
Unfortunately there still seems to be a problem with the ci settings 🤔 If |
@Skyenought Try again, the Pr was reverted. That part should pass now. Just merge main into your branch |
I've updated the branch, but it doesn't seem to work. @gaby |
Those are not the latest changes, pull again. |
You are missing the last commit from here: https://github.com/gofiber/fiber/commits/master |
sorry! @gaby |
@Skyenought Have you tried running the tests locally with |
cd fiberlog/
❯ go test ./... -v -race
=== RUN Test_DefaultLogger
--- PASS: Test_DefaultLogger (0.00s)
=== RUN Test_DefaultFormatLogger
--- PASS: Test_DefaultFormatLogger (0.00s)
=== RUN Test_CtxLogger
--- PASS: Test_CtxLogger (0.00s)
=== RUN Test_SetLevel
--- PASS: Test_SetLevel (0.00s)
=== RUN Test_DefaultSystemLogger
--- PASS: Test_DefaultSystemLogger (0.00s)
=== RUN Test_SetLogger
--- PASS: Test_SetLogger (0.00s)
PASS
ok github.com/gofiber/fiber/v2/fiberlog (cached)
The code that appears as |
Correct but app.go now uses fiberlog and that affects other parts of the code. The code in that folder is fine test wise. Now you have to test/fix the rest of the library since several parts import app.go |
GET |
I've triggered too many errchecks (I've solved golangci-lint, it's too strict), and now I'm thinking why the benchmark didn't pass😥 |
The benchmark thing is a known issue, since the runners can be unpredictible sometimes. Not checking errors is a real issue though |
ok,I see that those errors can be ignored, otherwise I wouldn't have ruled them out |
Co-authored-by: Tomás Warynyca <[email protected]>
After this PR merge, I will make the zap and zerolog changes available as soon as possible, and add examples to recipes |
What do you mean with "zap and zero log changes"? Btw can you post the benchmark numbers here, for the history |
I mean it will be in fiberzap and fiberzerolog to add support for log packages |
Ok, so a wrapper for the interface Don't forget the benchmark numbers |
It looks like the performance of // BenchmarkDefaultLoggerInfo-2 88129 26447 ns/op
func BenchmarkDefaultLoggerInfo(b *testing.B) {
for n := 0; n < b.N; n++ {
DefaultLogger().Info("test")
}
}
// BenchmarkLogPrintln-2 62989 26672 ns/op
func BenchmarkLogPrintln(b *testing.B) {
l := log.New(os.Stderr, "", log.LstdFlags|log.Lshortfile|log.Lmicroseconds)
for n := 0; n < b.N; n++ {
l.Println("[Info] test")
}
}
// BenchmarkDefaultLoggerInfof-2 56754 26381 ns/op
func BenchmarkDefaultLoggerInfof(b *testing.B) {
for n := 0; n < b.N; n++ {
DefaultLogger().Infof("test: %s", "log")
}
}
// BenchmarkLogPrintf-2 49744 27731 ns/op
func BenchmarkLogPrintf(b *testing.B) {
l := log.New(os.Stderr, "", log.LstdFlags|log.Lshortfile|log.Lmicroseconds)
for n := 0; n < b.N; n++ {
l.Printf("[Info] test %s", "log")
}
} |
Can you add the info about the allocations |
goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v2/log
cpu: Intel(R) Core(TM) i5-1035G1 CPU @ 1.00GHz
BenchmarkDefaultLoggerInfo-2 625828 1716 ns/op 561 B/op 4 allocs/op
BenchmarkDefaultLoggerInfof-2 668976 1929 ns/op 608 B/op 3 allocs/op
BenchmarkLogPrintln-2 944955 1330 ns/op 564 B/op 3 allocs/op
BenchmarkLogPrintf-2 846170 1303 ns/op 603 B/op 3 allocs/op
|
@Skyenought Can it be compared to this fiberzerolog? https://github.com/gofiber/contrib/tree/main/fiberzerolog |
The // 1581740 1119 ns/op 2 B/op 0 allocs/op
func BenchmarkNativeZap(b *testing.B) {
log, _ := zap.NewProduction()
for i := 0; i < b.N; i++ {
log.Info("Hello, World!")
}
}
// 1244044 1406 ns/op 18 B/op 1 allocs/op
func BenchmarkFiberZap(b *testing.B) {
log, _ := zap.NewProduction()
logger := NewLogger(LoggerConfig{
SetLogger: log,
})
fiberlog.SetLogger(logger)
for i := 0; i < b.N; i++ {
fiberlog.Info("Hello, World!")
}
} |
@Skyenought What is missing to support Zerolog? |
i am busy now,i think adapter for zerolog need to wait 1 week |
No worries, thanks! 💪 |
Description
Adding an interface for extending the log library
Pre-set log levels in advance
Provides a way to print logs, with default standard output on the console
Example
Fixes # (issue)
#2433
Type of change
Please delete options that are not relevant.
Checklist:
Commit formatting:
Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: https://gitmoji.carloscuesta.me/