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

🔥 Feature: Add AllLogger to Config #3153

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
28 changes: 28 additions & 0 deletions docs/middleware/logger.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

View workflow job for this annotation

GitHub Actions / markdownlint

Hard tabs

docs/middleware/logger.md:94:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md010.md
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

View workflow job for this annotation

GitHub Actions / markdownlint

Hard tabs

docs/middleware/logger.md:99:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md010.md
case log.LevelDebug:

Check failure on line 100 in docs/middleware/logger.md

View workflow job for this annotation

GitHub Actions / markdownlint

Hard tabs

docs/middleware/logger.md:100:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md010.md
l.logger.Debug(string(p))

Check failure on line 101 in docs/middleware/logger.md

View workflow job for this annotation

GitHub Actions / markdownlint

Hard tabs

docs/middleware/logger.md:101:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md010.md
case log.LevelError:

Check failure on line 102 in docs/middleware/logger.md

View workflow job for this annotation

GitHub Actions / markdownlint

Hard tabs

docs/middleware/logger.md:102:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md010.md
l.logger.Error(string(p))

Check failure on line 103 in docs/middleware/logger.md

View workflow job for this annotation

GitHub Actions / markdownlint

Hard tabs

docs/middleware/logger.md:103:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md010.md
}
return len(p), nil

Check failure on line 105 in docs/middleware/logger.md

View workflow job for this annotation

GitHub Actions / markdownlint

Hard tabs

docs/middleware/logger.md:105:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md010.md
}

func LoggerToWriter(customLogger fiberlog.AllLogger, level fiberlog.Level) io.Writer {
return &dumbLogger{

Check failure on line 109 in docs/middleware/logger.md

View workflow job for this annotation

GitHub Actions / markdownlint

Hard tabs

docs/middleware/logger.md:109:1 MD010/no-hard-tabs Hard tabs [Column: 1] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md010.md
logger: customLogger,
level: level,
}
}
Comment on lines +93 to +113
Copy link
Member

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

Copy link
Member

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 ?

func LoggerToWriter(customLogger fiberlog.AllLogger, level fiberlog.Level) io.Writer {
	return &struct {
		Write func(p []byte) (n int, err error)
	}{
		Write: func(p []byte) (n int, err error) {
			switch level {
			case fiberlog.LevelDebug:
				customLogger.Debug(string(p))
			case fiberlog.LevelError:
				customLogger.Error(string(p))
			}
			return len(p), nil
		},
	}
}

then we really don´t need extra code in the codebase

Copy link
Author

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.

Copy link
Member

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

Copy link
Author

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 the io.Writer interface.

Copy link
Member

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 ?

Copy link
Author

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 😅

Copy link
Member

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


app.Use(logger.New(logger.Config{
Output: LoggerToWriter(fiberlog.DefaultLogger(), fiberlog.LevelError),
}))
```

:::tip
Expand All @@ -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

View workflow job for this annotation

GitHub Actions / markdownlint

Table column count

docs/middleware/logger.md:138:409 MD056/table-column-count Table column count [Expected: 4; Actual: 5; Too many cells, extra data will be missing] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md056.md
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Approve LoggerFunc addition. Fix table formatting.

The new LoggerFunc configuration option is a great addition that aligns with the PR objectives. It provides flexibility for users to integrate various logging libraries.

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

138-138: Expected: 4; Actual: 5; Too many cells, extra data will be missing
Table column count

(MD056, table-column-count)

| 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) | - |
Expand Down
4 changes: 4 additions & 0 deletions log/default.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,10 @@
l.stdlog.SetOutput(writer)
}

func (l *defaultLogger) SetFlags(flag int) {
l.stdlog.SetFlags(flag)

Check warning on line 214 in log/default.go

View check run for this annotation

Codecov / codecov/patch

log/default.go#L213-L214

Added lines #L213 - L214 were not covered by tests
}
Comment on lines +213 to +215
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Add SetFlags to the AllLogger interface for consistency.

The SetFlags method in defaultLogger is not part of the AllLogger interface. To maintain interface consistency across the package, please add the SetFlags method to the AllLogger interface.

🔗 Analysis chain

Verify interface consistency across the package.

The SetFlags method is a valuable addition to the defaultLogger. However, it's important to ensure consistency across the package.

Please run the following script to check if the SetFlags method is added to the AllLogger interface:

If the SetFlags method is not part of the AllLogger interface, consider adding it to maintain consistency and allow users to set flags on any logger implementation.

🏁 Scripts executed

The 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
Expand Down
1 change: 1 addition & 0 deletions log/log.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ type CommonLogger interface {
type ControlLogger interface {
SetLevel(level Level)
SetOutput(w io.Writer)
SetFlags(flags int)
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Documentation Missing for SetFlags Method

The SetFlags(flags int) method in the ControlLogger interface lacks documentation. Providing comments or godoc annotations for this method is essential to clarify its purpose and usage, ensuring that other developers can effectively utilize and maintain this functionality.

  • Add documentation comments for the SetFlags method in log/log.go.
🔗 Analysis chain

New method added to ControlLogger interface

The addition of SetFlags(flags int) to the ControlLogger interface enhances the configurability of loggers implementing this interface. This change allows for more fine-grained control over the logging output format.

However, there are a few points to consider:

  1. The PR objectives mention introducing a new field for the middleware/logger, but this change only adds a method to an existing interface. It's not clear if this fully addresses the stated objectives.
  2. The flags parameter type (int) suggests this might be using predefined constants for different flag options. It would be helpful to have documentation or constants defined for the possible flag values.

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 SetFlags Documentation

🏁 Scripts executed

The 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.
Expand Down
66 changes: 66 additions & 0 deletions middleware/logger/logger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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

View workflow job for this annotation

GitHub Actions / lint

named return "n" with type "int" found (nonamedreturns)

Check failure on line 189 in middleware/logger/logger_test.go

View workflow job for this annotation

GitHub Actions / lint

named return "err" with type "error" found (nonamedreturns)
l.logger.Error(string(p))
return len(p), nil
}

Comment on lines +185 to +193
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add nil check for 'l.logger' in 'dumbLogger.Write'

In the Write method of dumbLogger, if l.logger is nil, calling l.logger.Error(string(p)) will result in a nil pointer dereference, causing a runtime panic. To ensure robustness, add a check to verify that l.logger is not nil before attempting to log.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
type dumbLogger struct {
logger fiberlog.AllLogger
}
func (l *dumbLogger) Write(p []byte) (n int, err error) {
l.logger.Error(string(p))
return len(p), nil
}
type dumbLogger struct {
logger fiberlog.AllLogger
}
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
}

func LoggerToWriter(customLogger fiberlog.AllLogger) io.Writer {
return &dumbLogger{logger: customLogger}
}
Comment on lines +194 to +196
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate 'customLogger' parameter in 'LoggerToWriter'

The LoggerToWriter function does not check if the customLogger parameter is nil. Passing a nil customLogger will result in a dumbLogger with a nil logger, which could cause a panic when Write is called. Add a check to ensure customLogger is not nil before creating the dumbLogger.

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 customLogger is nil.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func LoggerToWriter(customLogger fiberlog.AllLogger) io.Writer {
return &dumbLogger{logger: customLogger}
}
func LoggerToWriter(customLogger fiberlog.AllLogger) io.Writer {
if customLogger == nil {
return nil
}
return &dumbLogger{logger: customLogger}
}


// 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) {
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor benchmark tests to eliminate duplicate code

The benchmark cases DefaultFormatWithFiberLog in both Benchmark_Logger (lines 772-784) and Benchmark_Logger_Parallel (lines 928-940) have identical setup code. This duplication can be avoided by refactoring the common code into a helper function, enhancing maintainability and reducing potential for errors.

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{
Expand Down Expand Up @@ -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{
Expand Down
Loading