Skip to content

Commit

Permalink
🐛 bug: Use Content-Length for bytesReceived and bytesSent tags in Log…
Browse files Browse the repository at this point in the history
…ger Middleware (#3066)

* logger: Use Content-Length header for BytesReceived and BytesSent tags

* Use strconv.AppendInt instead of fasthttp.AppendUint
  • Loading branch information
gaby authored Jul 23, 2024
1 parent ef07360 commit a57b3c0
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 11 deletions.
3 changes: 1 addition & 2 deletions middleware/logger/default_logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"github.com/mattn/go-colorable"
"github.com/mattn/go-isatty"
"github.com/valyala/bytebufferpool"
"github.com/valyala/fasthttp"
)

// default logger for fiber
Expand Down Expand Up @@ -151,7 +150,7 @@ func beforeHandlerFunc(cfg Config) {

func appendInt(output Buffer, v int) (int, error) {
old := output.Len()
output.Set(fasthttp.AppendUint(output.Bytes(), v))
output.Set(strconv.AppendInt(output.Bytes(), int64(v), 10))
return output.Len() - old, nil
}

Expand Down
47 changes: 43 additions & 4 deletions middleware/logger/logger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -407,14 +407,40 @@ func Test_Response_Body(t *testing.T) {
require.Equal(t, expectedGetResponse, buf.String())

buf.Reset() // Reset buffer to test POST

_, err = app.Test(httptest.NewRequest(fiber.MethodPost, "/test", nil))
require.NoError(t, err)

expectedPostResponse := "Post in test"
require.NoError(t, err)
require.Equal(t, expectedPostResponse, buf.String())
}

// go test -run Test_Request_Body
func Test_Request_Body(t *testing.T) {
t.Parallel()
buf := bytebufferpool.Get()
defer bytebufferpool.Put(buf)
app := fiber.New()

app.Use(New(Config{
Format: "${bytesReceived} ${bytesSent} ${status}",
Output: buf,
}))

app.Post("/", func(c fiber.Ctx) error {
c.Response().Header.SetContentLength(5)
return c.SendString("World")
})

// Create a POST request with a body
body := []byte("Hello")
req := httptest.NewRequest(fiber.MethodPost, "/", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/octet-stream")

_, err := app.Test(req)
require.NoError(t, err)
require.Equal(t, "5 5 200", buf.String())
}

// go test -run Test_Logger_AppendUint
func Test_Logger_AppendUint(t *testing.T) {
t.Parallel()
Expand All @@ -432,10 +458,21 @@ func Test_Logger_AppendUint(t *testing.T) {
return c.SendString("hello")
})

app.Get("/content", func(c fiber.Ctx) error {
c.Response().Header.SetContentLength(5)
return c.SendString("hello")
})

resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil))
require.NoError(t, err)
require.Equal(t, fiber.StatusOK, resp.StatusCode)
require.Equal(t, "0 5 200", buf.String())
require.Equal(t, "-2 0 200", buf.String())

buf.Reset()
resp, err = app.Test(httptest.NewRequest(fiber.MethodGet, "/content", nil))
require.NoError(t, err)
require.Equal(t, fiber.StatusOK, resp.StatusCode)
require.Equal(t, "-2 5 200", buf.String())
}

// go test -run Test_Logger_Data_Race -race
Expand Down Expand Up @@ -618,7 +655,9 @@ func Test_Logger_ByteSent_Streaming(t *testing.T) {
resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil))
require.NoError(t, err)
require.Equal(t, fiber.StatusOK, resp.StatusCode)
require.Equal(t, "0 0 200", buf.String())

// -2 means identity, -1 means chunked, 200 status
require.Equal(t, "-2 -1 200", buf.String())
}

type fakeOutput int
Expand Down
7 changes: 2 additions & 5 deletions middleware/logger/tags.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,10 @@ func createTagMap(cfg *Config) map[string]LogFunc {
return output.Write(c.Body())
},
TagBytesReceived: func(output Buffer, c fiber.Ctx, _ *Data, _ string) (int, error) {
return appendInt(output, len(c.Request().Body()))
return appendInt(output, c.Request().Header.ContentLength())
},
TagBytesSent: func(output Buffer, c fiber.Ctx, _ *Data, _ string) (int, error) {
if c.Response().Header.ContentLength() < 0 {
return appendInt(output, 0)
}
return appendInt(output, len(c.Response().Body()))
return appendInt(output, c.Response().Header.ContentLength())
},
TagRoute: func(output Buffer, c fiber.Ctx, _ *Data, _ string) (int, error) {
return output.WriteString(c.Route().Path)
Expand Down

1 comment on commit a57b3c0

@github-actions
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite Current: a57b3c0 Previous: 87bb93e Ratio
Benchmark_Utils_GetOffer/1_parameter 215.4 ns/op 0 B/op 0 allocs/op 136.1 ns/op 0 B/op 0 allocs/op 1.58
Benchmark_Utils_GetOffer/1_parameter - ns/op 215.4 ns/op 136.1 ns/op 1.58
Benchmark_Utils_getGroupPath - allocs/op 4 allocs/op 2 allocs/op 2
Benchmark_Middleware_BasicAuth - B/op 80 B/op 48 B/op 1.67
Benchmark_Middleware_BasicAuth - allocs/op 5 allocs/op 3 allocs/op 1.67
Benchmark_Middleware_BasicAuth_Upper - B/op 80 B/op 48 B/op 1.67
Benchmark_Middleware_BasicAuth_Upper - allocs/op 5 allocs/op 3 allocs/op 1.67
Benchmark_CORS_NewHandler - B/op 16 B/op 0 B/op +∞
Benchmark_CORS_NewHandler - allocs/op 1 allocs/op 0 allocs/op +∞
Benchmark_CORS_NewHandlerSingleOrigin - B/op 16 B/op 0 B/op +∞
Benchmark_CORS_NewHandlerSingleOrigin - allocs/op 1 allocs/op 0 allocs/op +∞
Benchmark_CORS_NewHandlerPreflight 1156 ns/op 104 B/op 5 allocs/op 759.2 ns/op 0 B/op 0 allocs/op 1.52
Benchmark_CORS_NewHandlerPreflight - ns/op 1156 ns/op 759.2 ns/op 1.52
Benchmark_CORS_NewHandlerPreflight - B/op 104 B/op 0 B/op +∞
Benchmark_CORS_NewHandlerPreflight - allocs/op 5 allocs/op 0 allocs/op +∞
Benchmark_CORS_NewHandlerPreflightSingleOrigin 1154 ns/op 104 B/op 5 allocs/op 757.5 ns/op 0 B/op 0 allocs/op 1.52
Benchmark_CORS_NewHandlerPreflightSingleOrigin - ns/op 1154 ns/op 757.5 ns/op 1.52
Benchmark_CORS_NewHandlerPreflightSingleOrigin - B/op 104 B/op 0 B/op +∞
Benchmark_CORS_NewHandlerPreflightSingleOrigin - allocs/op 5 allocs/op 0 allocs/op +∞
Benchmark_CORS_NewHandlerPreflightWildcard 1062 ns/op 104 B/op 5 allocs/op 691 ns/op 0 B/op 0 allocs/op 1.54
Benchmark_CORS_NewHandlerPreflightWildcard - ns/op 1062 ns/op 691 ns/op 1.54
Benchmark_CORS_NewHandlerPreflightWildcard - B/op 104 B/op 0 B/op +∞
Benchmark_CORS_NewHandlerPreflightWildcard - allocs/op 5 allocs/op 0 allocs/op +∞
Benchmark_Middleware_CSRF_Check - allocs/op 11 allocs/op 7 allocs/op 1.57
Benchmark_Middleware_CSRF_GenerateToken - B/op 513 B/op 326 B/op 1.57
Benchmark_Middleware_CSRF_GenerateToken - allocs/op 10 allocs/op 6 allocs/op 1.67

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.