8000 refactor after code review · coder/coder@81e1527 · GitHub
[go: up one dir, main page]
Skip to content

Commit 81e1527

Browse files
ibetitsmikeibetitsmike
committed
refactor after code review
1 parent 8f550c5 commit 81e1527

File tree

2 files changed

+29
-41
lines changed

2 files changed

+29
-41
lines changed

‎coderd/httpmw/logger.go

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -54,11 +54,7 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler {
5454
)
5555
}
5656

57-
// We already capture most of this information in the span (minus
58-
// the response body which we don't want to capture anyways).
59-
tracing.RunWithoutSpan(r.Context(), func(ctx context.Context) {
60-
logContext.WriteLog(ctx, sw.Status)
61-
})
57+
logContext.WriteLog(r.Context(), sw.Status)
6258
})
6359
}
6460
}
@@ -68,27 +64,29 @@ type RequestLogger interface {
6864
WriteLog(ctx context.Context, status int)
6965
}
7066

71-
type RequestContextLogger struct {
67+
type SlogRequestLogger struct {
7268
log slog.Logger
7369
written bool
7470
message string
7571
start time.Time
7672
}
7773

74+
var _ RequestLogger = &SlogRequestLogger{}
75+
7876
func NewRequestLogger(log slog.Logger, message string, start time.Time) RequestLogger {
79-
return &RequestContextLogger{
77+
return &SlogRequestLogger{
8078
log: log,
8179
written: false,
8280
message: message,
8381
start: start,
8482
}
8583
}
8684

87-
func (c *RequestContextLogger) WithFields(fields ...slog.Field) {
85+
func (c *SlogRequestLogger) WithFields(fields ...slog.Field) {
8886
c.log = c.log.With(fields...)
8987
}
9088

91-
func (c *RequestContextLogger) WriteLog(ctx context.Context, status int) {
89+
func (c *SlogRequestLogger) WriteLog(ctx context.Context, status int) {
9290
if c.written {
9391
return
9492
}
@@ -100,14 +98,18 @@ func (c *RequestContextLogger) WriteLog(ctx context.Context, status int) {
10098
slog.F("status_code", status),
10199
slog.F("latency_ms", float64(end.Sub(c.start)/time.Millisecond)),
102100
)
103-
// We should not log at level ERROR for 5xx status codes because 5xx
104-
// includes proxy errors etc. It also causes slogtest to fail
105-
// instantly without an error message by default.
106-
if status >= http.StatusInternalServerError {
107-
logger.Warn(ctx, c.message)
108-
} else {
109-
logger.Debug(ctx, c.message)
110-
}
101+
// We already capture most of this information in the span (minus
102+
// the response body which we don't want to capture anyways).
103+
tracing.RunWithoutSpan(ctx, func(ctx context.Context) {
104+
// We should not log at level ERROR for 5xx status codes because 5xx
105+
// includes proxy errors etc. It also causes slogtest to fail
106+
// instantly without an error message by default.
107+
if status >= http.StatusInternalServerError {
108+
logger.Warn(ctx, c.message)
109+
} else {
110+
logger.Debug(ctx, c.message)
111+
}
112+
})
111113
}
112114

113115
type logContextKey struct{}

‎coderd/httpmw/logger_internal_test.go

Lines changed: 10 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import (
88
"testing"
99
"time"
1010

11+
"github.com/stretchr/testify/require"
12+
1113
"cdr.dev/slog"
1214
"github.com/coder/coder/v2/coderd/tracing"
1315 E7EE
"github.com/coder/coder/v2/testutil"
@@ -31,24 +33,16 @@ func TestRequestLogger_WriteLog(t *testing.T) {
3133
// Write log for 200 status
3234
logCtx.WriteLog(ctx, http.StatusOK)
3335

34-
if len(sink.entries) != 1 {
35-
t.Fatalf("expected 1 log entry, got %d", len(sink.entries))
36-
}
36+
require.Len(t, sink.entries, 1, "log was written twice")
3737

38-
if sink.entries[0].Message != "GET" {
39-
t.Errorf("expected log message to be 'GET', got '%s'", sink.entries[0].Message)
40-
}
38+
require.Equal(t, sink.entries[0].Message, "GET", "log message should be GET")
4139

42-
if sink.entries[0].Fields[0].Value != "custom_value" {
43-
t.Errorf("expected a custom_field with value custom_value, got '%s'", sink.entries[0].Fields[0].Value)
44-
}
40+
require.Equal(t, sink.entries[0].Fields[0].Value, "custom_value", "custom_field should be custom_value")
4541

4642
// Attempt to write again (should be skipped).
4743
logCtx.WriteLog(ctx, http.StatusInternalServerError)
4844

49-
if len(sink.entries) != 1 {
50-
t.Fatalf("expected 1 log entry after second write, got %d", len(sink.entries))
51-
}
45+
require.Len(t, sink.entries, 1, "log was written twice")
5246
}
5347

5448
func TestLoggerMiddleware_SingleRequest(t *testing.T) {
@@ -79,13 +73,9 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) {
7973
// Serve the request
8074
wrappedHandler.ServeHTTP(sw, req)
8175

82-
if len(sink.entries) != 1 {
83-
t.Fatalf("expected 1 log entry, got %d", len(sink.entries))
84-
}
76+
require.Len(t, sink.entries, 1, "log was written twice")
8577

86-
if sink.entries[0].Message != "GET" {
87-
t.Errorf("expected log message to be 'GET', got '%s'", sink.entries[0].Message)
88-
}
78+
require.Equal(t, sink.entries[0].Message, "GET", "log message should be GET")
8979
}
9080

9181
func TestLoggerMiddleware_WebSocket(t *testing.T) {
@@ -134,13 +124,9 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) {
134124
}
135125
defer conn.Close(websocket.StatusNormalClosure, "")
136126
wg.Wait()
137-
if len(sink.entries) != 1 {
138-
t.Fatalf("expected 1 log entry, got %d", len(sink.entries))
139-
}
127+
require.Len(t, sink.entries, 1, "log was written twice")
140128

141-
if sink.entries[0].Message != "GET" {
142-
t.Errorf("expected log message to be 'GET', got '%s'", sink.entries[0].Message)
143-
}
129+
require.Equal(t, sink.entries[0].Message, "GET", "log message should be GET")
144130
}
145131

146132
type fakeSink struct {

0 commit comments

Comments
 (0)
0