From 646b566fb552ae8624d7ee6dcc7fb42e53e273b2 Mon Sep 17 00:00:00 2001 From: Hao Wu Date: Thu, 20 Jun 2024 16:17:41 +0800 Subject: [PATCH 1/5] feat: Add support for logging with context --- client.go | 60 +++++++++++++++++++++++++++++++++++++++++++++++--- client_test.go | 7 ++++++ 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/client.go b/client.go index efee53c..87f3ab4 100644 --- a/client.go +++ b/client.go @@ -41,7 +41,7 @@ import ( "sync" "time" - cleanhttp "github.com/hashicorp/go-cleanhttp" + "github.com/hashicorp/go-cleanhttp" ) var ( @@ -364,6 +364,48 @@ func (h hookLogger) Printf(s string, args ...interface{}) { h.Info(fmt.Sprintf(s, args...)) } +// ContextLogger is an interface that provides methods for logging +// with context. The methods accept a context.Context, a message +// string and a variadic number of key-value pairs. +type ContextLogger interface { + ErrorContext(ctx context.Context, msg string, keysAndValues ...interface{}) + InfoContext(ctx context.Context, msg string, keysAndValues ...interface{}) + DebugContext(ctx context.Context, msg string, keysAndValues ...interface{}) + WarnContext(ctx context.Context, msg string, keysAndValues ...interface{}) +} + +// contextLogger adapts an ContextLogger to Logger for use by the existing hook functions +// without changing the API. +type contextLogger struct { + logger ContextLogger +} + +// NewContextLogger returns a new contextLogger +// for example: NewContextLogger(slog.Default()) +func NewContextLogger(logger ContextLogger) ContextLogger { + return &contextLogger{logger: logger} +} + +func (s contextLogger) ErrorContext(ctx context.Context, msg string, keysAndValues ...interface{}) { + s.logger.ErrorContext(ctx, msg, keysAndValues...) +} + +func (s contextLogger) InfoContext(ctx context.Context, msg string, keysAndValues ...interface{}) { + s.logger.InfoContext(ctx, msg, keysAndValues...) +} + +func (s contextLogger) DebugContext(ctx context.Context, msg string, keysAndValues ...interface{}) { + s.logger.DebugContext(ctx, msg, keysAndValues...) +} + +func (s contextLogger) WarnContext(ctx context.Context, msg string, keysAndValues ...interface{}) { + s.logger.WarnContext(ctx, msg, keysAndValues...) +} + +func (s contextLogger) Printf(msg string, keysAndValues ...interface{}) { + s.logger.InfoContext(context.Background(), msg, keysAndValues...) +} + // RequestLogHook allows a function to run before each retry. The HTTP // request which will be made, and the retry number (0 for the initial // request) are available to users. The internal logger is exposed to @@ -456,11 +498,11 @@ func (c *Client) logger() interface{} { } switch c.Logger.(type) { - case Logger, LeveledLogger: + case Logger, LeveledLogger, ContextLogger: // ok default: // This should happen in dev when they are setting Logger and work on code, not in prod. - panic(fmt.Sprintf("invalid logger type passed, must be Logger or LeveledLogger, was %T", c.Logger)) + panic(fmt.Sprintf("invalid logger type passed, must in Logger, LeveledLogger, ContextLogger, was %T", c.Logger)) } }) @@ -657,6 +699,8 @@ func (c *Client) Do(req *Request) (*http.Response, error) { if logger != nil { switch v := logger.(type) { + case ContextLogger: + v.DebugContext(req.Context(), "performing request", "method", req.Method, "url", redactURL(req.URL)) case LeveledLogger: v.Debug("performing request", "method", req.Method, "url", redactURL(req.URL)) case Logger: @@ -689,6 +733,8 @@ func (c *Client) Do(req *Request) (*http.Response, error) { if c.RequestLogHook != nil { switch v := logger.(type) { + case ContextLogger: + c.RequestLogHook(contextLogger{v}, req.Request, i) case LeveledLogger: c.RequestLogHook(hookLogger{v}, req.Request, i) case Logger: @@ -714,6 +760,8 @@ func (c *Client) Do(req *Request) (*http.Response, error) { } if err != nil { switch v := logger.(type) { + case ContextLogger: + v.ErrorContext(req.Context(), "request failed", "error", err, "method", req.Method, "url", redactURL(req.URL)) case LeveledLogger: v.Error("request failed", "error", err, "method", req.Method, "url", redactURL(req.URL)) case Logger: @@ -725,6 +773,8 @@ func (c *Client) Do(req *Request) (*http.Response, error) { if c.ResponseLogHook != nil { // Call the response logger function if provided. switch v := logger.(type) { + case ContextLogger: + c.ResponseLogHook(nil, resp) case LeveledLogger: c.ResponseLogHook(hookLogger{v}, resp) case Logger: @@ -758,6 +808,8 @@ func (c *Client) Do(req *Request) (*http.Response, error) { desc = fmt.Sprintf("%s (status: %d)", desc, resp.StatusCode) } switch v := logger.(type) { + case ContextLogger: + v.DebugContext(req.Context(), "retrying request", "request", desc, "timeout", wait, "remaining", remain) case LeveledLogger: v.Debug("retrying request", "request", desc, "timeout", wait, "remaining", remain) case Logger: @@ -832,6 +884,8 @@ func (c *Client) drainBody(body io.ReadCloser) { if err != nil { if c.logger() != nil { switch v := c.logger().(type) { + case ContextLogger: + v.ErrorContext(context.Background(), "error reading response body", "error", err) case LeveledLogger: v.Error("error reading response body", "error", err) case Logger: diff --git a/client_test.go b/client_test.go index cc05e91..a8eab65 100644 --- a/client_test.go +++ b/client_test.go @@ -570,6 +570,9 @@ func TestClient_RequestLogHook(t *testing.T) { t.Run("RequestLogHook successfully called with nil typed LeveledLogger", func(t *testing.T) { testClientRequestLogHook(t, LeveledLogger(nil)) }) + t.Run("RequestLogHook successfully called with nil typed ContextLogger", func(t *testing.T) { + testClientRequestLogHook(t, ContextLogger(nil)) + }) } func testClientRequestLogHook(t *testing.T, logger interface{}) { @@ -639,6 +642,10 @@ func TestClient_ResponseLogHook(t *testing.T) { buf := new(bytes.Buffer) testClientResponseLogHook(t, LeveledLogger(nil), buf) }) + t.Run("ResponseLogHook successfully called with nil typed ContextLogger", func(t *testing.T) { + buf := new(bytes.Buffer) + testClientResponseLogHook(t, ContextLogger(nil), buf) + }) } func testClientResponseLogHook(t *testing.T, l interface{}, buf *bytes.Buffer) { From 11b5c248598dbc8445b8bb9f86aff81030716464 Mon Sep 17 00:00:00 2001 From: Hao Wu Date: Thu, 1 Aug 2024 16:24:42 +0800 Subject: [PATCH 2/5] perf: Pass contextLogger as a parameter to ResponseLogHook --- client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client.go b/client.go index 87f3ab4..7fd7dec 100644 --- a/client.go +++ b/client.go @@ -774,7 +774,7 @@ func (c *Client) Do(req *Request) (*http.Response, error) { // Call the response logger function if provided. switch v := logger.(type) { case ContextLogger: - c.ResponseLogHook(nil, resp) + c.ResponseLogHook(contextLogger{v}, resp) case LeveledLogger: c.ResponseLogHook(hookLogger{v}, resp) case Logger: From 112f585650fc8e3470b1bb0b62f6f003bdb49b5f Mon Sep 17 00:00:00 2001 From: Hao Wu Date: Thu, 1 Aug 2024 16:46:39 +0800 Subject: [PATCH 3/5] perf: Add context as a parameter to drainBody, add context fields for contextLogger --- client.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/client.go b/client.go index 7fd7dec..16da5ad 100644 --- a/client.go +++ b/client.go @@ -377,6 +377,7 @@ type ContextLogger interface { // contextLogger adapts an ContextLogger to Logger for use by the existing hook functions // without changing the API. type contextLogger struct { + ctx context.Context logger ContextLogger } @@ -403,7 +404,7 @@ func (s contextLogger) WarnContext(ctx context.Context, msg string, keysAndValue } func (s contextLogger) Printf(msg string, keysAndValues ...interface{}) { - s.logger.InfoContext(context.Background(), msg, keysAndValues...) + s.logger.InfoContext(s.ctx, msg, keysAndValues...) } // RequestLogHook allows a function to run before each retry. The HTTP @@ -734,7 +735,7 @@ func (c *Client) Do(req *Request) (*http.Response, error) { if c.RequestLogHook != nil { switch v := logger.(type) { case ContextLogger: - c.RequestLogHook(contextLogger{v}, req.Request, i) + c.RequestLogHook(contextLogger{req.Context(), v}, req.Request, i) case LeveledLogger: c.RequestLogHook(hookLogger{v}, req.Request, i) case Logger: @@ -774,7 +775,7 @@ func (c *Client) Do(req *Request) (*http.Response, error) { // Call the response logger function if provided. switch v := logger.(type) { case ContextLogger: - c.ResponseLogHook(contextLogger{v}, resp) + c.ResponseLogHook(contextLogger{req.Context(), v}, resp) case LeveledLogger: c.ResponseLogHook(hookLogger{v}, resp) case Logger: @@ -798,7 +799,7 @@ func (c *Client) Do(req *Request) (*http.Response, error) { // We're going to retry, consume any response to reuse the connection. if doErr == nil { - c.drainBody(resp.Body) + c.drainBody(req.Context(), resp.Body) } wait := c.Backoff(c.RetryWaitMin, c.RetryWaitMax, i, resp) @@ -863,7 +864,7 @@ func (c *Client) Do(req *Request) (*http.Response, error) { // By default, we close the response body and return an error without // returning the response if resp != nil { - c.drainBody(resp.Body) + c.drainBody(req.Context(), resp.Body) } // this means CheckRetry thought the request was a failure, but didn't @@ -878,14 +879,14 @@ func (c *Client) Do(req *Request) (*http.Response, error) { } // Try to read the response body so we can reuse this connection. -func (c *Client) drainBody(body io.ReadCloser) { +func (c *Client) drainBody(ctx context.Context, body io.ReadCloser) { defer body.Close() _, err := io.Copy(io.Discard, io.LimitReader(body, respReadLimit)) if err != nil { if c.logger() != nil { switch v := c.logger().(type) { case ContextLogger: - v.ErrorContext(context.Background(), "error reading response body", "error", err) + v.ErrorContext(ctx, "error reading response body", "error", err) case LeveledLogger: v.Error("error reading response body", "error", err) case Logger: From 041cd5353fbf9f0df99b783c095db08730fb00cb Mon Sep 17 00:00:00 2001 From: Hao Wu Date: Thu, 1 Aug 2024 16:57:32 +0800 Subject: [PATCH 4/5] perf: Embed ContextLogger and Context to contextLogger, remove NewContextLogger --- client.go | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/client.go b/client.go index 16da5ad..39f86d7 100644 --- a/client.go +++ b/client.go @@ -377,34 +377,28 @@ type ContextLogger interface { // contextLogger adapts an ContextLogger to Logger for use by the existing hook functions // without changing the API. type contextLogger struct { - ctx context.Context - logger ContextLogger -} - -// NewContextLogger returns a new contextLogger -// for example: NewContextLogger(slog.Default()) -func NewContextLogger(logger ContextLogger) ContextLogger { - return &contextLogger{logger: logger} + context.Context + ContextLogger } func (s contextLogger) ErrorContext(ctx context.Context, msg string, keysAndValues ...interface{}) { - s.logger.ErrorContext(ctx, msg, keysAndValues...) + s.ErrorContext(ctx, msg, keysAndValues...) } func (s contextLogger) InfoContext(ctx context.Context, msg string, keysAndValues ...interface{}) { - s.logger.InfoContext(ctx, msg, keysAndValues...) + s.InfoContext(ctx, msg, keysAndValues...) } func (s contextLogger) DebugContext(ctx context.Context, msg string, keysAndValues ...interface{}) { - s.logger.DebugContext(ctx, msg, keysAndValues...) + s.DebugContext(ctx, msg, keysAndValues...) } func (s contextLogger) WarnContext(ctx context.Context, msg string, keysAndValues ...interface{}) { - s.logger.WarnContext(ctx, msg, keysAndValues...) + s.WarnContext(ctx, msg, keysAndValues...) } func (s contextLogger) Printf(msg string, keysAndValues ...interface{}) { - s.logger.InfoContext(s.ctx, msg, keysAndValues...) + s.InfoContext(s.Context, msg, keysAndValues...) } // RequestLogHook allows a function to run before each retry. The HTTP From 774d8a445b3dc8e35773378f181c866619e81087 Mon Sep 17 00:00:00 2001 From: Hao Wu Date: Fri, 25 Oct 2024 11:41:42 +0800 Subject: [PATCH 5/5] chore: Improve comment for Logger interface to clarify supported types --- client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client.go b/client.go index 39f86d7..1821f49 100644 --- a/client.go +++ b/client.go @@ -442,7 +442,7 @@ type PrepareRetry func(req *http.Request) error // like automatic retries to tolerate minor outages. type Client struct { HTTPClient *http.Client // Internal HTTP client. - Logger interface{} // Customer logger instance. Can be either Logger or LeveledLogger + Logger interface{} // Customer logger instance. Supports Logger, LeveledLogger, or ContextLogger. RetryWaitMin time.Duration // Minimum time to wait RetryWaitMax time.Duration // Maximum time to wait