-
-
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
🔥 Feature: Add AllLogger to Config #3153
base: main
Are you sure you want to change the base?
Changes from all commits
0c36bd1
4337f02
ea8d8ea
f1a4208
2221a32
e796231
9f406ae
3dd20b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,6 +88,33 @@ | |
app.Use(logger.New(logger.Config{ | ||
DisableColors: true, | ||
})) | ||
|
||
// Use Custom Logger with Fiber Logger Interface | ||
type dumbLogger struct { | ||
logger fiberlog.AllLogger | ||
Check failure on line 94 in docs/middleware/logger.md GitHub Actions / markdownlintHard tabs
|
||
level log.Level | ||
} | ||
|
||
func (l *dumbLogger) Write(p []byte) (n int, err error) { | ||
switch l.level { | ||
Check failure on line 99 in docs/middleware/logger.md GitHub Actions / markdownlintHard tabs
|
||
case log.LevelDebug: | ||
Check failure on line 100 in docs/middleware/logger.md GitHub Actions / markdownlintHard tabs
|
||
l.logger.Debug(string(p)) | ||
Check failure on line 101 in docs/middleware/logger.md GitHub Actions / markdownlintHard tabs
|
||
case log.LevelError: | ||
Check failure on line 102 in docs/middleware/logger.md GitHub Actions / markdownlintHard tabs
|
||
l.logger.Error(string(p)) | ||
Check failure on line 103 in docs/middleware/logger.md GitHub Actions / markdownlintHard tabs
|
||
} | ||
return len(p), nil | ||
Check failure on line 105 in docs/middleware/logger.md GitHub Actions / markdownlintHard tabs
|
||
} | ||
|
||
func LoggerToWriter(customLogger fiberlog.AllLogger, level fiberlog.Level) io.Writer { | ||
return &dumbLogger{ | ||
Check failure on line 109 in docs/middleware/logger.md GitHub Actions / markdownlintHard tabs
|
||
logger: customLogger, | ||
level: level, | ||
} | ||
} | ||
|
||
app.Use(logger.New(logger.Config{ | ||
Output: LoggerToWriter(fiberlog.DefaultLogger(), fiberlog.LevelError), | ||
})) | ||
``` | ||
|
||
:::tip | ||
|
@@ -108,6 +135,7 @@ | |
| TimeZone | `string` | TimeZone can be specified, such as "UTC" and "America/New_York" and "Asia/Chongqing", etc | `"Local"` | | ||
| TimeInterval | `time.Duration` | TimeInterval is the delay before the timestamp is updated. | `500 * time.Millisecond` | | ||
| Output | `io.Writer` | Output is a writer where logs are written. | `os.Stdout` | | ||
| LoggerFunc | `func(c fiber.Ctx, data *Data, cfg Config) error` | You can use custom loggers with Fiber by using this field. This field is really useful if you're using Zerolog, Zap, Logrus, apex/log etc. If you don't define anything for this field, it'll use the default logger of Fiber. | `see default_logger.go defaultLoggerInstance` | | | ||
Check failure on line 138 in docs/middleware/logger.md GitHub Actions / markdownlintTable column count
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Approve The new The table formatting needs to be adjusted to maintain consistency. Please split the description into multiple lines or consider using a list format for better readability. For example: | LoggerFunc | `func(c fiber.Ctx, data *Data, cfg Config) error` | Custom logger function. Useful for integrating logging libraries like Zerolog, Zap, Logrus, etc. If not defined, Fiber's default logger is used. | `see default_logger.go defaultLoggerInstance` | 🧰 Tools🪛 Markdownlint
|
||
| DisableColors | `bool` | DisableColors defines if the logs output should be colorized. | `false` | | ||
| enableColors | `bool` | Internal field for enabling colors in the log output. (This is not a user-configurable field) | - | | ||
| enableLatency | `bool` | Internal field for enabling latency measurement in logs. (This is not a user-configurable field) | - | | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -210,6 +210,10 @@ | |
l.stdlog.SetOutput(writer) | ||
} | ||
|
||
func (l *defaultLogger) SetFlags(flag int) { | ||
l.stdlog.SetFlags(flag) | ||
} | ||
Comment on lines
+213
to
+215
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Add The 🔗 Analysis chainVerify interface consistency across the package. The Please run the following script to check if the If the 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check if SetFlags method is added to the AllLogger interface
# Test: Search for the AllLogger interface definition
echo "Searching for AllLogger interface definition:"
rg --type go -A 10 "type AllLogger interface" ./log
# Test: Check if SetFlags is part of the AllLogger interface
echo "Checking if SetFlags is part of the AllLogger interface:"
rg --type go "SetFlags\(.*\)" ./log
Length of output: 905 |
||
|
||
// DefaultLogger returns the default logger. | ||
func DefaultLogger() AllLogger { | ||
return logger | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,7 @@ type CommonLogger interface { | |
type ControlLogger interface { | ||
SetLevel(level Level) | ||
SetOutput(w io.Writer) | ||
SetFlags(flags int) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Documentation Missing for The
🔗 Analysis chainNew method added to ControlLogger interface The addition of However, there are a few points to consider:
To ensure this change is consistent with the rest of the codebase and meets the PR objectives, let's run the following checks: Further Verification Needed for 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the implementation and usage of SetFlags method
# Check if there are any existing usages of SetFlags in the codebase
echo "Checking for existing SetFlags usages:"
rg --type go "SetFlags\(" -C 3
# Check if there are any flag constants defined in the codebase
echo "Checking for flag constants:"
rg --type go "const.*Flag" -C 3
# Check if there's any documentation for the SetFlags method
echo "Checking for SetFlags documentation:"
rg --type go "SetFlags.*godoc" -C 3
Length of output: 1844 Script: #!/bin/bash
# Description: Check for comments above the SetFlags method in log/log.go
# Extract lines around the SetFlags method to find any associated comments
rg --type go -A 5 -B 2 "SetFlags\(flags int\)" log/log.go
Length of output: 294 |
||
} | ||
|
||
// AllLogger is the combination of Logger, FormatLogger, CtxLogger and ControlLogger. | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -15,6 +15,7 @@ | |||||||||||||||||||||||||||||||||||||||
"time" | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
"github.com/gofiber/fiber/v3" | ||||||||||||||||||||||||||||||||||||||||
fiberlog "github.com/gofiber/fiber/v3/log" | ||||||||||||||||||||||||||||||||||||||||
"github.com/gofiber/fiber/v3/middleware/requestid" | ||||||||||||||||||||||||||||||||||||||||
"github.com/stretchr/testify/require" | ||||||||||||||||||||||||||||||||||||||||
"github.com/valyala/bytebufferpool" | ||||||||||||||||||||||||||||||||||||||||
|
@@ -181,6 +182,45 @@ | |||||||||||||||||||||||||||||||||||||||
require.Equal(t, fiber.StatusNotFound, resp.StatusCode) | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
type dumbLogger struct { | ||||||||||||||||||||||||||||||||||||||||
logger fiberlog.AllLogger | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
func (l *dumbLogger) Write(p []byte) (n int, err error) { | ||||||||||||||||||||||||||||||||||||||||
Check failure on line 189 in middleware/logger/logger_test.go GitHub Actions / lint
|
||||||||||||||||||||||||||||||||||||||||
l.logger.Error(string(p)) | ||||||||||||||||||||||||||||||||||||||||
return len(p), nil | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
Comment on lines
+185
to
+193
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add nil check for 'l.logger' in 'dumbLogger.Write' In the Apply this diff to add the nil check: func (l *dumbLogger) Write(p []byte) (n int, err error) {
+ if l.logger == nil {
+ return 0, errors.New("logger is nil")
+ }
l.logger.Error(string(p))
return len(p), nil
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
func LoggerToWriter(customLogger fiberlog.AllLogger) io.Writer { | ||||||||||||||||||||||||||||||||||||||||
return &dumbLogger{logger: customLogger} | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
Comment on lines
+194
to
+196
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validate 'customLogger' parameter in 'LoggerToWriter' The Apply this diff to validate the parameter: func LoggerToWriter(customLogger fiberlog.AllLogger) io.Writer {
+ if customLogger == nil {
+ return nil
+ }
return &dumbLogger{logger: customLogger}
} Alternatively, you might want to return an error or use a default logger if 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
// go test -run Test_Logger_Fiber_Logger | ||||||||||||||||||||||||||||||||||||||||
func Test_Logger_Fiber_Logger(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||
t.Parallel() | ||||||||||||||||||||||||||||||||||||||||
app := fiber.New() | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
buf := bytebufferpool.Get() | ||||||||||||||||||||||||||||||||||||||||
defer bytebufferpool.Put(buf) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
logger := fiberlog.DefaultLogger() | ||||||||||||||||||||||||||||||||||||||||
logger.SetFlags(0) | ||||||||||||||||||||||||||||||||||||||||
logger.SetOutput(buf) | ||||||||||||||||||||||||||||||||||||||||
app.Use(New(Config{ | ||||||||||||||||||||||||||||||||||||||||
Format: "${error}", | ||||||||||||||||||||||||||||||||||||||||
Output: LoggerToWriter(logger), | ||||||||||||||||||||||||||||||||||||||||
})) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
app.Get("/", func(_ fiber.Ctx) error { | ||||||||||||||||||||||||||||||||||||||||
return errors.New("some random error") | ||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil)) | ||||||||||||||||||||||||||||||||||||||||
require.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||
require.Equal(t, fiber.StatusInternalServerError, resp.StatusCode) | ||||||||||||||||||||||||||||||||||||||||
require.Equal(t, "[Error] some random error\n", buf.String()) | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
type fakeErrorOutput int | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
func (o *fakeErrorOutput) Write([]byte) (int, error) { | ||||||||||||||||||||||||||||||||||||||||
|
@@ -729,6 +769,19 @@ | |||||||||||||||||||||||||||||||||||||||
benchmarkSetup(bb, app, "/") | ||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
b.Run("DefaultFormatWithFiberLog", func(bb *testing.B) { | ||||||||||||||||||||||||||||||||||||||||
app := fiber.New() | ||||||||||||||||||||||||||||||||||||||||
logger := fiberlog.DefaultLogger() | ||||||||||||||||||||||||||||||||||||||||
logger.SetOutput(io.Discard) | ||||||||||||||||||||||||||||||||||||||||
app.Use(New(Config{ | ||||||||||||||||||||||||||||||||||||||||
Output: LoggerToWriter(logger), | ||||||||||||||||||||||||||||||||||||||||
})) | ||||||||||||||||||||||||||||||||||||||||
app.Get("/", func(c fiber.Ctx) error { | ||||||||||||||||||||||||||||||||||||||||
return c.SendString("Hello, World!") | ||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||
benchmarkSetup(bb, app, "/") | ||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
Comment on lines
+772
to
+784
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Refactor benchmark tests to eliminate duplicate code The benchmark cases You can extract the common setup code into a helper function: func setupBenchmarkWithFiberLog() (*fiber.App, func(fiber.Ctx) error) {
app := fiber.New()
logger := fiberlog.DefaultLogger()
logger.SetOutput(io.Discard)
app.Use(New(Config{
Output: LoggerToWriter(logger),
}))
handler := func(c fiber.Ctx) error {
return c.SendString("Hello, World!")
}
return app, handler
} Then use this helper function in your benchmarks: b.Run("DefaultFormatWithFiberLog", func(bb *testing.B) {
app, handler := setupBenchmarkWithFiberLog()
app.Get("/", handler)
benchmarkSetup(bb, app, "/")
}) This approach promotes code reuse and simplifies future modifications. Also applies to: 928-940 |
||||||||||||||||||||||||||||||||||||||||
b.Run("WithTagParameter", func(bb *testing.B) { | ||||||||||||||||||||||||||||||||||||||||
app := fiber.New() | ||||||||||||||||||||||||||||||||||||||||
app.Use(New(Config{ | ||||||||||||||||||||||||||||||||||||||||
|
@@ -872,6 +925,19 @@ | |||||||||||||||||||||||||||||||||||||||
benchmarkSetupParallel(bb, app, "/") | ||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
b.Run("DefaultFormatWithFiberLog", func(bb *testing.B) { | ||||||||||||||||||||||||||||||||||||||||
app := fiber.New() | ||||||||||||||||||||||||||||||||||||||||
logger := fiberlog.DefaultLogger() | ||||||||||||||||||||||||||||||||||||||||
logger.SetOutput(io.Discard) | ||||||||||||||||||||||||||||||||||||||||
app.Use(New(Config{ | ||||||||||||||||||||||||||||||||||||||||
Output: LoggerToWriter(logger), | ||||||||||||||||||||||||||||||||||||||||
})) | ||||||||||||||||||||||||||||||||||||||||
app.Get("/", func(c fiber.Ctx) error { | ||||||||||||||||||||||||||||||||||||||||
return c.SendString("Hello, World!") | ||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||
benchmarkSetupParallel(bb, app, "/") | ||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
b.Run("DefaultFormatDisableColors", func(bb *testing.B) { | ||||||||||||||||||||||||||||||||||||||||
app := fiber.New() | ||||||||||||||||||||||||||||||||||||||||
app.Use(New(Config{ | ||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haochunchang can we put that or a better generic wrapper in the code so that not every dev has to create this struct? or would that be too undynamic?
what do you think? @gaby @efectn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should recommend something like this ?
then we really don´t need extra code in the codebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can provide the
LoggerToWriter
in the example to show how to use fiberlog interface in the middleware.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok @haochunchang pls adjust it a little bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve tried to condensed into a single function wrapper that return io.Writer but it seems it still need to declare the
Write
method in order to satisfy theio.Writer
interface.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haochunchang yeah, but in my example its there
so can you update the PR with this code snippet ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean adding it into the example?
Cause I failed to adopt this into the unit tests 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haochunchang
yeah add it in the readme in a new sub section