From 0e397edf6197b7650c3f25a303a61b5d916924ce Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Wed, 2 Apr 2025 11:32:07 +0000 Subject: [PATCH 01/36] initial implementation of RequestLoggerContext and example usage in ProvisionerJobs logs --- coderd/httpmw/logger.go | 43 ++++++++++++++++++++++++++++++++++++++- coderd/provisionerjobs.go | 4 ++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/coderd/httpmw/logger.go b/coderd/httpmw/logger.go index 79e95cf859d8e..44f6b9aa55526 100644 --- a/coderd/httpmw/logger.go +++ b/coderd/httpmw/logger.go @@ -35,7 +35,14 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler { slog.F("start", start), ) - next.ServeHTTP(sw, r) + logContext := &RequestLoggerContext{} + defer func() { + logContext.WriteLog(r.Context(), "", sw.Status) + }() + + ctx := context.WithValue(r.Context(), logContextKey{}, logContext) + + next.ServeHTTP(sw, r.WithContext(ctx)) end := time.Now() @@ -74,3 +81,37 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler { }) } } + +type RequestLoggerContext struct { + Fields map[string]any + + log *slog.Logger + written bool +} + +func (c *RequestLoggerContext) WriteLog(ctx context.Context, msg string, status int) { + if c.written { + return + } + c.written = true + // append extra fields to the logger + for k, v := range c.Fields { + c.log.With(slog.F(k, v)) + } + + if status >= http.StatusInternalServerError { + c.log.Error(ctx, msg) + } else { + c.log.Debug(ctx, msg) + } +} + +type logContextKey struct{} + +func FromContext(ctx context.Context) *RequestLoggerContext { + val := ctx.Value(logContextKey{}) + if logCtx, ok := val.(*RequestLoggerContext); ok { + return logCtx + } + return nil +} diff --git a/coderd/provisionerjobs.go b/coderd/provisionerjobs.go index 47963798f4d32..fd88d1158312c 100644 --- a/coderd/provisionerjobs.go +++ b/coderd/provisionerjobs.go @@ -554,6 +554,10 @@ func (f *logFollower) follow() { return } + // write the initial logs to the connection + httpmw.FromContext(f.ctx).WriteLog( + f.ctx, "ProvisionerJobs log follower WS connection established", http.StatusAccepted) + // no need to wait if the job is done if f.complete { return From c384706d86b73c21c478213a62dbb788446c2ec7 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Wed, 2 Apr 2025 11:40:02 +0000 Subject: [PATCH 02/36] fix empty RequestLoggerContext --- coderd/httpmw/logger.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/coderd/httpmw/logger.go b/coderd/httpmw/logger.go index 44f6b9aa55526..f00bcdb8407b9 100644 --- a/coderd/httpmw/logger.go +++ b/coderd/httpmw/logger.go @@ -35,7 +35,11 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler { slog.F("start", start), ) - logContext := &RequestLoggerContext{} + logContext := &RequestLoggerContext{ + Fields: map[string]any{}, + log: &slog.Logger{}, + written: false, + } defer func() { logContext.WriteLog(r.Context(), "", sw.Status) }() From 686f4db43b5ff704a45664cdde2c0fdabdd13f80 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Wed, 2 Apr 2025 11:40:52 +0000 Subject: [PATCH 03/36] referenced httplog while creating RequestLoggerContext --- coderd/httpmw/logger.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/httpmw/logger.go b/coderd/httpmw/logger.go index f00bcdb8407b9..1c8845831e657 100644 --- a/coderd/httpmw/logger.go +++ b/coderd/httpmw/logger.go @@ -37,7 +37,7 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler { logContext := &RequestLoggerContext{ Fields: map[string]any{}, - log: &slog.Logger{}, + log: &httplog, written: false, } defer func() { From 7edb8c3787297d8c0e1e5000bacde8256ff56f1d Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Wed, 2 Apr 2025 12:00:52 +0000 Subject: [PATCH 04/36] refactored fields extension on the logger --- coderd/httpmw/logger.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/coderd/httpmw/logger.go b/coderd/httpmw/logger.go index 1c8845831e657..a5a9bd8c70248 100644 --- a/coderd/httpmw/logger.go +++ b/coderd/httpmw/logger.go @@ -36,7 +36,6 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler { ) logContext := &RequestLoggerContext{ - Fields: map[string]any{}, log: &httplog, written: false, } @@ -87,21 +86,20 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler { } type RequestLoggerContext struct { - Fields map[string]any - log *slog.Logger written bool } +func (c *RequestLoggerContext) WithFields(fields ...slog.Field) { + newLogger := c.log.With(fields...) + c.log = &newLogger +} + func (c *RequestLoggerContext) WriteLog(ctx context.Context, msg string, status int) { if c.written { return } c.written = true - // append extra fields to the logger - for k, v := range c.Fields { - c.log.With(slog.F(k, v)) - } if status >= http.StatusInternalServerError { c.log.Error(ctx, msg) From 5feb4e435257fdcc725496b953ba4f4d0ad0d60f Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Wed, 2 Apr 2025 12:17:33 +0000 Subject: [PATCH 05/36] fixed log formatting --- coderd/httpmw/logger.go | 10 ++++++---- coderd/provisionerjobs.go | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/coderd/httpmw/logger.go b/coderd/httpmw/logger.go index a5a9bd8c70248..a38824b349bdf 100644 --- a/coderd/httpmw/logger.go +++ b/coderd/httpmw/logger.go @@ -38,9 +38,10 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler { logContext := &RequestLoggerContext{ log: &httplog, written: false, + message: r.Method, } defer func() { - logContext.WriteLog(r.Context(), "", sw.Status) + logContext.WriteLog(r.Context(), sw.Status) }() ctx := context.WithValue(r.Context(), logContextKey{}, logContext) @@ -88,6 +89,7 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler { type RequestLoggerContext struct { log *slog.Logger written bool + message string } func (c *RequestLoggerContext) WithFields(fields ...slog.Field) { @@ -95,16 +97,16 @@ func (c *RequestLoggerContext) WithFields(fields ...slog.Field) { c.log = &newLogger } -func (c *RequestLoggerContext) WriteLog(ctx context.Context, msg string, status int) { +func (c *RequestLoggerContext) WriteLog(ctx context.Context, status int) { if c.written { return } c.written = true if status >= http.StatusInternalServerError { - c.log.Error(ctx, msg) + c.log.Error(ctx, c.message, "status_code", status) } else { - c.log.Debug(ctx, msg) + c.log.Debug(ctx, c.message, "status_code", status) } } diff --git a/coderd/provisionerjobs.go b/coderd/provisionerjobs.go index fd88d1158312c..256071c6d2a66 100644 --- a/coderd/provisionerjobs.go +++ b/coderd/provisionerjobs.go @@ -556,7 +556,7 @@ func (f *logFollower) follow() { // write the initial logs to the connection httpmw.FromContext(f.ctx).WriteLog( - f.ctx, "ProvisionerJobs log follower WS connection established", http.StatusAccepted) + f.ctx, http.StatusAccepted) // no need to wait if the job is done if f.complete { From 7500ef5cd27be66417dfd87825ca846f7b68484f Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Wed, 2 Apr 2025 12:18:01 +0000 Subject: [PATCH 06/36] typo comment fix --- coderd/provisionerjobs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/provisionerjobs.go b/coderd/provisionerjobs.go index 256071c6d2a66..619188edd1f5d 100644 --- a/coderd/provisionerjobs.go +++ b/coderd/provisionerjobs.go @@ -554,7 +554,7 @@ func (f *logFollower) follow() { return } - // write the initial logs to the connection + // write the initial connection to the logs httpmw.FromContext(f.ctx).WriteLog( f.ctx, http.StatusAccepted) From 66e2c3ec6d42aeb6fcdd9f78d71dc0ca4d3553e7 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Wed, 2 Apr 2025 16:29:14 +0000 Subject: [PATCH 07/36] added logging to long lived connection opening functions --- coderd/inboxnotifications.go | 3 +++ coderd/provisionerjobs.go | 5 ++--- coderd/workspaceagents.go | 9 +++++++++ 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/coderd/inboxnotifications.go b/coderd/inboxnotifications.go index 6da047241d790..582e478f53ec7 100644 --- a/coderd/inboxnotifications.go +++ b/coderd/inboxnotifications.go @@ -219,6 +219,9 @@ func (api *API) watchInboxNotifications(rw http.ResponseWriter, r *http.Request) encoder := wsjson.NewEncoder[codersdk.GetInboxNotificationResponse](conn, websocket.MessageText) defer encoder.Close(websocket.StatusNormalClosure) + // log the initial connection + httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) + for { select { case <-ctx.Done(): diff --git a/coderd/provisionerjobs.go b/coderd/provisionerjobs.go index 619188edd1f5d..6a34e48c30a0c 100644 --- a/coderd/provisionerjobs.go +++ b/coderd/provisionerjobs.go @@ -554,9 +554,8 @@ func (f *logFollower) follow() { return } - // write the initial connection to the logs - httpmw.FromContext(f.ctx).WriteLog( - f.ctx, http.StatusAccepted) + // log the initial connection + httpmw.FromContext(f.ctx).WriteLog(f.ctx, http.StatusAccepted) // no need to wait if the job is done if f.complete { diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 1573ef70eb443..4b82beeb1bf1d 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -555,6 +555,9 @@ func (api *API) workspaceAgentLogs(rw http.ResponseWriter, r *http.Request) { t := time.NewTicker(recheckInterval) defer t.Stop() + // log the initial connection + httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) + go func() { defer func() { logger.Debug(ctx, "end log streaming loop") @@ -928,6 +931,9 @@ func (api *API) derpMapUpdates(rw http.ResponseWriter, r *http.Request) { encoder := wsjson.NewEncoder[*tailcfg.DERPMap](ws, websocket.MessageBinary) defer encoder.Close(websocket.StatusGoingAway) + // log the initial connection + httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) + go func(ctx context.Context) { // TODO(mafredri): Is this too frequent? Use separate ping disconnect timeout? t := time.NewTicker(api.AgentConnectionUpdateFrequency) @@ -1315,6 +1321,9 @@ func (api *API) watchWorkspaceAgentMetadata( sendTicker := time.NewTicker(sendInterval) defer sendTicker.Stop() + // log the initial connection + httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) + // Send initial metadata. sendMetadata() From 4e5f36ec1ee504b4f125e9fc9a9051c5e452aa88 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Wed, 2 Apr 2025 16:54:53 +0000 Subject: [PATCH 08/36] added logging to other places --- enterprise/coderd/provisionerdaemons.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 5b0f0ca197743..3ce3340382574 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -376,6 +376,10 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) logger.Debug(ctx, "drpc server error", slog.Error(err)) }, }) + + // log the initial connection + httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) + err = server.Serve(ctx, session) srvCancel() logger.Info(ctx, "provisioner daemon disconnected", slog.Error(err)) From b7822b3427c8cb923d154e1d2c202b3f2d9e01ad Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 3 Apr 2025 08:50:49 +0000 Subject: [PATCH 09/36] updated comments --- coderd/inboxnotifications.go | 2 +- coderd/provisionerjobs.go | 2 +- coderd/workspaceagents.go | 6 +++--- enterprise/coderd/provisionerdaemons.go | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/coderd/inboxnotifications.go b/coderd/inboxnotifications.go index 582e478f53ec7..c532f8068706d 100644 --- a/coderd/inboxnotifications.go +++ b/coderd/inboxnotifications.go @@ -219,7 +219,7 @@ func (api *API) watchInboxNotifications(rw http.ResponseWriter, r *http.Request) encoder := wsjson.NewEncoder[codersdk.GetInboxNotificationResponse](conn, websocket.MessageText) defer encoder.Close(websocket.StatusNormalClosure) - // log the initial connection + // Log the request immediately instead of after it completes. httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) for { diff --git a/coderd/provisionerjobs.go b/coderd/provisionerjobs.go index 6a34e48c30a0c..22c201fc9e609 100644 --- a/coderd/provisionerjobs.go +++ b/coderd/provisionerjobs.go @@ -554,7 +554,7 @@ func (f *logFollower) follow() { return } - // log the initial connection + // Log the request immediately instead of after it completes. httpmw.FromContext(f.ctx).WriteLog(f.ctx, http.StatusAccepted) // no need to wait if the job is done diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 4b82beeb1bf1d..93d77f13f2a39 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -555,7 +555,7 @@ func (api *API) workspaceAgentLogs(rw http.ResponseWriter, r *http.Request) { t := time.NewTicker(recheckInterval) defer t.Stop() - // log the initial connection + // Log the request immediately instead of after it completes. httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) go func() { @@ -931,7 +931,7 @@ func (api *API) derpMapUpdates(rw http.ResponseWriter, r *http.Request) { encoder := wsjson.NewEncoder[*tailcfg.DERPMap](ws, websocket.MessageBinary) defer encoder.Close(websocket.StatusGoingAway) - // log the initial connection + // Log the request immediately instead of after it completes. httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) go func(ctx context.Context) { @@ -1321,7 +1321,7 @@ func (api *API) watchWorkspaceAgentMetadata( sendTicker := time.NewTicker(sendInterval) defer sendTicker.Stop() - // log the initial connection + // Log the request immediately instead of after it completes. httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) // Send initial metadata. diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 3ce3340382574..0412302e825e6 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -377,7 +377,7 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) }, }) - // log the initial connection + // Log the request immediately instead of after it completes. httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) err = server.Serve(ctx, session) From a7724308f11c41e4e739233b04a31bd40fa1a212 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 3 Apr 2025 08:52:03 +0000 Subject: [PATCH 10/36] renamed context fetcher --- coderd/httpmw/logger.go | 2 +- coderd/inboxnotifications.go | 2 +- coderd/provisionerjobs.go | 2 +- coderd/workspaceagents.go | 6 +++--- enterprise/coderd/provisionerdaemons.go | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/coderd/httpmw/logger.go b/coderd/httpmw/logger.go index a38824b349bdf..aaad04b0bbe54 100644 --- a/coderd/httpmw/logger.go +++ b/coderd/httpmw/logger.go @@ -112,7 +112,7 @@ func (c *RequestLoggerContext) WriteLog(ctx context.Context, status int) { type logContextKey struct{} -func FromContext(ctx context.Context) *RequestLoggerContext { +func RequestLoggerFromContext(ctx context.Context) *RequestLoggerContext { val := ctx.Value(logContextKey{}) if logCtx, ok := val.(*RequestLoggerContext); ok { return logCtx diff --git a/coderd/inboxnotifications.go b/coderd/inboxnotifications.go index c532f8068706d..ea20c60de3cce 100644 --- a/coderd/inboxnotifications.go +++ b/coderd/inboxnotifications.go @@ -220,7 +220,7 @@ func (api *API) watchInboxNotifications(rw http.ResponseWriter, r *http.Request) defer encoder.Close(websocket.StatusNormalClosure) // Log the request immediately instead of after it completes. - httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) + httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) for { select { diff --git a/coderd/provisionerjobs.go b/coderd/provisionerjobs.go index 22c201fc9e609..335643390796f 100644 --- a/coderd/provisionerjobs.go +++ b/coderd/provisionerjobs.go @@ -555,7 +555,7 @@ func (f *logFollower) follow() { } // Log the request immediately instead of after it completes. - httpmw.FromContext(f.ctx).WriteLog(f.ctx, http.StatusAccepted) + httpmw.RequestLoggerFromContext(f.ctx).WriteLog(f.ctx, http.StatusAccepted) // no need to wait if the job is done if f.complete { diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 93d77f13f2a39..1744c0c6749ca 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -556,7 +556,7 @@ func (api *API) workspaceAgentLogs(rw http.ResponseWriter, r *http.Request) { defer t.Stop() // Log the request immediately instead of after it completes. - httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) + httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) go func() { defer func() { @@ -932,7 +932,7 @@ func (api *API) derpMapUpdates(rw http.ResponseWriter, r *http.Request) { defer encoder.Close(websocket.StatusGoingAway) // Log the request immediately instead of after it completes. - httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) + httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) go func(ctx context.Context) { // TODO(mafredri): Is this too frequent? Use separate ping disconnect timeout? @@ -1322,7 +1322,7 @@ func (api *API) watchWorkspaceAgentMetadata( defer sendTicker.Stop() // Log the request immediately instead of after it completes. - httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) + httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) // Send initial metadata. sendMetadata() diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 0412302e825e6..15e3c3901ade3 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -378,7 +378,7 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) }) // Log the request immediately instead of after it completes. - httpmw.FromContext(ctx).WriteLog(ctx, http.StatusAccepted) + httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) err = server.Serve(ctx, session) srvCancel() From 2a1dbe318dfd1aaf4abf0270024b07265d346852 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 3 Apr 2025 11:29:27 +0000 Subject: [PATCH 11/36] refactored logging in the logger middleware and added tests for logger --- coderd/httpmw/logger.go | 58 +++++++++++++++++------------------- coderd/httpmw/logger_test.go | 50 +++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 31 deletions(-) create mode 100644 coderd/httpmw/logger_test.go diff --git a/coderd/httpmw/logger.go b/coderd/httpmw/logger.go index aaad04b0bbe54..a797bf1d344f3 100644 --- a/coderd/httpmw/logger.go +++ b/coderd/httpmw/logger.go @@ -35,66 +35,53 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler { slog.F("start", start), ) - logContext := &RequestLoggerContext{ - log: &httplog, - written: false, - message: r.Method, - } - defer func() { - logContext.WriteLog(r.Context(), sw.Status) - }() + logContext := NewRequestLoggerContext(httplog, r.Method, start) ctx := context.WithValue(r.Context(), logContextKey{}, logContext) next.ServeHTTP(sw, r.WithContext(ctx)) - end := time.Now() - // Don't log successful health check requests. if r.URL.Path == "/api/v2" && sw.Status == http.StatusOK { return } - httplog = httplog.With( - slog.F("took", end.Sub(start)), - slog.F("status_code", sw.Status), - slog.F("latency_ms", float64(end.Sub(start)/time.Millisecond)), - ) - - // For status codes 400 and higher we + // For status codes 500 and higher we // want to log the response body. if sw.Status >= http.StatusInternalServerError { - httplog = httplog.With( + logContext.WithFields( slog.F("response_body", string(sw.ResponseBody())), ) } - // We should not log at level ERROR for 5xx status codes because 5xx - // includes proxy errors etc. It also causes slogtest to fail - // instantly without an error message by default. - logLevelFn := httplog.Debug - if sw.Status >= http.StatusInternalServerError { - logLevelFn = httplog.Warn - } - // We already capture most of this information in the span (minus // the response body which we don't want to capture anyways). tracing.RunWithoutSpan(r.Context(), func(ctx context.Context) { - logLevelFn(ctx, r.Method) + // logLevelFn(ctx, r.Method) + logContext.WriteLog(r.Context(), sw.Status) }) }) } } type RequestLoggerContext struct { - log *slog.Logger + log slog.Logger written bool message string + start time.Time +} + +func NewRequestLoggerContext(log slog.Logger, message string, start time.Time) *RequestLoggerContext { + return &RequestLoggerContext{ + log: log, + written: false, + message: message, + start: start, + } } func (c *RequestLoggerContext) WithFields(fields ...slog.Field) { - newLogger := c.log.With(fields...) - c.log = &newLogger + c.log = c.log.With(fields...) } func (c *RequestLoggerContext) WriteLog(ctx context.Context, status int) { @@ -102,7 +89,16 @@ func (c *RequestLoggerContext) WriteLog(ctx context.Context, status int) { return } c.written = true - + end := time.Now() + + c.WithFields( + slog.F("took", end.Sub(c.start)), + slog.F("status_code", status), + slog.F("latency_ms", float64(end.Sub(c.start)/time.Millisecond)), + ) + // We should not log at level ERROR for 5xx status codes because 5xx + // includes proxy errors etc. It also causes slogtest to fail + // instantly without an error message by default. if status >= http.StatusInternalServerError { c.log.Error(ctx, c.message, "status_code", status) } else { diff --git a/coderd/httpmw/logger_test.go b/coderd/httpmw/logger_test.go new file mode 100644 index 0000000000000..f29c91e577165 --- /dev/null +++ b/coderd/httpmw/logger_test.go @@ -0,0 +1,50 @@ +package httpmw + +import ( + "context" + "net/http" + "testing" + "time" + + "cdr.dev/slog" + "cdr.dev/slog/sloggers/slogtest" +) + +func TestRequestLoggerContext_WriteLog(t *testing.T) { + t.Parallel() + ctx := context.Background() + + testLogger := slogtest.Make(t, nil) + startTime := time.Now() + + logCtx := NewRequestLoggerContext(testLogger, "GET", startTime) + + // Add custom fields + logCtx.WithFields( + slog.F("custom_field", "custom_value"), + ) + + // Write log for 200 status + logCtx.WriteLog(ctx, http.StatusOK) + + if !logCtxWrittenOnce(t, logCtx) { + t.Error("expected log to be written once") + } + // Attempt to write again (should be skipped). + // If the error log entry gets written, + // slogtest will fail the test. + logCtx.WriteLog(ctx, http.StatusInternalServerError) +} + +func logCtxWrittenOnce(t *testing.T, logCtx *RequestLoggerContext) bool { + return logCtxWrittenField(t, logCtx) +} + +func logCtxWrittenField(t *testing.T, logCtx *RequestLoggerContext) bool { + logCtxWritten := logCtx != nil && logCtx.written + if !logCtxWritten { + t.Error("log was not marked as written") + return false + } + return true +} From 5a0fa0588a21113c0f6ea608deba03e90eff9763 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 3 Apr 2025 11:31:38 +0000 Subject: [PATCH 12/36] removed unecessary helpers --- coderd/httpmw/logger_test.go | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/coderd/httpmw/logger_test.go b/coderd/httpmw/logger_test.go index f29c91e577165..5995062318e2b 100644 --- a/coderd/httpmw/logger_test.go +++ b/coderd/httpmw/logger_test.go @@ -27,7 +27,7 @@ func TestRequestLoggerContext_WriteLog(t *testing.T) { // Write log for 200 status logCtx.WriteLog(ctx, http.StatusOK) - if !logCtxWrittenOnce(t, logCtx) { + if !logCtx.written { t.Error("expected log to be written once") } // Attempt to write again (should be skipped). @@ -35,16 +35,3 @@ func TestRequestLoggerContext_WriteLog(t *testing.T) { // slogtest will fail the test. logCtx.WriteLog(ctx, http.StatusInternalServerError) } - -func logCtxWrittenOnce(t *testing.T, logCtx *RequestLoggerContext) bool { - return logCtxWrittenField(t, logCtx) -} - -func logCtxWrittenField(t *testing.T, logCtx *RequestLoggerContext) bool { - logCtxWritten := logCtx != nil && logCtx.written - if !logCtxWritten { - t.Error("log was not marked as written") - return false - } - return true -} From 30bb53d17c45147098038a0347a874eca17d483f Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 3 Apr 2025 11:43:44 +0000 Subject: [PATCH 13/36] added test for an http request --- coderd/httpmw/logger_test.go | 39 ++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/coderd/httpmw/logger_test.go b/coderd/httpmw/logger_test.go index 5995062318e2b..3c55b954829a4 100644 --- a/coderd/httpmw/logger_test.go +++ b/coderd/httpmw/logger_test.go @@ -3,11 +3,13 @@ package httpmw import ( "context" "net/http" + "net/http/httptest" "testing" "time" "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/tracing" ) func TestRequestLoggerContext_WriteLog(t *testing.T) { @@ -15,9 +17,8 @@ func TestRequestLoggerContext_WriteLog(t *testing.T) { ctx := context.Background() testLogger := slogtest.Make(t, nil) - startTime := time.Now() - logCtx := NewRequestLoggerContext(testLogger, "GET", startTime) + logCtx := NewRequestLoggerContext(testLogger, "GET", time.Now()) // Add custom fields logCtx.WithFields( @@ -35,3 +36,37 @@ func TestRequestLoggerContext_WriteLog(t *testing.T) { // slogtest will fail the test. logCtx.WriteLog(ctx, http.StatusInternalServerError) } + +func TestLoggerMiddleware(t *testing.T) { + t.Parallel() + + // Create a test logger + testLogger := slogtest.Make(t, nil) + + // Create a test handler to simulate an HTTP request + testHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusOK) + _, _ = rw.Write([]byte("OK")) + }) + + // Wrap the test handler with the Logger middleware + loggerMiddleware := Logger(testLogger) + wrappedHandler := loggerMiddleware(testHandler) + + // Create a test HTTP request + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/test-path", nil) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } + + sw := &tracing.StatusWriter{ResponseWriter: httptest.NewRecorder()} + + // Serve the request + wrappedHandler.ServeHTTP(sw, req) + + logCtx := RequestLoggerFromContext(context.Background()) + // Verify that the log was written + if logCtx != nil && !logCtx.written { + t.Error("expected log to be written exactly once") + } +} From 89e987e04b1c1ff737adcee3df9e3578bee71165 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 3 Apr 2025 12:26:17 +0000 Subject: [PATCH 14/36] linting fixes --- coderd/httpmw/logger.go | 7 +++---- coderd/httpmw/{logger_test.go => logger_internal_test.go} | 0 2 files changed, 3 insertions(+), 4 deletions(-) rename coderd/httpmw/{logger_test.go => logger_internal_test.go} (100%) diff --git a/coderd/httpmw/logger.go b/coderd/httpmw/logger.go index a797bf1d344f3..bebc8742780f3 100644 --- a/coderd/httpmw/logger.go +++ b/coderd/httpmw/logger.go @@ -57,8 +57,7 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler { // We already capture most of this information in the span (minus // the response body which we don't want to capture anyways). tracing.RunWithoutSpan(r.Context(), func(ctx context.Context) { - // logLevelFn(ctx, r.Method) - logContext.WriteLog(r.Context(), sw.Status) + logContext.WriteLog(ctx, sw.Status) }) }) } @@ -100,9 +99,9 @@ func (c *RequestLoggerContext) WriteLog(ctx context.Context, status int) { // includes proxy errors etc. It also causes slogtest to fail // instantly without an error message by default. if status >= http.StatusInternalServerError { - c.log.Error(ctx, c.message, "status_code", status) + c.log.Error(ctx, c.message) } else { - c.log.Debug(ctx, c.message, "status_code", status) + c.log.Debug(ctx, c.message) } } diff --git a/coderd/httpmw/logger_test.go b/coderd/httpmw/logger_internal_test.go similarity index 100% rename from coderd/httpmw/logger_test.go rename to coderd/httpmw/logger_internal_test.go From 1649116b5597c844b0a993c50795382a91c336f4 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 3 Apr 2025 13:01:12 +0000 Subject: [PATCH 15/36] added handling of internal test circumventing RequestLoggerContext injection path --- coderd/inboxnotifications.go | 5 ++++- coderd/provisionerjobs.go | 5 ++++- coderd/workspaceagents.go | 15 ++++++++++++--- enterprise/coderd/provisionerdaemons.go | 5 ++++- 4 files changed, 24 insertions(+), 6 deletions(-) diff --git a/coderd/inboxnotifications.go b/coderd/inboxnotifications.go index ea20c60de3cce..76bc916415960 100644 --- a/coderd/inboxnotifications.go +++ b/coderd/inboxnotifications.go @@ -220,7 +220,10 @@ func (api *API) watchInboxNotifications(rw http.ResponseWriter, r *http.Request) defer encoder.Close(websocket.StatusNormalClosure) // Log the request immediately instead of after it completes. - httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) + requestLogger := httpmw.RequestLoggerFromContext(ctx) + if requestLogger != nil { + requestLogger.WriteLog(ctx, http.StatusAccepted) + } for { select { diff --git a/coderd/provisionerjobs.go b/coderd/provisionerjobs.go index 335643390796f..66ca9a097fa25 100644 --- a/coderd/provisionerjobs.go +++ b/coderd/provisionerjobs.go @@ -555,7 +555,10 @@ func (f *logFollower) follow() { } // Log the request immediately instead of after it completes. - httpmw.RequestLoggerFromContext(f.ctx).WriteLog(f.ctx, http.StatusAccepted) + requestLogger := httpmw.RequestLoggerFromContext(f.ctx) + if requestLogger != nil { + requestLogger.WriteLog(f.ctx, http.StatusAccepted) + } // no need to wait if the job is done if f.complete { diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 1744c0c6749ca..ebf397b01d88c 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -556,7 +556,10 @@ func (api *API) workspaceAgentLogs(rw http.ResponseWriter, r *http.Request) { defer t.Stop() // Log the request immediately instead of after it completes. - httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) + requestLogger := httpmw.RequestLoggerFromContext(ctx) + if requestLogger != nil { + requestLogger.WriteLog(ctx, http.StatusAccepted) + } go func() { defer func() { @@ -932,7 +935,10 @@ func (api *API) derpMapUpdates(rw http.ResponseWriter, r *http.Request) { defer encoder.Close(websocket.StatusGoingAway) // Log the request immediately instead of after it completes. - httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) + requestLogger := httpmw.RequestLoggerFromContext(ctx) + if requestLogger != nil { + requestLogger.WriteLog(ctx, http.StatusAccepted) + } go func(ctx context.Context) { // TODO(mafredri): Is this too frequent? Use separate ping disconnect timeout? @@ -1322,7 +1328,10 @@ func (api *API) watchWorkspaceAgentMetadata( defer sendTicker.Stop() // Log the request immediately instead of after it completes. - httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) + requestLogger := httpmw.RequestLoggerFromContext(ctx) + if requestLogger != nil { + requestLogger.WriteLog(ctx, http.StatusAccepted) + } // Send initial metadata. sendMetadata() diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 15e3c3901ade3..7c4edf82c018f 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -378,7 +378,10 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) }) // Log the request immediately instead of after it completes. - httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) + requestLogger := httpmw.RequestLoggerFromContext(ctx) + if requestLogger != nil { + requestLogger.WriteLog(ctx, http.StatusAccepted) + } err = server.Serve(ctx, session) srvCancel() From 7ea2dd62cd3c21ae0926e83806767204c698f4b9 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 3 Apr 2025 13:15:38 +0000 Subject: [PATCH 16/36] reverted the original behavior of logging WARN on Status Code >=500 --- coderd/httpmw/logger.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/httpmw/logger.go b/coderd/httpmw/logger.go index bebc8742780f3..a6de068d91230 100644 --- a/coderd/httpmw/logger.go +++ b/coderd/httpmw/logger.go @@ -99,7 +99,7 @@ func (c *RequestLoggerContext) WriteLog(ctx context.Context, status int) { // includes proxy errors etc. It also causes slogtest to fail // instantly without an error message by default. if status >= http.StatusInternalServerError { - c.log.Error(ctx, c.message) + c.log.Warn(ctx, c.message) } else { c.log.Debug(ctx, c.message) } From 338fd20caeb4d5b43a7aae6b5cbdbe5b80215c42 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Fri, 4 Apr 2025 12:37:11 +0000 Subject: [PATCH 17/36] refactored logger to remove extending it with fields permanently --- coderd/httpmw/logger.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/httpmw/logger.go b/coderd/httpmw/logger.go index a6de068d91230..5935746fde5eb 100644 --- a/coderd/httpmw/logger.go +++ b/coderd/httpmw/logger.go @@ -90,7 +90,7 @@ func (c *RequestLoggerContext) WriteLog(ctx context.Context, status int) { c.written = true end := time.Now() - c.WithFields( + logger := c.log.With( slog.F("took", end.Sub(c.start)), slog.F("status_code", status), slog.F("latency_ms", float64(end.Sub(c.start)/time.Millisecond)), @@ -99,9 +99,9 @@ func (c *RequestLoggerContext) WriteLog(ctx context.Context, status int) { // includes proxy errors etc. It also causes slogtest to fail // instantly without an error message by default. if status >= http.StatusInternalServerError { - c.log.Warn(ctx, c.message) + logger.Warn(ctx, c.message) } else { - c.log.Debug(ctx, c.message) + logger.Debug(ctx, c.message) } } From 2708286aabddcf3a53b29fca525f2a792bba3eb5 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Sun, 6 Apr 2025 16:42:53 +0000 Subject: [PATCH 18/36] Removed nil checks when fetching RequestLogger Refactored RequestLoggerContext to RequestLogger Refactored provisionerjobs_internal tests to inject MockRequestLogger Added Mocks for RequestLogger --- coderd/httpmw/logger.go | 27 ++++++--- coderd/httpmw/logger_internal_test.go | 19 ++++-- coderd/httpmw/mocks/mock_request_logger.go | 70 ++++++++++++++++++++++ coderd/inboxnotifications.go | 5 +- coderd/provisionerjobs.go | 5 +- coderd/provisionerjobs_internal_test.go | 7 +++ coderd/workspaceagents.go | 15 +---- enterprise/coderd/provisionerdaemons.go | 5 +- 8 files changed, 114 insertions(+), 39 deletions(-) create mode 100644 coderd/httpmw/mocks/mock_request_logger.go diff --git a/coderd/httpmw/logger.go b/coderd/httpmw/logger.go index 5935746fde5eb..15923f0917bb4 100644 --- a/coderd/httpmw/logger.go +++ b/coderd/httpmw/logger.go @@ -35,9 +35,9 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler { slog.F("start", start), ) - logContext := NewRequestLoggerContext(httplog, r.Method, start) + logContext := NewRequestLogger(httplog, r.Method, start) - ctx := context.WithValue(r.Context(), logContextKey{}, logContext) + ctx := WithRequestLogger(r.Context(), logContext) next.ServeHTTP(sw, r.WithContext(ctx)) @@ -63,15 +63,20 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler { } } -type RequestLoggerContext struct { +type RequestLogger interface { + WithFields(fields ...slog.Field) + WriteLog(ctx context.Context, status int) +} + +type RequestContextLogger struct { log slog.Logger written bool message string start time.Time } -func NewRequestLoggerContext(log slog.Logger, message string, start time.Time) *RequestLoggerContext { - return &RequestLoggerContext{ +func NewRequestLogger(log slog.Logger, message string, start time.Time) RequestLogger { + return &RequestContextLogger{ log: log, written: false, message: message, @@ -79,11 +84,11 @@ func NewRequestLoggerContext(log slog.Logger, message string, start time.Time) * } } -func (c *RequestLoggerContext) WithFields(fields ...slog.Field) { +func (c *RequestContextLogger) WithFields(fields ...slog.Field) { c.log = c.log.With(fields...) } -func (c *RequestLoggerContext) WriteLog(ctx context.Context, status int) { +func (c *RequestContextLogger) WriteLog(ctx context.Context, status int) { if c.written { return } @@ -107,9 +112,13 @@ func (c *RequestLoggerContext) WriteLog(ctx context.Context, status int) { type logContextKey struct{} -func RequestLoggerFromContext(ctx context.Context) *RequestLoggerContext { +func WithRequestLogger(ctx context.Context, rl RequestLogger) context.Context { + return context.WithValue(ctx, logContextKey{}, rl) +} + +func RequestLoggerFromContext(ctx context.Context) RequestLogger { val := ctx.Value(logContextKey{}) - if logCtx, ok := val.(*RequestLoggerContext); ok { + if logCtx, ok := val.(RequestLogger); ok { return logCtx } return nil diff --git a/coderd/httpmw/logger_internal_test.go b/coderd/httpmw/logger_internal_test.go index 3c55b954829a4..fdca3c9b589e3 100644 --- a/coderd/httpmw/logger_internal_test.go +++ b/coderd/httpmw/logger_internal_test.go @@ -12,13 +12,13 @@ import ( "github.com/coder/coder/v2/coderd/tracing" ) -func TestRequestLoggerContext_WriteLog(t *testing.T) { +func TestRequestLogger_WriteLog(t *testing.T) { t.Parallel() ctx := context.Background() testLogger := slogtest.Make(t, nil) - logCtx := NewRequestLoggerContext(testLogger, "GET", time.Now()) + logCtx := NewRequestLogger(testLogger, "GET", time.Now()) // Add custom fields logCtx.WithFields( @@ -28,9 +28,13 @@ func TestRequestLoggerContext_WriteLog(t *testing.T) { // Write log for 200 status logCtx.WriteLog(ctx, http.StatusOK) - if !logCtx.written { - t.Error("expected log to be written once") + if logCtx != nil { + requestCtxLog, ok := logCtx.(*RequestContextLogger) + if ok && !requestCtxLog.written { + t.Error("expected log to be written once") + } } + // Attempt to write again (should be skipped). // If the error log entry gets written, // slogtest will fail the test. @@ -66,7 +70,10 @@ func TestLoggerMiddleware(t *testing.T) { logCtx := RequestLoggerFromContext(context.Background()) // Verify that the log was written - if logCtx != nil && !logCtx.written { - t.Error("expected log to be written exactly once") + if logCtx != nil { + requestCtxLog, ok := logCtx.(*RequestContextLogger) + if ok && !requestCtxLog.written { + t.Error("expected log to be written once") + } } } diff --git a/coderd/httpmw/mocks/mock_request_logger.go b/coderd/httpmw/mocks/mock_request_logger.go new file mode 100644 index 0000000000000..9515fffc740bc --- /dev/null +++ b/coderd/httpmw/mocks/mock_request_logger.go @@ -0,0 +1,70 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/coder/coder/v2/coderd/httpmw (interfaces: RequestLogger) +// +// Generated by this command: +// +// mockgen -destination=mocks/mock_request_logger.go -package=mocks . RequestLogger +// + +// Package mocks is a generated GoMock package. +package mocks + +import ( + context "context" + reflect "reflect" + + slog "cdr.dev/slog" + gomock "go.uber.org/mock/gomock" +) + +// MockRequestLogger is a mock of RequestLogger interface. +type MockRequestLogger struct { + ctrl *gomock.Controller + recorder *MockRequestLoggerMockRecorder + isgomock struct{} +} + +// MockRequestLoggerMockRecorder is the mock recorder for MockRequestLogger. +type MockRequestLoggerMockRecorder struct { + mock *MockRequestLogger +} + +// NewMockRequestLogger creates a new mock instance. +func NewMockRequestLogger(ctrl *gomock.Controller) *MockRequestLogger { + mock := &MockRequestLogger{ctrl: ctrl} + mock.recorder = &MockRequestLoggerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockRequestLogger) EXPECT() *MockRequestLoggerMockRecorder { + return m.recorder +} + +// WithFields mocks base method. +func (m *MockRequestLogger) WithFields(fields ...slog.Field) { + m.ctrl.T.Helper() + varargs := []any{} + for _, a := range fields { + varargs = append(varargs, a) + } + m.ctrl.Call(m, "WithFields", varargs...) +} + +// WithFields indicates an expected call of WithFields. +func (mr *MockRequestLoggerMockRecorder) WithFields(fields ...any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithFields", reflect.TypeOf((*MockRequestLogger)(nil).WithFields), fields...) +} + +// WriteLog mocks base method. +func (m *MockRequestLogger) WriteLog(ctx context.Context, status int) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "WriteLog", ctx, status) +} + +// WriteLog indicates an expected call of WriteLog. +func (mr *MockRequestLoggerMockRecorder) WriteLog(ctx, status any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WriteLog", reflect.TypeOf((*MockRequestLogger)(nil).WriteLog), ctx, status) +} diff --git a/coderd/inboxnotifications.go b/coderd/inboxnotifications.go index 76bc916415960..ea20c60de3cce 100644 --- a/coderd/inboxnotifications.go +++ b/coderd/inboxnotifications.go @@ -220,10 +220,7 @@ func (api *API) watchInboxNotifications(rw http.ResponseWriter, r *http.Request) defer encoder.Close(websocket.StatusNormalClosure) // Log the request immediately instead of after it completes. - requestLogger := httpmw.RequestLoggerFromContext(ctx) - if requestLogger != nil { - requestLogger.WriteLog(ctx, http.StatusAccepted) - } + httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) for { select { diff --git a/coderd/provisionerjobs.go b/coderd/provisionerjobs.go index 66ca9a097fa25..335643390796f 100644 --- a/coderd/provisionerjobs.go +++ b/coderd/provisionerjobs.go @@ -555,10 +555,7 @@ func (f *logFollower) follow() { } // Log the request immediately instead of after it completes. - requestLogger := httpmw.RequestLoggerFromContext(f.ctx) - if requestLogger != nil { - requestLogger.WriteLog(f.ctx, http.StatusAccepted) - } + httpmw.RequestLoggerFromContext(f.ctx).WriteLog(f.ctx, http.StatusAccepted) // no need to wait if the job is done if f.complete { diff --git a/coderd/provisionerjobs_internal_test.go b/coderd/provisionerjobs_internal_test.go index af5a7d66a6f4c..5089891bff36b 100644 --- a/coderd/provisionerjobs_internal_test.go +++ b/coderd/provisionerjobs_internal_test.go @@ -19,6 +19,8 @@ import ( "github.com/coder/coder/v2/coderd/database/dbmock" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/database/pubsub" + "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/httpmw/mocks" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/provisionersdk" "github.com/coder/coder/v2/testutil" @@ -305,11 +307,16 @@ func Test_logFollower_EndOfLogs(t *testing.T) { JobStatus: database.ProvisionerJobStatusRunning, } + mockLogger := mocks.NewMockRequestLogger(ctrl) + mockLogger.EXPECT().WriteLog(gomock.Any(), http.StatusAccepted).Times(1) + ctx = httpmw.WithRequestLogger(ctx, mockLogger) + // we need an HTTP server to get a websocket srv := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { uut := newLogFollower(ctx, logger, mDB, ps, rw, r, job, 0) uut.follow() })) + defer srv.Close() // job was incomplete when we create the logFollower, and still incomplete when it queries diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index ebf397b01d88c..1744c0c6749ca 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -556,10 +556,7 @@ func (api *API) workspaceAgentLogs(rw http.ResponseWriter, r *http.Request) { defer t.Stop() // Log the request immediately instead of after it completes. - requestLogger := httpmw.RequestLoggerFromContext(ctx) - if requestLogger != nil { - requestLogger.WriteLog(ctx, http.StatusAccepted) - } + httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) go func() { defer func() { @@ -935,10 +932,7 @@ func (api *API) derpMapUpdates(rw http.ResponseWriter, r *http.Request) { defer encoder.Close(websocket.StatusGoingAway) // Log the request immediately instead of after it completes. - requestLogger := httpmw.RequestLoggerFromContext(ctx) - if requestLogger != nil { - requestLogger.WriteLog(ctx, http.StatusAccepted) - } + httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) go func(ctx context.Context) { // TODO(mafredri): Is this too frequent? Use separate ping disconnect timeout? @@ -1328,10 +1322,7 @@ func (api *API) watchWorkspaceAgentMetadata( defer sendTicker.Stop() // Log the request immediately instead of after it completes. - requestLogger := httpmw.RequestLoggerFromContext(ctx) - if requestLogger != nil { - requestLogger.WriteLog(ctx, http.StatusAccepted) - } + httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) // Send initial metadata. sendMetadata() diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 7c4edf82c018f..15e3c3901ade3 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -378,10 +378,7 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) }) // Log the request immediately instead of after it completes. - requestLogger := httpmw.RequestLoggerFromContext(ctx) - if requestLogger != nil { - requestLogger.WriteLog(ctx, http.StatusAccepted) - } + httpmw.RequestLoggerFromContext(ctx).WriteLog(ctx, http.StatusAccepted) err = server.Serve(ctx, session) srvCancel() From ca3c4d3b79d6e160d541230c7d250e75dc9c5f60 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Sun, 6 Apr 2025 19:11:50 +0000 Subject: [PATCH 19/36] refactored tests to use a fakesink to verify the written messages --- coderd/httpmw/logger_internal_test.go | 59 +++++++++++++++++---------- 1 file changed, 38 insertions(+), 21 deletions(-) diff --git a/coderd/httpmw/logger_internal_test.go b/coderd/httpmw/logger_internal_test.go index fdca3c9b589e3..052802b9feb76 100644 --- a/coderd/httpmw/logger_internal_test.go +++ b/coderd/httpmw/logger_internal_test.go @@ -8,7 +8,6 @@ import ( "time" "cdr.dev/slog" - "cdr.dev/slog/sloggers/slogtest" "github.com/coder/coder/v2/coderd/tracing" ) @@ -16,9 +15,10 @@ func TestRequestLogger_WriteLog(t *testing.T) { t.Parallel() ctx := context.Background() - testLogger := slogtest.Make(t, nil) - - logCtx := NewRequestLogger(testLogger, "GET", time.Now()) + sink := &fakeSink{} + logger := slog.Make(sink) + logger = logger.Leveled(slog.LevelDebug) + logCtx := NewRequestLogger(logger, "GET", time.Now()) // Add custom fields logCtx.WithFields( @@ -28,24 +28,32 @@ func TestRequestLogger_WriteLog(t *testing.T) { // Write log for 200 status logCtx.WriteLog(ctx, http.StatusOK) - if logCtx != nil { - requestCtxLog, ok := logCtx.(*RequestContextLogger) - if ok && !requestCtxLog.written { - t.Error("expected log to be written once") - } + if len(sink.entries) != 1 { + t.Fatalf("expected 1 log entry, got %d", len(sink.entries)) + } + + if sink.entries[0].Message != "GET" { + t.Errorf("expected log message to be 'GET', got '%s'", sink.entries[0].Message) + } + + if sink.entries[0].Fields[0].Value != "custom_value" { + t.Errorf("expected a custom_field with value custom_value, got '%s'", sink.entries[0].Fields[0].Value) } // Attempt to write again (should be skipped). - // If the error log entry gets written, - // slogtest will fail the test. logCtx.WriteLog(ctx, http.StatusInternalServerError) + + if len(sink.entries) != 1 { + t.Fatalf("expected 1 log entry after second write, got %d", len(sink.entries)) + } } func TestLoggerMiddleware(t *testing.T) { t.Parallel() - // Create a test logger - testLogger := slogtest.Make(t, nil) + sink := &fakeSink{} + logger := slog.Make(sink) + logger = logger.Leveled(slog.LevelDebug) // Create a test handler to simulate an HTTP request testHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { @@ -54,7 +62,7 @@ func TestLoggerMiddleware(t *testing.T) { }) // Wrap the test handler with the Logger middleware - loggerMiddleware := Logger(testLogger) + loggerMiddleware := Logger(logger) wrappedHandler := loggerMiddleware(testHandler) // Create a test HTTP request @@ -68,12 +76,21 @@ func TestLoggerMiddleware(t *testing.T) { // Serve the request wrappedHandler.ServeHTTP(sw, req) - logCtx := RequestLoggerFromContext(context.Background()) - // Verify that the log was written - if logCtx != nil { - requestCtxLog, ok := logCtx.(*RequestContextLogger) - if ok && !requestCtxLog.written { - t.Error("expected log to be written once") - } + if len(sink.entries) != 1 { + t.Fatalf("expected 1 log entry, got %d", len(sink.entries)) } + + if sink.entries[0].Message != "GET" { + t.Errorf("expected log message to be 'GET', got '%s'", sink.entries[0].Message) + } +} + +type fakeSink struct { + entries []slog.SinkEntry } + +func (s *fakeSink) LogEntry(_ context.Context, e slog.SinkEntry) { + s.entries = append(s.entries, e) +} + +func (*fakeSink) Sync() {} From dfa2f4fc2f1c8fdaab83ac8bf95fac34a1ae38bf Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Sun, 6 Apr 2025 23:02:03 +0000 Subject: [PATCH 20/36] added WebSocket test to verify early logging --- coderd/httpmw/logger_internal_test.go | 53 ++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/coderd/httpmw/logger_internal_test.go b/coderd/httpmw/logger_internal_test.go index 052802b9feb76..f657ff416dde5 100644 --- a/coderd/httpmw/logger_internal_test.go +++ b/coderd/httpmw/logger_internal_test.go @@ -8,7 +8,10 @@ import ( "time" "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/tracing" + "github.com/coder/coder/v2/testutil" + "github.com/coder/websocket" ) func TestRequestLogger_WriteLog(t *testing.T) { @@ -48,7 +51,7 @@ func TestRequestLogger_WriteLog(t *testing.T) { } } -func TestLoggerMiddleware(t *testing.T) { +func TestLoggerMiddleware_SingleRequest(t *testing.T) { t.Parallel() sink := &fakeSink{} @@ -85,6 +88,54 @@ func TestLoggerMiddleware(t *testing.T) { } } +func TestLoggerMiddleware_WebSocket(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + sink := &fakeSink{} + logger := slog.Make(sink) + logger = logger.Leveled(slog.LevelDebug) + + // Create a test handler to simulate a WebSocket connection + testHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + _, err := websocket.Accept(rw, r, nil) + if err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, nil) + return + } + time.Sleep(1000) + }) + + // Wrap the test handler with the Logger middleware + loggerMiddleware := Logger(logger) + wrappedHandler := loggerMiddleware(testHandler) + + // RequestLogger expects the ResponseWriter to be *tracing.StatusWriter + customHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + sw := &tracing.StatusWriter{ResponseWriter: rw} + wrappedHandler.ServeHTTP(sw, r) + }) + + // Create a test HTTP request + srv := httptest.NewServer(customHandler) + defer srv.Close() + + conn, _, err := websocket.Dial(ctx, srv.URL, nil) + if err != nil { + t.Fatalf("failed to create WebSocket connection: %v", err) + } + defer conn.Close(websocket.StatusNormalClosure, "") + + if len(sink.entries) != 1 { + t.Fatalf("expected 1 log entry, got %d", len(sink.entries)) + } + + if sink.entries[0].Message != "GET" { + t.Errorf("expected log message to be 'GET', got '%s'", sink.entries[0].Message) + } +} + type fakeSink struct { entries []slog.SinkEntry } From 845ff0fc1631b432118047b18ef0f71b19463df7 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Sun, 6 Apr 2025 23:06:32 +0000 Subject: [PATCH 21/36] fix lint for response body leak --- coderd/httpmw/logger_internal_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/coderd/httpmw/logger_internal_test.go b/coderd/httpmw/logger_internal_test.go index f657ff416dde5..b1ddbefdc0bf9 100644 --- a/coderd/httpmw/logger_internal_test.go +++ b/coderd/httpmw/logger_internal_test.go @@ -121,10 +121,13 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { srv := httptest.NewServer(customHandler) defer srv.Close() - conn, _, err := websocket.Dial(ctx, srv.URL, nil) + conn, resp, err := websocket.Dial(ctx, srv.URL, nil) if err != nil { t.Fatalf("failed to create WebSocket connection: %v", err) } + if resp != nil { + defer resp.Body.Close() + } defer conn.Close(websocket.StatusNormalClosure, "") if len(sink.entries) != 1 { From 1f7acfb7611af5ae7322e10f2d67d9f8debbdafd Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Sun, 6 Apr 2025 23:16:18 +0000 Subject: [PATCH 22/36] reverted bodyclose lint fix --- coderd/httpmw/logger_internal_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/coderd/httpmw/logger_internal_test.go b/coderd/httpmw/logger_internal_test.go index b1ddbefdc0bf9..fbe6c38a2a6b0 100644 --- a/coderd/httpmw/logger_internal_test.go +++ b/coderd/httpmw/logger_internal_test.go @@ -121,13 +121,11 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { srv := httptest.NewServer(customHandler) defer srv.Close() - conn, resp, err := websocket.Dial(ctx, srv.URL, nil) + // nolint: bodyclose + conn, _, err := websocket.Dial(ctx, srv.URL, nil) if err != nil { t.Fatalf("failed to create WebSocket connection: %v", err) } - if resp != nil { - defer resp.Body.Close() - } defer conn.Close(websocket.StatusNormalClosure, "") if len(sink.entries) != 1 { From 58aa736801fc625184ca1086b470a0420320b0b9 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Sun, 6 Apr 2025 23:42:27 +0000 Subject: [PATCH 23/36] fix for race condition in the test --- coderd/httpmw/logger_internal_test.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/coderd/httpmw/logger_internal_test.go b/coderd/httpmw/logger_internal_test.go index fbe6c38a2a6b0..a1ff8f7a7465a 100644 --- a/coderd/httpmw/logger_internal_test.go +++ b/coderd/httpmw/logger_internal_test.go @@ -4,11 +4,11 @@ import ( "context" "net/http" "net/http/httptest" + "sync" "testing" "time" "cdr.dev/slog" - "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/tracing" "github.com/coder/coder/v2/testutil" "github.com/coder/websocket" @@ -96,15 +96,22 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { sink := &fakeSink{} logger := slog.Make(sink) logger = logger.Leveled(slog.LevelDebug) - + var wg sync.WaitGroup // Create a test handler to simulate a WebSocket connection testHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - _, err := websocket.Accept(rw, r, nil) + + conn, err := websocket.Accept(rw, r, nil) if err != nil { - httpapi.Write(ctx, rw, http.StatusBadRequest, nil) + t.Errorf("failed to accept websocket: %v", err) return } - time.Sleep(1000) + defer conn.Close(websocket.StatusNormalClosure, "") + defer wg.Done() + + // Send a couple of messages for testing + _ = conn.Write(ctx, websocket.MessageText, []byte("ping")) + _ = conn.Write(ctx, websocket.MessageText, []byte("pong")) + }) // Wrap the test handler with the Logger middleware @@ -120,9 +127,10 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { // Create a test HTTP request srv := httptest.NewServer(customHandler) defer srv.Close() - + wg.Add(1) // nolint: bodyclose conn, _, err := websocket.Dial(ctx, srv.URL, nil) + wg.Wait() if err != nil { t.Fatalf("failed to create WebSocket connection: %v", err) } From 2c63b32a1b002e6cd3583e794ce370731232686a Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Sun, 6 Apr 2025 23:58:17 +0000 Subject: [PATCH 24/36] linter fixes --- coderd/httpmw/logger_internal_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/coderd/httpmw/logger_internal_test.go b/coderd/httpmw/logger_internal_test.go index a1ff8f7a7465a..fa120d169372c 100644 --- a/coderd/httpmw/logger_internal_test.go +++ b/coderd/httpmw/logger_internal_test.go @@ -99,7 +99,6 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { var wg sync.WaitGroup // Create a test handler to simulate a WebSocket connection testHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - conn, err := websocket.Accept(rw, r, nil) if err != nil { t.Errorf("failed to accept websocket: %v", err) @@ -111,7 +110,6 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { // Send a couple of messages for testing _ = conn.Write(ctx, websocket.MessageText, []byte("ping")) _ = conn.Write(ctx, websocket.MessageText, []byte("pong")) - }) // Wrap the test handler with the Logger middleware From d268a3ce0948436fe9237642e9eb077e8b4f3008 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 7 Apr 2025 00:02:52 +0000 Subject: [PATCH 25/36] actually added logging early to test early logging --- coderd/httpmw/logger_internal_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/coderd/httpmw/logger_internal_test.go b/coderd/httpmw/logger_internal_test.go index fa120d169372c..9c3608037ee8c 100644 --- a/coderd/httpmw/logger_internal_test.go +++ b/coderd/httpmw/logger_internal_test.go @@ -4,7 +4,6 @@ import ( "context" "net/http" "net/http/httptest" - "sync" "testing" "time" @@ -96,7 +95,7 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { sink := &fakeSink{} logger := slog.Make(sink) logger = logger.Leveled(slog.LevelDebug) - var wg sync.WaitGroup + // Create a test handler to simulate a WebSocket connection testHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { conn, err := websocket.Accept(rw, r, nil) @@ -104,8 +103,9 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { t.Errorf("failed to accept websocket: %v", err) return } + requestLgr := RequestLoggerFromContext(r.Context()) + requestLgr.WriteLog(r.Context(), http.StatusSwitchingProtocols) defer conn.Close(websocket.StatusNormalClosure, "") - defer wg.Done() // Send a couple of messages for testing _ = conn.Write(ctx, websocket.MessageText, []byte("ping")) @@ -125,10 +125,8 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { // Create a test HTTP request srv := httptest.NewServer(customHandler) defer srv.Close() - wg.Add(1) // nolint: bodyclose conn, _, err := websocket.Dial(ctx, srv.URL, nil) - wg.Wait() if err != nil { t.Fatalf("failed to create WebSocket connection: %v", err) } From 8f550c5b6797edb71c06eef8d8ad6cb09751f056 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 7 Apr 2025 00:18:37 +0000 Subject: [PATCH 26/36] added a WaitGroup in logger internal tests to remove the race condition --- coderd/httpmw/logger_internal_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/coderd/httpmw/logger_internal_test.go b/coderd/httpmw/logger_internal_test.go index 9c3608037ee8c..6548b8686e505 100644 --- a/coderd/httpmw/logger_internal_test.go +++ b/coderd/httpmw/logger_internal_test.go @@ -4,6 +4,7 @@ import ( "context" "net/http" "net/http/httptest" + "sync" "testing" "time" @@ -95,7 +96,7 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { sink := &fakeSink{} logger := slog.Make(sink) logger = logger.Leveled(slog.LevelDebug) - + wg := sync.WaitGroup{} // Create a test handler to simulate a WebSocket connection testHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { conn, err := websocket.Accept(rw, r, nil) @@ -105,6 +106,7 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { } requestLgr := RequestLoggerFromContext(r.Context()) requestLgr.WriteLog(r.Context(), http.StatusSwitchingProtocols) + wg.Done() defer conn.Close(websocket.StatusNormalClosure, "") // Send a couple of messages for testing @@ -122,16 +124,16 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { wrappedHandler.ServeHTTP(sw, r) }) - // Create a test HTTP request srv := httptest.NewServer(customHandler) defer srv.Close() + wg.Add(1) // nolint: bodyclose conn, _, err := websocket.Dial(ctx, srv.URL, nil) if err != nil { t.Fatalf("failed to create WebSocket connection: %v", err) } defer conn.Close(websocket.StatusNormalClosure, "") - + wg.Wait() if len(sink.entries) != 1 { t.Fatalf("expected 1 log entry, got %d", len(sink.entries)) } From 81e15279c80e9e3e81bc22ebcc011500bc8aee07 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 7 Apr 2025 09:04:53 +0000 Subject: [PATCH 27/36] refactor after code review --- coderd/httpmw/logger.go | 36 ++++++++++++++------------- coderd/httpmw/logger_internal_test.go | 34 ++++++++----------------- 2 files changed, 29 insertions(+), 41 deletions(-) diff --git a/coderd/httpmw/logger.go b/coderd/httpmw/logger.go index 15923f0917bb4..0da964407b3e4 100644 --- a/coderd/httpmw/logger.go +++ b/coderd/httpmw/logger.go @@ -54,11 +54,7 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler { ) } - // We already capture most of this information in the span (minus - // the response body which we don't want to capture anyways). - tracing.RunWithoutSpan(r.Context(), func(ctx context.Context) { - logContext.WriteLog(ctx, sw.Status) - }) + logContext.WriteLog(r.Context(), sw.Status) }) } } @@ -68,15 +64,17 @@ type RequestLogger interface { WriteLog(ctx context.Context, status int) } -type RequestContextLogger struct { +type SlogRequestLogger struct { log slog.Logger written bool message string start time.Time } +var _ RequestLogger = &SlogRequestLogger{} + func NewRequestLogger(log slog.Logger, message string, start time.Time) RequestLogger { - return &RequestContextLogger{ + return &SlogRequestLogger{ log: log, written: false, message: message, @@ -84,11 +82,11 @@ func NewRequestLogger(log slog.Logger, message string, start time.Time) RequestL } } -func (c *RequestContextLogger) WithFields(fields ...slog.Field) { +func (c *SlogRequestLogger) WithFields(fields ...slog.Field) { c.log = c.log.With(fields...) } -func (c *RequestContextLogger) WriteLog(ctx context.Context, status int) { +func (c *SlogRequestLogger) WriteLog(ctx context.Context, status int) { if c.written { return } @@ -100,14 +98,18 @@ func (c *RequestContextLogger) WriteLog(ctx context.Context, status int) { slog.F("status_code", status), slog.F("latency_ms", float64(end.Sub(c.start)/time.Millisecond)), ) - // We should not log at level ERROR for 5xx status codes because 5xx - // includes proxy errors etc. It also causes slogtest to fail - // instantly without an error message by default. - if status >= http.StatusInternalServerError { - logger.Warn(ctx, c.message) - } else { - logger.Debug(ctx, c.message) - } + // We already capture most of this information in the span (minus + // the response body which we don't want to capture anyways). + tracing.RunWithoutSpan(ctx, func(ctx context.Context) { + // We should not log at level ERROR for 5xx status codes because 5xx + // includes proxy errors etc. It also causes slogtest to fail + // instantly without an error message by default. + if status >= http.StatusInternalServerError { + logger.Warn(ctx, c.message) + } else { + logger.Debug(ctx, c.message) + } + }) } type logContextKey struct{} diff --git a/coderd/httpmw/logger_internal_test.go b/coderd/httpmw/logger_internal_test.go index 6548b8686e505..8dcac2d5713eb 100644 --- a/coderd/httpmw/logger_internal_test.go +++ b/coderd/httpmw/logger_internal_test.go @@ -8,6 +8,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + "cdr.dev/slog" "github.com/coder/coder/v2/coderd/tracing" "github.com/coder/coder/v2/testutil" @@ -31,24 +33,16 @@ func TestRequestLogger_WriteLog(t *testing.T) { // Write log for 200 status logCtx.WriteLog(ctx, http.StatusOK) - if len(sink.entries) != 1 { - t.Fatalf("expected 1 log entry, got %d", len(sink.entries)) - } + require.Len(t, sink.entries, 1, "log was written twice") - if sink.entries[0].Message != "GET" { - t.Errorf("expected log message to be 'GET', got '%s'", sink.entries[0].Message) - } + require.Equal(t, sink.entries[0].Message, "GET", "log message should be GET") - if sink.entries[0].Fields[0].Value != "custom_value" { - t.Errorf("expected a custom_field with value custom_value, got '%s'", sink.entries[0].Fields[0].Value) - } + require.Equal(t, sink.entries[0].Fields[0].Value, "custom_value", "custom_field should be custom_value") // Attempt to write again (should be skipped). logCtx.WriteLog(ctx, http.StatusInternalServerError) - if len(sink.entries) != 1 { - t.Fatalf("expected 1 log entry after second write, got %d", len(sink.entries)) - } + require.Len(t, sink.entries, 1, "log was written twice") } func TestLoggerMiddleware_SingleRequest(t *testing.T) { @@ -79,13 +73,9 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) { // Serve the request wrappedHandler.ServeHTTP(sw, req) - if len(sink.entries) != 1 { - t.Fatalf("expected 1 log entry, got %d", len(sink.entries)) - } + require.Len(t, sink.entries, 1, "log was written twice") - if sink.entries[0].Message != "GET" { - t.Errorf("expected log message to be 'GET', got '%s'", sink.entries[0].Message) - } + require.Equal(t, sink.entries[0].Message, "GET", "log message should be GET") } func TestLoggerMiddleware_WebSocket(t *testing.T) { @@ -134,13 +124,9 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { } defer conn.Close(websocket.StatusNormalClosure, "") wg.Wait() - if len(sink.entries) != 1 { - t.Fatalf("expected 1 log entry, got %d", len(sink.entries)) - } + require.Len(t, sink.entries, 1, "log was written twice") - if sink.entries[0].Message != "GET" { - t.Errorf("expected log message to be 'GET', got '%s'", sink.entries[0].Message) - } + require.Equal(t, sink.entries[0].Message, "GET", "log message should be GET") } type fakeSink struct { From b9e2b615ffcdf5bc33d6cc60ec770cdfb6bc3b82 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 7 Apr 2025 09:28:42 +0000 Subject: [PATCH 28/36] added checking the logger fields in the test --- coderd/httpmw/logger_internal_test.go | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/coderd/httpmw/logger_internal_test.go b/coderd/httpmw/logger_internal_test.go index 8dcac2d5713eb..e7301e92e89a5 100644 --- a/coderd/httpmw/logger_internal_test.go +++ b/coderd/httpmw/logger_internal_test.go @@ -52,6 +52,9 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) { logger := slog.Make(sink) logger = logger.Leveled(slog.LevelDebug) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + // Create a test handler to simulate an HTTP request testHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { rw.WriteHeader(http.StatusOK) @@ -63,7 +66,7 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) { wrappedHandler := loggerMiddleware(testHandler) // Create a test HTTP request - req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/test-path", nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, "/test-path", nil) if err != nil { t.Fatalf("failed to create request: %v", err) } @@ -76,6 +79,24 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) { require.Len(t, sink.entries, 1, "log was written twice") require.Equal(t, sink.entries[0].Message, "GET", "log message should be GET") + + fieldsMap := make(map[string]interface{}) + for _, field := range sink.entries[0].Fields { + fieldsMap[field.Name] = field.Value + } + + // Check that the log contains the expected fields + requiredFields := []string{"host", "path", "proto", "remote_addr", "start", "took", "status_code", "latency_ms"} + for _, field := range requiredFields { + _, exists := fieldsMap[field] + require.True(t, exists, "field %q is missing in log fields", field) + } + + require.Len(t, sink.entries[0].Fields, len(requiredFields), "log should contain only the required fields") + + // Check value of the status code + require.Equal(t, fieldsMap["status_code"], http.StatusOK, "status_code should be 200") + } func TestLoggerMiddleware_WebSocket(t *testing.T) { From deb1a413885b46a3236fc3201e0d456cf3b520e1 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 7 Apr 2025 09:37:19 +0000 Subject: [PATCH 29/36] refactored mock request logger to fit current codebase --- .../mock_request_logger.go => loggermock/loggermock.go} | 6 +++--- coderd/provisionerjobs_internal_test.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) rename coderd/httpmw/{mocks/mock_request_logger.go => loggermock/loggermock.go} (92%) diff --git a/coderd/httpmw/mocks/mock_request_logger.go b/coderd/httpmw/loggermock/loggermock.go similarity index 92% rename from coderd/httpmw/mocks/mock_request_logger.go rename to coderd/httpmw/loggermock/loggermock.go index 9515fffc740bc..47818ca11d9e6 100644 --- a/coderd/httpmw/mocks/mock_request_logger.go +++ b/coderd/httpmw/loggermock/loggermock.go @@ -3,11 +3,11 @@ // // Generated by this command: // -// mockgen -destination=mocks/mock_request_logger.go -package=mocks . RequestLogger +// mockgen -destination=loggermock/loggermock.go -package=loggermock . RequestLogger // -// Package mocks is a generated GoMock package. -package mocks +// Package loggermock is a generated GoMock package. +package loggermock import ( context "context" diff --git a/coderd/provisionerjobs_internal_test.go b/coderd/provisionerjobs_internal_test.go index 5089891bff36b..c2c0a60c75ba0 100644 --- a/coderd/provisionerjobs_internal_test.go +++ b/coderd/provisionerjobs_internal_test.go @@ -20,7 +20,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/database/pubsub" "github.com/coder/coder/v2/coderd/httpmw" - "github.com/coder/coder/v2/coderd/httpmw/mocks" + "github.com/coder/coder/v2/coderd/httpmw/loggermock" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/provisionersdk" "github.com/coder/coder/v2/testutil" @@ -307,7 +307,7 @@ func Test_logFollower_EndOfLogs(t *testing.T) { JobStatus: database.ProvisionerJobStatusRunning, } - mockLogger := mocks.NewMockRequestLogger(ctrl) + mockLogger := loggermock.NewMockRequestLogger(ctrl) mockLogger.EXPECT().WriteLog(gomock.Any(), http.StatusAccepted).Times(1) ctx = httpmw.WithRequestLogger(ctx, mockLogger) From 09e46d03a7abe466965645d096892b4c4002426e Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 7 Apr 2025 09:45:36 +0000 Subject: [PATCH 30/36] added mocked files to Makefile --- Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index e8cdcd3a3a1ba..29473c5d20ecb 100644 --- a/Makefile +++ b/Makefile @@ -581,7 +581,8 @@ GEN_FILES := \ $(TAILNETTEST_MOCKS) \ coderd/database/pubsub/psmock/psmock.go \ agent/agentcontainers/acmock/acmock.go \ - agent/agentcontainers/dcspec/dcspec_gen.go + agent/agentcontainers/dcspec/dcspec_gen.go \ + coderd/httmpw/loggermock/loggermock.go # all gen targets should be added here and to gen/mark-fresh gen: gen/db gen/golden-files $(GEN_FILES) @@ -630,6 +631,7 @@ gen/mark-fresh: coderd/database/pubsub/psmock/psmock.go \ agent/agentcontainers/acmock/acmock.go \ agent/agentcontainers/dcspec/dcspec_gen.go \ + coderd/httmpw/loggermock/loggermock.go \ " for file in $$files; do From 9b16cf95dc491ad815def44664c23969c90c0def Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 7 Apr 2025 09:58:19 +0000 Subject: [PATCH 31/36] format fix --- coderd/httpmw/logger_internal_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/httpmw/logger_internal_test.go b/coderd/httpmw/logger_internal_test.go index e7301e92e89a5..89a6590a0f1fe 100644 --- a/coderd/httpmw/logger_internal_test.go +++ b/coderd/httpmw/logger_internal_test.go @@ -96,7 +96,6 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) { // Check value of the status code require.Equal(t, fieldsMap["status_code"], http.StatusOK, "status_code should be 200") - } func TestLoggerMiddleware_WebSocket(t *testing.T) { From 2e331a3c267c3701ae9903585fce1a1bfff9c089 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 7 Apr 2025 10:05:27 +0000 Subject: [PATCH 32/36] fixed path in Makefile --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 29473c5d20ecb..66a7095af4b7b 100644 --- a/Makefile +++ b/Makefile @@ -631,7 +631,7 @@ gen/mark-fresh: coderd/database/pubsub/psmock/psmock.go \ agent/agentcontainers/acmock/acmock.go \ agent/agentcontainers/dcspec/dcspec_gen.go \ - coderd/httmpw/loggermock/loggermock.go \ + coderd/httpmw/loggermock/loggermock.go \ " for file in $$files; do From 7e9f08316ea26d50796bb48f5133c88baf86d56a Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 7 Apr 2025 10:20:59 +0000 Subject: [PATCH 33/36] fixed Makefile path --- Makefile | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 66a7095af4b7b..08a0dea20e7c8 100644 --- a/Makefile +++ b/Makefile @@ -582,7 +582,7 @@ GEN_FILES := \ coderd/database/pubsub/psmock/psmock.go \ agent/agentcontainers/acmock/acmock.go \ agent/agentcontainers/dcspec/dcspec_gen.go \ - coderd/httmpw/loggermock/loggermock.go + coderd/httpmw/loggermock/loggermock.go # all gen targets should be added here and to gen/mark-fresh gen: gen/db gen/golden-files $(GEN_FILES) @@ -671,6 +671,10 @@ agent/agentcontainers/acmock/acmock.go: agent/agentcontainers/containers.go go generate ./agent/agentcontainers/acmock/ touch "$@" +coderd/httpmw/loggermock/loggermock.go: coderd/httpmw/logger.go + go generate ./coderd/httpmw/loggermock/loggermock/ + touch "$@" + agent/agentcontainers/dcspec/dcspec_gen.go: \ node_modules/.installed \ agent/agentcontainers/dcspec/devContainer.base.schema.json \ From b82776f9e2c3e476049792a7686136571891dfb6 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 7 Apr 2025 10:27:00 +0000 Subject: [PATCH 34/36] fixed nested path in Makefile --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 08a0dea20e7c8..6486f5cbed5fa 100644 --- a/Makefile +++ b/Makefile @@ -672,7 +672,7 @@ agent/agentcontainers/acmock/acmock.go: agent/agentcontainers/containers.go touch "$@" coderd/httpmw/loggermock/loggermock.go: coderd/httpmw/logger.go - go generate ./coderd/httpmw/loggermock/loggermock/ + go generate ./coderd/httpmw/loggermock/ touch "$@" agent/agentcontainers/dcspec/dcspec_gen.go: \ From cb118a9d17f23d0437f6026cd74a2dc671433d87 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 8 Apr 2025 07:42:59 +0000 Subject: [PATCH 35/36] added additional checks --- coderd/httpmw/logger_internal_test.go | 58 ++++++++++++++++----------- 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/coderd/httpmw/logger_internal_test.go b/coderd/httpmw/logger_internal_test.go index 89a6590a0f1fe..5f87e15797d53 100644 --- a/coderd/httpmw/logger_internal_test.go +++ b/coderd/httpmw/logger_internal_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "cdr.dev/slog" @@ -35,9 +36,9 @@ func TestRequestLogger_WriteLog(t *testing.T) { require.Len(t, sink.entries, 1, "log was written twice") - require.Equal(t, sink.entries[0].Message, "GET", "log message should be GET") + require.Equal(t, sink.entries[0].Message, "GET") - require.Equal(t, sink.entries[0].Fields[0].Value, "custom_value", "custom_field should be custom_value") + require.Equal(t, sink.entries[0].Fields[0].Value, "custom_value") // Attempt to write again (should be skipped). logCtx.WriteLog(ctx, http.StatusInternalServerError) @@ -67,9 +68,7 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) { // Create a test HTTP request req, err := http.NewRequestWithContext(ctx, http.MethodGet, "/test-path", nil) - if err != nil { - t.Fatalf("failed to create request: %v", err) - } + require.NoError(t, err, "failed to create request") sw := &tracing.StatusWriter{ResponseWriter: httptest.NewRecorder()} @@ -78,7 +77,7 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) { require.Len(t, sink.entries, 1, "log was written twice") - require.Equal(t, sink.entries[0].Message, "GET", "log message should be GET") + require.Equal(t, sink.entries[0].Message, "GET") fieldsMap := make(map[string]interface{}) for _, field := range sink.entries[0].Fields { @@ -95,7 +94,7 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) { require.Len(t, sink.entries[0].Fields, len(requiredFields), "log should contain only the required fields") // Check value of the status code - require.Equal(t, fieldsMap["status_code"], http.StatusOK, "status_code should be 200") + require.Equal(t, fieldsMap["status_code"], http.StatusOK) } func TestLoggerMiddleware_WebSocket(t *testing.T) { @@ -103,25 +102,25 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() - sink := &fakeSink{} + sink := &fakeSink{ + newEntries: make(chan slog.SinkEntry, 2), + } logger := slog.Make(sink) logger = logger.Leveled(slog.LevelDebug) + done := make(chan struct{}) wg := sync.WaitGroup{} // Create a test handler to simulate a WebSocket connection testHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { conn, err := websocket.Accept(rw, r, nil) - if err != nil { - t.Errorf("failed to accept websocket: %v", err) + if !assert.NoError(t, err, "failed to accept websocket") { return } - requestLgr := RequestLoggerFromContext(r.Context()) - requestLgr.WriteLog(r.Context(), http.StatusSwitchingProtocols) - wg.Done() defer conn.Close(websocket.StatusNormalClosure, "") - // Send a couple of messages for testing - _ = conn.Write(ctx, websocket.MessageText, []byte("ping")) - _ = conn.Write(ctx, websocket.MessageText, []byte("pong")) + requestLgr := RequestLoggerFromContext(r.Context()) + requestLgr.WriteLog(r.Context(), http.StatusSwitchingProtocols) + // Block so we can be sure the end of the middleware isn't being called. + wg.Wait() }) // Wrap the test handler with the Logger middleware @@ -130,6 +129,7 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { // RequestLogger expects the ResponseWriter to be *tracing.StatusWriter customHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + defer close(done) sw := &tracing.StatusWriter{ResponseWriter: rw} wrappedHandler.ServeHTTP(sw, r) }) @@ -139,22 +139,34 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { wg.Add(1) // nolint: bodyclose conn, _, err := websocket.Dial(ctx, srv.URL, nil) - if err != nil { - t.Fatalf("failed to create WebSocket connection: %v", err) - } + require.NoError(t, err, "failed to dial WebSocket") defer conn.Close(websocket.StatusNormalClosure, "") - wg.Wait() - require.Len(t, sink.entries, 1, "log was written twice") - require.Equal(t, sink.entries[0].Message, "GET", "log message should be GET") + // Wait for the log from within the handler + newEntry := testutil.RequireRecvCtx(ctx, t, sink.newEntries) + require.Equal(t, newEntry.Message, "GET") + + // Signal the websocket handler to return + wg.Done() + + // Wait for the request to finish completely and verify we only logged once + _ = testutil.RequireRecvCtx(ctx, t, done) + require.Len(t, sink.entries, 1, "log was written twice") } type fakeSink struct { - entries []slog.SinkEntry + entries []slog.SinkEntry + newEntries chan slog.SinkEntry } func (s *fakeSink) LogEntry(_ context.Context, e slog.SinkEntry) { s.entries = append(s.entries, e) + if s.newEntries != nil { + select { + case s.newEntries <- e: + default: + } + } } func (*fakeSink) Sync() {} From 754cee6edd2cbfa72dedd1ed02f976abc2dcade8 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 8 Apr 2025 07:55:28 +0000 Subject: [PATCH 36/36] refactored the WebSocket test to actually read and close the connection --- coderd/httpmw/logger_internal_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/coderd/httpmw/logger_internal_test.go b/coderd/httpmw/logger_internal_test.go index 5f87e15797d53..d3035e50d98c9 100644 --- a/coderd/httpmw/logger_internal_test.go +++ b/coderd/httpmw/logger_internal_test.go @@ -115,7 +115,7 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { if !assert.NoError(t, err, "failed to accept websocket") { return } - defer conn.Close(websocket.StatusNormalClosure, "") + defer conn.Close(websocket.StatusGoingAway, "") requestLgr := RequestLoggerFromContext(r.Context()) requestLgr.WriteLog(r.Context(), http.StatusSwitchingProtocols) @@ -146,8 +146,10 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) { newEntry := testutil.RequireRecvCtx(ctx, t, sink.newEntries) require.Equal(t, newEntry.Message, "GET") - // Signal the websocket handler to return + // Signal the websocket handler to return (and read to handle the close frame) wg.Done() + _, _, err = conn.Read(ctx) + require.ErrorAs(t, err, &websocket.CloseError{}, "websocket read should fail with close error") // Wait for the request to finish completely and verify we only logged once _ = testutil.RequireRecvCtx(ctx, t, done)