From 4b5086695057999860d8511e5baf2eb832ca7301 Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Thu, 19 Dec 2024 09:11:20 +0000 Subject: [PATCH 01/16] HTTP client round trippers logging, metrics and request id --- httpclient/config.go | 161 ++++++++++++++++++++ httpclient/config_test.go | 45 ++++++ httpclient/httpclient.go | 32 +++- httpclient/httpclient_test.go | 39 +++++ httpclient/logging_round_tripper.go | 58 +++++++ httpclient/logging_round_tripper_test.go | 64 ++++++++ httpclient/metrics_round_tripper.go | 98 ++++++++++++ httpclient/metrics_round_tripper_test.go | 48 ++++++ httpclient/request_id_round_tripper.go | 36 +++++ httpclient/request_id_round_tripper_test.go | 36 +++++ 10 files changed, 616 insertions(+), 1 deletion(-) create mode 100644 httpclient/config.go create mode 100644 httpclient/config_test.go create mode 100644 httpclient/httpclient_test.go create mode 100644 httpclient/logging_round_tripper.go create mode 100644 httpclient/logging_round_tripper_test.go create mode 100644 httpclient/metrics_round_tripper.go create mode 100644 httpclient/metrics_round_tripper_test.go create mode 100644 httpclient/request_id_round_tripper.go create mode 100644 httpclient/request_id_round_tripper_test.go diff --git a/httpclient/config.go b/httpclient/config.go new file mode 100644 index 0000000..4d7717d --- /dev/null +++ b/httpclient/config.go @@ -0,0 +1,161 @@ +/* +Copyright © 2024 Acronis International GmbH. + +Released under MIT license. +*/ + +package httpclient + +import ( + "github.com/acronis/go-appkit/config" + "time" +) + +const ( + // DefaultLimit is the default limit for rate limiting. + DefaultLimit = 10 + + // DefaultBurst is the default burst for rate limiting. + DefaultBurst = 100 + + // DefaultWaitTimeout is the default wait timeout for rate limiting. + DefaultWaitTimeout = 10 * time.Second + + // configuration properties + cfgKeyRetriesMax = "retries.max_retry_attempts" + cfgKeyRateLimitsLimit = "rate_limits.limit" + cfgKeyRateLimitsBurst = "rate_limits.burst" + cfgKeyRateLimitsWaitTimeout = "rate_limits.wait_timeout" + cfgKeyTimeout = "timeout" +) + +// RateLimitConfig represents configuration options for HTTP client rate limits. +type RateLimitConfig struct { + Limit int `mapstructure:"limit"` + Burst int `mapstructure:"burst"` + WaitTimeout time.Duration `mapstructure:"wait_timeout"` +} + +// Set is part of config interface implementation. +func (c *RateLimitConfig) Set(dp config.DataProvider) (err error) { + limit, err := dp.GetInt(cfgKeyRateLimitsLimit) + if err != nil { + return err + } + c.Limit = limit + + burst, err := dp.GetInt(cfgKeyRateLimitsBurst) + if err != nil { + return nil + } + c.Burst = burst + + waitTimeout, err := dp.GetDuration(cfgKeyRateLimitsWaitTimeout) + if err != nil { + return err + } + c.WaitTimeout = waitTimeout + + return nil +} + +// SetProviderDefaults is part of config interface implementation. +func (c *RateLimitConfig) SetProviderDefaults(dp config.DataProvider) { + dp.SetDefault(cfgKeyRateLimitsLimit, DefaultLimit) + dp.SetDefault(cfgKeyRateLimitsBurst, DefaultBurst) + dp.SetDefault(cfgKeyRateLimitsWaitTimeout, DefaultWaitTimeout) +} + +// TransportOpts returns transport options. +func (c *RateLimitConfig) TransportOpts() RateLimitingRoundTripperOpts { + return RateLimitingRoundTripperOpts{ + Burst: c.Burst, + WaitTimeout: c.WaitTimeout, + } +} + +// RetriesConfig represents configuration options for HTTP client retries policy. +type RetriesConfig struct { + MaxRetryAttempts int `mapstructure:"max_retry_attempts"` +} + +// Set is part of config interface implementation. +func (c *RetriesConfig) Set(dp config.DataProvider) error { + maxRetryAttempts, err := dp.GetInt(cfgKeyRetriesMax) + if err != nil { + return err + } + c.MaxRetryAttempts = maxRetryAttempts + + return nil +} + +// SetProviderDefaults is part of config interface implementation. +func (c *RetriesConfig) SetProviderDefaults(dp config.DataProvider) { + dp.SetDefault(cfgKeyRetriesMax, DefaultMaxRetryAttempts) +} + +// TransportOpts returns transport options. +func (c *RetriesConfig) TransportOpts() RetryableRoundTripperOpts { + return RetryableRoundTripperOpts{MaxRetryAttempts: c.MaxRetryAttempts} +} + +// Config represents options for HTTP client configuration. +type Config struct { + Retries *RetriesConfig `mapstructure:"retries"` + RateLimits *RateLimitConfig `mapstructure:"rate_limits"` + Timeout time.Duration `mapstructure:"timeout"` +} + +// Set is part of config interface implementation. +func (c *Config) Set(dp config.DataProvider) error { + timeout, err := dp.GetDuration(cfgKeyTimeout) + if err != nil { + return err + } + c.Timeout = timeout + + err = c.Retries.Set(config.NewKeyPrefixedDataProvider(dp, "")) + if err != nil { + return err + } + + err = c.RateLimits.Set(config.NewKeyPrefixedDataProvider(dp, "")) + if err != nil { + return err + } + + return nil +} + +// SetProviderDefaults is part of config interface implementation. +func (c *Config) SetProviderDefaults(dp config.DataProvider) { + if c.Retries == nil { + c.Retries = &RetriesConfig{} + } + c.Retries.SetProviderDefaults(dp) + + if c.RateLimits == nil { + c.RateLimits = &RateLimitConfig{} + } + c.RateLimits.SetProviderDefaults(dp) + + if c.Timeout == 0 { + c.Timeout = DefaultWaitTimeout + } +} + +// NewHTTPClientConfig is unified configuration format for HTTP clients. +func NewHTTPClientConfig() *Config { + return &Config{ + Retries: &RetriesConfig{ + MaxRetryAttempts: DefaultMaxRetryAttempts, + }, + RateLimits: &RateLimitConfig{ + Limit: DefaultLimit, + Burst: DefaultBurst, + WaitTimeout: DefaultWaitTimeout, + }, + Timeout: DefaultWaitTimeout, + } +} diff --git a/httpclient/config_test.go b/httpclient/config_test.go new file mode 100644 index 0000000..4513468 --- /dev/null +++ b/httpclient/config_test.go @@ -0,0 +1,45 @@ +/* +Copyright © 2024 Acronis International GmbH. + +Released under MIT license. +*/ + +package httpclient + +import ( + "bytes" + "github.com/acronis/go-appkit/config" + "github.com/stretchr/testify/require" + "testing" + "time" +) + +func TestConfigWithLoader(t *testing.T) { + yamlData := []byte(` +retries: + max_retry_attempts: 30 +rate_limits: + limit: 300 + burst: 3000 + wait_timeout: 3s +timeout: 30s +`) + + actualConfig := &Config{} + err := config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.NoError(t, err, "load configuration") + + expectedConfig := &Config{ + Retries: &RetriesConfig{ + MaxRetryAttempts: 30, + }, + RateLimits: &RateLimitConfig{ + Limit: 300, + Burst: 3000, + WaitTimeout: 3 * time.Second, + }, + Timeout: 30 * time.Second, + } + + require.Equal(t, expectedConfig, actualConfig, "configuration does not match expected") +} diff --git a/httpclient/httpclient.go b/httpclient/httpclient.go index b293d7f..5aba447 100644 --- a/httpclient/httpclient.go +++ b/httpclient/httpclient.go @@ -6,7 +6,10 @@ Released under MIT license. package httpclient -import "net/http" +import ( + "fmt" + "net/http" +) // CloneHTTPRequest creates a shallow copy of the request along with a deep copy of the Headers. func CloneHTTPRequest(req *http.Request) *http.Request { @@ -26,3 +29,30 @@ func CloneHTTPHeader(in http.Header) http.Header { } return out } + +// MustHTTPClient wraps delegate transports with logging, metrics, rate limiting, retryable, user agent, request id +// and panics if any error occurs. +func MustHTTPClient(cfg *Config, userAgent, reqType string, delegate http.RoundTripper) *http.Client { + if delegate == nil { + delegate = http.DefaultTransport + } + + delegate = NewLoggingRoundTripper(delegate, reqType) + delegate = NewMetricsRoundTripper(delegate, reqType) + + delegate, err := NewRateLimitingRoundTripperWithOpts( + delegate, cfg.RateLimits.Limit, cfg.RateLimits.TransportOpts(), + ) + if err != nil { + panic(fmt.Errorf("create rate limiting round tripper: %w", err)) + } + + delegate, err = NewRetryableRoundTripperWithOpts(delegate, cfg.Retries.TransportOpts()) + if err != nil { + panic(fmt.Errorf("create retryable round tripper: %w", err)) + } + + delegate = NewUserAgentRoundTripper(delegate, userAgent) + delegate = NewRequestIDRoundTripper(delegate) + return &http.Client{Transport: delegate, Timeout: cfg.Timeout} +} diff --git a/httpclient/httpclient_test.go b/httpclient/httpclient_test.go new file mode 100644 index 0000000..3667efb --- /dev/null +++ b/httpclient/httpclient_test.go @@ -0,0 +1,39 @@ +/* +Copyright © 2024 Acronis International GmbH. + +Released under MIT license. +*/ + +package httpclient + +import ( + "context" + "github.com/acronis/go-appkit/httpserver/middleware" + "github.com/acronis/go-appkit/log/logtest" + "github.com/stretchr/testify/require" + "net/http" + "net/http/httptest" + "testing" +) + +func TestMustHTTPClientLoggingRoundTripper(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusTeapot) + })) + defer server.Close() + + logger := logtest.NewRecorder() + cfg := NewHTTPClientConfig() + client := MustHTTPClient(cfg, "test-agent", "test-request", nil) + ctx := middleware.NewContextWithLogger(context.Background(), logger) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + defer func() { _ = r.Body.Close() }() + require.NoError(t, err) + require.NotEmpty(t, logger.Entries()) + + loggerEntry := logger.Entries()[0] + require.Contains(t, loggerEntry.Text, "external request POST "+server.URL+" req type test-request status code 418") +} diff --git a/httpclient/logging_round_tripper.go b/httpclient/logging_round_tripper.go new file mode 100644 index 0000000..1b6c248 --- /dev/null +++ b/httpclient/logging_round_tripper.go @@ -0,0 +1,58 @@ +/* +Copyright © 2024 Acronis International GmbH. + +Released under MIT license. +*/ + +package httpclient + +import ( + "fmt" + "github.com/acronis/go-appkit/httpserver/middleware" + "net/http" + "time" +) + +// LoggingRoundTripper implements http.RoundTripper for logging requests. +type LoggingRoundTripper struct { + Delegate http.RoundTripper + ReqType string +} + +// NewLoggingRoundTripper creates an HTTP transport that log requests. +func NewLoggingRoundTripper(delegate http.RoundTripper, reqType string) http.RoundTripper { + return &LoggingRoundTripper{ + Delegate: delegate, + ReqType: reqType, + } +} + +// RoundTrip adds logging capabilities to the HTTP transport. +func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { + ctx := r.Context() + logger := middleware.GetLoggerFromContext(ctx) + start := time.Now() + + resp, err := rt.Delegate.RoundTrip(r) + if logger != nil { + common := "external request %s %s req type %s " + elapsed := time.Since(start) + + args := []interface{}{r.Method, r.URL.String(), rt.ReqType, elapsed.Seconds(), err} + message := common + "time taken %.3f, err %+v" + + if resp != nil { + args = []interface{}{r.Method, r.URL.String(), rt.ReqType, resp.StatusCode, elapsed.Seconds(), err} + message = common + "status code %d, time taken %.3f, err %+v" + } + + logger.Infof(message, args...) + + loggingParams := middleware.GetLoggingParamsFromContext(ctx) + if loggingParams != nil { + loggingParams.AddTimeSlotDurationInMs(fmt.Sprintf("external_request_%s_ms", rt.ReqType), elapsed) + } + } + + return resp, err +} diff --git a/httpclient/logging_round_tripper_test.go b/httpclient/logging_round_tripper_test.go new file mode 100644 index 0000000..03f876d --- /dev/null +++ b/httpclient/logging_round_tripper_test.go @@ -0,0 +1,64 @@ +/* +Copyright © 2024 Acronis International GmbH. + +Released under MIT license. +*/ + +package httpclient + +import ( + "context" + "github.com/acronis/go-appkit/httpserver/middleware" + "github.com/acronis/go-appkit/log/logtest" + "github.com/stretchr/testify/require" + "net" + "net/http" + "net/http/httptest" + "testing" +) + +func TestNewLoggingRoundTripper(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusTeapot) + })) + defer server.Close() + + logger := logtest.NewRecorder() + loggerRoundTripper := NewLoggingRoundTripper(http.DefaultTransport, "test-request") + client := &http.Client{Transport: loggerRoundTripper} + ctx := middleware.NewContextWithLogger(context.Background(), logger) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + defer func() { _ = r.Body.Close() }() + require.NoError(t, err) + require.NotEmpty(t, logger.Entries()) + + loggerEntry := logger.Entries()[0] + require.Contains(t, loggerEntry.Text, "external request POST "+server.URL+" req type test-request status code 418") +} + +func TestMustHTTPClientLoggingRoundTripperError(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + serverURL := "http://" + ln.Addr().String() + _ = ln.Close() + + logger := logtest.NewRecorder() + cfg := NewHTTPClientConfig() + client := MustHTTPClient(cfg, "test-agent", "test-request", nil) + ctx := middleware.NewContextWithLogger(context.Background(), logger) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + require.Error(t, err) + require.Nil(t, r) + require.NotEmpty(t, logger.Entries()) + + loggerEntry := logger.Entries()[0] + require.Contains(t, loggerEntry.Text, "external request POST "+serverURL+" req type test-request") + require.Contains(t, loggerEntry.Text, "err dial tcp "+ln.Addr().String()+": connect: connection refused") + require.NotContains(t, loggerEntry.Text, "status code") +} diff --git a/httpclient/metrics_round_tripper.go b/httpclient/metrics_round_tripper.go new file mode 100644 index 0000000..aa6be60 --- /dev/null +++ b/httpclient/metrics_round_tripper.go @@ -0,0 +1,98 @@ +/* +Copyright © 2024 Acronis International GmbH. + +Released under MIT license. +*/ + +package httpclient + +import ( + "fmt" + "github.com/acronis/go-appkit/httpserver/middleware" + "github.com/acronis/go-appkit/log" + "github.com/prometheus/client_golang/prometheus" + "net/http" + "strconv" + "time" +) + +var ( + ExternalHTTPRequestDuration *prometheus.HistogramVec + ClassifyRequest func(r *http.Request, reqType string, logger log.FieldLogger) string +) + +// MustInitAndRegisterMetrics initializes and registers external HTTP request duration metric. +// Panic will be raised in case of error. +func MustInitAndRegisterMetrics(namespace string) { + ExternalHTTPRequestDuration = prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Namespace: namespace, + Name: "external_http_request_duration", + Help: "external HTTP request duration in seconds", + Buckets: []float64{0.01, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 30, 60, 150, 300, 600}, + }, + []string{"type", "remote_address", "summary", "status"}, + ) + prometheus.MustRegister(ExternalHTTPRequestDuration) +} + +// UnregisterMetrics unregisters external HTTP request duration metric. +func UnregisterMetrics() { + if ExternalHTTPRequestDuration != nil { + prometheus.Unregister(ExternalHTTPRequestDuration) + } +} + +// MetricsRoundTripper is an HTTP transport that measures requests done. +type MetricsRoundTripper struct { + Delegate http.RoundTripper + ReqType string +} + +// NewMetricsRoundTripper creates an HTTP transport that measures requests done. +func NewMetricsRoundTripper(delegate http.RoundTripper, reqType string) http.RoundTripper { + return &MetricsRoundTripper{Delegate: delegate, ReqType: reqType} +} + +// RoundTrip measures external requests done. +func (rt *MetricsRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { + if ExternalHTTPRequestDuration == nil { + return rt.Delegate.RoundTrip(r) + } + + logger := middleware.GetLoggerFromContext(r.Context()) + start := time.Now() + status := "failed" + + resp, err := rt.Delegate.RoundTrip(r) + if err == nil && resp != nil { + status = strconv.Itoa(resp.StatusCode) + if resp.StatusCode >= http.StatusBadRequest && logger != nil { + logger.Warnf("external request %s %s completed with HTTP status %d", r.Method, r.URL.String(), status) + } + } else if err != nil && logger != nil { + logger.Warnf("external request %s %s: %s", r.Method, r.URL.String(), err.Error()) + } + + ExternalHTTPRequestDuration.WithLabelValues( + rt.ReqType, r.Host, requestSummary(r, rt.ReqType, logger), status, + ).Observe(time.Since(start).Seconds()) + + return resp, err +} + +// requestSummary does request classification, producing non-parameterized summary for given request. +func requestSummary(r *http.Request, reqType string, logger log.FieldLogger) string { + if ClassifyRequest != nil { + return ClassifyRequest(r, reqType, logger) + } + + if logger != nil { + logger.Debugf( + "request classifier is not initialized, request %s %s will be marked as req type '%s' in metrics", + r.Method, r.URL.String(), reqType, + ) + } + + return fmt.Sprintf("%s %s", r.Method, reqType) +} diff --git a/httpclient/metrics_round_tripper_test.go b/httpclient/metrics_round_tripper_test.go new file mode 100644 index 0000000..198c3af --- /dev/null +++ b/httpclient/metrics_round_tripper_test.go @@ -0,0 +1,48 @@ +/* +Copyright © 2024 Acronis International GmbH. + +Released under MIT license. +*/ + +package httpclient + +import ( + "context" + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/require" + "net/http" + "net/http/httptest" + "testing" +) + +func TestNewMetricsRoundTripper(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusTeapot) + })) + defer server.Close() + + MustInitAndRegisterMetrics("") + defer UnregisterMetrics() + + metricsRoundTripper := NewMetricsRoundTripper(http.DefaultTransport, "test-request") + client := &http.Client{Transport: metricsRoundTripper} + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, server.URL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + defer func() { _ = r.Body.Close() }() + require.NoError(t, err) + + ch := make(chan prometheus.Metric, 1) + go func() { + ExternalHTTPRequestDuration.Collect(ch) + close(ch) + }() + + var metricCount int + for range ch { + metricCount++ + } + + require.Equal(t, metricCount, 1) +} diff --git a/httpclient/request_id_round_tripper.go b/httpclient/request_id_round_tripper.go new file mode 100644 index 0000000..3c96122 --- /dev/null +++ b/httpclient/request_id_round_tripper.go @@ -0,0 +1,36 @@ +/* +Copyright © 2024 Acronis International GmbH. + +Released under MIT license. +*/ + +package httpclient + +import ( + "github.com/acronis/go-appkit/httpserver/middleware" + "net/http" +) + +// RequestIDRoundTripper for X-Request-ID header to the request. +type RequestIDRoundTripper struct { + Delegate http.RoundTripper +} + +// NewRequestIDRoundTripper creates an HTTP transport with X-Request-ID header support. +func NewRequestIDRoundTripper(delegate http.RoundTripper) http.RoundTripper { + return &RequestIDRoundTripper{ + Delegate: delegate, + } +} + +// RoundTrip adds X-Request-ID header to the request. +func (rt *RequestIDRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { + requestID := middleware.GetRequestIDFromContext(r.Context()) + if r.Header.Get("X-Request-ID") != "" || requestID == "" { + return rt.Delegate.RoundTrip(r) + } + + r = CloneHTTPRequest(r) + r.Header.Set("X-Request-ID", requestID) + return rt.Delegate.RoundTrip(r) +} diff --git a/httpclient/request_id_round_tripper_test.go b/httpclient/request_id_round_tripper_test.go new file mode 100644 index 0000000..9242992 --- /dev/null +++ b/httpclient/request_id_round_tripper_test.go @@ -0,0 +1,36 @@ +/* +Copyright © 2024 Acronis International GmbH. + +Released under MIT license. +*/ + +package httpclient + +import ( + "context" + "github.com/acronis/go-appkit/httpserver/middleware" + "github.com/stretchr/testify/require" + "net/http" + "net/http/httptest" + "testing" +) + +func TestNewRequestIDRoundTripper(t *testing.T) { + requestID := "12345" + + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + require.Equal(t, requestID, r.Header.Get("X-Request-ID")) + rw.WriteHeader(http.StatusTeapot) + })) + defer server.Close() + + requestIDRoundTripper := NewRequestIDRoundTripper(http.DefaultTransport) + client := &http.Client{Transport: requestIDRoundTripper} + ctx := middleware.NewContextWithRequestID(context.Background(), requestID) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + defer func() { _ = r.Body.Close() }() + require.NoError(t, err) +} From 6d7fa55fb7a40978ba7ff542234faf773edcd18c Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Tue, 24 Dec 2024 18:18:14 +0000 Subject: [PATCH 02/16] Client providers, configurations and validations --- httpclient/config.go | 210 ++++++++++++++------ httpclient/config_test.go | 143 ++++++++++++- httpclient/httpclient.go | 104 ++++++++-- httpclient/httpclient_test.go | 83 +++++++- httpclient/logging_round_tripper.go | 88 +++++++- httpclient/logging_round_tripper_test.go | 125 +++++++++++- httpclient/metrics_round_tripper.go | 43 ++-- httpclient/metrics_round_tripper_test.go | 2 +- httpclient/request_id_round_tripper.go | 26 ++- httpclient/request_id_round_tripper_test.go | 24 +++ 10 files changed, 719 insertions(+), 129 deletions(-) diff --git a/httpclient/config.go b/httpclient/config.go index 4d7717d..c62020a 100644 --- a/httpclient/config.go +++ b/httpclient/config.go @@ -7,46 +7,62 @@ Released under MIT license. package httpclient import ( + "errors" "github.com/acronis/go-appkit/config" "time" ) const ( - // DefaultLimit is the default limit for rate limiting. - DefaultLimit = 10 - - // DefaultBurst is the default burst for rate limiting. - DefaultBurst = 100 - - // DefaultWaitTimeout is the default wait timeout for rate limiting. - DefaultWaitTimeout = 10 * time.Second + DefaultClientWaitTimeout = 10 * time.Second // configuration properties - cfgKeyRetriesMax = "retries.max_retry_attempts" - cfgKeyRateLimitsLimit = "rate_limits.limit" - cfgKeyRateLimitsBurst = "rate_limits.burst" - cfgKeyRateLimitsWaitTimeout = "rate_limits.wait_timeout" - cfgKeyTimeout = "timeout" + cfgKeyRetriesEnabled = "retries.enabled" + cfgKeyRetriesMax = "retries.maxAttempts" + cfgKeyRateLimitsEnabled = "rateLimits.enabled" + cfgKeyRateLimitsLimit = "rateLimits.limit" + cfgKeyRateLimitsBurst = "rateLimits.burst" + cfgKeyRateLimitsWaitTimeout = "rateLimits.waitTimeout" + cfgKeyLoggerEnabled = "logger.enabled" + cfgKeyLoggerMode = "logger.mode" + cfgKeyLoggerSlowRequestThreshold = "logger.slowRequestThreshold" + cfgKeyMetricsEnabled = "metrics.enabled" + cfgKeyTimeout = "timeout" ) +var _ config.Config = (*Config)(nil) +var _ config.KeyPrefixProvider = (*Config)(nil) + // RateLimitConfig represents configuration options for HTTP client rate limits. type RateLimitConfig struct { + Enabled bool `mapstructure:"enabled"` Limit int `mapstructure:"limit"` Burst int `mapstructure:"burst"` - WaitTimeout time.Duration `mapstructure:"wait_timeout"` + WaitTimeout time.Duration `mapstructure:"waitTimeout"` } // Set is part of config interface implementation. func (c *RateLimitConfig) Set(dp config.DataProvider) (err error) { + enabled, err := dp.GetBool(cfgKeyRateLimitsEnabled) + if err != nil { + return err + } + c.Enabled = enabled + limit, err := dp.GetInt(cfgKeyRateLimitsLimit) if err != nil { return err } + if limit <= 0 { + return errors.New("client rate limit must be positive") + } c.Limit = limit burst, err := dp.GetInt(cfgKeyRateLimitsBurst) if err != nil { - return nil + return err + } + if burst < 0 { + return errors.New("client burst must be positive") } c.Burst = burst @@ -54,17 +70,16 @@ func (c *RateLimitConfig) Set(dp config.DataProvider) (err error) { if err != nil { return err } + if waitTimeout < 0 { + return errors.New("client wait timeout must be positive") + } c.WaitTimeout = waitTimeout return nil } // SetProviderDefaults is part of config interface implementation. -func (c *RateLimitConfig) SetProviderDefaults(dp config.DataProvider) { - dp.SetDefault(cfgKeyRateLimitsLimit, DefaultLimit) - dp.SetDefault(cfgKeyRateLimitsBurst, DefaultBurst) - dp.SetDefault(cfgKeyRateLimitsWaitTimeout, DefaultWaitTimeout) -} +func (c *RateLimitConfig) SetProviderDefaults(_ config.DataProvider) {} // TransportOpts returns transport options. func (c *RateLimitConfig) TransportOpts() RateLimitingRoundTripperOpts { @@ -76,86 +91,163 @@ func (c *RateLimitConfig) TransportOpts() RateLimitingRoundTripperOpts { // RetriesConfig represents configuration options for HTTP client retries policy. type RetriesConfig struct { - MaxRetryAttempts int `mapstructure:"max_retry_attempts"` + Enabled bool `mapstructure:"enabled"` + MaxAttempts int `mapstructure:"maxAttempts"` } // Set is part of config interface implementation. func (c *RetriesConfig) Set(dp config.DataProvider) error { - maxRetryAttempts, err := dp.GetInt(cfgKeyRetriesMax) + enabled, err := dp.GetBool(cfgKeyRetriesEnabled) if err != nil { return err } - c.MaxRetryAttempts = maxRetryAttempts + c.Enabled = enabled + + maxAttempts, err := dp.GetInt(cfgKeyRetriesMax) + if err != nil { + return err + } + if maxAttempts < 0 { + return errors.New("client max retry attempts must be positive") + } + c.MaxAttempts = maxAttempts return nil } // SetProviderDefaults is part of config interface implementation. -func (c *RetriesConfig) SetProviderDefaults(dp config.DataProvider) { - dp.SetDefault(cfgKeyRetriesMax, DefaultMaxRetryAttempts) -} +func (c *RetriesConfig) SetProviderDefaults(_ config.DataProvider) {} // TransportOpts returns transport options. func (c *RetriesConfig) TransportOpts() RetryableRoundTripperOpts { - return RetryableRoundTripperOpts{MaxRetryAttempts: c.MaxRetryAttempts} + return RetryableRoundTripperOpts{MaxRetryAttempts: c.MaxAttempts} } -// Config represents options for HTTP client configuration. -type Config struct { - Retries *RetriesConfig `mapstructure:"retries"` - RateLimits *RateLimitConfig `mapstructure:"rate_limits"` - Timeout time.Duration `mapstructure:"timeout"` +// LoggerConfig represents configuration options for HTTP client logs. +type LoggerConfig struct { + Enabled bool `mapstructure:"enabled"` + SlowRequestThreshold time.Duration `mapstructure:"slowRequestThreshold"` + Mode string `mapstructure:"mode"` } // Set is part of config interface implementation. -func (c *Config) Set(dp config.DataProvider) error { - timeout, err := dp.GetDuration(cfgKeyTimeout) +func (c *LoggerConfig) Set(dp config.DataProvider) error { + enabled, err := dp.GetBool(cfgKeyLoggerEnabled) if err != nil { return err } - c.Timeout = timeout + c.Enabled = enabled - err = c.Retries.Set(config.NewKeyPrefixedDataProvider(dp, "")) + slowRequestThreshold, err := dp.GetDuration(cfgKeyLoggerSlowRequestThreshold) if err != nil { return err } + if slowRequestThreshold < 0 { + return errors.New("client logger slow request threshold can not be negative") + } + c.SlowRequestThreshold = slowRequestThreshold - err = c.RateLimits.Set(config.NewKeyPrefixedDataProvider(dp, "")) + mode, err := dp.GetString(cfgKeyLoggerMode) if err != nil { return err } + if !LoggerMode(mode).IsValid() { + return errors.New("client logger invalid mode, choose one of: [none, all, failed]") + } + c.Mode = mode return nil } // SetProviderDefaults is part of config interface implementation. -func (c *Config) SetProviderDefaults(dp config.DataProvider) { - if c.Retries == nil { - c.Retries = &RetriesConfig{} - } - c.Retries.SetProviderDefaults(dp) +func (c *LoggerConfig) SetProviderDefaults(_ config.DataProvider) {} - if c.RateLimits == nil { - c.RateLimits = &RateLimitConfig{} +// TransportOpts returns transport options. +func (c *LoggerConfig) TransportOpts() LoggingRoundTripperOpts { + return LoggingRoundTripperOpts{ + Mode: c.Mode, + SlowRequestThreshold: c.SlowRequestThreshold, } - c.RateLimits.SetProviderDefaults(dp) +} - if c.Timeout == 0 { - c.Timeout = DefaultWaitTimeout +// MetricsConfig represents configuration options for HTTP client logs. +type MetricsConfig struct { + Enabled bool `mapstructure:"enabled"` +} + +// Set is part of config interface implementation. +func (c *MetricsConfig) Set(dp config.DataProvider) error { + enabled, err := dp.GetBool(cfgKeyMetricsEnabled) + if err != nil { + return err } + c.Enabled = enabled + + return nil } -// NewHTTPClientConfig is unified configuration format for HTTP clients. -func NewHTTPClientConfig() *Config { - return &Config{ - Retries: &RetriesConfig{ - MaxRetryAttempts: DefaultMaxRetryAttempts, - }, - RateLimits: &RateLimitConfig{ - Limit: DefaultLimit, - Burst: DefaultBurst, - WaitTimeout: DefaultWaitTimeout, - }, - Timeout: DefaultWaitTimeout, +// SetProviderDefaults is part of config interface implementation. +func (c *MetricsConfig) SetProviderDefaults(_ config.DataProvider) {} + +// Config represents options for HTTP client configuration. +type Config struct { + Retries RetriesConfig `mapstructure:"retries"` + RateLimits RateLimitConfig `mapstructure:"rateLimits"` + Logger LoggerConfig `mapstructure:"logger"` + Metrics MetricsConfig `mapstructure:"metrics"` + Timeout time.Duration `mapstructure:"timeout"` + + keyPrefix string +} + +// NewConfig creates a new instance of the Config. +func NewConfig() *Config { + return NewConfigWithKeyPrefix("") +} + +// NewConfigWithKeyPrefix creates a new instance of the Config. +// Allows specifying key prefix which will be used for parsing configuration parameters. +func NewConfigWithKeyPrefix(keyPrefix string) *Config { + return &Config{keyPrefix: keyPrefix} +} + +// KeyPrefix returns a key prefix with which all configuration parameters should be presented. +func (c *Config) KeyPrefix() string { + return c.keyPrefix +} + +// Set is part of config interface implementation. +func (c *Config) Set(dp config.DataProvider) error { + timeout, err := dp.GetDuration(cfgKeyTimeout) + if err != nil { + return err } + c.Timeout = timeout + + err = c.Retries.Set(config.NewKeyPrefixedDataProvider(dp, c.keyPrefix)) + if err != nil { + return err + } + + err = c.RateLimits.Set(config.NewKeyPrefixedDataProvider(dp, c.keyPrefix)) + if err != nil { + return err + } + + err = c.Logger.Set(config.NewKeyPrefixedDataProvider(dp, c.keyPrefix)) + if err != nil { + return err + } + + err = c.Metrics.Set(config.NewKeyPrefixedDataProvider(dp, c.keyPrefix)) + if err != nil { + return err + } + + return nil +} + +// SetProviderDefaults is part of config interface implementation. +func (c *Config) SetProviderDefaults(_ config.DataProvider) { + c.Timeout = DefaultClientWaitTimeout } diff --git a/httpclient/config_test.go b/httpclient/config_test.go index 4513468..1cf9a8e 100644 --- a/httpclient/config_test.go +++ b/httpclient/config_test.go @@ -17,11 +17,19 @@ import ( func TestConfigWithLoader(t *testing.T) { yamlData := []byte(` retries: - max_retry_attempts: 30 -rate_limits: + enabled: true + maxAttempts: 30 +rateLimits: + enabled: true limit: 300 burst: 3000 - wait_timeout: 3s + waitTimeout: 3s +logger: + enabled: true + slowRequestThreshold: 5s + mode: all +metrics: + enabled: true timeout: 30s `) @@ -30,16 +38,139 @@ timeout: 30s require.NoError(t, err, "load configuration") expectedConfig := &Config{ - Retries: &RetriesConfig{ - MaxRetryAttempts: 30, + Retries: RetriesConfig{ + Enabled: true, + MaxAttempts: 30, }, - RateLimits: &RateLimitConfig{ + RateLimits: RateLimitConfig{ + Enabled: true, Limit: 300, Burst: 3000, WaitTimeout: 3 * time.Second, }, + Logger: LoggerConfig{ + Enabled: true, + SlowRequestThreshold: 5 * time.Second, + Mode: "all", + }, + Metrics: MetricsConfig{Enabled: true}, Timeout: 30 * time.Second, } require.Equal(t, expectedConfig, actualConfig, "configuration does not match expected") } + +func TestConfigRateLimitInvalid(t *testing.T) { + yamlData := []byte(` +retries: + enabled: true + maxAttempts: 30 +rateLimits: + enabled: true + limit: -300 + burst: 3000 + waitTimeout: 3s +timeout: 30s +`) + + actualConfig := &Config{} + err := config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.Error(t, err) + require.Equal(t, "client rate limit must be positive", err.Error()) + + yamlData = []byte(` +retries: + enabled: true + maxAttempts: 30 +rateLimits: + enabled: true + limit: 300 + burst: -3 + waitTimeout: 3s +timeout: 30s +`) + + actualConfig = &Config{} + err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.Error(t, err) + require.Equal(t, "client burst must be positive", err.Error()) + + yamlData = []byte(` +retries: + enabled: true + maxAttempts: 30 +rateLimits: + enabled: true + limit: 300 + burst: 3 + waitTimeout: -3s +timeout: 30s +`) + + actualConfig = &Config{} + err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.Error(t, err) + require.Equal(t, "client wait timeout must be positive", err.Error()) +} + +func TestConfigRetriesInvalid(t *testing.T) { + yamlData := []byte(` +retries: + enabled: true + maxAttempts: -30 +timeout: 30s +`) + + actualConfig := &Config{} + err := config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.Error(t, err) + require.Equal(t, "client max retry attempts must be positive", err.Error()) +} + +func TestConfigLoggerInvalid(t *testing.T) { + yamlData := []byte(` +retries: + enabled: true + maxAttempts: 30 +rateLimits: + enabled: true + limit: 300 + burst: 3000 + waitTimeout: 3s +logger: + enabled: true + slowRequestThreshold: -5s + mode: all +metrics: + enabled: true +timeout: 30s +`) + + actualConfig := &Config{} + err := config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.Error(t, err) + require.Equal(t, "client logger slow request threshold can not be negative", err.Error()) + + yamlData = []byte(` +retries: + enabled: true + maxAttempts: 30 +rateLimits: + enabled: true + limit: 300 + burst: 3000 + waitTimeout: 3s +logger: + enabled: true + slowRequestThreshold: 5s + mode: invalid +metrics: + enabled: true +timeout: 30s +`) + + actualConfig = &Config{} + err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.Error(t, err) + require.Equal(t, "client logger invalid mode, choose one of: [none, all, failed]", err.Error()) +} diff --git a/httpclient/httpclient.go b/httpclient/httpclient.go index 5aba447..c919889 100644 --- a/httpclient/httpclient.go +++ b/httpclient/httpclient.go @@ -7,7 +7,9 @@ Released under MIT license. package httpclient import ( + "context" "fmt" + "github.com/acronis/go-appkit/log" "net/http" ) @@ -30,29 +32,99 @@ func CloneHTTPHeader(in http.Header) http.Header { return out } -// MustHTTPClient wraps delegate transports with logging, metrics, rate limiting, retryable, user agent, request id -// and panics if any error occurs. -func MustHTTPClient(cfg *Config, userAgent, reqType string, delegate http.RoundTripper) *http.Client { +// ClientProviders for further customization of the client logging and request id. +type ClientProviders struct { + Logger func(ctx context.Context) log.FieldLogger + RequestID func(ctx context.Context) string +} + +// NewHTTPClient wraps delegate transports with logging, metrics, rate limiting, retryable, user agent, request id +// and returns an error if any occurs. +func NewHTTPClient( + cfg *Config, + userAgent string, + reqType string, + delegate http.RoundTripper, + providers ClientProviders, +) (*http.Client, error) { + var err error + if delegate == nil { - delegate = http.DefaultTransport + delegate = http.DefaultTransport.(*http.Transport).Clone() } - delegate = NewLoggingRoundTripper(delegate, reqType) - delegate = NewMetricsRoundTripper(delegate, reqType) + if cfg.Logger.Enabled { + opts := cfg.Logger.TransportOpts() + opts.LoggerProvider = providers.Logger + delegate = NewLoggingRoundTripperWithOpts(delegate, reqType, opts) + } - delegate, err := NewRateLimitingRoundTripperWithOpts( - delegate, cfg.RateLimits.Limit, cfg.RateLimits.TransportOpts(), - ) - if err != nil { - panic(fmt.Errorf("create rate limiting round tripper: %w", err)) + if cfg.Metrics.Enabled { + delegate = NewMetricsRoundTripper(delegate, reqType) } - delegate, err = NewRetryableRoundTripperWithOpts(delegate, cfg.Retries.TransportOpts()) - if err != nil { - panic(fmt.Errorf("create retryable round tripper: %w", err)) + if cfg.RateLimits.Enabled { + delegate, err = NewRateLimitingRoundTripperWithOpts( + delegate, cfg.RateLimits.Limit, cfg.RateLimits.TransportOpts(), + ) + if err != nil { + return nil, fmt.Errorf("create rate limiting round tripper: %w", err) + } + } + + if cfg.Retries.Enabled { + opts := cfg.Retries.TransportOpts() + opts.LoggerProvider = providers.Logger + delegate, err = NewRetryableRoundTripperWithOpts(delegate, opts) + if err != nil { + return nil, fmt.Errorf("create retryable round tripper: %w", err) + } } delegate = NewUserAgentRoundTripper(delegate, userAgent) - delegate = NewRequestIDRoundTripper(delegate) - return &http.Client{Transport: delegate, Timeout: cfg.Timeout} + delegate = NewRequestIDRoundTripperWithOpts(delegate, RequestIDRoundTripperOpts{ + RequestIDProvider: providers.RequestID, + }) + + return &http.Client{Transport: delegate, Timeout: cfg.Timeout}, nil +} + +// MustHTTPClient wraps delegate transports with logging, metrics, rate limiting, retryable, user agent, request id +// and panics if any error occurs. +func MustHTTPClient( + cfg *Config, + userAgent, + reqType string, + delegate http.RoundTripper, + providers ClientProviders, +) *http.Client { + client, err := NewHTTPClient(cfg, userAgent, reqType, delegate, providers) + if err != nil { + panic(err) + } + + return client +} + +// ClientOpts provides options for NewHTTPClientWithOpts and MustHTTPClientWithOpts functions. +type ClientOpts struct { + Config Config + UserAgent string + ReqType string + Delegate http.RoundTripper + Providers ClientProviders +} + +// NewHTTPClientWithOpts wraps delegate transports with options +// logging, metrics, rate limiting, retryable, user agent, request id +// and returns an error if any occurs. +func NewHTTPClientWithOpts(opts ClientOpts) (*http.Client, error) { + return NewHTTPClient(&opts.Config, opts.UserAgent, opts.ReqType, opts.Delegate, opts.Providers) +} + +// MustHTTPClientWithOpts wraps delegate transports with options +// logging, metrics, rate limiting, retryable, user agent, request id +// and panics if any error occurs. +func MustHTTPClientWithOpts(opts ClientOpts) *http.Client { + return MustHTTPClient(&opts.Config, opts.UserAgent, opts.ReqType, opts.Delegate, opts.Providers) } diff --git a/httpclient/httpclient_test.go b/httpclient/httpclient_test.go index 3667efb..a440d95 100644 --- a/httpclient/httpclient_test.go +++ b/httpclient/httpclient_test.go @@ -16,6 +16,31 @@ import ( "testing" ) +func TestNewHTTPClientLoggingRoundTripper(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusTeapot) + })) + defer server.Close() + + logger := logtest.NewRecorder() + cfg := NewConfig() + cfg.Logger.Enabled = true + client, err := NewHTTPClient(cfg, "test-agent", "test-request", nil, ClientProviders{}) + require.NoError(t, err) + + ctx := middleware.NewContextWithLogger(context.Background(), logger) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + defer func() { _ = r.Body.Close() }() + require.NoError(t, err) + require.NotEmpty(t, logger.Entries()) + + loggerEntry := logger.Entries()[0] + require.Contains(t, loggerEntry.Text, "client http request POST "+server.URL+" req type test-request status code 418") +} + func TestMustHTTPClientLoggingRoundTripper(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { rw.WriteHeader(http.StatusTeapot) @@ -23,8 +48,9 @@ func TestMustHTTPClientLoggingRoundTripper(t *testing.T) { defer server.Close() logger := logtest.NewRecorder() - cfg := NewHTTPClientConfig() - client := MustHTTPClient(cfg, "test-agent", "test-request", nil) + cfg := NewConfig() + cfg.Logger.Enabled = true + client := MustHTTPClient(cfg, "test-agent", "test-request", nil, ClientProviders{}) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) require.NoError(t, err) @@ -35,5 +61,56 @@ func TestMustHTTPClientLoggingRoundTripper(t *testing.T) { require.NotEmpty(t, logger.Entries()) loggerEntry := logger.Entries()[0] - require.Contains(t, loggerEntry.Text, "external request POST "+server.URL+" req type test-request status code 418") + require.Contains(t, loggerEntry.Text, "client http request POST "+server.URL+" req type test-request status code 418") +} + +func TestNewHTTPClientWithOptsRoundTripper(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusTeapot) + })) + defer server.Close() + + logger := logtest.NewRecorder() + cfg := NewConfig() + cfg.Logger.Enabled = true + client, err := NewHTTPClientWithOpts(ClientOpts{ + Config: *cfg, + UserAgent: "test-agent", + ReqType: "test-request", + Delegate: nil, + }) + require.NoError(t, err) + ctx := middleware.NewContextWithLogger(context.Background(), logger) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + defer func() { _ = r.Body.Close() }() + require.NoError(t, err) + require.NotEmpty(t, logger.Entries()) +} + +func TestMustHTTPClientWithOptsRoundTripper(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusTeapot) + })) + defer server.Close() + + logger := logtest.NewRecorder() + cfg := NewConfig() + cfg.Logger.Enabled = true + client := MustHTTPClientWithOpts(ClientOpts{ + Config: *cfg, + UserAgent: "test-agent", + ReqType: "test-request", + Delegate: nil, + }) + ctx := middleware.NewContextWithLogger(context.Background(), logger) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + defer func() { _ = r.Body.Close() }() + require.NoError(t, err) + require.NotEmpty(t, logger.Entries()) } diff --git a/httpclient/logging_round_tripper.go b/httpclient/logging_round_tripper.go index 1b6c248..fddd86c 100644 --- a/httpclient/logging_round_tripper.go +++ b/httpclient/logging_round_tripper.go @@ -7,16 +7,49 @@ Released under MIT license. package httpclient import ( + "context" "fmt" "github.com/acronis/go-appkit/httpserver/middleware" + "github.com/acronis/go-appkit/log" "net/http" "time" ) +// LoggerMode represents a mode of logging. +type LoggerMode string + +const ( + LoggerModeNone LoggerMode = "none" + LoggerModeAll LoggerMode = "all" + LoggerModeFailed LoggerMode = "failed" +) + +// IsValid checks if the logger mode is valid. +func (lm LoggerMode) IsValid() bool { + switch lm { + case LoggerModeNone, LoggerModeAll, LoggerModeFailed: + return true + } + return false +} + // LoggingRoundTripper implements http.RoundTripper for logging requests. type LoggingRoundTripper struct { Delegate http.RoundTripper ReqType string + Opts LoggingRoundTripperOpts +} + +// LoggingRoundTripperOpts represents an options for LoggingRoundTripper. +type LoggingRoundTripperOpts struct { + // LoggerProvider is a function that provides a context-specific logger. + LoggerProvider func(ctx context.Context) log.FieldLogger + + // Mode of logging: none, all, failed. + Mode string + + // SlowRequestThreshold is a threshold for slow requests. + SlowRequestThreshold time.Duration } // NewLoggingRoundTripper creates an HTTP transport that log requests. @@ -24,33 +57,68 @@ func NewLoggingRoundTripper(delegate http.RoundTripper, reqType string) http.Rou return &LoggingRoundTripper{ Delegate: delegate, ReqType: reqType, + Opts: LoggingRoundTripperOpts{}, + } +} + +// NewLoggingRoundTripperWithOpts creates an HTTP transport that log requests with options. +func NewLoggingRoundTripperWithOpts( + delegate http.RoundTripper, reqType string, opts LoggingRoundTripperOpts, +) http.RoundTripper { + return &LoggingRoundTripper{ + Delegate: delegate, + ReqType: reqType, + Opts: opts, } } +// getLogger returns a logger from the context or from the options. +func (rt *LoggingRoundTripper) getLogger(ctx context.Context) log.FieldLogger { + if rt.Opts.LoggerProvider != nil { + return rt.Opts.LoggerProvider(ctx) + } + + return middleware.GetLoggerFromContext(ctx) +} + // RoundTrip adds logging capabilities to the HTTP transport. func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { + if rt.Opts.Mode == string(LoggerModeNone) { + return rt.Delegate.RoundTrip(r) + } + ctx := r.Context() - logger := middleware.GetLoggerFromContext(ctx) + logger := rt.getLogger(ctx) start := time.Now() - - resp, err := rt.Delegate.RoundTrip(r) - if logger != nil { - common := "external request %s %s req type %s " - elapsed := time.Since(start) + resp, err := rt.Delegate.RoundTrip(r) + elapsed := time.Since(start) + if logger != nil && elapsed >= rt.Opts.SlowRequestThreshold { + common := "client http request %s %s req type %s " args := []interface{}{r.Method, r.URL.String(), rt.ReqType, elapsed.Seconds(), err} message := common + "time taken %.3f, err %+v" + loggerAtLevel := logger.Infof + shouldModeLog := true if resp != nil { + if rt.Opts.Mode == string(LoggerModeFailed) && resp.StatusCode < http.StatusBadRequest { + shouldModeLog = false + } + args = []interface{}{r.Method, r.URL.String(), rt.ReqType, resp.StatusCode, elapsed.Seconds(), err} message = common + "status code %d, time taken %.3f, err %+v" } - logger.Infof(message, args...) + if err != nil { + loggerAtLevel = logger.Errorf + } - loggingParams := middleware.GetLoggingParamsFromContext(ctx) - if loggingParams != nil { - loggingParams.AddTimeSlotDurationInMs(fmt.Sprintf("external_request_%s_ms", rt.ReqType), elapsed) + if shouldModeLog { + loggerAtLevel(message, args...) + loggingParams := middleware.GetLoggingParamsFromContext(ctx) + if loggingParams != nil { + loggingParams.AddTimeSlotDurationInMs(fmt.Sprintf("external_request_%s_ms", rt.ReqType), elapsed) + } } } diff --git a/httpclient/logging_round_tripper_test.go b/httpclient/logging_round_tripper_test.go index 03f876d..8dc4813 100644 --- a/httpclient/logging_round_tripper_test.go +++ b/httpclient/logging_round_tripper_test.go @@ -9,6 +9,7 @@ package httpclient import ( "context" "github.com/acronis/go-appkit/httpserver/middleware" + "github.com/acronis/go-appkit/log" "github.com/acronis/go-appkit/log/logtest" "github.com/stretchr/testify/require" "net" @@ -36,7 +37,7 @@ func TestNewLoggingRoundTripper(t *testing.T) { require.NotEmpty(t, logger.Entries()) loggerEntry := logger.Entries()[0] - require.Contains(t, loggerEntry.Text, "external request POST "+server.URL+" req type test-request status code 418") + require.Contains(t, loggerEntry.Text, "client http request POST "+server.URL+" req type test-request status code 418") } func TestMustHTTPClientLoggingRoundTripperError(t *testing.T) { @@ -46,8 +47,9 @@ func TestMustHTTPClientLoggingRoundTripperError(t *testing.T) { _ = ln.Close() logger := logtest.NewRecorder() - cfg := NewHTTPClientConfig() - client := MustHTTPClient(cfg, "test-agent", "test-request", nil) + cfg := NewConfig() + cfg.Logger.Enabled = true + client := MustHTTPClient(cfg, "test-agent", "test-request", nil, ClientProviders{}) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) require.NoError(t, err) @@ -58,7 +60,122 @@ func TestMustHTTPClientLoggingRoundTripperError(t *testing.T) { require.NotEmpty(t, logger.Entries()) loggerEntry := logger.Entries()[0] - require.Contains(t, loggerEntry.Text, "external request POST "+serverURL+" req type test-request") + require.Contains(t, loggerEntry.Text, "client http request POST "+serverURL+" req type test-request") require.Contains(t, loggerEntry.Text, "err dial tcp "+ln.Addr().String()+": connect: connection refused") require.NotContains(t, loggerEntry.Text, "status code") } + +func TestMustHTTPClientLoggingRoundTripperDisabled(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + serverURL := "http://" + ln.Addr().String() + _ = ln.Close() + + logger := logtest.NewRecorder() + cfg := NewConfig() + client := MustHTTPClient(cfg, "test-agent", "test-request", nil, ClientProviders{}) + ctx := middleware.NewContextWithLogger(context.Background(), logger) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + require.Error(t, err) + require.Nil(t, r) + require.Empty(t, logger.Entries()) +} + +func TestNewLoggingRoundTripperModes(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + rw.WriteHeader(http.StatusBadRequest) + } else { + rw.WriteHeader(http.StatusOK) + } + })) + defer server.Close() + + tests := []struct { + name string + method string + mode string + want int + }{ + { + name: "no requests are logged because of 'none' mode", + method: http.MethodGet, + mode: "none", + }, + { + name: "4xx and 5xx requests are logged because of 'failed' mode", + method: http.MethodPost, + mode: "failed", + want: 1, + }, + { + name: "only 4xx and 5xx requests so no logs because of 'failed' mode for 2xx", + method: http.MethodGet, + mode: "failed", + }, + { + name: "success requests are logged because of 'all' mode", + method: http.MethodGet, + mode: "all", + want: 1, + }, + { + name: "failed requests are logged because of 'all' mode", + method: http.MethodPost, + mode: "all", + want: 1, + }, + } + + for i := range tests { + tt := tests[i] + t.Run(tt.name, func(t *testing.T) { + logger := logtest.NewRecorder() + loggerRoundTripper := NewLoggingRoundTripperWithOpts( + http.DefaultTransport, + "test-request", + LoggingRoundTripperOpts{Mode: tt.mode}, + ) + client := &http.Client{Transport: loggerRoundTripper} + ctx := middleware.NewContextWithLogger(context.Background(), logger) + req, err := http.NewRequestWithContext(ctx, tt.method, server.URL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + defer func() { _ = r.Body.Close() }() + require.NoError(t, err) + require.Len(t, logger.Entries(), tt.want) + }) + } +} + +func TestMustHTTPClientLoggingRoundTripperOpts(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + serverURL := "http://" + ln.Addr().String() + _ = ln.Close() + + logger := logtest.NewRecorder() + cfg := NewConfig() + cfg.Logger.Enabled = true + + var loggerProviderCalled bool + client := MustHTTPClient(cfg, "test-agent", "test-request", nil, ClientProviders{ + Logger: func(ctx context.Context) log.FieldLogger { + loggerProviderCalled = true + return logger + }, + }) + ctx := middleware.NewContextWithLogger(context.Background(), nil) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + require.Error(t, err) + require.Nil(t, r) + require.True(t, loggerProviderCalled) + require.Len(t, logger.Entries(), 1) +} diff --git a/httpclient/metrics_round_tripper.go b/httpclient/metrics_round_tripper.go index aa6be60..f5c1340 100644 --- a/httpclient/metrics_round_tripper.go +++ b/httpclient/metrics_round_tripper.go @@ -8,8 +8,6 @@ package httpclient import ( "fmt" - "github.com/acronis/go-appkit/httpserver/middleware" - "github.com/acronis/go-appkit/log" "github.com/prometheus/client_golang/prometheus" "net/http" "strconv" @@ -17,29 +15,29 @@ import ( ) var ( - ExternalHTTPRequestDuration *prometheus.HistogramVec - ClassifyRequest func(r *http.Request, reqType string, logger log.FieldLogger) string + ClientHTTPRequestDuration *prometheus.HistogramVec + ClassifyRequest func(r *http.Request, reqType string) string ) // MustInitAndRegisterMetrics initializes and registers external HTTP request duration metric. // Panic will be raised in case of error. func MustInitAndRegisterMetrics(namespace string) { - ExternalHTTPRequestDuration = prometheus.NewHistogramVec( + ClientHTTPRequestDuration = prometheus.NewHistogramVec( prometheus.HistogramOpts{ Namespace: namespace, - Name: "external_http_request_duration", - Help: "external HTTP request duration in seconds", + Name: "http_client_request_duration_seconds", + Help: "A histogram of the http client requests durations.", Buckets: []float64{0.01, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 30, 60, 150, 300, 600}, }, []string{"type", "remote_address", "summary", "status"}, ) - prometheus.MustRegister(ExternalHTTPRequestDuration) + prometheus.MustRegister(ClientHTTPRequestDuration) } // UnregisterMetrics unregisters external HTTP request duration metric. func UnregisterMetrics() { - if ExternalHTTPRequestDuration != nil { - prometheus.Unregister(ExternalHTTPRequestDuration) + if ClientHTTPRequestDuration != nil { + prometheus.Unregister(ClientHTTPRequestDuration) } } @@ -56,42 +54,29 @@ func NewMetricsRoundTripper(delegate http.RoundTripper, reqType string) http.Rou // RoundTrip measures external requests done. func (rt *MetricsRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - if ExternalHTTPRequestDuration == nil { + if ClientHTTPRequestDuration == nil { return rt.Delegate.RoundTrip(r) } - logger := middleware.GetLoggerFromContext(r.Context()) + status := "0" start := time.Now() - status := "failed" resp, err := rt.Delegate.RoundTrip(r) if err == nil && resp != nil { status = strconv.Itoa(resp.StatusCode) - if resp.StatusCode >= http.StatusBadRequest && logger != nil { - logger.Warnf("external request %s %s completed with HTTP status %d", r.Method, r.URL.String(), status) - } - } else if err != nil && logger != nil { - logger.Warnf("external request %s %s: %s", r.Method, r.URL.String(), err.Error()) } - ExternalHTTPRequestDuration.WithLabelValues( - rt.ReqType, r.Host, requestSummary(r, rt.ReqType, logger), status, + ClientHTTPRequestDuration.WithLabelValues( + rt.ReqType, r.Host, requestSummary(r, rt.ReqType), status, ).Observe(time.Since(start).Seconds()) return resp, err } // requestSummary does request classification, producing non-parameterized summary for given request. -func requestSummary(r *http.Request, reqType string, logger log.FieldLogger) string { +func requestSummary(r *http.Request, reqType string) string { if ClassifyRequest != nil { - return ClassifyRequest(r, reqType, logger) - } - - if logger != nil { - logger.Debugf( - "request classifier is not initialized, request %s %s will be marked as req type '%s' in metrics", - r.Method, r.URL.String(), reqType, - ) + return ClassifyRequest(r, reqType) } return fmt.Sprintf("%s %s", r.Method, reqType) diff --git a/httpclient/metrics_round_tripper_test.go b/httpclient/metrics_round_tripper_test.go index 198c3af..a532522 100644 --- a/httpclient/metrics_round_tripper_test.go +++ b/httpclient/metrics_round_tripper_test.go @@ -35,7 +35,7 @@ func TestNewMetricsRoundTripper(t *testing.T) { ch := make(chan prometheus.Metric, 1) go func() { - ExternalHTTPRequestDuration.Collect(ch) + ClientHTTPRequestDuration.Collect(ch) close(ch) }() diff --git a/httpclient/request_id_round_tripper.go b/httpclient/request_id_round_tripper.go index 3c96122..6d2ddea 100644 --- a/httpclient/request_id_round_tripper.go +++ b/httpclient/request_id_round_tripper.go @@ -7,6 +7,7 @@ Released under MIT license. package httpclient import ( + "context" "github.com/acronis/go-appkit/httpserver/middleware" "net/http" ) @@ -14,6 +15,12 @@ import ( // RequestIDRoundTripper for X-Request-ID header to the request. type RequestIDRoundTripper struct { Delegate http.RoundTripper + Opts RequestIDRoundTripperOpts +} + +// RequestIDRoundTripperOpts for X-Request-ID header to the request options. +type RequestIDRoundTripperOpts struct { + RequestIDProvider func(ctx context.Context) string } // NewRequestIDRoundTripper creates an HTTP transport with X-Request-ID header support. @@ -23,9 +30,26 @@ func NewRequestIDRoundTripper(delegate http.RoundTripper) http.RoundTripper { } } +// NewRequestIDRoundTripperWithOpts creates an HTTP transport with X-Request-ID header support with options. +func NewRequestIDRoundTripperWithOpts(delegate http.RoundTripper, opts RequestIDRoundTripperOpts) http.RoundTripper { + return &RequestIDRoundTripper{ + Delegate: delegate, + Opts: opts, + } +} + +// getRequestIDProvider returns a function with the request ID provider. +func (rt *RequestIDRoundTripper) getRequestIDProvider() func(ctx context.Context) string { + if rt.Opts.RequestIDProvider != nil { + return rt.Opts.RequestIDProvider + } + + return middleware.GetRequestIDFromContext +} + // RoundTrip adds X-Request-ID header to the request. func (rt *RequestIDRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - requestID := middleware.GetRequestIDFromContext(r.Context()) + requestID := rt.getRequestIDProvider()(r.Context()) if r.Header.Get("X-Request-ID") != "" || requestID == "" { return rt.Delegate.RoundTrip(r) } diff --git a/httpclient/request_id_round_tripper_test.go b/httpclient/request_id_round_tripper_test.go index 9242992..438c924 100644 --- a/httpclient/request_id_round_tripper_test.go +++ b/httpclient/request_id_round_tripper_test.go @@ -34,3 +34,27 @@ func TestNewRequestIDRoundTripper(t *testing.T) { defer func() { _ = r.Body.Close() }() require.NoError(t, err) } + +func TestNewRequestIDRoundTripperWithOpts(t *testing.T) { + requestID := "12345" + prefix := "my_custom_request_provider" + + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + require.Equal(t, prefix+requestID, r.Header.Get("X-Request-ID")) + rw.WriteHeader(http.StatusTeapot) + })) + defer server.Close() + + requestIDRoundTripper := NewRequestIDRoundTripperWithOpts(http.DefaultTransport, RequestIDRoundTripperOpts{ + RequestIDProvider: func(ctx context.Context) string { + return prefix + requestID + }, + }) + client := &http.Client{Transport: requestIDRoundTripper} + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, server.URL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + defer func() { _ = r.Body.Close() }() + require.NoError(t, err) +} From 2d02ab65830d8466324f54fee9b9cf28798952be Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Thu, 26 Dec 2024 14:23:05 +0000 Subject: [PATCH 03/16] Retries with policy options --- httpclient/config.go | 191 ++++++++++++++++++++++--- httpclient/config_test.go | 172 +++++++++++----------- httpclient/httpclient.go | 21 ++- httpclient/httpclient_test.go | 31 ++++ httpclient/logging_round_tripper.go | 9 +- httpclient/metrics_round_tripper.go | 5 +- httpclient/request_id_round_tripper.go | 6 +- 7 files changed, 321 insertions(+), 114 deletions(-) diff --git a/httpclient/config.go b/httpclient/config.go index c62020a..534b6c9 100644 --- a/httpclient/config.go +++ b/httpclient/config.go @@ -9,24 +9,37 @@ package httpclient import ( "errors" "github.com/acronis/go-appkit/config" + "github.com/acronis/go-appkit/retry" + "github.com/cenkalti/backoff/v4" "time" ) const ( + // DefaultClientWaitTimeout is a default timeout for a client to wait for a request. DefaultClientWaitTimeout = 10 * time.Second + // RetryPolicyExponential is a policy for exponential retries. + RetryPolicyExponential = "exponential" + + // RetryPolicyConstant is a policy for constant retries. + RetryPolicyConstant = "constant" + // configuration properties - cfgKeyRetriesEnabled = "retries.enabled" - cfgKeyRetriesMax = "retries.maxAttempts" - cfgKeyRateLimitsEnabled = "rateLimits.enabled" - cfgKeyRateLimitsLimit = "rateLimits.limit" - cfgKeyRateLimitsBurst = "rateLimits.burst" - cfgKeyRateLimitsWaitTimeout = "rateLimits.waitTimeout" - cfgKeyLoggerEnabled = "logger.enabled" - cfgKeyLoggerMode = "logger.mode" - cfgKeyLoggerSlowRequestThreshold = "logger.slowRequestThreshold" - cfgKeyMetricsEnabled = "metrics.enabled" - cfgKeyTimeout = "timeout" + cfgKeyRetriesEnabled = "retries.enabled" + cfgKeyRetriesMax = "retries.maxAttempts" + cfgKeyRetriesPolicyStrategy = "retries.policy.strategy" + cfgKeyRetriesPolicyExponentialInitialInterval = "retries.policy.exponentialBackoffInitialInterval" + cfgKeyRetriesPolicyExponentialMultiplier = "retries.policy.exponentialBackoffMultiplier" + cfgKeyRetriesPolicyConstantInternal = "retries.policy.constantBackoffInterval" + cfgKeyRateLimitsEnabled = "rateLimits.enabled" + cfgKeyRateLimitsLimit = "rateLimits.limit" + cfgKeyRateLimitsBurst = "rateLimits.burst" + cfgKeyRateLimitsWaitTimeout = "rateLimits.waitTimeout" + cfgKeyLoggerEnabled = "logger.enabled" + cfgKeyLoggerMode = "logger.mode" + cfgKeyLoggerSlowRequestThreshold = "logger.slowRequestThreshold" + cfgKeyMetricsEnabled = "metrics.enabled" + cfgKeyTimeout = "timeout" ) var _ config.Config = (*Config)(nil) @@ -34,9 +47,16 @@ var _ config.KeyPrefixProvider = (*Config)(nil) // RateLimitConfig represents configuration options for HTTP client rate limits. type RateLimitConfig struct { - Enabled bool `mapstructure:"enabled"` - Limit int `mapstructure:"limit"` - Burst int `mapstructure:"burst"` + // Enabled is a flag that enables rate limiting. + Enabled bool `mapstructure:"enabled"` + + // Limit is the maximum number of requests that can be made. + Limit int `mapstructure:"limit"` + + // Burst allow temporary spikes in request rate. + Burst int `mapstructure:"burst"` + + // WaitTimeout is the maximum time to wait for a request to be made. WaitTimeout time.Duration `mapstructure:"waitTimeout"` } @@ -48,6 +68,10 @@ func (c *RateLimitConfig) Set(dp config.DataProvider) (err error) { } c.Enabled = enabled + if !c.Enabled { + return nil + } + limit, err := dp.GetInt(cfgKeyRateLimitsLimit) if err != nil { return err @@ -89,10 +113,104 @@ func (c *RateLimitConfig) TransportOpts() RateLimitingRoundTripperOpts { } } +// PolicyConfig represents configuration options for policy retry. +type PolicyConfig struct { + // Strategy is a strategy for retry policy. + Strategy string `mapstructure:"strategy"` + + // ExponentialBackoffInitialInterval is the initial interval for exponential backoff. + ExponentialBackoffInitialInterval time.Duration `mapstructure:"exponentialBackoffInitialInterval"` + + // ExponentialBackoffMultiplier is the multiplier for exponential backoff. + ExponentialBackoffMultiplier float64 `mapstructure:"exponentialBackoffMultiplier"` + + // ConstantBackoffInterval is the interval for constant backoff. + ConstantBackoffInterval time.Duration `mapstructure:"constantBackoffInterval"` +} + +// Set is part of config interface implementation. +func (c *PolicyConfig) Set(dp config.DataProvider) (err error) { + strategy, err := dp.GetString(cfgKeyRetriesPolicyStrategy) + if err != nil { + return err + } + c.Strategy = strategy + + if c.Strategy != "" && c.Strategy != RetryPolicyExponential && c.Strategy != RetryPolicyConstant { + return errors.New("client retry policy must be one of: [exponential, constant]") + } + + if c.Strategy == RetryPolicyExponential { + var interval time.Duration + interval, err = dp.GetDuration(cfgKeyRetriesPolicyExponentialInitialInterval) + if err != nil { + return nil + } + if interval < 0 { + return errors.New("client exponential backoff initial interval must be positive") + } + c.ExponentialBackoffInitialInterval = interval + + var multiplier float64 + multiplier, err = dp.GetFloat64(cfgKeyRetriesPolicyExponentialMultiplier) + if err != nil { + return err + } + if multiplier <= 1 { + return errors.New("client exponential backoff multiplier must be greater than 1") + } + c.ExponentialBackoffMultiplier = multiplier + + return nil + } else if c.Strategy == RetryPolicyConstant { + var interval time.Duration + interval, err = dp.GetDuration(cfgKeyRetriesPolicyConstantInternal) + if err != nil { + return err + } + if interval < 0 { + return errors.New("client constant backoff interval must be positive") + } + c.ConstantBackoffInterval = interval + } + + return nil +} + +// SetProviderDefaults is part of config interface implementation. +func (c *PolicyConfig) SetProviderDefaults(_ config.DataProvider) {} + // RetriesConfig represents configuration options for HTTP client retries policy. type RetriesConfig struct { - Enabled bool `mapstructure:"enabled"` - MaxAttempts int `mapstructure:"maxAttempts"` + // Enabled is a flag that enables retries. + Enabled bool `mapstructure:"enabled"` + + // MaxAttempts is the maximum number of attempts to retry the request. + MaxAttempts int `mapstructure:"maxAttempts"` + + // Policy of a retry: [exponential, constant]. default is exponential. + Policy PolicyConfig `mapstructure:"policy"` +} + +// GetPolicy returns a retry policy based on strategy or nil if none is provided. +func (c *RetriesConfig) GetPolicy() retry.Policy { + if c.Policy.Strategy == RetryPolicyExponential { + return retry.PolicyFunc(func() backoff.BackOff { + bf := backoff.NewExponentialBackOff() + bf.InitialInterval = c.Policy.ExponentialBackoffInitialInterval + bf.Multiplier = c.Policy.ExponentialBackoffMultiplier + bf.Reset() + return bf + }) + } else if c.Policy.Strategy == RetryPolicyConstant { + return retry.PolicyFunc(func() backoff.BackOff { + bf := backoff.NewConstantBackOff(c.Policy.ConstantBackoffInterval) + bf.Reset() + return bf + }) + } + + return nil } // Set is part of config interface implementation. @@ -103,6 +221,10 @@ func (c *RetriesConfig) Set(dp config.DataProvider) error { } c.Enabled = enabled + if !c.Enabled { + return nil + } + maxAttempts, err := dp.GetInt(cfgKeyRetriesMax) if err != nil { return err @@ -112,6 +234,11 @@ func (c *RetriesConfig) Set(dp config.DataProvider) error { } c.MaxAttempts = maxAttempts + err = c.Policy.Set(config.NewKeyPrefixedDataProvider(dp, "")) + if err != nil { + return err + } + return nil } @@ -125,9 +252,14 @@ func (c *RetriesConfig) TransportOpts() RetryableRoundTripperOpts { // LoggerConfig represents configuration options for HTTP client logs. type LoggerConfig struct { - Enabled bool `mapstructure:"enabled"` + // Enabled is a flag that enables logging. + Enabled bool `mapstructure:"enabled"` + + // SlowRequestThreshold is a threshold for slow requests. SlowRequestThreshold time.Duration `mapstructure:"slowRequestThreshold"` - Mode string `mapstructure:"mode"` + + // Mode of logging. + Mode string `mapstructure:"mode"` } // Set is part of config interface implementation. @@ -138,6 +270,10 @@ func (c *LoggerConfig) Set(dp config.DataProvider) error { } c.Enabled = enabled + if !c.Enabled { + return nil + } + slowRequestThreshold, err := dp.GetDuration(cfgKeyLoggerSlowRequestThreshold) if err != nil { return err @@ -172,6 +308,7 @@ func (c *LoggerConfig) TransportOpts() LoggingRoundTripperOpts { // MetricsConfig represents configuration options for HTTP client logs. type MetricsConfig struct { + // Enabled is a flag that enables metrics. Enabled bool `mapstructure:"enabled"` } @@ -191,12 +328,22 @@ func (c *MetricsConfig) SetProviderDefaults(_ config.DataProvider) {} // Config represents options for HTTP client configuration. type Config struct { - Retries RetriesConfig `mapstructure:"retries"` + // Retries is a configuration for HTTP client retries policy. + Retries RetriesConfig `mapstructure:"retries"` + + // RateLimits is a configuration for HTTP client rate limits. RateLimits RateLimitConfig `mapstructure:"rateLimits"` - Logger LoggerConfig `mapstructure:"logger"` - Metrics MetricsConfig `mapstructure:"metrics"` - Timeout time.Duration `mapstructure:"timeout"` + // Logger is a configuration for HTTP client logs. + Logger LoggerConfig `mapstructure:"logger"` + + // Metrics is a configuration for HTTP client metrics. + Metrics MetricsConfig `mapstructure:"metrics"` + + // Timeout is the maximum time to wait for a request to be made. + Timeout time.Duration `mapstructure:"timeout"` + + // keyPrefix is a prefix for configuration parameters. keyPrefix string } diff --git a/httpclient/config_test.go b/httpclient/config_test.go index 1cf9a8e..3cbe31a 100644 --- a/httpclient/config_test.go +++ b/httpclient/config_test.go @@ -9,30 +9,15 @@ package httpclient import ( "bytes" "github.com/acronis/go-appkit/config" + "github.com/acronis/go-appkit/retry" "github.com/stretchr/testify/require" + "strings" "testing" "time" ) func TestConfigWithLoader(t *testing.T) { - yamlData := []byte(` -retries: - enabled: true - maxAttempts: 30 -rateLimits: - enabled: true - limit: 300 - burst: 3000 - waitTimeout: 3s -logger: - enabled: true - slowRequestThreshold: 5s - mode: all -metrics: - enabled: true -timeout: 30s -`) - + yamlData := testYamlData(nil) actualConfig := &Config{} err := config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.NoError(t, err, "load configuration") @@ -41,6 +26,11 @@ timeout: 30s Retries: RetriesConfig{ Enabled: true, MaxAttempts: 30, + Policy: PolicyConfig{ + Strategy: RetryPolicyExponential, + ExponentialBackoffInitialInterval: 3 * time.Second, + ExponentialBackoffMultiplier: 2, + }, }, RateLimits: RateLimitConfig{ Enabled: true, @@ -60,101 +50,115 @@ timeout: 30s require.Equal(t, expectedConfig, actualConfig, "configuration does not match expected") } -func TestConfigRateLimitInvalid(t *testing.T) { - yamlData := []byte(` -retries: - enabled: true - maxAttempts: 30 -rateLimits: - enabled: true - limit: -300 - burst: 3000 - waitTimeout: 3s -timeout: 30s -`) - +func TestConfigRateLimit(t *testing.T) { + yamlData := testYamlData([][]string{{"limit: 300", "limit: -300"}}) actualConfig := &Config{} err := config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) require.Equal(t, "client rate limit must be positive", err.Error()) - yamlData = []byte(` -retries: - enabled: true - maxAttempts: 30 -rateLimits: - enabled: true - limit: 300 - burst: -3 - waitTimeout: 3s -timeout: 30s -`) - + yamlData = testYamlData([][]string{{"burst: 3000", "burst: -3"}}) actualConfig = &Config{} err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) require.Equal(t, "client burst must be positive", err.Error()) - yamlData = []byte(` -retries: - enabled: true - maxAttempts: 30 -rateLimits: - enabled: true - limit: 300 - burst: 3 - waitTimeout: -3s -timeout: 30s -`) - + yamlData = testYamlData([][]string{{"waitTimeout: 3s", "waitTimeout: -3s"}}) actualConfig = &Config{} err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) require.Equal(t, "client wait timeout must be positive", err.Error()) } -func TestConfigRetriesInvalid(t *testing.T) { - yamlData := []byte(` -retries: - enabled: true - maxAttempts: -30 -timeout: 30s -`) - +func TestConfigRetries(t *testing.T) { + yamlData := testYamlData([][]string{{"maxAttempts: 30", "maxAttempts: -30"}}) actualConfig := &Config{} err := config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) require.Equal(t, "client max retry attempts must be positive", err.Error()) } -func TestConfigLoggerInvalid(t *testing.T) { +func TestConfigLogger(t *testing.T) { + yamlData := testYamlData([][]string{{"slowRequestThreshold: 5s", "slowRequestThreshold: -5s"}}) + actualConfig := &Config{} + err := config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.Error(t, err) + require.Equal(t, "client logger slow request threshold can not be negative", err.Error()) + + yamlData = testYamlData([][]string{{"mode: all", "mode: invalid"}}) + actualConfig = &Config{} + err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.Error(t, err) + require.Equal(t, "client logger invalid mode, choose one of: [none, all, failed]", err.Error()) +} + +func TestConfigRetriesPolicy(t *testing.T) { + yamlData := testYamlData([][]string{{"strategy: exponential", "strategy: invalid"}}) + actualConfig := &Config{} + err := config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.Error(t, err) + require.Equal(t, "client retry policy must be one of: [exponential, constant]", err.Error()) + + yamlData = testYamlData([][]string{ + {"exponentialBackoffInitialInterval: 3s", "exponentialBackoffInitialInterval: -1s"}, + }) + err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.Error(t, err) + require.Equal(t, "client exponential backoff initial interval must be positive", err.Error()) + + yamlData = testYamlData([][]string{{"exponentialBackoffMultiplier: 2", "exponentialBackoffMultiplier: 1"}}) + err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.Error(t, err) + require.Equal(t, "client exponential backoff multiplier must be greater than 1", err.Error()) + + yamlData = testYamlData([][]string{ + {"strategy: exponential", "strategy: constant"}, + {"constantBackoffInterval: 2s", "constantBackoffInterval: -3s"}, + }) + err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.Error(t, err) + require.Equal(t, "client constant backoff interval must be positive", err.Error()) + + yamlData = testYamlData([][]string{ + {"strategy: exponential", "strategy:"}, + }) + err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.NoError(t, err) + require.Nil(t, actualConfig.Retries.GetPolicy()) + + yamlData = testYamlData(nil) + err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) + require.NoError(t, err) + require.Implements(t, (*retry.Policy)(nil), actualConfig.Retries.GetPolicy()) +} + +func TestConfigDisableWithLoader(t *testing.T) { yamlData := []byte(` retries: - enabled: true - maxAttempts: 30 + enabled: false rateLimits: - enabled: true - limit: 300 - burst: 3000 - waitTimeout: 3s + enabled: false logger: - enabled: true - slowRequestThreshold: -5s - mode: all + enabled: false metrics: - enabled: true + enabled: false timeout: 30s `) - actualConfig := &Config{} err := config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) - require.Error(t, err) - require.Equal(t, "client logger slow request threshold can not be negative", err.Error()) + require.NoError(t, err) +} - yamlData = []byte(` +func testYamlData(replacements [][]string) []byte { + yamlData := ` retries: enabled: true maxAttempts: 30 + policy: + strategy: exponential + exponentialBackoffInitialInterval: 3s + exponentialBackoffMultiplier: 2 + constantBackoffInterval: 2s rateLimits: enabled: true limit: 300 @@ -163,14 +167,14 @@ rateLimits: logger: enabled: true slowRequestThreshold: 5s - mode: invalid + mode: all metrics: enabled: true timeout: 30s -`) +` + for i := range replacements { + yamlData = strings.Replace(yamlData, replacements[i][0], replacements[i][1], 1) + } - actualConfig = &Config{} - err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) - require.Error(t, err) - require.Equal(t, "client logger invalid mode, choose one of: [none, all, failed]", err.Error()) + return []byte(yamlData) } diff --git a/httpclient/httpclient.go b/httpclient/httpclient.go index c919889..b41ca28 100644 --- a/httpclient/httpclient.go +++ b/httpclient/httpclient.go @@ -34,7 +34,10 @@ func CloneHTTPHeader(in http.Header) http.Header { // ClientProviders for further customization of the client logging and request id. type ClientProviders struct { - Logger func(ctx context.Context) log.FieldLogger + // Logger is a function that provides a context-specific logger. + Logger func(ctx context.Context) log.FieldLogger + + // RequestID is a function that provides a request ID. RequestID func(ctx context.Context) string } @@ -75,6 +78,7 @@ func NewHTTPClient( if cfg.Retries.Enabled { opts := cfg.Retries.TransportOpts() opts.LoggerProvider = providers.Logger + opts.BackoffPolicy = cfg.Retries.GetPolicy() delegate, err = NewRetryableRoundTripperWithOpts(delegate, opts) if err != nil { return nil, fmt.Errorf("create retryable round tripper: %w", err) @@ -108,10 +112,19 @@ func MustHTTPClient( // ClientOpts provides options for NewHTTPClientWithOpts and MustHTTPClientWithOpts functions. type ClientOpts struct { - Config Config + // Config is the configuration for the HTTP client. + Config Config + + // UserAgent is a user agent string. UserAgent string - ReqType string - Delegate http.RoundTripper + + // ReqType is a type of request. + ReqType string + + // Delegate is the next RoundTripper in the chain. + Delegate http.RoundTripper + + // Providers are the functions that provide a context-specific logger and request ID. Providers ClientProviders } diff --git a/httpclient/httpclient_test.go b/httpclient/httpclient_test.go index a440d95..6f94e87 100644 --- a/httpclient/httpclient_test.go +++ b/httpclient/httpclient_test.go @@ -14,6 +14,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" ) func TestNewHTTPClientLoggingRoundTripper(t *testing.T) { @@ -114,3 +115,33 @@ func TestMustHTTPClientWithOptsRoundTripper(t *testing.T) { require.NoError(t, err) require.NotEmpty(t, logger.Entries()) } + +func TestMustHTTPClientWithOptsRoundTripperPolicy(t *testing.T) { + var retriesCount int + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + retriesCount++ + rw.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() + + cfg := NewConfig() + cfg.Retries.Enabled = true + cfg.Retries.MaxAttempts = 1 + cfg.Retries.Policy.Strategy = RetryPolicyExponential + cfg.Retries.Policy.ExponentialBackoffInitialInterval = 2 * time.Millisecond + cfg.Retries.Policy.ExponentialBackoffMultiplier = 1.1 + + client := MustHTTPClientWithOpts(ClientOpts{ + Config: *cfg, + UserAgent: "test-agent", + ReqType: "test-request", + Delegate: nil, + }) + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, server.URL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + defer func() { _ = r.Body.Close() }() + require.NoError(t, err) + require.Equal(t, 2, retriesCount) +} diff --git a/httpclient/logging_round_tripper.go b/httpclient/logging_round_tripper.go index fddd86c..1fbf661 100644 --- a/httpclient/logging_round_tripper.go +++ b/httpclient/logging_round_tripper.go @@ -35,9 +35,14 @@ func (lm LoggerMode) IsValid() bool { // LoggingRoundTripper implements http.RoundTripper for logging requests. type LoggingRoundTripper struct { + // Delegate is the next RoundTripper in the chain. Delegate http.RoundTripper - ReqType string - Opts LoggingRoundTripperOpts + + // ReqType is a type of request. + ReqType string + + // Opts are the options for the logging round tripper. + Opts LoggingRoundTripperOpts } // LoggingRoundTripperOpts represents an options for LoggingRoundTripper. diff --git a/httpclient/metrics_round_tripper.go b/httpclient/metrics_round_tripper.go index f5c1340..faf27dd 100644 --- a/httpclient/metrics_round_tripper.go +++ b/httpclient/metrics_round_tripper.go @@ -43,8 +43,11 @@ func UnregisterMetrics() { // MetricsRoundTripper is an HTTP transport that measures requests done. type MetricsRoundTripper struct { + // Delegate is the next RoundTripper in the chain. Delegate http.RoundTripper - ReqType string + + // ReqType is a type of request. + ReqType string } // NewMetricsRoundTripper creates an HTTP transport that measures requests done. diff --git a/httpclient/request_id_round_tripper.go b/httpclient/request_id_round_tripper.go index 6d2ddea..6e51e4d 100644 --- a/httpclient/request_id_round_tripper.go +++ b/httpclient/request_id_round_tripper.go @@ -14,12 +14,16 @@ import ( // RequestIDRoundTripper for X-Request-ID header to the request. type RequestIDRoundTripper struct { + // Delegate is the next RoundTripper in the chain. Delegate http.RoundTripper - Opts RequestIDRoundTripperOpts + + // Opts are the options for the request ID round tripper. + Opts RequestIDRoundTripperOpts } // RequestIDRoundTripperOpts for X-Request-ID header to the request options. type RequestIDRoundTripperOpts struct { + // RequestIDProvider is a function that provides a request ID. RequestIDProvider func(ctx context.Context) string } From 196f0323bee203fc66261fe7097d76cf21236d39 Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Thu, 9 Jan 2025 14:08:55 +0000 Subject: [PATCH 04/16] Clients new and opts, retry config properties and standard errors, logging mode, metrics and request id --- httpclient/config.go | 63 ++++++++++++------------ httpclient/config_test.go | 47 +++++++++--------- httpclient/httpclient.go | 50 +++++++++---------- httpclient/httpclient_test.go | 21 ++++---- httpclient/logging_round_tripper.go | 38 +++++++------- httpclient/logging_round_tripper_test.go | 12 ++--- httpclient/metrics_round_tripper.go | 42 +++++++++------- httpclient/metrics_round_tripper_test.go | 2 +- httpclient/request_id_round_tripper.go | 5 +- 9 files changed, 145 insertions(+), 135 deletions(-) diff --git a/httpclient/config.go b/httpclient/config.go index 534b6c9..9c47e33 100644 --- a/httpclient/config.go +++ b/httpclient/config.go @@ -27,17 +27,17 @@ const ( // configuration properties cfgKeyRetriesEnabled = "retries.enabled" cfgKeyRetriesMax = "retries.maxAttempts" - cfgKeyRetriesPolicyStrategy = "retries.policy.strategy" - cfgKeyRetriesPolicyExponentialInitialInterval = "retries.policy.exponentialBackoffInitialInterval" - cfgKeyRetriesPolicyExponentialMultiplier = "retries.policy.exponentialBackoffMultiplier" - cfgKeyRetriesPolicyConstantInternal = "retries.policy.constantBackoffInterval" + cfgKeyRetriesPolicyStrategy = "retries.policy" + cfgKeyRetriesPolicyExponentialInitialInterval = "retries.exponentialBackoff.initialInterval" + cfgKeyRetriesPolicyExponentialMultiplier = "retries.exponentialBackoff.multiplier" + cfgKeyRetriesPolicyConstantInternal = "retries.constantBackoff.interval" cfgKeyRateLimitsEnabled = "rateLimits.enabled" cfgKeyRateLimitsLimit = "rateLimits.limit" cfgKeyRateLimitsBurst = "rateLimits.burst" cfgKeyRateLimitsWaitTimeout = "rateLimits.waitTimeout" - cfgKeyLoggerEnabled = "logger.enabled" - cfgKeyLoggerMode = "logger.mode" - cfgKeyLoggerSlowRequestThreshold = "logger.slowRequestThreshold" + cfgKeyLogEnabled = "log.enabled" + cfgKeyLogMode = "log.mode" + cfgKeyLogSlowRequestThreshold = "log.slowRequestThreshold" cfgKeyMetricsEnabled = "metrics.enabled" cfgKeyTimeout = "timeout" ) @@ -77,7 +77,7 @@ func (c *RateLimitConfig) Set(dp config.DataProvider) (err error) { return err } if limit <= 0 { - return errors.New("client rate limit must be positive") + return dp.WrapKeyErr(cfgKeyRateLimitsLimit, errors.New("must be positive")) } c.Limit = limit @@ -86,7 +86,7 @@ func (c *RateLimitConfig) Set(dp config.DataProvider) (err error) { return err } if burst < 0 { - return errors.New("client burst must be positive") + return dp.WrapKeyErr(cfgKeyRateLimitsBurst, errors.New("must be positive")) } c.Burst = burst @@ -95,7 +95,7 @@ func (c *RateLimitConfig) Set(dp config.DataProvider) (err error) { return err } if waitTimeout < 0 { - return errors.New("client wait timeout must be positive") + return dp.WrapKeyErr(cfgKeyRateLimitsWaitTimeout, errors.New("must be positive")) } c.WaitTimeout = waitTimeout @@ -137,7 +137,7 @@ func (c *PolicyConfig) Set(dp config.DataProvider) (err error) { c.Strategy = strategy if c.Strategy != "" && c.Strategy != RetryPolicyExponential && c.Strategy != RetryPolicyConstant { - return errors.New("client retry policy must be one of: [exponential, constant]") + return dp.WrapKeyErr(cfgKeyRetriesPolicyStrategy, errors.New("must be one of: [exponential, constant]")) } if c.Strategy == RetryPolicyExponential { @@ -147,7 +147,7 @@ func (c *PolicyConfig) Set(dp config.DataProvider) (err error) { return nil } if interval < 0 { - return errors.New("client exponential backoff initial interval must be positive") + return dp.WrapKeyErr(cfgKeyRetriesPolicyExponentialInitialInterval, errors.New("must be positive")) } c.ExponentialBackoffInitialInterval = interval @@ -157,7 +157,7 @@ func (c *PolicyConfig) Set(dp config.DataProvider) (err error) { return err } if multiplier <= 1 { - return errors.New("client exponential backoff multiplier must be greater than 1") + return dp.WrapKeyErr(cfgKeyRetriesPolicyExponentialMultiplier, errors.New("must be greater than 1")) } c.ExponentialBackoffMultiplier = multiplier @@ -169,7 +169,7 @@ func (c *PolicyConfig) Set(dp config.DataProvider) (err error) { return err } if interval < 0 { - return errors.New("client constant backoff interval must be positive") + return dp.WrapKeyErr(cfgKeyRetriesPolicyConstantInternal, errors.New("must be positive")) } c.ConstantBackoffInterval = interval } @@ -230,7 +230,7 @@ func (c *RetriesConfig) Set(dp config.DataProvider) error { return err } if maxAttempts < 0 { - return errors.New("client max retry attempts must be positive") + return dp.WrapKeyErr(cfgKeyRetriesMax, errors.New("must be positive")) } c.MaxAttempts = maxAttempts @@ -250,8 +250,8 @@ func (c *RetriesConfig) TransportOpts() RetryableRoundTripperOpts { return RetryableRoundTripperOpts{MaxRetryAttempts: c.MaxAttempts} } -// LoggerConfig represents configuration options for HTTP client logs. -type LoggerConfig struct { +// LogConfig represents configuration options for HTTP client logs. +type LogConfig struct { // Enabled is a flag that enables logging. Enabled bool `mapstructure:"enabled"` @@ -259,12 +259,12 @@ type LoggerConfig struct { SlowRequestThreshold time.Duration `mapstructure:"slowRequestThreshold"` // Mode of logging. - Mode string `mapstructure:"mode"` + Mode LoggingMode `mapstructure:"mode"` } // Set is part of config interface implementation. -func (c *LoggerConfig) Set(dp config.DataProvider) error { - enabled, err := dp.GetBool(cfgKeyLoggerEnabled) +func (c *LogConfig) Set(dp config.DataProvider) error { + enabled, err := dp.GetBool(cfgKeyLogEnabled) if err != nil { return err } @@ -274,32 +274,33 @@ func (c *LoggerConfig) Set(dp config.DataProvider) error { return nil } - slowRequestThreshold, err := dp.GetDuration(cfgKeyLoggerSlowRequestThreshold) + slowRequestThreshold, err := dp.GetDuration(cfgKeyLogSlowRequestThreshold) if err != nil { return err } if slowRequestThreshold < 0 { - return errors.New("client logger slow request threshold can not be negative") + return dp.WrapKeyErr(cfgKeyLogSlowRequestThreshold, errors.New("can not be negative")) } c.SlowRequestThreshold = slowRequestThreshold - mode, err := dp.GetString(cfgKeyLoggerMode) + mode, err := dp.GetString(cfgKeyLogMode) if err != nil { return err } - if !LoggerMode(mode).IsValid() { - return errors.New("client logger invalid mode, choose one of: [none, all, failed]") + loggingMode := LoggingMode(mode) + if !loggingMode.IsValid() { + return dp.WrapKeyErr(cfgKeyLogMode, errors.New("choose one of: [none, all, failed]")) } - c.Mode = mode + c.Mode = loggingMode return nil } // SetProviderDefaults is part of config interface implementation. -func (c *LoggerConfig) SetProviderDefaults(_ config.DataProvider) {} +func (c *LogConfig) SetProviderDefaults(_ config.DataProvider) {} // TransportOpts returns transport options. -func (c *LoggerConfig) TransportOpts() LoggingRoundTripperOpts { +func (c *LogConfig) TransportOpts() LoggingRoundTripperOpts { return LoggingRoundTripperOpts{ Mode: c.Mode, SlowRequestThreshold: c.SlowRequestThreshold, @@ -334,8 +335,8 @@ type Config struct { // RateLimits is a configuration for HTTP client rate limits. RateLimits RateLimitConfig `mapstructure:"rateLimits"` - // Logger is a configuration for HTTP client logs. - Logger LoggerConfig `mapstructure:"logger"` + // Log is a configuration for HTTP client logs. + Log LogConfig `mapstructure:"log"` // Metrics is a configuration for HTTP client metrics. Metrics MetricsConfig `mapstructure:"metrics"` @@ -381,7 +382,7 @@ func (c *Config) Set(dp config.DataProvider) error { return err } - err = c.Logger.Set(config.NewKeyPrefixedDataProvider(dp, c.keyPrefix)) + err = c.Log.Set(config.NewKeyPrefixedDataProvider(dp, c.keyPrefix)) if err != nil { return err } diff --git a/httpclient/config_test.go b/httpclient/config_test.go index 3cbe31a..c084a51 100644 --- a/httpclient/config_test.go +++ b/httpclient/config_test.go @@ -38,7 +38,7 @@ func TestConfigWithLoader(t *testing.T) { Burst: 3000, WaitTimeout: 3 * time.Second, }, - Logger: LoggerConfig{ + Log: LogConfig{ Enabled: true, SlowRequestThreshold: 5 * time.Second, Mode: "all", @@ -55,19 +55,19 @@ func TestConfigRateLimit(t *testing.T) { actualConfig := &Config{} err := config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) - require.Equal(t, "client rate limit must be positive", err.Error()) + require.Equal(t, "rateLimits.limit: must be positive", err.Error()) yamlData = testYamlData([][]string{{"burst: 3000", "burst: -3"}}) actualConfig = &Config{} err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) - require.Equal(t, "client burst must be positive", err.Error()) + require.Equal(t, "rateLimits.burst: must be positive", err.Error()) yamlData = testYamlData([][]string{{"waitTimeout: 3s", "waitTimeout: -3s"}}) actualConfig = &Config{} err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) - require.Equal(t, "client wait timeout must be positive", err.Error()) + require.Equal(t, "rateLimits.waitTimeout: must be positive", err.Error()) } func TestConfigRetries(t *testing.T) { @@ -75,7 +75,7 @@ func TestConfigRetries(t *testing.T) { actualConfig := &Config{} err := config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) - require.Equal(t, "client max retry attempts must be positive", err.Error()) + require.Equal(t, "retries.maxAttempts: must be positive", err.Error()) } func TestConfigLogger(t *testing.T) { @@ -83,44 +83,44 @@ func TestConfigLogger(t *testing.T) { actualConfig := &Config{} err := config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) - require.Equal(t, "client logger slow request threshold can not be negative", err.Error()) + require.Equal(t, "log.slowRequestThreshold: can not be negative", err.Error()) yamlData = testYamlData([][]string{{"mode: all", "mode: invalid"}}) actualConfig = &Config{} err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) - require.Equal(t, "client logger invalid mode, choose one of: [none, all, failed]", err.Error()) + require.Equal(t, "log.mode: choose one of: [none, all, failed]", err.Error()) } func TestConfigRetriesPolicy(t *testing.T) { - yamlData := testYamlData([][]string{{"strategy: exponential", "strategy: invalid"}}) + yamlData := testYamlData([][]string{{"policy: exponential", "policy: invalid"}}) actualConfig := &Config{} err := config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) - require.Equal(t, "client retry policy must be one of: [exponential, constant]", err.Error()) + require.Equal(t, "retries.policy: must be one of: [exponential, constant]", err.Error()) yamlData = testYamlData([][]string{ - {"exponentialBackoffInitialInterval: 3s", "exponentialBackoffInitialInterval: -1s"}, + {"initialInterval: 3s", "initialInterval: -1s"}, }) err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) - require.Equal(t, "client exponential backoff initial interval must be positive", err.Error()) + require.Equal(t, "retries.exponentialBackoff.initialInterval: must be positive", err.Error()) - yamlData = testYamlData([][]string{{"exponentialBackoffMultiplier: 2", "exponentialBackoffMultiplier: 1"}}) + yamlData = testYamlData([][]string{{"multiplier: 2", "multiplier: 1"}}) err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) - require.Equal(t, "client exponential backoff multiplier must be greater than 1", err.Error()) + require.Equal(t, "retries.exponentialBackoff.multiplier: must be greater than 1", err.Error()) yamlData = testYamlData([][]string{ - {"strategy: exponential", "strategy: constant"}, - {"constantBackoffInterval: 2s", "constantBackoffInterval: -3s"}, + {"policy: exponential", "policy: constant"}, + {"interval: 2s", "interval: -3s"}, }) err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) - require.Equal(t, "client constant backoff interval must be positive", err.Error()) + require.Equal(t, "retries.constantBackoff.interval: must be positive", err.Error()) yamlData = testYamlData([][]string{ - {"strategy: exponential", "strategy:"}, + {"policy: exponential", "policy:"}, }) err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.NoError(t, err) @@ -154,17 +154,18 @@ func testYamlData(replacements [][]string) []byte { retries: enabled: true maxAttempts: 30 - policy: - strategy: exponential - exponentialBackoffInitialInterval: 3s - exponentialBackoffMultiplier: 2 - constantBackoffInterval: 2s + policy: exponential + exponentialBackoff: + initialInterval: 3s + multiplier: 2 + constantBackoff: + interval: 2s rateLimits: enabled: true limit: 300 burst: 3000 waitTimeout: 3s -logger: +log: enabled: true slowRequestThreshold: 5s mode: all diff --git a/httpclient/httpclient.go b/httpclient/httpclient.go index b41ca28..1feaa91 100644 --- a/httpclient/httpclient.go +++ b/httpclient/httpclient.go @@ -41,9 +41,9 @@ type ClientProviders struct { RequestID func(ctx context.Context) string } -// NewHTTPClient wraps delegate transports with logging, metrics, rate limiting, retryable, user agent, request id +// New wraps delegate transports with logging, metrics, rate limiting, retryable, user agent, request id // and returns an error if any occurs. -func NewHTTPClient( +func New( cfg *Config, userAgent string, reqType string, @@ -56,8 +56,8 @@ func NewHTTPClient( delegate = http.DefaultTransport.(*http.Transport).Clone() } - if cfg.Logger.Enabled { - opts := cfg.Logger.TransportOpts() + if cfg.Log.Enabled { + opts := cfg.Log.TransportOpts() opts.LoggerProvider = providers.Logger delegate = NewLoggingRoundTripperWithOpts(delegate, reqType, opts) } @@ -75,6 +75,14 @@ func NewHTTPClient( } } + if userAgent != "" { + delegate = NewUserAgentRoundTripper(delegate, userAgent) + } + + delegate = NewRequestIDRoundTripperWithOpts(delegate, RequestIDRoundTripperOpts{ + RequestIDProvider: providers.RequestID, + }) + if cfg.Retries.Enabled { opts := cfg.Retries.TransportOpts() opts.LoggerProvider = providers.Logger @@ -85,24 +93,19 @@ func NewHTTPClient( } } - delegate = NewUserAgentRoundTripper(delegate, userAgent) - delegate = NewRequestIDRoundTripperWithOpts(delegate, RequestIDRoundTripperOpts{ - RequestIDProvider: providers.RequestID, - }) - return &http.Client{Transport: delegate, Timeout: cfg.Timeout}, nil } -// MustHTTPClient wraps delegate transports with logging, metrics, rate limiting, retryable, user agent, request id +// Must wraps delegate transports with logging, metrics, rate limiting, retryable, user agent, request id // and panics if any error occurs. -func MustHTTPClient( +func Must( cfg *Config, - userAgent, + userAgent string, reqType string, delegate http.RoundTripper, providers ClientProviders, ) *http.Client { - client, err := NewHTTPClient(cfg, userAgent, reqType, delegate, providers) + client, err := New(cfg, userAgent, reqType, delegate, providers) if err != nil { panic(err) } @@ -110,15 +113,12 @@ func MustHTTPClient( return client } -// ClientOpts provides options for NewHTTPClientWithOpts and MustHTTPClientWithOpts functions. -type ClientOpts struct { - // Config is the configuration for the HTTP client. - Config Config - +// Opts provides options for NewWithOpts and MustWithOpts functions. +type Opts struct { // UserAgent is a user agent string. UserAgent string - // ReqType is a type of request. + // ReqType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. ReqType string // Delegate is the next RoundTripper in the chain. @@ -128,16 +128,16 @@ type ClientOpts struct { Providers ClientProviders } -// NewHTTPClientWithOpts wraps delegate transports with options +// NewWithOpts wraps delegate transports with options // logging, metrics, rate limiting, retryable, user agent, request id // and returns an error if any occurs. -func NewHTTPClientWithOpts(opts ClientOpts) (*http.Client, error) { - return NewHTTPClient(&opts.Config, opts.UserAgent, opts.ReqType, opts.Delegate, opts.Providers) +func NewWithOpts(cfg *Config, opts Opts) (*http.Client, error) { + return New(cfg, opts.UserAgent, opts.ReqType, opts.Delegate, opts.Providers) } -// MustHTTPClientWithOpts wraps delegate transports with options +// MustWithOpts wraps delegate transports with options // logging, metrics, rate limiting, retryable, user agent, request id // and panics if any error occurs. -func MustHTTPClientWithOpts(opts ClientOpts) *http.Client { - return MustHTTPClient(&opts.Config, opts.UserAgent, opts.ReqType, opts.Delegate, opts.Providers) +func MustWithOpts(cfg *Config, opts Opts) *http.Client { + return Must(cfg, opts.UserAgent, opts.ReqType, opts.Delegate, opts.Providers) } diff --git a/httpclient/httpclient_test.go b/httpclient/httpclient_test.go index 6f94e87..7029d42 100644 --- a/httpclient/httpclient_test.go +++ b/httpclient/httpclient_test.go @@ -25,8 +25,8 @@ func TestNewHTTPClientLoggingRoundTripper(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() - cfg.Logger.Enabled = true - client, err := NewHTTPClient(cfg, "test-agent", "test-request", nil, ClientProviders{}) + cfg.Log.Enabled = true + client, err := New(cfg, "test-agent", "test-request", nil, ClientProviders{}) require.NoError(t, err) ctx := middleware.NewContextWithLogger(context.Background(), logger) @@ -50,8 +50,8 @@ func TestMustHTTPClientLoggingRoundTripper(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() - cfg.Logger.Enabled = true - client := MustHTTPClient(cfg, "test-agent", "test-request", nil, ClientProviders{}) + cfg.Log.Enabled = true + client := Must(cfg, "test-agent", "test-request", nil, ClientProviders{}) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) require.NoError(t, err) @@ -73,9 +73,8 @@ func TestNewHTTPClientWithOptsRoundTripper(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() - cfg.Logger.Enabled = true - client, err := NewHTTPClientWithOpts(ClientOpts{ - Config: *cfg, + cfg.Log.Enabled = true + client, err := NewWithOpts(cfg, Opts{ UserAgent: "test-agent", ReqType: "test-request", Delegate: nil, @@ -99,9 +98,8 @@ func TestMustHTTPClientWithOptsRoundTripper(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() - cfg.Logger.Enabled = true - client := MustHTTPClientWithOpts(ClientOpts{ - Config: *cfg, + cfg.Log.Enabled = true + client := MustWithOpts(cfg, Opts{ UserAgent: "test-agent", ReqType: "test-request", Delegate: nil, @@ -131,8 +129,7 @@ func TestMustHTTPClientWithOptsRoundTripperPolicy(t *testing.T) { cfg.Retries.Policy.ExponentialBackoffInitialInterval = 2 * time.Millisecond cfg.Retries.Policy.ExponentialBackoffMultiplier = 1.1 - client := MustHTTPClientWithOpts(ClientOpts{ - Config: *cfg, + client := MustWithOpts(cfg, Opts{ UserAgent: "test-agent", ReqType: "test-request", Delegate: nil, diff --git a/httpclient/logging_round_tripper.go b/httpclient/logging_round_tripper.go index 1fbf661..d0c0161 100644 --- a/httpclient/logging_round_tripper.go +++ b/httpclient/logging_round_tripper.go @@ -15,19 +15,19 @@ import ( "time" ) -// LoggerMode represents a mode of logging. -type LoggerMode string +// LoggingMode represents a mode of logging. +type LoggingMode string const ( - LoggerModeNone LoggerMode = "none" - LoggerModeAll LoggerMode = "all" - LoggerModeFailed LoggerMode = "failed" + LoggingModeNone LoggingMode = "none" + LoggingModeAll LoggingMode = "all" + LoggingModeFailed LoggingMode = "failed" ) // IsValid checks if the logger mode is valid. -func (lm LoggerMode) IsValid() bool { +func (lm LoggingMode) IsValid() bool { switch lm { - case LoggerModeNone, LoggerModeAll, LoggerModeFailed: + case LoggingModeNone, LoggingModeAll, LoggingModeFailed: return true } return false @@ -38,7 +38,7 @@ type LoggingRoundTripper struct { // Delegate is the next RoundTripper in the chain. Delegate http.RoundTripper - // ReqType is a type of request. + // ReqType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. ReqType string // Opts are the options for the logging round tripper. @@ -48,10 +48,11 @@ type LoggingRoundTripper struct { // LoggingRoundTripperOpts represents an options for LoggingRoundTripper. type LoggingRoundTripperOpts struct { // LoggerProvider is a function that provides a context-specific logger. + // middleware.GetLoggerFromContext is used by default. LoggerProvider func(ctx context.Context) log.FieldLogger // Mode of logging: none, all, failed. - Mode string + Mode LoggingMode // SlowRequestThreshold is a threshold for slow requests. SlowRequestThreshold time.Duration @@ -88,7 +89,7 @@ func (rt *LoggingRoundTripper) getLogger(ctx context.Context) log.FieldLogger { // RoundTrip adds logging capabilities to the HTTP transport. func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - if rt.Opts.Mode == string(LoggerModeNone) { + if rt.Opts.Mode == LoggingModeNone { return rt.Delegate.RoundTrip(r) } @@ -103,11 +104,10 @@ func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error args := []interface{}{r.Method, r.URL.String(), rt.ReqType, elapsed.Seconds(), err} message := common + "time taken %.3f, err %+v" loggerAtLevel := logger.Infof - shouldModeLog := true if resp != nil { - if rt.Opts.Mode == string(LoggerModeFailed) && resp.StatusCode < http.StatusBadRequest { - shouldModeLog = false + if rt.Opts.Mode == LoggingModeFailed && resp.StatusCode < http.StatusBadRequest { + return resp, err } args = []interface{}{r.Method, r.URL.String(), rt.ReqType, resp.StatusCode, elapsed.Seconds(), err} @@ -118,11 +118,13 @@ func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error loggerAtLevel = logger.Errorf } - if shouldModeLog { - loggerAtLevel(message, args...) - loggingParams := middleware.GetLoggingParamsFromContext(ctx) - if loggingParams != nil { - loggingParams.AddTimeSlotDurationInMs(fmt.Sprintf("external_request_%s_ms", rt.ReqType), elapsed) + loggerAtLevel(message, args...) + loggingParams := middleware.GetLoggingParamsFromContext(ctx) + if loggingParams != nil { + loggingParams.AddTimeSlotDurationInMs(fmt.Sprintf("external_request_%s_ms", rt.ReqType), elapsed) + requestID := middleware.GetRequestIDFromContext(ctx) + if requestID != "" { + loggingParams.ExtendFields(log.String("request_id", requestID)) } } } diff --git a/httpclient/logging_round_tripper_test.go b/httpclient/logging_round_tripper_test.go index 8dc4813..f8a708f 100644 --- a/httpclient/logging_round_tripper_test.go +++ b/httpclient/logging_round_tripper_test.go @@ -48,8 +48,8 @@ func TestMustHTTPClientLoggingRoundTripperError(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() - cfg.Logger.Enabled = true - client := MustHTTPClient(cfg, "test-agent", "test-request", nil, ClientProviders{}) + cfg.Log.Enabled = true + client := Must(cfg, "test-agent", "test-request", nil, ClientProviders{}) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) require.NoError(t, err) @@ -73,7 +73,7 @@ func TestMustHTTPClientLoggingRoundTripperDisabled(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() - client := MustHTTPClient(cfg, "test-agent", "test-request", nil, ClientProviders{}) + client := Must(cfg, "test-agent", "test-request", nil, ClientProviders{}) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) require.NoError(t, err) @@ -97,7 +97,7 @@ func TestNewLoggingRoundTripperModes(t *testing.T) { tests := []struct { name string method string - mode string + mode LoggingMode want int }{ { @@ -160,10 +160,10 @@ func TestMustHTTPClientLoggingRoundTripperOpts(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() - cfg.Logger.Enabled = true + cfg.Log.Enabled = true var loggerProviderCalled bool - client := MustHTTPClient(cfg, "test-agent", "test-request", nil, ClientProviders{ + client := Must(cfg, "test-agent", "test-request", nil, ClientProviders{ Logger: func(ctx context.Context) log.FieldLogger { loggerProviderCalled = true return logger diff --git a/httpclient/metrics_round_tripper.go b/httpclient/metrics_round_tripper.go index faf27dd..d6641c3 100644 --- a/httpclient/metrics_round_tripper.go +++ b/httpclient/metrics_round_tripper.go @@ -15,29 +15,37 @@ import ( ) var ( - ClientHTTPRequestDuration *prometheus.HistogramVec - ClassifyRequest func(r *http.Request, reqType string) string + metrics *PrometheusMetrics + ClassifyRequest func(r *http.Request, reqType string) string ) +type PrometheusMetrics struct { + // HTTPRequestDuration is a histogram of the http client requests durations. + HTTPRequestDuration *prometheus.HistogramVec +} + // MustInitAndRegisterMetrics initializes and registers external HTTP request duration metric. // Panic will be raised in case of error. func MustInitAndRegisterMetrics(namespace string) { - ClientHTTPRequestDuration = prometheus.NewHistogramVec( - prometheus.HistogramOpts{ - Namespace: namespace, - Name: "http_client_request_duration_seconds", - Help: "A histogram of the http client requests durations.", - Buckets: []float64{0.01, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 30, 60, 150, 300, 600}, - }, - []string{"type", "remote_address", "summary", "status"}, - ) - prometheus.MustRegister(ClientHTTPRequestDuration) + metrics = &PrometheusMetrics{ + HTTPRequestDuration: prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Namespace: namespace, + Name: "http_client_request_duration_seconds", + Help: "A histogram of the http client requests durations.", + Buckets: []float64{0.01, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 30, 60, 150, 300, 600}, + }, + []string{"type", "remote_address", "summary", "status"}, + ), + } + prometheus.MustRegister(metrics.HTTPRequestDuration) } // UnregisterMetrics unregisters external HTTP request duration metric. func UnregisterMetrics() { - if ClientHTTPRequestDuration != nil { - prometheus.Unregister(ClientHTTPRequestDuration) + if metrics != nil { + prometheus.Unregister(metrics.HTTPRequestDuration) + metrics = nil } } @@ -46,7 +54,7 @@ type MetricsRoundTripper struct { // Delegate is the next RoundTripper in the chain. Delegate http.RoundTripper - // ReqType is a type of request. + // ReqType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. ReqType string } @@ -57,7 +65,7 @@ func NewMetricsRoundTripper(delegate http.RoundTripper, reqType string) http.Rou // RoundTrip measures external requests done. func (rt *MetricsRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - if ClientHTTPRequestDuration == nil { + if metrics.HTTPRequestDuration == nil { return rt.Delegate.RoundTrip(r) } @@ -69,7 +77,7 @@ func (rt *MetricsRoundTripper) RoundTrip(r *http.Request) (*http.Response, error status = strconv.Itoa(resp.StatusCode) } - ClientHTTPRequestDuration.WithLabelValues( + metrics.HTTPRequestDuration.WithLabelValues( rt.ReqType, r.Host, requestSummary(r, rt.ReqType), status, ).Observe(time.Since(start).Seconds()) diff --git a/httpclient/metrics_round_tripper_test.go b/httpclient/metrics_round_tripper_test.go index a532522..78207e9 100644 --- a/httpclient/metrics_round_tripper_test.go +++ b/httpclient/metrics_round_tripper_test.go @@ -35,7 +35,7 @@ func TestNewMetricsRoundTripper(t *testing.T) { ch := make(chan prometheus.Metric, 1) go func() { - ClientHTTPRequestDuration.Collect(ch) + metrics.HTTPRequestDuration.Collect(ch) close(ch) }() diff --git a/httpclient/request_id_round_tripper.go b/httpclient/request_id_round_tripper.go index 6e51e4d..0b653c8 100644 --- a/httpclient/request_id_round_tripper.go +++ b/httpclient/request_id_round_tripper.go @@ -24,6 +24,7 @@ type RequestIDRoundTripper struct { // RequestIDRoundTripperOpts for X-Request-ID header to the request options. type RequestIDRoundTripperOpts struct { // RequestIDProvider is a function that provides a request ID. + // middleware.GetRequestIDFromContext is used by default. RequestIDProvider func(ctx context.Context) string } @@ -53,11 +54,11 @@ func (rt *RequestIDRoundTripper) getRequestIDProvider() func(ctx context.Context // RoundTrip adds X-Request-ID header to the request. func (rt *RequestIDRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - requestID := rt.getRequestIDProvider()(r.Context()) - if r.Header.Get("X-Request-ID") != "" || requestID == "" { + if r.Header.Get("X-Request-ID") != "" { return rt.Delegate.RoundTrip(r) } + requestID := rt.getRequestIDProvider()(r.Context()) r = CloneHTTPRequest(r) r.Header.Set("X-Request-ID", requestID) return rt.Delegate.RoundTrip(r) From 5b42988681497a0c5c8c6f7eb01dc912410d6b82 Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Thu, 9 Jan 2025 19:20:54 +0000 Subject: [PATCH 05/16] Metrics collector and roundtriper opts --- httpclient/config.go | 43 ++++++------ httpclient/httpclient.go | 19 +++-- httpclient/httpclient_test.go | 4 +- httpclient/logging_round_tripper.go | 55 +++++++++------ httpclient/logging_round_tripper_test.go | 19 +++-- httpclient/metrics_round_tripper.go | 89 +++++++++++++++--------- httpclient/metrics_round_tripper_test.go | 11 +-- 7 files changed, 151 insertions(+), 89 deletions(-) diff --git a/httpclient/config.go b/httpclient/config.go index 9c47e33..011accf 100644 --- a/httpclient/config.go +++ b/httpclient/config.go @@ -15,6 +15,9 @@ import ( ) const ( + // DefaultReqType is a default request type. + DefaultReqType = "go-appkit" + // DefaultClientWaitTimeout is a default timeout for a client to wait for a request. DefaultClientWaitTimeout = 10 * time.Second @@ -48,16 +51,16 @@ var _ config.KeyPrefixProvider = (*Config)(nil) // RateLimitConfig represents configuration options for HTTP client rate limits. type RateLimitConfig struct { // Enabled is a flag that enables rate limiting. - Enabled bool `mapstructure:"enabled"` + Enabled bool // Limit is the maximum number of requests that can be made. - Limit int `mapstructure:"limit"` + Limit int // Burst allow temporary spikes in request rate. - Burst int `mapstructure:"burst"` + Burst int // WaitTimeout is the maximum time to wait for a request to be made. - WaitTimeout time.Duration `mapstructure:"waitTimeout"` + WaitTimeout time.Duration } // Set is part of config interface implementation. @@ -116,16 +119,16 @@ func (c *RateLimitConfig) TransportOpts() RateLimitingRoundTripperOpts { // PolicyConfig represents configuration options for policy retry. type PolicyConfig struct { // Strategy is a strategy for retry policy. - Strategy string `mapstructure:"strategy"` + Strategy string // ExponentialBackoffInitialInterval is the initial interval for exponential backoff. - ExponentialBackoffInitialInterval time.Duration `mapstructure:"exponentialBackoffInitialInterval"` + ExponentialBackoffInitialInterval time.Duration // ExponentialBackoffMultiplier is the multiplier for exponential backoff. - ExponentialBackoffMultiplier float64 `mapstructure:"exponentialBackoffMultiplier"` + ExponentialBackoffMultiplier float64 // ConstantBackoffInterval is the interval for constant backoff. - ConstantBackoffInterval time.Duration `mapstructure:"constantBackoffInterval"` + ConstantBackoffInterval time.Duration } // Set is part of config interface implementation. @@ -183,13 +186,13 @@ func (c *PolicyConfig) SetProviderDefaults(_ config.DataProvider) {} // RetriesConfig represents configuration options for HTTP client retries policy. type RetriesConfig struct { // Enabled is a flag that enables retries. - Enabled bool `mapstructure:"enabled"` + Enabled bool // MaxAttempts is the maximum number of attempts to retry the request. - MaxAttempts int `mapstructure:"maxAttempts"` + MaxAttempts int // Policy of a retry: [exponential, constant]. default is exponential. - Policy PolicyConfig `mapstructure:"policy"` + Policy PolicyConfig } // GetPolicy returns a retry policy based on strategy or nil if none is provided. @@ -253,13 +256,13 @@ func (c *RetriesConfig) TransportOpts() RetryableRoundTripperOpts { // LogConfig represents configuration options for HTTP client logs. type LogConfig struct { // Enabled is a flag that enables logging. - Enabled bool `mapstructure:"enabled"` + Enabled bool // SlowRequestThreshold is a threshold for slow requests. - SlowRequestThreshold time.Duration `mapstructure:"slowRequestThreshold"` + SlowRequestThreshold time.Duration // Mode of logging. - Mode LoggingMode `mapstructure:"mode"` + Mode LoggingMode } // Set is part of config interface implementation. @@ -310,7 +313,7 @@ func (c *LogConfig) TransportOpts() LoggingRoundTripperOpts { // MetricsConfig represents configuration options for HTTP client logs. type MetricsConfig struct { // Enabled is a flag that enables metrics. - Enabled bool `mapstructure:"enabled"` + Enabled bool } // Set is part of config interface implementation. @@ -330,19 +333,19 @@ func (c *MetricsConfig) SetProviderDefaults(_ config.DataProvider) {} // Config represents options for HTTP client configuration. type Config struct { // Retries is a configuration for HTTP client retries policy. - Retries RetriesConfig `mapstructure:"retries"` + Retries RetriesConfig // RateLimits is a configuration for HTTP client rate limits. - RateLimits RateLimitConfig `mapstructure:"rateLimits"` + RateLimits RateLimitConfig // Log is a configuration for HTTP client logs. - Log LogConfig `mapstructure:"log"` + Log LogConfig // Metrics is a configuration for HTTP client metrics. - Metrics MetricsConfig `mapstructure:"metrics"` + Metrics MetricsConfig // Timeout is the maximum time to wait for a request to be made. - Timeout time.Duration `mapstructure:"timeout"` + Timeout time.Duration // keyPrefix is a prefix for configuration parameters. keyPrefix string diff --git a/httpclient/httpclient.go b/httpclient/httpclient.go index 1feaa91..0df66fc 100644 --- a/httpclient/httpclient.go +++ b/httpclient/httpclient.go @@ -49,6 +49,7 @@ func New( reqType string, delegate http.RoundTripper, providers ClientProviders, + collector MetricsCollector, ) (*http.Client, error) { var err error @@ -59,11 +60,15 @@ func New( if cfg.Log.Enabled { opts := cfg.Log.TransportOpts() opts.LoggerProvider = providers.Logger - delegate = NewLoggingRoundTripperWithOpts(delegate, reqType, opts) + opts.ReqType = reqType + delegate = NewLoggingRoundTripperWithOpts(delegate, opts) } if cfg.Metrics.Enabled { - delegate = NewMetricsRoundTripper(delegate, reqType) + delegate = NewMetricsRoundTripperWithOpts(delegate, MetricsRoundTripperOpts{ + ReqType: reqType, + Collector: collector, + }) } if cfg.RateLimits.Enabled { @@ -104,8 +109,9 @@ func Must( reqType string, delegate http.RoundTripper, providers ClientProviders, + collector MetricsCollector, ) *http.Client { - client, err := New(cfg, userAgent, reqType, delegate, providers) + client, err := New(cfg, userAgent, reqType, delegate, providers, collector) if err != nil { panic(err) } @@ -126,18 +132,21 @@ type Opts struct { // Providers are the functions that provide a context-specific logger and request ID. Providers ClientProviders + + // Collector is a metrics collector. + Collector MetricsCollector } // NewWithOpts wraps delegate transports with options // logging, metrics, rate limiting, retryable, user agent, request id // and returns an error if any occurs. func NewWithOpts(cfg *Config, opts Opts) (*http.Client, error) { - return New(cfg, opts.UserAgent, opts.ReqType, opts.Delegate, opts.Providers) + return New(cfg, opts.UserAgent, opts.ReqType, opts.Delegate, opts.Providers, opts.Collector) } // MustWithOpts wraps delegate transports with options // logging, metrics, rate limiting, retryable, user agent, request id // and panics if any error occurs. func MustWithOpts(cfg *Config, opts Opts) *http.Client { - return Must(cfg, opts.UserAgent, opts.ReqType, opts.Delegate, opts.Providers) + return Must(cfg, opts.UserAgent, opts.ReqType, opts.Delegate, opts.Providers, opts.Collector) } diff --git a/httpclient/httpclient_test.go b/httpclient/httpclient_test.go index 7029d42..0a35535 100644 --- a/httpclient/httpclient_test.go +++ b/httpclient/httpclient_test.go @@ -26,7 +26,7 @@ func TestNewHTTPClientLoggingRoundTripper(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() cfg.Log.Enabled = true - client, err := New(cfg, "test-agent", "test-request", nil, ClientProviders{}) + client, err := New(cfg, "test-agent", "test-request", nil, ClientProviders{}, nil) require.NoError(t, err) ctx := middleware.NewContextWithLogger(context.Background(), logger) @@ -51,7 +51,7 @@ func TestMustHTTPClientLoggingRoundTripper(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() cfg.Log.Enabled = true - client := Must(cfg, "test-agent", "test-request", nil, ClientProviders{}) + client := Must(cfg, "test-agent", "test-request", nil, ClientProviders{}, nil) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) require.NoError(t, err) diff --git a/httpclient/logging_round_tripper.go b/httpclient/logging_round_tripper.go index d0c0161..60ac894 100644 --- a/httpclient/logging_round_tripper.go +++ b/httpclient/logging_round_tripper.go @@ -41,47 +41,62 @@ type LoggingRoundTripper struct { // ReqType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. ReqType string - // Opts are the options for the logging round tripper. - Opts LoggingRoundTripperOpts -} + // Mode of logging: none, all, failed. + Mode LoggingMode + + // SlowRequestThreshold is a threshold for slow requests. + SlowRequestThreshold time.Duration -// LoggingRoundTripperOpts represents an options for LoggingRoundTripper. -type LoggingRoundTripperOpts struct { // LoggerProvider is a function that provides a context-specific logger. // middleware.GetLoggerFromContext is used by default. LoggerProvider func(ctx context.Context) log.FieldLogger +} + +// LoggingRoundTripperOpts represents an options for LoggingRoundTripper. +type LoggingRoundTripperOpts struct { + // ReqType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. + ReqType string // Mode of logging: none, all, failed. Mode LoggingMode // SlowRequestThreshold is a threshold for slow requests. SlowRequestThreshold time.Duration + + // LoggerProvider is a function that provides a context-specific logger. + // middleware.GetLoggerFromContext is used by default. + LoggerProvider func(ctx context.Context) log.FieldLogger } // NewLoggingRoundTripper creates an HTTP transport that log requests. -func NewLoggingRoundTripper(delegate http.RoundTripper, reqType string) http.RoundTripper { - return &LoggingRoundTripper{ - Delegate: delegate, - ReqType: reqType, - Opts: LoggingRoundTripperOpts{}, - } +func NewLoggingRoundTripper(delegate http.RoundTripper) http.RoundTripper { + return NewLoggingRoundTripperWithOpts(delegate, LoggingRoundTripperOpts{ + ReqType: DefaultReqType, + }) } // NewLoggingRoundTripperWithOpts creates an HTTP transport that log requests with options. func NewLoggingRoundTripperWithOpts( - delegate http.RoundTripper, reqType string, opts LoggingRoundTripperOpts, + delegate http.RoundTripper, opts LoggingRoundTripperOpts, ) http.RoundTripper { + reqType := opts.ReqType + if reqType == "" { + reqType = DefaultReqType + } + return &LoggingRoundTripper{ - Delegate: delegate, - ReqType: reqType, - Opts: opts, + Delegate: delegate, + ReqType: reqType, + Mode: opts.Mode, + SlowRequestThreshold: opts.SlowRequestThreshold, + LoggerProvider: opts.LoggerProvider, } } // getLogger returns a logger from the context or from the options. func (rt *LoggingRoundTripper) getLogger(ctx context.Context) log.FieldLogger { - if rt.Opts.LoggerProvider != nil { - return rt.Opts.LoggerProvider(ctx) + if rt.LoggerProvider != nil { + return rt.LoggerProvider(ctx) } return middleware.GetLoggerFromContext(ctx) @@ -89,7 +104,7 @@ func (rt *LoggingRoundTripper) getLogger(ctx context.Context) log.FieldLogger { // RoundTrip adds logging capabilities to the HTTP transport. func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - if rt.Opts.Mode == LoggingModeNone { + if rt.Mode == LoggingModeNone { return rt.Delegate.RoundTrip(r) } @@ -99,14 +114,14 @@ func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error resp, err := rt.Delegate.RoundTrip(r) elapsed := time.Since(start) - if logger != nil && elapsed >= rt.Opts.SlowRequestThreshold { + if logger != nil && elapsed >= rt.SlowRequestThreshold { common := "client http request %s %s req type %s " args := []interface{}{r.Method, r.URL.String(), rt.ReqType, elapsed.Seconds(), err} message := common + "time taken %.3f, err %+v" loggerAtLevel := logger.Infof if resp != nil { - if rt.Opts.Mode == LoggingModeFailed && resp.StatusCode < http.StatusBadRequest { + if rt.Mode == LoggingModeFailed && resp.StatusCode < http.StatusBadRequest { return resp, err } diff --git a/httpclient/logging_round_tripper_test.go b/httpclient/logging_round_tripper_test.go index f8a708f..195b0e1 100644 --- a/httpclient/logging_round_tripper_test.go +++ b/httpclient/logging_round_tripper_test.go @@ -25,7 +25,7 @@ func TestNewLoggingRoundTripper(t *testing.T) { defer server.Close() logger := logtest.NewRecorder() - loggerRoundTripper := NewLoggingRoundTripper(http.DefaultTransport, "test-request") + loggerRoundTripper := NewLoggingRoundTripper(http.DefaultTransport) client := &http.Client{Transport: loggerRoundTripper} ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) @@ -37,7 +37,10 @@ func TestNewLoggingRoundTripper(t *testing.T) { require.NotEmpty(t, logger.Entries()) loggerEntry := logger.Entries()[0] - require.Contains(t, loggerEntry.Text, "client http request POST "+server.URL+" req type test-request status code 418") + require.Contains( + t, loggerEntry.Text, + "client http request POST "+server.URL+" req type "+DefaultReqType+" status code 418", + ) } func TestMustHTTPClientLoggingRoundTripperError(t *testing.T) { @@ -49,7 +52,7 @@ func TestMustHTTPClientLoggingRoundTripperError(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() cfg.Log.Enabled = true - client := Must(cfg, "test-agent", "test-request", nil, ClientProviders{}) + client := Must(cfg, "test-agent", "test-request", nil, ClientProviders{}, nil) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) require.NoError(t, err) @@ -73,7 +76,7 @@ func TestMustHTTPClientLoggingRoundTripperDisabled(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() - client := Must(cfg, "test-agent", "test-request", nil, ClientProviders{}) + client := Must(cfg, "test-agent", "test-request", nil, ClientProviders{}, nil) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) require.NoError(t, err) @@ -136,8 +139,10 @@ func TestNewLoggingRoundTripperModes(t *testing.T) { logger := logtest.NewRecorder() loggerRoundTripper := NewLoggingRoundTripperWithOpts( http.DefaultTransport, - "test-request", - LoggingRoundTripperOpts{Mode: tt.mode}, + LoggingRoundTripperOpts{ + ReqType: "test-request", + Mode: tt.mode, + }, ) client := &http.Client{Transport: loggerRoundTripper} ctx := middleware.NewContextWithLogger(context.Background(), logger) @@ -168,7 +173,7 @@ func TestMustHTTPClientLoggingRoundTripperOpts(t *testing.T) { loggerProviderCalled = true return logger }, - }) + }, nil) ctx := middleware.NewContextWithLogger(context.Background(), nil) req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) require.NoError(t, err) diff --git a/httpclient/metrics_round_tripper.go b/httpclient/metrics_round_tripper.go index d6641c3..5d703f7 100644 --- a/httpclient/metrics_round_tripper.go +++ b/httpclient/metrics_round_tripper.go @@ -15,40 +15,48 @@ import ( ) var ( - metrics *PrometheusMetrics + // ClassifyRequest does request classification, producing non-parameterized summary for given request. ClassifyRequest func(r *http.Request, reqType string) string ) -type PrometheusMetrics struct { - // HTTPRequestDuration is a histogram of the http client requests durations. - HTTPRequestDuration *prometheus.HistogramVec +// MetricsCollector is an interface for collecting metrics for client requests. +type MetricsCollector interface { + // RequestDuration observes the duration of the request and the status code. + RequestDuration(reqType, remoteAddress, summary, status string, startTime time.Time) } -// MustInitAndRegisterMetrics initializes and registers external HTTP request duration metric. -// Panic will be raised in case of error. -func MustInitAndRegisterMetrics(namespace string) { - metrics = &PrometheusMetrics{ - HTTPRequestDuration: prometheus.NewHistogramVec( - prometheus.HistogramOpts{ - Namespace: namespace, - Name: "http_client_request_duration_seconds", - Help: "A histogram of the http client requests durations.", - Buckets: []float64{0.01, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 30, 60, 150, 300, 600}, - }, - []string{"type", "remote_address", "summary", "status"}, - ), - } - prometheus.MustRegister(metrics.HTTPRequestDuration) +// PrometheusMetricsCollector is a Prometheus metrics collector. +type PrometheusMetricsCollector struct { + // Durations is a histogram of the http client requests durations. + Durations *prometheus.HistogramVec } -// UnregisterMetrics unregisters external HTTP request duration metric. -func UnregisterMetrics() { - if metrics != nil { - prometheus.Unregister(metrics.HTTPRequestDuration) - metrics = nil +func NewPrometheusMetricsCollector(namespace string) *PrometheusMetricsCollector { + return &PrometheusMetricsCollector{ + Durations: prometheus.NewHistogramVec(prometheus.HistogramOpts{ + Namespace: namespace, + Name: "http_client_request_duration_seconds", + Help: "A histogram of the http client requests durations.", + Buckets: []float64{0.01, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 30, 60, 150, 300, 600}, + }, []string{"type", "remote_address", "summary", "status"}), } } +// MustRegister registers the Prometheus metrics. +func (p *PrometheusMetricsCollector) MustRegister() { + prometheus.MustRegister(p.Durations) +} + +// RequestDuration observes the duration of the request and the status code. +func (p *PrometheusMetricsCollector) RequestDuration(reqType, host, summary, status string, start time.Time) { + p.Durations.WithLabelValues(reqType, host, summary, status).Observe(time.Since(start).Seconds()) +} + +// Unregister the Prometheus metrics. +func (p *PrometheusMetricsCollector) Unregister() { + prometheus.Unregister(p.Durations) +} + // MetricsRoundTripper is an HTTP transport that measures requests done. type MetricsRoundTripper struct { // Delegate is the next RoundTripper in the chain. @@ -56,16 +64,38 @@ type MetricsRoundTripper struct { // ReqType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. ReqType string + + // Collector is a metrics collector. + Collector MetricsCollector +} + +// MetricsRoundTripperOpts is an HTTP transport that measures requests done. +type MetricsRoundTripperOpts struct { + // ReqType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. + ReqType string + + // Collector is a metrics collector. + Collector MetricsCollector } // NewMetricsRoundTripper creates an HTTP transport that measures requests done. -func NewMetricsRoundTripper(delegate http.RoundTripper, reqType string) http.RoundTripper { - return &MetricsRoundTripper{Delegate: delegate, ReqType: reqType} +func NewMetricsRoundTripper(delegate http.RoundTripper) http.RoundTripper { + return NewMetricsRoundTripperWithOpts(delegate, MetricsRoundTripperOpts{}) +} + +// NewMetricsRoundTripperWithOpts creates an HTTP transport that measures requests done. +func NewMetricsRoundTripperWithOpts(delegate http.RoundTripper, opts MetricsRoundTripperOpts) http.RoundTripper { + reqType := DefaultReqType + if opts.ReqType == "" { + reqType = opts.ReqType + } + + return &MetricsRoundTripper{Delegate: delegate, ReqType: reqType, Collector: opts.Collector} } // RoundTrip measures external requests done. func (rt *MetricsRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - if metrics.HTTPRequestDuration == nil { + if rt.Collector == nil { return rt.Delegate.RoundTrip(r) } @@ -77,10 +107,7 @@ func (rt *MetricsRoundTripper) RoundTrip(r *http.Request) (*http.Response, error status = strconv.Itoa(resp.StatusCode) } - metrics.HTTPRequestDuration.WithLabelValues( - rt.ReqType, r.Host, requestSummary(r, rt.ReqType), status, - ).Observe(time.Since(start).Seconds()) - + rt.Collector.RequestDuration(rt.ReqType, r.Host, requestSummary(r, rt.ReqType), status, start) return resp, err } diff --git a/httpclient/metrics_round_tripper_test.go b/httpclient/metrics_round_tripper_test.go index 78207e9..586ad6f 100644 --- a/httpclient/metrics_round_tripper_test.go +++ b/httpclient/metrics_round_tripper_test.go @@ -21,10 +21,13 @@ func TestNewMetricsRoundTripper(t *testing.T) { })) defer server.Close() - MustInitAndRegisterMetrics("") - defer UnregisterMetrics() + collector := NewPrometheusMetricsCollector("") + defer collector.Unregister() - metricsRoundTripper := NewMetricsRoundTripper(http.DefaultTransport, "test-request") + metricsRoundTripper := NewMetricsRoundTripperWithOpts(http.DefaultTransport, MetricsRoundTripperOpts{ + ReqType: "test-request", + Collector: collector, + }) client := &http.Client{Transport: metricsRoundTripper} req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, server.URL, nil) require.NoError(t, err) @@ -35,7 +38,7 @@ func TestNewMetricsRoundTripper(t *testing.T) { ch := make(chan prometheus.Metric, 1) go func() { - metrics.HTTPRequestDuration.Collect(ch) + collector.Durations.Collect(ch) close(ch) }() From cf5c9ab3ca08bfee3aac0f3a5d0396aa809da802 Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Thu, 9 Jan 2025 21:20:16 +0000 Subject: [PATCH 06/16] Client opts, retry policy type and logging request id --- httpclient/config.go | 13 ++- httpclient/httpclient.go | 120 +++++++++++++---------- httpclient/httpclient_test.go | 29 ++++-- httpclient/logging_round_tripper.go | 10 +- httpclient/logging_round_tripper_test.go | 45 +++++++-- 5 files changed, 141 insertions(+), 76 deletions(-) diff --git a/httpclient/config.go b/httpclient/config.go index 011accf..4e034be 100644 --- a/httpclient/config.go +++ b/httpclient/config.go @@ -14,6 +14,9 @@ import ( "time" ) +// RetryPolicy represents a retry policy strategy. +type RetryPolicy string + const ( // DefaultReqType is a default request type. DefaultReqType = "go-appkit" @@ -22,10 +25,10 @@ const ( DefaultClientWaitTimeout = 10 * time.Second // RetryPolicyExponential is a policy for exponential retries. - RetryPolicyExponential = "exponential" + RetryPolicyExponential RetryPolicy = "exponential" // RetryPolicyConstant is a policy for constant retries. - RetryPolicyConstant = "constant" + RetryPolicyConstant RetryPolicy = "constant" // configuration properties cfgKeyRetriesEnabled = "retries.enabled" @@ -118,8 +121,8 @@ func (c *RateLimitConfig) TransportOpts() RateLimitingRoundTripperOpts { // PolicyConfig represents configuration options for policy retry. type PolicyConfig struct { - // Strategy is a strategy for retry policy. - Strategy string + // Strategy is a retry policy strategy. + Strategy RetryPolicy // ExponentialBackoffInitialInterval is the initial interval for exponential backoff. ExponentialBackoffInitialInterval time.Duration @@ -137,7 +140,7 @@ func (c *PolicyConfig) Set(dp config.DataProvider) (err error) { if err != nil { return err } - c.Strategy = strategy + c.Strategy = RetryPolicy(strategy) if c.Strategy != "" && c.Strategy != RetryPolicyExponential && c.Strategy != RetryPolicyConstant { return dp.WrapKeyErr(cfgKeyRetriesPolicyStrategy, errors.New("must be one of: [exponential, constant]")) diff --git a/httpclient/httpclient.go b/httpclient/httpclient.go index 0df66fc..e9b6e57 100644 --- a/httpclient/httpclient.go +++ b/httpclient/httpclient.go @@ -32,45 +32,19 @@ func CloneHTTPHeader(in http.Header) http.Header { return out } -// ClientProviders for further customization of the client logging and request id. -type ClientProviders struct { - // Logger is a function that provides a context-specific logger. - Logger func(ctx context.Context) log.FieldLogger - - // RequestID is a function that provides a request ID. - RequestID func(ctx context.Context) string -} - -// New wraps delegate transports with logging, metrics, rate limiting, retryable, user agent, request id +// New wraps delegate transports with logging, rate limiting, retryable, request id // and returns an error if any occurs. -func New( - cfg *Config, - userAgent string, - reqType string, - delegate http.RoundTripper, - providers ClientProviders, - collector MetricsCollector, -) (*http.Client, error) { +func New(cfg *Config) (*http.Client, error) { var err error + var delegate http.RoundTripper - if delegate == nil { - delegate = http.DefaultTransport.(*http.Transport).Clone() - } + delegate = http.DefaultTransport.(*http.Transport).Clone() if cfg.Log.Enabled { opts := cfg.Log.TransportOpts() - opts.LoggerProvider = providers.Logger - opts.ReqType = reqType delegate = NewLoggingRoundTripperWithOpts(delegate, opts) } - if cfg.Metrics.Enabled { - delegate = NewMetricsRoundTripperWithOpts(delegate, MetricsRoundTripperOpts{ - ReqType: reqType, - Collector: collector, - }) - } - if cfg.RateLimits.Enabled { delegate, err = NewRateLimitingRoundTripperWithOpts( delegate, cfg.RateLimits.Limit, cfg.RateLimits.TransportOpts(), @@ -80,17 +54,10 @@ func New( } } - if userAgent != "" { - delegate = NewUserAgentRoundTripper(delegate, userAgent) - } - - delegate = NewRequestIDRoundTripperWithOpts(delegate, RequestIDRoundTripperOpts{ - RequestIDProvider: providers.RequestID, - }) + delegate = NewRequestIDRoundTripper(delegate) if cfg.Retries.Enabled { opts := cfg.Retries.TransportOpts() - opts.LoggerProvider = providers.Logger opts.BackoffPolicy = cfg.Retries.GetPolicy() delegate, err = NewRetryableRoundTripperWithOpts(delegate, opts) if err != nil { @@ -101,17 +68,10 @@ func New( return &http.Client{Transport: delegate, Timeout: cfg.Timeout}, nil } -// Must wraps delegate transports with logging, metrics, rate limiting, retryable, user agent, request id +// Must wraps delegate transports with logging, rate limiting, retryable, request id // and panics if any error occurs. -func Must( - cfg *Config, - userAgent string, - reqType string, - delegate http.RoundTripper, - providers ClientProviders, - collector MetricsCollector, -) *http.Client { - client, err := New(cfg, userAgent, reqType, delegate, providers, collector) +func Must(cfg *Config) *http.Client { + client, err := New(cfg) if err != nil { panic(err) } @@ -130,8 +90,11 @@ type Opts struct { // Delegate is the next RoundTripper in the chain. Delegate http.RoundTripper - // Providers are the functions that provide a context-specific logger and request ID. - Providers ClientProviders + // LoggerProvider is a function that provides a context-specific logger. + LoggerProvider func(ctx context.Context) log.FieldLogger + + // RequestIDProvider is a function that provides a request ID. + RequestIDProvider func(ctx context.Context) string // Collector is a metrics collector. Collector MetricsCollector @@ -141,12 +104,65 @@ type Opts struct { // logging, metrics, rate limiting, retryable, user agent, request id // and returns an error if any occurs. func NewWithOpts(cfg *Config, opts Opts) (*http.Client, error) { - return New(cfg, opts.UserAgent, opts.ReqType, opts.Delegate, opts.Providers, opts.Collector) + var err error + delegate := opts.Delegate + + if delegate == nil { + delegate = http.DefaultTransport.(*http.Transport).Clone() + } + + if cfg.Log.Enabled { + logOpts := cfg.Log.TransportOpts() + logOpts.LoggerProvider = opts.LoggerProvider + logOpts.ReqType = opts.ReqType + delegate = NewLoggingRoundTripperWithOpts(delegate, logOpts) + } + + if cfg.Metrics.Enabled { + delegate = NewMetricsRoundTripperWithOpts(delegate, MetricsRoundTripperOpts{ + ReqType: opts.ReqType, + Collector: opts.Collector, + }) + } + + if cfg.RateLimits.Enabled { + delegate, err = NewRateLimitingRoundTripperWithOpts( + delegate, cfg.RateLimits.Limit, cfg.RateLimits.TransportOpts(), + ) + if err != nil { + return nil, fmt.Errorf("create rate limiting round tripper: %w", err) + } + } + + if opts.ReqType != "" { + delegate = NewUserAgentRoundTripper(delegate, opts.ReqType) + } + + delegate = NewRequestIDRoundTripperWithOpts(delegate, RequestIDRoundTripperOpts{ + RequestIDProvider: opts.RequestIDProvider, + }) + + if cfg.Retries.Enabled { + retryOpts := cfg.Retries.TransportOpts() + retryOpts.LoggerProvider = opts.LoggerProvider + retryOpts.BackoffPolicy = cfg.Retries.GetPolicy() + delegate, err = NewRetryableRoundTripperWithOpts(delegate, retryOpts) + if err != nil { + return nil, fmt.Errorf("create retryable round tripper: %w", err) + } + } + + return &http.Client{Transport: delegate, Timeout: cfg.Timeout}, nil } // MustWithOpts wraps delegate transports with options // logging, metrics, rate limiting, retryable, user agent, request id // and panics if any error occurs. func MustWithOpts(cfg *Config, opts Opts) *http.Client { - return Must(cfg, opts.UserAgent, opts.ReqType, opts.Delegate, opts.Providers, opts.Collector) + client, err := NewWithOpts(cfg, opts) + if err != nil { + panic(err) + } + + return client } diff --git a/httpclient/httpclient_test.go b/httpclient/httpclient_test.go index 0a35535..7c66586 100644 --- a/httpclient/httpclient_test.go +++ b/httpclient/httpclient_test.go @@ -8,6 +8,7 @@ package httpclient import ( "context" + "fmt" "github.com/acronis/go-appkit/httpserver/middleware" "github.com/acronis/go-appkit/log/logtest" "github.com/stretchr/testify/require" @@ -26,7 +27,7 @@ func TestNewHTTPClientLoggingRoundTripper(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() cfg.Log.Enabled = true - client, err := New(cfg, "test-agent", "test-request", nil, ClientProviders{}, nil) + client, err := New(cfg) require.NoError(t, err) ctx := middleware.NewContextWithLogger(context.Background(), logger) @@ -37,9 +38,10 @@ func TestNewHTTPClientLoggingRoundTripper(t *testing.T) { defer func() { _ = r.Body.Close() }() require.NoError(t, err) require.NotEmpty(t, logger.Entries()) - - loggerEntry := logger.Entries()[0] - require.Contains(t, loggerEntry.Text, "client http request POST "+server.URL+" req type test-request status code 418") + require.Contains( + t, logger.Entries()[0].Text, + fmt.Sprintf("client http request POST %s req type %s status code 418", server.URL, DefaultReqType), + ) } func TestMustHTTPClientLoggingRoundTripper(t *testing.T) { @@ -51,7 +53,7 @@ func TestMustHTTPClientLoggingRoundTripper(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() cfg.Log.Enabled = true - client := Must(cfg, "test-agent", "test-request", nil, ClientProviders{}, nil) + client := Must(cfg) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) require.NoError(t, err) @@ -60,9 +62,10 @@ func TestMustHTTPClientLoggingRoundTripper(t *testing.T) { defer func() { _ = r.Body.Close() }() require.NoError(t, err) require.NotEmpty(t, logger.Entries()) - - loggerEntry := logger.Entries()[0] - require.Contains(t, loggerEntry.Text, "client http request POST "+server.URL+" req type test-request status code 418") + require.Contains( + t, logger.Entries()[0].Text, + fmt.Sprintf("client http request POST %s req type %s status code 418", server.URL, DefaultReqType), + ) } func TestNewHTTPClientWithOptsRoundTripper(t *testing.T) { @@ -88,6 +91,11 @@ func TestNewHTTPClientWithOptsRoundTripper(t *testing.T) { defer func() { _ = r.Body.Close() }() require.NoError(t, err) require.NotEmpty(t, logger.Entries()) + require.Contains( + t, logger.Entries()[0].Text, fmt.Sprintf( + "client http request POST %s req type test-request status code 418", server.URL, + ), + ) } func TestMustHTTPClientWithOptsRoundTripper(t *testing.T) { @@ -112,6 +120,11 @@ func TestMustHTTPClientWithOptsRoundTripper(t *testing.T) { defer func() { _ = r.Body.Close() }() require.NoError(t, err) require.NotEmpty(t, logger.Entries()) + require.Contains( + t, logger.Entries()[0].Text, fmt.Sprintf( + "client http request POST %s req type test-request status code 418", server.URL, + ), + ) } func TestMustHTTPClientWithOptsRoundTripperPolicy(t *testing.T) { diff --git a/httpclient/logging_round_tripper.go b/httpclient/logging_round_tripper.go index 60ac894..5c4cf17 100644 --- a/httpclient/logging_round_tripper.go +++ b/httpclient/logging_round_tripper.go @@ -133,14 +133,16 @@ func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error loggerAtLevel = logger.Errorf } + requestID := middleware.GetRequestIDFromContext(ctx) + if requestID != "" { + message += " request id %s " + args = append(args, requestID) + } + loggerAtLevel(message, args...) loggingParams := middleware.GetLoggingParamsFromContext(ctx) if loggingParams != nil { loggingParams.AddTimeSlotDurationInMs(fmt.Sprintf("external_request_%s_ms", rt.ReqType), elapsed) - requestID := middleware.GetRequestIDFromContext(ctx) - if requestID != "" { - loggingParams.ExtendFields(log.String("request_id", requestID)) - } } } diff --git a/httpclient/logging_round_tripper_test.go b/httpclient/logging_round_tripper_test.go index 195b0e1..838f6f5 100644 --- a/httpclient/logging_round_tripper_test.go +++ b/httpclient/logging_round_tripper_test.go @@ -8,6 +8,7 @@ package httpclient import ( "context" + "fmt" "github.com/acronis/go-appkit/httpserver/middleware" "github.com/acronis/go-appkit/log" "github.com/acronis/go-appkit/log/logtest" @@ -39,7 +40,7 @@ func TestNewLoggingRoundTripper(t *testing.T) { loggerEntry := logger.Entries()[0] require.Contains( t, loggerEntry.Text, - "client http request POST "+server.URL+" req type "+DefaultReqType+" status code 418", + fmt.Sprintf("client http request POST %s req type %s status code 418", server.URL, DefaultReqType), ) } @@ -52,7 +53,7 @@ func TestMustHTTPClientLoggingRoundTripperError(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() cfg.Log.Enabled = true - client := Must(cfg, "test-agent", "test-request", nil, ClientProviders{}, nil) + client := Must(cfg) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) require.NoError(t, err) @@ -63,7 +64,10 @@ func TestMustHTTPClientLoggingRoundTripperError(t *testing.T) { require.NotEmpty(t, logger.Entries()) loggerEntry := logger.Entries()[0] - require.Contains(t, loggerEntry.Text, "client http request POST "+serverURL+" req type test-request") + require.Contains( + t, loggerEntry.Text, + fmt.Sprintf("client http request POST %s req type %s", serverURL, DefaultReqType), + ) require.Contains(t, loggerEntry.Text, "err dial tcp "+ln.Addr().String()+": connect: connection refused") require.NotContains(t, loggerEntry.Text, "status code") } @@ -76,7 +80,7 @@ func TestMustHTTPClientLoggingRoundTripperDisabled(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() - client := Must(cfg, "test-agent", "test-request", nil, ClientProviders{}, nil) + client := Must(cfg) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) require.NoError(t, err) @@ -168,12 +172,15 @@ func TestMustHTTPClientLoggingRoundTripperOpts(t *testing.T) { cfg.Log.Enabled = true var loggerProviderCalled bool - client := Must(cfg, "test-agent", "test-request", nil, ClientProviders{ - Logger: func(ctx context.Context) log.FieldLogger { + client := MustWithOpts(cfg, Opts{ + UserAgent: "test-agent", + ReqType: "test-request", + LoggerProvider: func(ctx context.Context) log.FieldLogger { loggerProviderCalled = true return logger }, - }, nil) + }) + ctx := middleware.NewContextWithLogger(context.Background(), nil) req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) require.NoError(t, err) @@ -184,3 +191,27 @@ func TestMustHTTPClientLoggingRoundTripperOpts(t *testing.T) { require.True(t, loggerProviderCalled) require.Len(t, logger.Entries(), 1) } + +func TestNewLoggingRoundTripperWithRequestID(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusTeapot) + })) + defer server.Close() + + requestID := "12345" + logger := logtest.NewRecorder() + loggerRoundTripper := NewLoggingRoundTripper(http.DefaultTransport) + client := &http.Client{Transport: loggerRoundTripper} + ctx := middleware.NewContextWithLogger(context.Background(), logger) + ctx = middleware.NewContextWithRequestID(ctx, requestID) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + defer func() { _ = r.Body.Close() }() + require.NoError(t, err) + require.NotEmpty(t, logger.Entries()) + + loggerEntry := logger.Entries()[0] + require.Contains(t, loggerEntry.Text, fmt.Sprintf("request id %s", requestID)) +} From 1ec8dbe10c1f027846233ddf3d58cd26df29b2c2 Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Thu, 9 Jan 2025 21:42:04 +0000 Subject: [PATCH 07/16] retry policy exponential and constant configs --- httpclient/config.go | 61 ++++++++++++++++++----------- httpclient/config_test.go | 8 ++-- httpclient/httpclient_test.go | 8 ++-- httpclient/metrics_round_tripper.go | 1 + 4 files changed, 49 insertions(+), 29 deletions(-) diff --git a/httpclient/config.go b/httpclient/config.go index 4e034be..54074a6 100644 --- a/httpclient/config.go +++ b/httpclient/config.go @@ -33,7 +33,7 @@ const ( // configuration properties cfgKeyRetriesEnabled = "retries.enabled" cfgKeyRetriesMax = "retries.maxAttempts" - cfgKeyRetriesPolicyStrategy = "retries.policy" + cfgKeyRetriesPolicy = "retries.policy" cfgKeyRetriesPolicyExponentialInitialInterval = "retries.exponentialBackoff.initialInterval" cfgKeyRetriesPolicyExponentialMultiplier = "retries.exponentialBackoff.multiplier" cfgKeyRetriesPolicyConstantInternal = "retries.constantBackoff.interval" @@ -121,32 +121,44 @@ func (c *RateLimitConfig) TransportOpts() RateLimitingRoundTripperOpts { // PolicyConfig represents configuration options for policy retry. type PolicyConfig struct { - // Strategy is a retry policy strategy. - Strategy RetryPolicy + // Policy is a retry policy strategy. + Policy RetryPolicy - // ExponentialBackoffInitialInterval is the initial interval for exponential backoff. - ExponentialBackoffInitialInterval time.Duration + // ExponentialBackoff is the configuration for exponential backoff. + ExponentialBackoff ExponentialBackoffConfig - // ExponentialBackoffMultiplier is the multiplier for exponential backoff. - ExponentialBackoffMultiplier float64 + // ConstantBackoff is the configuration for constant backoff. + ConstantBackoff ConstantBackoffConfig +} + +// ExponentialBackoffConfig represents configuration options for exponential backoff. +type ExponentialBackoffConfig struct { + // InitialInterval is the initial interval for exponential backoff. + InitialInterval time.Duration + + // Multiplier is the multiplier for exponential backoff. + Multiplier float64 +} - // ConstantBackoffInterval is the interval for constant backoff. - ConstantBackoffInterval time.Duration +// ConstantBackoffConfig represents configuration options for constant backoff. +type ConstantBackoffConfig struct { + // Interval is the interval for constant backoff. + Interval time.Duration } // Set is part of config interface implementation. func (c *PolicyConfig) Set(dp config.DataProvider) (err error) { - strategy, err := dp.GetString(cfgKeyRetriesPolicyStrategy) + policy, err := dp.GetString(cfgKeyRetriesPolicy) if err != nil { return err } - c.Strategy = RetryPolicy(strategy) + c.Policy = RetryPolicy(policy) - if c.Strategy != "" && c.Strategy != RetryPolicyExponential && c.Strategy != RetryPolicyConstant { - return dp.WrapKeyErr(cfgKeyRetriesPolicyStrategy, errors.New("must be one of: [exponential, constant]")) + if c.Policy != "" && c.Policy != RetryPolicyExponential && c.Policy != RetryPolicyConstant { + return dp.WrapKeyErr(cfgKeyRetriesPolicy, errors.New("must be one of: [exponential, constant]")) } - if c.Strategy == RetryPolicyExponential { + if c.Policy == RetryPolicyExponential { var interval time.Duration interval, err = dp.GetDuration(cfgKeyRetriesPolicyExponentialInitialInterval) if err != nil { @@ -155,7 +167,6 @@ func (c *PolicyConfig) Set(dp config.DataProvider) (err error) { if interval < 0 { return dp.WrapKeyErr(cfgKeyRetriesPolicyExponentialInitialInterval, errors.New("must be positive")) } - c.ExponentialBackoffInitialInterval = interval var multiplier float64 multiplier, err = dp.GetFloat64(cfgKeyRetriesPolicyExponentialMultiplier) @@ -165,10 +176,14 @@ func (c *PolicyConfig) Set(dp config.DataProvider) (err error) { if multiplier <= 1 { return dp.WrapKeyErr(cfgKeyRetriesPolicyExponentialMultiplier, errors.New("must be greater than 1")) } - c.ExponentialBackoffMultiplier = multiplier + + c.ExponentialBackoff = ExponentialBackoffConfig{ + InitialInterval: interval, + Multiplier: multiplier, + } return nil - } else if c.Strategy == RetryPolicyConstant { + } else if c.Policy == RetryPolicyConstant { var interval time.Duration interval, err = dp.GetDuration(cfgKeyRetriesPolicyConstantInternal) if err != nil { @@ -177,7 +192,7 @@ func (c *PolicyConfig) Set(dp config.DataProvider) (err error) { if interval < 0 { return dp.WrapKeyErr(cfgKeyRetriesPolicyConstantInternal, errors.New("must be positive")) } - c.ConstantBackoffInterval = interval + c.ConstantBackoff = ConstantBackoffConfig{Interval: interval} } return nil @@ -200,17 +215,17 @@ type RetriesConfig struct { // GetPolicy returns a retry policy based on strategy or nil if none is provided. func (c *RetriesConfig) GetPolicy() retry.Policy { - if c.Policy.Strategy == RetryPolicyExponential { + if c.Policy.Policy == RetryPolicyExponential { return retry.PolicyFunc(func() backoff.BackOff { bf := backoff.NewExponentialBackOff() - bf.InitialInterval = c.Policy.ExponentialBackoffInitialInterval - bf.Multiplier = c.Policy.ExponentialBackoffMultiplier + bf.InitialInterval = c.Policy.ExponentialBackoff.InitialInterval + bf.Multiplier = c.Policy.ExponentialBackoff.Multiplier bf.Reset() return bf }) - } else if c.Policy.Strategy == RetryPolicyConstant { + } else if c.Policy.Policy == RetryPolicyConstant { return retry.PolicyFunc(func() backoff.BackOff { - bf := backoff.NewConstantBackOff(c.Policy.ConstantBackoffInterval) + bf := backoff.NewConstantBackOff(c.Policy.ConstantBackoff.Interval) bf.Reset() return bf }) diff --git a/httpclient/config_test.go b/httpclient/config_test.go index c084a51..f5846bf 100644 --- a/httpclient/config_test.go +++ b/httpclient/config_test.go @@ -27,9 +27,11 @@ func TestConfigWithLoader(t *testing.T) { Enabled: true, MaxAttempts: 30, Policy: PolicyConfig{ - Strategy: RetryPolicyExponential, - ExponentialBackoffInitialInterval: 3 * time.Second, - ExponentialBackoffMultiplier: 2, + Policy: RetryPolicyExponential, + ExponentialBackoff: ExponentialBackoffConfig{ + InitialInterval: 3 * time.Second, + Multiplier: 2, + }, }, }, RateLimits: RateLimitConfig{ diff --git a/httpclient/httpclient_test.go b/httpclient/httpclient_test.go index 7c66586..046ed60 100644 --- a/httpclient/httpclient_test.go +++ b/httpclient/httpclient_test.go @@ -138,9 +138,11 @@ func TestMustHTTPClientWithOptsRoundTripperPolicy(t *testing.T) { cfg := NewConfig() cfg.Retries.Enabled = true cfg.Retries.MaxAttempts = 1 - cfg.Retries.Policy.Strategy = RetryPolicyExponential - cfg.Retries.Policy.ExponentialBackoffInitialInterval = 2 * time.Millisecond - cfg.Retries.Policy.ExponentialBackoffMultiplier = 1.1 + cfg.Retries.Policy.Policy = RetryPolicyExponential + cfg.Retries.Policy.ExponentialBackoff = ExponentialBackoffConfig{ + InitialInterval: 2 * time.Millisecond, + Multiplier: 1.1, + } client := MustWithOpts(cfg, Opts{ UserAgent: "test-agent", diff --git a/httpclient/metrics_round_tripper.go b/httpclient/metrics_round_tripper.go index 5d703f7..2dfdaeb 100644 --- a/httpclient/metrics_round_tripper.go +++ b/httpclient/metrics_round_tripper.go @@ -31,6 +31,7 @@ type PrometheusMetricsCollector struct { Durations *prometheus.HistogramVec } +// NewPrometheusMetricsCollector creates a new Prometheus metrics collector. func NewPrometheusMetricsCollector(namespace string) *PrometheusMetricsCollector { return &PrometheusMetricsCollector{ Durations: prometheus.NewHistogramVec(prometheus.HistogramOpts{ From e257a0f394b7c24a34c2769429e6d8aa18be7b51 Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Fri, 10 Jan 2025 10:10:49 +0000 Subject: [PATCH 08/16] New opts, request type and rate limits config --- httpclient/config.go | 16 ++++----- httpclient/config_test.go | 2 +- httpclient/httpclient.go | 46 +++++------------------- httpclient/httpclient_test.go | 19 +++++----- httpclient/logging_round_tripper.go | 26 +++++++------- httpclient/logging_round_tripper_test.go | 14 ++++---- httpclient/metrics_round_tripper.go | 32 ++++++++--------- httpclient/metrics_round_tripper_test.go | 4 +-- 8 files changed, 63 insertions(+), 96 deletions(-) diff --git a/httpclient/config.go b/httpclient/config.go index 54074a6..4d194bc 100644 --- a/httpclient/config.go +++ b/httpclient/config.go @@ -18,8 +18,8 @@ import ( type RetryPolicy string const ( - // DefaultReqType is a default request type. - DefaultReqType = "go-appkit" + // DefaultRequestType is a default request type. + DefaultRequestType = "go-appkit" // DefaultClientWaitTimeout is a default timeout for a client to wait for a request. DefaultClientWaitTimeout = 10 * time.Second @@ -51,8 +51,8 @@ const ( var _ config.Config = (*Config)(nil) var _ config.KeyPrefixProvider = (*Config)(nil) -// RateLimitConfig represents configuration options for HTTP client rate limits. -type RateLimitConfig struct { +// RateLimitsConfig represents configuration options for HTTP client rate limits. +type RateLimitsConfig struct { // Enabled is a flag that enables rate limiting. Enabled bool @@ -67,7 +67,7 @@ type RateLimitConfig struct { } // Set is part of config interface implementation. -func (c *RateLimitConfig) Set(dp config.DataProvider) (err error) { +func (c *RateLimitsConfig) Set(dp config.DataProvider) (err error) { enabled, err := dp.GetBool(cfgKeyRateLimitsEnabled) if err != nil { return err @@ -109,10 +109,10 @@ func (c *RateLimitConfig) Set(dp config.DataProvider) (err error) { } // SetProviderDefaults is part of config interface implementation. -func (c *RateLimitConfig) SetProviderDefaults(_ config.DataProvider) {} +func (c *RateLimitsConfig) SetProviderDefaults(_ config.DataProvider) {} // TransportOpts returns transport options. -func (c *RateLimitConfig) TransportOpts() RateLimitingRoundTripperOpts { +func (c *RateLimitsConfig) TransportOpts() RateLimitingRoundTripperOpts { return RateLimitingRoundTripperOpts{ Burst: c.Burst, WaitTimeout: c.WaitTimeout, @@ -354,7 +354,7 @@ type Config struct { Retries RetriesConfig // RateLimits is a configuration for HTTP client rate limits. - RateLimits RateLimitConfig + RateLimits RateLimitsConfig // Log is a configuration for HTTP client logs. Log LogConfig diff --git a/httpclient/config_test.go b/httpclient/config_test.go index f5846bf..3858a53 100644 --- a/httpclient/config_test.go +++ b/httpclient/config_test.go @@ -34,7 +34,7 @@ func TestConfigWithLoader(t *testing.T) { }, }, }, - RateLimits: RateLimitConfig{ + RateLimits: RateLimitsConfig{ Enabled: true, Limit: 300, Burst: 3000, diff --git a/httpclient/httpclient.go b/httpclient/httpclient.go index e9b6e57..bd14235 100644 --- a/httpclient/httpclient.go +++ b/httpclient/httpclient.go @@ -35,37 +35,7 @@ func CloneHTTPHeader(in http.Header) http.Header { // New wraps delegate transports with logging, rate limiting, retryable, request id // and returns an error if any occurs. func New(cfg *Config) (*http.Client, error) { - var err error - var delegate http.RoundTripper - - delegate = http.DefaultTransport.(*http.Transport).Clone() - - if cfg.Log.Enabled { - opts := cfg.Log.TransportOpts() - delegate = NewLoggingRoundTripperWithOpts(delegate, opts) - } - - if cfg.RateLimits.Enabled { - delegate, err = NewRateLimitingRoundTripperWithOpts( - delegate, cfg.RateLimits.Limit, cfg.RateLimits.TransportOpts(), - ) - if err != nil { - return nil, fmt.Errorf("create rate limiting round tripper: %w", err) - } - } - - delegate = NewRequestIDRoundTripper(delegate) - - if cfg.Retries.Enabled { - opts := cfg.Retries.TransportOpts() - opts.BackoffPolicy = cfg.Retries.GetPolicy() - delegate, err = NewRetryableRoundTripperWithOpts(delegate, opts) - if err != nil { - return nil, fmt.Errorf("create retryable round tripper: %w", err) - } - } - - return &http.Client{Transport: delegate, Timeout: cfg.Timeout}, nil + return NewWithOpts(cfg, Opts{}) } // Must wraps delegate transports with logging, rate limiting, retryable, request id @@ -84,8 +54,8 @@ type Opts struct { // UserAgent is a user agent string. UserAgent string - // ReqType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. - ReqType string + // RequestType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. + RequestType string // Delegate is the next RoundTripper in the chain. Delegate http.RoundTripper @@ -114,14 +84,14 @@ func NewWithOpts(cfg *Config, opts Opts) (*http.Client, error) { if cfg.Log.Enabled { logOpts := cfg.Log.TransportOpts() logOpts.LoggerProvider = opts.LoggerProvider - logOpts.ReqType = opts.ReqType + logOpts.RequestType = opts.RequestType delegate = NewLoggingRoundTripperWithOpts(delegate, logOpts) } if cfg.Metrics.Enabled { delegate = NewMetricsRoundTripperWithOpts(delegate, MetricsRoundTripperOpts{ - ReqType: opts.ReqType, - Collector: opts.Collector, + RequestType: opts.RequestType, + Collector: opts.Collector, }) } @@ -134,8 +104,8 @@ func NewWithOpts(cfg *Config, opts Opts) (*http.Client, error) { } } - if opts.ReqType != "" { - delegate = NewUserAgentRoundTripper(delegate, opts.ReqType) + if opts.UserAgent != "" { + delegate = NewUserAgentRoundTripper(delegate, opts.UserAgent) } delegate = NewRequestIDRoundTripperWithOpts(delegate, RequestIDRoundTripperOpts{ diff --git a/httpclient/httpclient_test.go b/httpclient/httpclient_test.go index 046ed60..a6a0acf 100644 --- a/httpclient/httpclient_test.go +++ b/httpclient/httpclient_test.go @@ -40,7 +40,7 @@ func TestNewHTTPClientLoggingRoundTripper(t *testing.T) { require.NotEmpty(t, logger.Entries()) require.Contains( t, logger.Entries()[0].Text, - fmt.Sprintf("client http request POST %s req type %s status code 418", server.URL, DefaultReqType), + fmt.Sprintf("client http request POST %s req type %s status code 418", server.URL, DefaultRequestType), ) } @@ -64,7 +64,7 @@ func TestMustHTTPClientLoggingRoundTripper(t *testing.T) { require.NotEmpty(t, logger.Entries()) require.Contains( t, logger.Entries()[0].Text, - fmt.Sprintf("client http request POST %s req type %s status code 418", server.URL, DefaultReqType), + fmt.Sprintf("client http request POST %s req type %s status code 418", server.URL, DefaultRequestType), ) } @@ -78,9 +78,8 @@ func TestNewHTTPClientWithOptsRoundTripper(t *testing.T) { cfg := NewConfig() cfg.Log.Enabled = true client, err := NewWithOpts(cfg, Opts{ - UserAgent: "test-agent", - ReqType: "test-request", - Delegate: nil, + UserAgent: "test-agent", + RequestType: "test-request", }) require.NoError(t, err) ctx := middleware.NewContextWithLogger(context.Background(), logger) @@ -108,9 +107,8 @@ func TestMustHTTPClientWithOptsRoundTripper(t *testing.T) { cfg := NewConfig() cfg.Log.Enabled = true client := MustWithOpts(cfg, Opts{ - UserAgent: "test-agent", - ReqType: "test-request", - Delegate: nil, + UserAgent: "test-agent", + RequestType: "test-request", }) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) @@ -145,9 +143,8 @@ func TestMustHTTPClientWithOptsRoundTripperPolicy(t *testing.T) { } client := MustWithOpts(cfg, Opts{ - UserAgent: "test-agent", - ReqType: "test-request", - Delegate: nil, + UserAgent: "test-agent", + RequestType: "test-request", }) req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, server.URL, nil) require.NoError(t, err) diff --git a/httpclient/logging_round_tripper.go b/httpclient/logging_round_tripper.go index 5c4cf17..6658826 100644 --- a/httpclient/logging_round_tripper.go +++ b/httpclient/logging_round_tripper.go @@ -38,8 +38,8 @@ type LoggingRoundTripper struct { // Delegate is the next RoundTripper in the chain. Delegate http.RoundTripper - // ReqType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. - ReqType string + // RequestType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. + RequestType string // Mode of logging: none, all, failed. Mode LoggingMode @@ -54,8 +54,8 @@ type LoggingRoundTripper struct { // LoggingRoundTripperOpts represents an options for LoggingRoundTripper. type LoggingRoundTripperOpts struct { - // ReqType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. - ReqType string + // RequestType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. + RequestType string // Mode of logging: none, all, failed. Mode LoggingMode @@ -71,7 +71,7 @@ type LoggingRoundTripperOpts struct { // NewLoggingRoundTripper creates an HTTP transport that log requests. func NewLoggingRoundTripper(delegate http.RoundTripper) http.RoundTripper { return NewLoggingRoundTripperWithOpts(delegate, LoggingRoundTripperOpts{ - ReqType: DefaultReqType, + RequestType: DefaultRequestType, }) } @@ -79,14 +79,14 @@ func NewLoggingRoundTripper(delegate http.RoundTripper) http.RoundTripper { func NewLoggingRoundTripperWithOpts( delegate http.RoundTripper, opts LoggingRoundTripperOpts, ) http.RoundTripper { - reqType := opts.ReqType - if reqType == "" { - reqType = DefaultReqType + requestType := opts.RequestType + if requestType == "" { + requestType = DefaultRequestType } return &LoggingRoundTripper{ Delegate: delegate, - ReqType: reqType, + RequestType: requestType, Mode: opts.Mode, SlowRequestThreshold: opts.SlowRequestThreshold, LoggerProvider: opts.LoggerProvider, @@ -116,7 +116,7 @@ func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error elapsed := time.Since(start) if logger != nil && elapsed >= rt.SlowRequestThreshold { common := "client http request %s %s req type %s " - args := []interface{}{r.Method, r.URL.String(), rt.ReqType, elapsed.Seconds(), err} + args := []interface{}{r.Method, r.URL.String(), rt.RequestType, elapsed.Seconds(), err} message := common + "time taken %.3f, err %+v" loggerAtLevel := logger.Infof @@ -125,7 +125,7 @@ func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error return resp, err } - args = []interface{}{r.Method, r.URL.String(), rt.ReqType, resp.StatusCode, elapsed.Seconds(), err} + args = []interface{}{r.Method, r.URL.String(), rt.RequestType, resp.StatusCode, elapsed.Seconds(), err} message = common + "status code %d, time taken %.3f, err %+v" } @@ -133,7 +133,7 @@ func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error loggerAtLevel = logger.Errorf } - requestID := middleware.GetRequestIDFromContext(ctx) + requestID := r.Header.Get("X-Request-ID") if requestID != "" { message += " request id %s " args = append(args, requestID) @@ -142,7 +142,7 @@ func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error loggerAtLevel(message, args...) loggingParams := middleware.GetLoggingParamsFromContext(ctx) if loggingParams != nil { - loggingParams.AddTimeSlotDurationInMs(fmt.Sprintf("external_request_%s_ms", rt.ReqType), elapsed) + loggingParams.AddTimeSlotDurationInMs(fmt.Sprintf("external_request_%s_ms", rt.RequestType), elapsed) } } diff --git a/httpclient/logging_round_tripper_test.go b/httpclient/logging_round_tripper_test.go index 838f6f5..17f599f 100644 --- a/httpclient/logging_round_tripper_test.go +++ b/httpclient/logging_round_tripper_test.go @@ -40,7 +40,7 @@ func TestNewLoggingRoundTripper(t *testing.T) { loggerEntry := logger.Entries()[0] require.Contains( t, loggerEntry.Text, - fmt.Sprintf("client http request POST %s req type %s status code 418", server.URL, DefaultReqType), + fmt.Sprintf("client http request POST %s req type %s status code 418", server.URL, DefaultRequestType), ) } @@ -66,7 +66,7 @@ func TestMustHTTPClientLoggingRoundTripperError(t *testing.T) { loggerEntry := logger.Entries()[0] require.Contains( t, loggerEntry.Text, - fmt.Sprintf("client http request POST %s req type %s", serverURL, DefaultReqType), + fmt.Sprintf("client http request POST %s req type %s", serverURL, DefaultRequestType), ) require.Contains(t, loggerEntry.Text, "err dial tcp "+ln.Addr().String()+": connect: connection refused") require.NotContains(t, loggerEntry.Text, "status code") @@ -144,8 +144,8 @@ func TestNewLoggingRoundTripperModes(t *testing.T) { loggerRoundTripper := NewLoggingRoundTripperWithOpts( http.DefaultTransport, LoggingRoundTripperOpts{ - ReqType: "test-request", - Mode: tt.mode, + RequestType: "test-request", + Mode: tt.mode, }, ) client := &http.Client{Transport: loggerRoundTripper} @@ -173,8 +173,8 @@ func TestMustHTTPClientLoggingRoundTripperOpts(t *testing.T) { var loggerProviderCalled bool client := MustWithOpts(cfg, Opts{ - UserAgent: "test-agent", - ReqType: "test-request", + UserAgent: "test-agent", + RequestType: "test-request", LoggerProvider: func(ctx context.Context) log.FieldLogger { loggerProviderCalled = true return logger @@ -203,9 +203,9 @@ func TestNewLoggingRoundTripperWithRequestID(t *testing.T) { loggerRoundTripper := NewLoggingRoundTripper(http.DefaultTransport) client := &http.Client{Transport: loggerRoundTripper} ctx := middleware.NewContextWithLogger(context.Background(), logger) - ctx = middleware.NewContextWithRequestID(ctx, requestID) req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) require.NoError(t, err) + req.Header.Set("X-Request-ID", requestID) r, err := client.Do(req) defer func() { _ = r.Body.Close() }() diff --git a/httpclient/metrics_round_tripper.go b/httpclient/metrics_round_tripper.go index 2dfdaeb..a1c9cea 100644 --- a/httpclient/metrics_round_tripper.go +++ b/httpclient/metrics_round_tripper.go @@ -16,13 +16,13 @@ import ( var ( // ClassifyRequest does request classification, producing non-parameterized summary for given request. - ClassifyRequest func(r *http.Request, reqType string) string + ClassifyRequest func(r *http.Request, requestType string) string ) // MetricsCollector is an interface for collecting metrics for client requests. type MetricsCollector interface { // RequestDuration observes the duration of the request and the status code. - RequestDuration(reqType, remoteAddress, summary, status string, startTime time.Time) + RequestDuration(requestType, remoteAddress, summary, status string, startTime time.Time) } // PrometheusMetricsCollector is a Prometheus metrics collector. @@ -49,8 +49,8 @@ func (p *PrometheusMetricsCollector) MustRegister() { } // RequestDuration observes the duration of the request and the status code. -func (p *PrometheusMetricsCollector) RequestDuration(reqType, host, summary, status string, start time.Time) { - p.Durations.WithLabelValues(reqType, host, summary, status).Observe(time.Since(start).Seconds()) +func (p *PrometheusMetricsCollector) RequestDuration(requestType, host, summary, status string, start time.Time) { + p.Durations.WithLabelValues(requestType, host, summary, status).Observe(time.Since(start).Seconds()) } // Unregister the Prometheus metrics. @@ -63,8 +63,8 @@ type MetricsRoundTripper struct { // Delegate is the next RoundTripper in the chain. Delegate http.RoundTripper - // ReqType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. - ReqType string + // RequestType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. + RequestType string // Collector is a metrics collector. Collector MetricsCollector @@ -72,8 +72,8 @@ type MetricsRoundTripper struct { // MetricsRoundTripperOpts is an HTTP transport that measures requests done. type MetricsRoundTripperOpts struct { - // ReqType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. - ReqType string + // RequestType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. + RequestType string // Collector is a metrics collector. Collector MetricsCollector @@ -86,12 +86,12 @@ func NewMetricsRoundTripper(delegate http.RoundTripper) http.RoundTripper { // NewMetricsRoundTripperWithOpts creates an HTTP transport that measures requests done. func NewMetricsRoundTripperWithOpts(delegate http.RoundTripper, opts MetricsRoundTripperOpts) http.RoundTripper { - reqType := DefaultReqType - if opts.ReqType == "" { - reqType = opts.ReqType + requestType := DefaultRequestType + if opts.RequestType == "" { + requestType = opts.RequestType } - return &MetricsRoundTripper{Delegate: delegate, ReqType: reqType, Collector: opts.Collector} + return &MetricsRoundTripper{Delegate: delegate, RequestType: requestType, Collector: opts.Collector} } // RoundTrip measures external requests done. @@ -108,15 +108,15 @@ func (rt *MetricsRoundTripper) RoundTrip(r *http.Request) (*http.Response, error status = strconv.Itoa(resp.StatusCode) } - rt.Collector.RequestDuration(rt.ReqType, r.Host, requestSummary(r, rt.ReqType), status, start) + rt.Collector.RequestDuration(rt.RequestType, r.Host, requestSummary(r, rt.RequestType), status, start) return resp, err } // requestSummary does request classification, producing non-parameterized summary for given request. -func requestSummary(r *http.Request, reqType string) string { +func requestSummary(r *http.Request, requestType string) string { if ClassifyRequest != nil { - return ClassifyRequest(r, reqType) + return ClassifyRequest(r, requestType) } - return fmt.Sprintf("%s %s", r.Method, reqType) + return fmt.Sprintf("%s %s", r.Method, requestType) } diff --git a/httpclient/metrics_round_tripper_test.go b/httpclient/metrics_round_tripper_test.go index 586ad6f..97e6704 100644 --- a/httpclient/metrics_round_tripper_test.go +++ b/httpclient/metrics_round_tripper_test.go @@ -25,8 +25,8 @@ func TestNewMetricsRoundTripper(t *testing.T) { defer collector.Unregister() metricsRoundTripper := NewMetricsRoundTripperWithOpts(http.DefaultTransport, MetricsRoundTripperOpts{ - ReqType: "test-request", - Collector: collector, + RequestType: "test-request", + Collector: collector, }) client := &http.Client{Transport: metricsRoundTripper} req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, server.URL, nil) From d67e6c99da8ec4bec1bb2c314462b83ce34f9a7d Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Fri, 10 Jan 2025 11:44:42 +0000 Subject: [PATCH 09/16] Logger levels --- httpclient/config.go | 2 +- httpclient/logging_round_tripper.go | 70 ++++++----- httpclient/logging_round_tripper_test.go | 151 ++++++++++++----------- 3 files changed, 118 insertions(+), 105 deletions(-) diff --git a/httpclient/config.go b/httpclient/config.go index 4d194bc..35da658 100644 --- a/httpclient/config.go +++ b/httpclient/config.go @@ -279,7 +279,7 @@ type LogConfig struct { // SlowRequestThreshold is a threshold for slow requests. SlowRequestThreshold time.Duration - // Mode of logging. + // Mode of logging: all or failed. Mode LoggingMode } diff --git a/httpclient/logging_round_tripper.go b/httpclient/logging_round_tripper.go index 6658826..13a1204 100644 --- a/httpclient/logging_round_tripper.go +++ b/httpclient/logging_round_tripper.go @@ -19,7 +19,6 @@ import ( type LoggingMode string const ( - LoggingModeNone LoggingMode = "none" LoggingModeAll LoggingMode = "all" LoggingModeFailed LoggingMode = "failed" ) @@ -27,7 +26,7 @@ const ( // IsValid checks if the logger mode is valid. func (lm LoggingMode) IsValid() bool { switch lm { - case LoggingModeNone, LoggingModeAll, LoggingModeFailed: + case LoggingModeAll, LoggingModeFailed: return true } return false @@ -41,7 +40,7 @@ type LoggingRoundTripper struct { // RequestType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. RequestType string - // Mode of logging: none, all, failed. + // Mode of logging: all or failed. Mode LoggingMode // SlowRequestThreshold is a threshold for slow requests. @@ -57,7 +56,7 @@ type LoggingRoundTripperOpts struct { // RequestType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. RequestType string - // Mode of logging: none, all, failed. + // Mode of logging: all or failed. Mode LoggingMode // SlowRequestThreshold is a threshold for slow requests. @@ -104,46 +103,49 @@ func (rt *LoggingRoundTripper) getLogger(ctx context.Context) log.FieldLogger { // RoundTrip adds logging capabilities to the HTTP transport. func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - if rt.Mode == LoggingModeNone { + ctx := r.Context() + logger := rt.getLogger(ctx) + if logger == nil { return rt.Delegate.RoundTrip(r) } - ctx := r.Context() - logger := rt.getLogger(ctx) start := time.Now() - resp, err := rt.Delegate.RoundTrip(r) elapsed := time.Since(start) - if logger != nil && elapsed >= rt.SlowRequestThreshold { - common := "client http request %s %s req type %s " - args := []interface{}{r.Method, r.URL.String(), rt.RequestType, elapsed.Seconds(), err} - message := common + "time taken %.3f, err %+v" - loggerAtLevel := logger.Infof - - if resp != nil { - if rt.Mode == LoggingModeFailed && resp.StatusCode < http.StatusBadRequest { - return resp, err - } - - args = []interface{}{r.Method, r.URL.String(), rt.RequestType, resp.StatusCode, elapsed.Seconds(), err} - message = common + "status code %d, time taken %.3f, err %+v" + common := "client http request %s %s req type %s " + args := []interface{}{r.Method, r.URL.String(), rt.RequestType, elapsed.Seconds(), err} + message := common + "time taken %.3f, err %+v" + loggerAtLevel := logger.Infof + + isSlowRequest := elapsed >= rt.SlowRequestThreshold + var isSuccessful bool + + if resp != nil { + isSuccessful = resp.StatusCode < http.StatusBadRequest + if rt.Mode == LoggingModeFailed && isSuccessful && !isSlowRequest { + return resp, err } - if err != nil { - loggerAtLevel = logger.Errorf - } + args = []interface{}{r.Method, r.URL.String(), rt.RequestType, resp.StatusCode, elapsed.Seconds(), err} + message = common + "status code %d, time taken %.3f, err %+v" + } - requestID := r.Header.Get("X-Request-ID") - if requestID != "" { - message += " request id %s " - args = append(args, requestID) - } + if err != nil || !isSuccessful { + loggerAtLevel = logger.Errorf + } else if isSlowRequest { + loggerAtLevel = logger.Warnf + } - loggerAtLevel(message, args...) - loggingParams := middleware.GetLoggingParamsFromContext(ctx) - if loggingParams != nil { - loggingParams.AddTimeSlotDurationInMs(fmt.Sprintf("external_request_%s_ms", rt.RequestType), elapsed) - } + requestID := r.Header.Get("X-Request-ID") + if requestID != "" { + message += " request id %s " + args = append(args, requestID) + } + + loggerAtLevel(message, args...) + loggingParams := middleware.GetLoggingParamsFromContext(ctx) + if loggingParams != nil { + loggingParams.AddTimeSlotDurationInMs(fmt.Sprintf("external_request_%s_ms", rt.RequestType), elapsed) } return resp, err diff --git a/httpclient/logging_round_tripper_test.go b/httpclient/logging_round_tripper_test.go index 17f599f..ad21bb8 100644 --- a/httpclient/logging_round_tripper_test.go +++ b/httpclient/logging_round_tripper_test.go @@ -17,6 +17,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" ) func TestNewLoggingRoundTripper(t *testing.T) { @@ -91,76 +92,6 @@ func TestMustHTTPClientLoggingRoundTripperDisabled(t *testing.T) { require.Empty(t, logger.Entries()) } -func TestNewLoggingRoundTripperModes(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - if r.Method == http.MethodPost { - rw.WriteHeader(http.StatusBadRequest) - } else { - rw.WriteHeader(http.StatusOK) - } - })) - defer server.Close() - - tests := []struct { - name string - method string - mode LoggingMode - want int - }{ - { - name: "no requests are logged because of 'none' mode", - method: http.MethodGet, - mode: "none", - }, - { - name: "4xx and 5xx requests are logged because of 'failed' mode", - method: http.MethodPost, - mode: "failed", - want: 1, - }, - { - name: "only 4xx and 5xx requests so no logs because of 'failed' mode for 2xx", - method: http.MethodGet, - mode: "failed", - }, - { - name: "success requests are logged because of 'all' mode", - method: http.MethodGet, - mode: "all", - want: 1, - }, - { - name: "failed requests are logged because of 'all' mode", - method: http.MethodPost, - mode: "all", - want: 1, - }, - } - - for i := range tests { - tt := tests[i] - t.Run(tt.name, func(t *testing.T) { - logger := logtest.NewRecorder() - loggerRoundTripper := NewLoggingRoundTripperWithOpts( - http.DefaultTransport, - LoggingRoundTripperOpts{ - RequestType: "test-request", - Mode: tt.mode, - }, - ) - client := &http.Client{Transport: loggerRoundTripper} - ctx := middleware.NewContextWithLogger(context.Background(), logger) - req, err := http.NewRequestWithContext(ctx, tt.method, server.URL, nil) - require.NoError(t, err) - - r, err := client.Do(req) - defer func() { _ = r.Body.Close() }() - require.NoError(t, err) - require.Len(t, logger.Entries(), tt.want) - }) - } -} - func TestMustHTTPClientLoggingRoundTripperOpts(t *testing.T) { ln, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) @@ -215,3 +146,83 @@ func TestNewLoggingRoundTripperWithRequestID(t *testing.T) { loggerEntry := logger.Entries()[0] require.Contains(t, loggerEntry.Text, fmt.Sprintf("request id %s", requestID)) } + +func TestNewLoggingRoundTripperLevels(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + rw.WriteHeader(http.StatusTeapot) + } else { + rw.WriteHeader(http.StatusOK) + } + })) + defer server.Close() + + tests := []struct { + name string + opts LoggingRoundTripperOpts + level log.Level + method string + noLogs bool + }{ + { + name: "log mode 'all' failed requests with error level", + opts: LoggingRoundTripperOpts{Mode: LoggingModeAll}, + level: log.LevelError, + method: http.MethodPost, + }, + { + name: "log mode 'failed' failed requests with error level", + opts: LoggingRoundTripperOpts{Mode: LoggingModeFailed}, + level: log.LevelError, + method: http.MethodPost, + }, + { + name: "log mode 'all' slow successful requests with warn level", + opts: LoggingRoundTripperOpts{Mode: LoggingModeAll}, + level: log.LevelWarn, + method: http.MethodGet, + }, + { + name: "log mode 'failed' slow successful requests with warn level", + opts: LoggingRoundTripperOpts{Mode: LoggingModeFailed}, + level: log.LevelWarn, + method: http.MethodGet, + }, + { + name: "log mode 'all' requests with info level under slow threshold", + opts: LoggingRoundTripperOpts{Mode: LoggingModeAll, SlowRequestThreshold: 5 * time.Second}, + level: log.LevelInfo, + method: http.MethodGet, + }, + { + name: "log mode 'failed' do not log requests for successful requests under slow threshold", + opts: LoggingRoundTripperOpts{Mode: LoggingModeFailed, SlowRequestThreshold: 5 * time.Second}, + method: http.MethodGet, + noLogs: true, + }, + } + + for i := range tests { + tt := tests[i] + t.Run(tt.name, func(t *testing.T) { + logger := logtest.NewRecorder() + loggerRoundTripper := NewLoggingRoundTripperWithOpts(http.DefaultTransport, tt.opts) + client := &http.Client{Transport: loggerRoundTripper} + ctx := middleware.NewContextWithLogger(context.Background(), logger) + req, err := http.NewRequestWithContext(ctx, tt.method, server.URL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + defer func() { _ = r.Body.Close() }() + require.NoError(t, err) + + entries := logger.Entries() + if tt.noLogs { + require.Empty(t, entries) + return + } + require.NotEmpty(t, entries) + require.Equal(t, tt.level, entries[0].Level) + }) + } +} From 2ca89dab150cacac59b76159c730472885e39b93 Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Fri, 10 Jan 2025 13:56:00 +0000 Subject: [PATCH 10/16] Retry policy config, round tripper sorting, metric round tripper client type --- httpclient/auth_bearer_round_tripper.go | 2 +- httpclient/config.go | 153 +++++++++----------- httpclient/config_test.go | 18 +-- httpclient/httpclient.go | 57 +++----- httpclient/httpclient_test.go | 39 ++--- httpclient/logging_round_tripper.go | 38 ++--- httpclient/logging_round_tripper_test.go | 26 ++-- httpclient/metrics_round_tripper.go | 57 ++++---- httpclient/metrics_round_tripper_test.go | 10 +- httpclient/request_id_round_tripper.go | 5 +- httpclient/request_id_round_tripper_test.go | 6 +- httpclient/retryable_round_tripper.go | 2 +- httpclient/user_agent_round_tripper.go | 2 +- 13 files changed, 189 insertions(+), 226 deletions(-) diff --git a/httpclient/auth_bearer_round_tripper.go b/httpclient/auth_bearer_round_tripper.go index d0deba3..2d55b3c 100644 --- a/httpclient/auth_bearer_round_tripper.go +++ b/httpclient/auth_bearer_round_tripper.go @@ -70,7 +70,7 @@ func (rt *AuthBearerRoundTripper) RoundTrip(req *http.Request) (*http.Response, if err != nil { return nil, &AuthBearerRoundTripperError{Inner: err} } - req = CloneHTTPRequest(req) // Per RoundTripper contract. + req = req.Clone(req.Context()) // Per RoundTripper contract. req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) return rt.Delegate.RoundTrip(req) } diff --git a/httpclient/config.go b/httpclient/config.go index 35da658..431484d 100644 --- a/httpclient/config.go +++ b/httpclient/config.go @@ -8,19 +8,18 @@ package httpclient import ( "errors" + "time" + + "github.com/cenkalti/backoff/v4" + "github.com/acronis/go-appkit/config" "github.com/acronis/go-appkit/retry" - "github.com/cenkalti/backoff/v4" - "time" ) // RetryPolicy represents a retry policy strategy. type RetryPolicy string const ( - // DefaultRequestType is a default request type. - DefaultRequestType = "go-appkit" - // DefaultClientWaitTimeout is a default timeout for a client to wait for a request. DefaultClientWaitTimeout = 10 * time.Second @@ -119,18 +118,6 @@ func (c *RateLimitsConfig) TransportOpts() RateLimitingRoundTripperOpts { } } -// PolicyConfig represents configuration options for policy retry. -type PolicyConfig struct { - // Policy is a retry policy strategy. - Policy RetryPolicy - - // ExponentialBackoff is the configuration for exponential backoff. - ExponentialBackoff ExponentialBackoffConfig - - // ConstantBackoff is the configuration for constant backoff. - ConstantBackoff ConstantBackoffConfig -} - // ExponentialBackoffConfig represents configuration options for exponential backoff. type ExponentialBackoffConfig struct { // InitialInterval is the initial interval for exponential backoff. @@ -146,61 +133,6 @@ type ConstantBackoffConfig struct { Interval time.Duration } -// Set is part of config interface implementation. -func (c *PolicyConfig) Set(dp config.DataProvider) (err error) { - policy, err := dp.GetString(cfgKeyRetriesPolicy) - if err != nil { - return err - } - c.Policy = RetryPolicy(policy) - - if c.Policy != "" && c.Policy != RetryPolicyExponential && c.Policy != RetryPolicyConstant { - return dp.WrapKeyErr(cfgKeyRetriesPolicy, errors.New("must be one of: [exponential, constant]")) - } - - if c.Policy == RetryPolicyExponential { - var interval time.Duration - interval, err = dp.GetDuration(cfgKeyRetriesPolicyExponentialInitialInterval) - if err != nil { - return nil - } - if interval < 0 { - return dp.WrapKeyErr(cfgKeyRetriesPolicyExponentialInitialInterval, errors.New("must be positive")) - } - - var multiplier float64 - multiplier, err = dp.GetFloat64(cfgKeyRetriesPolicyExponentialMultiplier) - if err != nil { - return err - } - if multiplier <= 1 { - return dp.WrapKeyErr(cfgKeyRetriesPolicyExponentialMultiplier, errors.New("must be greater than 1")) - } - - c.ExponentialBackoff = ExponentialBackoffConfig{ - InitialInterval: interval, - Multiplier: multiplier, - } - - return nil - } else if c.Policy == RetryPolicyConstant { - var interval time.Duration - interval, err = dp.GetDuration(cfgKeyRetriesPolicyConstantInternal) - if err != nil { - return err - } - if interval < 0 { - return dp.WrapKeyErr(cfgKeyRetriesPolicyConstantInternal, errors.New("must be positive")) - } - c.ConstantBackoff = ConstantBackoffConfig{Interval: interval} - } - - return nil -} - -// SetProviderDefaults is part of config interface implementation. -func (c *PolicyConfig) SetProviderDefaults(_ config.DataProvider) {} - // RetriesConfig represents configuration options for HTTP client retries policy. type RetriesConfig struct { // Enabled is a flag that enables retries. @@ -210,22 +142,28 @@ type RetriesConfig struct { MaxAttempts int // Policy of a retry: [exponential, constant]. default is exponential. - Policy PolicyConfig + Policy RetryPolicy + + // ExponentialBackoff is the configuration for exponential backoff. + ExponentialBackoff ExponentialBackoffConfig + + // ConstantBackoff is the configuration for constant backoff. + ConstantBackoff ConstantBackoffConfig } // GetPolicy returns a retry policy based on strategy or nil if none is provided. func (c *RetriesConfig) GetPolicy() retry.Policy { - if c.Policy.Policy == RetryPolicyExponential { + if c.Policy == RetryPolicyExponential { return retry.PolicyFunc(func() backoff.BackOff { bf := backoff.NewExponentialBackOff() - bf.InitialInterval = c.Policy.ExponentialBackoff.InitialInterval - bf.Multiplier = c.Policy.ExponentialBackoff.Multiplier + bf.InitialInterval = c.ExponentialBackoff.InitialInterval + bf.Multiplier = c.ExponentialBackoff.Multiplier bf.Reset() return bf }) - } else if c.Policy.Policy == RetryPolicyConstant { + } else if c.Policy == RetryPolicyConstant { return retry.PolicyFunc(func() backoff.BackOff { - bf := backoff.NewConstantBackOff(c.Policy.ConstantBackoff.Interval) + bf := backoff.NewConstantBackOff(c.ConstantBackoff.Interval) bf.Reset() return bf }) @@ -255,12 +193,7 @@ func (c *RetriesConfig) Set(dp config.DataProvider) error { } c.MaxAttempts = maxAttempts - err = c.Policy.Set(config.NewKeyPrefixedDataProvider(dp, "")) - if err != nil { - return err - } - - return nil + return c.setPolicy(dp) } // SetProviderDefaults is part of config interface implementation. @@ -271,6 +204,58 @@ func (c *RetriesConfig) TransportOpts() RetryableRoundTripperOpts { return RetryableRoundTripperOpts{MaxRetryAttempts: c.MaxAttempts} } +// setPolicy sets the policy based on the configuration. +func (c *RetriesConfig) setPolicy(dp config.DataProvider) error { + policy, err := dp.GetString(cfgKeyRetriesPolicy) + if err != nil { + return err + } + c.Policy = RetryPolicy(policy) + + if c.Policy != "" && c.Policy != RetryPolicyExponential && c.Policy != RetryPolicyConstant { + return dp.WrapKeyErr(cfgKeyRetriesPolicy, errors.New("must be one of: [exponential, constant]")) + } + + if c.Policy == RetryPolicyExponential { + var interval time.Duration + interval, err = dp.GetDuration(cfgKeyRetriesPolicyExponentialInitialInterval) + if err != nil { + return nil + } + if interval < 0 { + return dp.WrapKeyErr(cfgKeyRetriesPolicyExponentialInitialInterval, errors.New("must be positive")) + } + + var multiplier float64 + multiplier, err = dp.GetFloat64(cfgKeyRetriesPolicyExponentialMultiplier) + if err != nil { + return err + } + if multiplier <= 1 { + return dp.WrapKeyErr(cfgKeyRetriesPolicyExponentialMultiplier, errors.New("must be greater than 1")) + } + + c.ExponentialBackoff = ExponentialBackoffConfig{ + InitialInterval: interval, + Multiplier: multiplier, + } + + return nil + } else if c.Policy == RetryPolicyConstant { + var interval time.Duration + interval, err = dp.GetDuration(cfgKeyRetriesPolicyConstantInternal) + if err != nil { + return err + } + if interval < 0 { + return dp.WrapKeyErr(cfgKeyRetriesPolicyConstantInternal, errors.New("must be positive")) + } + c.ConstantBackoff = ConstantBackoffConfig{Interval: interval} + } + + return nil +} + // LogConfig represents configuration options for HTTP client logs. type LogConfig struct { // Enabled is a flag that enables logging. diff --git a/httpclient/config_test.go b/httpclient/config_test.go index 3858a53..5c7b72a 100644 --- a/httpclient/config_test.go +++ b/httpclient/config_test.go @@ -8,12 +8,14 @@ package httpclient import ( "bytes" - "github.com/acronis/go-appkit/config" - "github.com/acronis/go-appkit/retry" - "github.com/stretchr/testify/require" "strings" "testing" "time" + + "github.com/stretchr/testify/require" + + "github.com/acronis/go-appkit/config" + "github.com/acronis/go-appkit/retry" ) func TestConfigWithLoader(t *testing.T) { @@ -26,12 +28,10 @@ func TestConfigWithLoader(t *testing.T) { Retries: RetriesConfig{ Enabled: true, MaxAttempts: 30, - Policy: PolicyConfig{ - Policy: RetryPolicyExponential, - ExponentialBackoff: ExponentialBackoffConfig{ - InitialInterval: 3 * time.Second, - Multiplier: 2, - }, + Policy: RetryPolicyExponential, + ExponentialBackoff: ExponentialBackoffConfig{ + InitialInterval: 3 * time.Second, + Multiplier: 2, }, }, RateLimits: RateLimitsConfig{ diff --git a/httpclient/httpclient.go b/httpclient/httpclient.go index bd14235..88622b9 100644 --- a/httpclient/httpclient.go +++ b/httpclient/httpclient.go @@ -9,28 +9,10 @@ package httpclient import ( "context" "fmt" - "github.com/acronis/go-appkit/log" "net/http" -) - -// CloneHTTPRequest creates a shallow copy of the request along with a deep copy of the Headers. -func CloneHTTPRequest(req *http.Request) *http.Request { - r := new(http.Request) - *r = *req - r.Header = CloneHTTPHeader(req.Header) - return r -} -// CloneHTTPHeader creates a deep copy of an http.Header. -func CloneHTTPHeader(in http.Header) http.Header { - out := make(http.Header, len(in)) - for key, values := range in { - newValues := make([]string, len(values)) - copy(newValues, values) - out[key] = newValues - } - return out -} + "github.com/acronis/go-appkit/log" +) // New wraps delegate transports with logging, rate limiting, retryable, request id // and returns an error if any occurs. @@ -54,8 +36,8 @@ type Opts struct { // UserAgent is a user agent string. UserAgent string - // RequestType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. - RequestType string + // ClientType is a target service. e.g. 'auth-service' + ClientType string // Delegate is the next RoundTripper in the chain. Delegate http.RoundTripper @@ -74,6 +56,10 @@ type Opts struct { // logging, metrics, rate limiting, retryable, user agent, request id // and returns an error if any occurs. func NewWithOpts(cfg *Config, opts Opts) (*http.Client, error) { + if cfg == nil { + return nil, fmt.Errorf("config must be provided") + } + var err error delegate := opts.Delegate @@ -81,20 +67,6 @@ func NewWithOpts(cfg *Config, opts Opts) (*http.Client, error) { delegate = http.DefaultTransport.(*http.Transport).Clone() } - if cfg.Log.Enabled { - logOpts := cfg.Log.TransportOpts() - logOpts.LoggerProvider = opts.LoggerProvider - logOpts.RequestType = opts.RequestType - delegate = NewLoggingRoundTripperWithOpts(delegate, logOpts) - } - - if cfg.Metrics.Enabled { - delegate = NewMetricsRoundTripperWithOpts(delegate, MetricsRoundTripperOpts{ - RequestType: opts.RequestType, - Collector: opts.Collector, - }) - } - if cfg.RateLimits.Enabled { delegate, err = NewRateLimitingRoundTripperWithOpts( delegate, cfg.RateLimits.Limit, cfg.RateLimits.TransportOpts(), @@ -122,6 +94,19 @@ func NewWithOpts(cfg *Config, opts Opts) (*http.Client, error) { } } + if cfg.Metrics.Enabled { + delegate = NewMetricsRoundTripperWithOpts(delegate, opts.Collector, MetricsRoundTripperOpts{ + ClientType: opts.ClientType, + }) + } + + if cfg.Log.Enabled { + logOpts := cfg.Log.TransportOpts() + logOpts.LoggerProvider = opts.LoggerProvider + logOpts.ClientType = opts.ClientType + delegate = NewLoggingRoundTripperWithOpts(delegate, logOpts) + } + return &http.Client{Transport: delegate, Timeout: cfg.Timeout}, nil } diff --git a/httpclient/httpclient_test.go b/httpclient/httpclient_test.go index a6a0acf..322efc7 100644 --- a/httpclient/httpclient_test.go +++ b/httpclient/httpclient_test.go @@ -9,13 +9,15 @@ package httpclient import ( "context" "fmt" - "github.com/acronis/go-appkit/httpserver/middleware" - "github.com/acronis/go-appkit/log/logtest" - "github.com/stretchr/testify/require" "net/http" "net/http/httptest" "testing" "time" + + "github.com/stretchr/testify/require" + + "github.com/acronis/go-appkit/httpserver/middleware" + "github.com/acronis/go-appkit/log/logtest" ) func TestNewHTTPClientLoggingRoundTripper(t *testing.T) { @@ -38,10 +40,6 @@ func TestNewHTTPClientLoggingRoundTripper(t *testing.T) { defer func() { _ = r.Body.Close() }() require.NoError(t, err) require.NotEmpty(t, logger.Entries()) - require.Contains( - t, logger.Entries()[0].Text, - fmt.Sprintf("client http request POST %s req type %s status code 418", server.URL, DefaultRequestType), - ) } func TestMustHTTPClientLoggingRoundTripper(t *testing.T) { @@ -62,10 +60,6 @@ func TestMustHTTPClientLoggingRoundTripper(t *testing.T) { defer func() { _ = r.Body.Close() }() require.NoError(t, err) require.NotEmpty(t, logger.Entries()) - require.Contains( - t, logger.Entries()[0].Text, - fmt.Sprintf("client http request POST %s req type %s status code 418", server.URL, DefaultRequestType), - ) } func TestNewHTTPClientWithOptsRoundTripper(t *testing.T) { @@ -78,8 +72,8 @@ func TestNewHTTPClientWithOptsRoundTripper(t *testing.T) { cfg := NewConfig() cfg.Log.Enabled = true client, err := NewWithOpts(cfg, Opts{ - UserAgent: "test-agent", - RequestType: "test-request", + UserAgent: "test-agent", + ClientType: "test-client-type", }) require.NoError(t, err) ctx := middleware.NewContextWithLogger(context.Background(), logger) @@ -92,7 +86,7 @@ func TestNewHTTPClientWithOptsRoundTripper(t *testing.T) { require.NotEmpty(t, logger.Entries()) require.Contains( t, logger.Entries()[0].Text, fmt.Sprintf( - "client http request POST %s req type test-request status code 418", server.URL, + "client http request POST %s status code 418", server.URL, ), ) } @@ -107,8 +101,8 @@ func TestMustHTTPClientWithOptsRoundTripper(t *testing.T) { cfg := NewConfig() cfg.Log.Enabled = true client := MustWithOpts(cfg, Opts{ - UserAgent: "test-agent", - RequestType: "test-request", + UserAgent: "test-agent", + ClientType: "test-client-type", }) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) @@ -119,10 +113,9 @@ func TestMustHTTPClientWithOptsRoundTripper(t *testing.T) { require.NoError(t, err) require.NotEmpty(t, logger.Entries()) require.Contains( - t, logger.Entries()[0].Text, fmt.Sprintf( - "client http request POST %s req type test-request status code 418", server.URL, - ), + t, logger.Entries()[0].Text, fmt.Sprintf("client http request POST %s status code 418", server.URL), ) + require.Contains(t, logger.Entries()[0].Text, "client type test-client-type") } func TestMustHTTPClientWithOptsRoundTripperPolicy(t *testing.T) { @@ -136,15 +129,15 @@ func TestMustHTTPClientWithOptsRoundTripperPolicy(t *testing.T) { cfg := NewConfig() cfg.Retries.Enabled = true cfg.Retries.MaxAttempts = 1 - cfg.Retries.Policy.Policy = RetryPolicyExponential - cfg.Retries.Policy.ExponentialBackoff = ExponentialBackoffConfig{ + cfg.Retries.Policy = RetryPolicyExponential + cfg.Retries.ExponentialBackoff = ExponentialBackoffConfig{ InitialInterval: 2 * time.Millisecond, Multiplier: 1.1, } client := MustWithOpts(cfg, Opts{ - UserAgent: "test-agent", - RequestType: "test-request", + UserAgent: "test-agent", + ClientType: "test-client-type", }) req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, server.URL, nil) require.NoError(t, err) diff --git a/httpclient/logging_round_tripper.go b/httpclient/logging_round_tripper.go index 13a1204..f460f86 100644 --- a/httpclient/logging_round_tripper.go +++ b/httpclient/logging_round_tripper.go @@ -9,10 +9,11 @@ package httpclient import ( "context" "fmt" - "github.com/acronis/go-appkit/httpserver/middleware" - "github.com/acronis/go-appkit/log" "net/http" "time" + + "github.com/acronis/go-appkit/httpserver/middleware" + "github.com/acronis/go-appkit/log" ) // LoggingMode represents a mode of logging. @@ -37,8 +38,8 @@ type LoggingRoundTripper struct { // Delegate is the next RoundTripper in the chain. Delegate http.RoundTripper - // RequestType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. - RequestType string + // ClientType is a target service. e.g. 'auth-service' + ClientType string // Mode of logging: all or failed. Mode LoggingMode @@ -53,8 +54,8 @@ type LoggingRoundTripper struct { // LoggingRoundTripperOpts represents an options for LoggingRoundTripper. type LoggingRoundTripperOpts struct { - // RequestType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. - RequestType string + // ClientType is a target service. e.g. 'auth-service' + ClientType string // Mode of logging: all or failed. Mode LoggingMode @@ -69,23 +70,16 @@ type LoggingRoundTripperOpts struct { // NewLoggingRoundTripper creates an HTTP transport that log requests. func NewLoggingRoundTripper(delegate http.RoundTripper) http.RoundTripper { - return NewLoggingRoundTripperWithOpts(delegate, LoggingRoundTripperOpts{ - RequestType: DefaultRequestType, - }) + return NewLoggingRoundTripperWithOpts(delegate, LoggingRoundTripperOpts{}) } // NewLoggingRoundTripperWithOpts creates an HTTP transport that log requests with options. func NewLoggingRoundTripperWithOpts( delegate http.RoundTripper, opts LoggingRoundTripperOpts, ) http.RoundTripper { - requestType := opts.RequestType - if requestType == "" { - requestType = DefaultRequestType - } - return &LoggingRoundTripper{ Delegate: delegate, - RequestType: requestType, + ClientType: opts.ClientType, Mode: opts.Mode, SlowRequestThreshold: opts.SlowRequestThreshold, LoggerProvider: opts.LoggerProvider, @@ -112,8 +106,8 @@ func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error start := time.Now() resp, err := rt.Delegate.RoundTrip(r) elapsed := time.Since(start) - common := "client http request %s %s req type %s " - args := []interface{}{r.Method, r.URL.String(), rt.RequestType, elapsed.Seconds(), err} + common := "client http request %s %s " + args := []interface{}{r.Method, r.URL.String(), elapsed.Seconds(), err} message := common + "time taken %.3f, err %+v" loggerAtLevel := logger.Infof @@ -126,7 +120,7 @@ func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error return resp, err } - args = []interface{}{r.Method, r.URL.String(), rt.RequestType, resp.StatusCode, elapsed.Seconds(), err} + args = []interface{}{r.Method, r.URL.String(), resp.StatusCode, elapsed.Seconds(), err} message = common + "status code %d, time taken %.3f, err %+v" } @@ -136,6 +130,11 @@ func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error loggerAtLevel = logger.Warnf } + if rt.ClientType != "" { + message += " client type %s " + args = append(args, rt.ClientType) + } + requestID := r.Header.Get("X-Request-ID") if requestID != "" { message += " request id %s " @@ -143,9 +142,10 @@ func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error } loggerAtLevel(message, args...) + loggingParams := middleware.GetLoggingParamsFromContext(ctx) if loggingParams != nil { - loggingParams.AddTimeSlotDurationInMs(fmt.Sprintf("external_request_%s_ms", rt.RequestType), elapsed) + loggingParams.AddTimeSlotDurationInMs(fmt.Sprintf("external_request_%s_ms", rt.ClientType), elapsed) } return resp, err diff --git a/httpclient/logging_round_tripper_test.go b/httpclient/logging_round_tripper_test.go index ad21bb8..06a1666 100644 --- a/httpclient/logging_round_tripper_test.go +++ b/httpclient/logging_round_tripper_test.go @@ -9,15 +9,17 @@ package httpclient import ( "context" "fmt" - "github.com/acronis/go-appkit/httpserver/middleware" - "github.com/acronis/go-appkit/log" - "github.com/acronis/go-appkit/log/logtest" - "github.com/stretchr/testify/require" "net" "net/http" "net/http/httptest" "testing" "time" + + "github.com/stretchr/testify/require" + + "github.com/acronis/go-appkit/httpserver/middleware" + "github.com/acronis/go-appkit/log" + "github.com/acronis/go-appkit/log/logtest" ) func TestNewLoggingRoundTripper(t *testing.T) { @@ -41,7 +43,7 @@ func TestNewLoggingRoundTripper(t *testing.T) { loggerEntry := logger.Entries()[0] require.Contains( t, loggerEntry.Text, - fmt.Sprintf("client http request POST %s req type %s status code 418", server.URL, DefaultRequestType), + fmt.Sprintf("client http request POST %s", server.URL), ) } @@ -59,16 +61,12 @@ func TestMustHTTPClientLoggingRoundTripperError(t *testing.T) { req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) require.NoError(t, err) - r, err := client.Do(req) + r, err := client.Do(req) // nolint:bodyclose require.Error(t, err) require.Nil(t, r) require.NotEmpty(t, logger.Entries()) loggerEntry := logger.Entries()[0] - require.Contains( - t, loggerEntry.Text, - fmt.Sprintf("client http request POST %s req type %s", serverURL, DefaultRequestType), - ) require.Contains(t, loggerEntry.Text, "err dial tcp "+ln.Addr().String()+": connect: connection refused") require.NotContains(t, loggerEntry.Text, "status code") } @@ -86,7 +84,7 @@ func TestMustHTTPClientLoggingRoundTripperDisabled(t *testing.T) { req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) require.NoError(t, err) - r, err := client.Do(req) + r, err := client.Do(req) // nolint:bodyclose require.Error(t, err) require.Nil(t, r) require.Empty(t, logger.Entries()) @@ -104,8 +102,8 @@ func TestMustHTTPClientLoggingRoundTripperOpts(t *testing.T) { var loggerProviderCalled bool client := MustWithOpts(cfg, Opts{ - UserAgent: "test-agent", - RequestType: "test-request", + UserAgent: "test-agent", + ClientType: "test-client-type", LoggerProvider: func(ctx context.Context) log.FieldLogger { loggerProviderCalled = true return logger @@ -116,7 +114,7 @@ func TestMustHTTPClientLoggingRoundTripperOpts(t *testing.T) { req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) require.NoError(t, err) - r, err := client.Do(req) + r, err := client.Do(req) // nolint:bodyclose require.Error(t, err) require.Nil(t, r) require.True(t, loggerProviderCalled) diff --git a/httpclient/metrics_round_tripper.go b/httpclient/metrics_round_tripper.go index a1c9cea..66bd3cf 100644 --- a/httpclient/metrics_round_tripper.go +++ b/httpclient/metrics_round_tripper.go @@ -8,15 +8,11 @@ package httpclient import ( "fmt" - "github.com/prometheus/client_golang/prometheus" "net/http" "strconv" "time" -) -var ( - // ClassifyRequest does request classification, producing non-parameterized summary for given request. - ClassifyRequest func(r *http.Request, requestType string) string + "github.com/prometheus/client_golang/prometheus" ) // MetricsCollector is an interface for collecting metrics for client requests. @@ -39,7 +35,7 @@ func NewPrometheusMetricsCollector(namespace string) *PrometheusMetricsCollector Name: "http_client_request_duration_seconds", Help: "A histogram of the http client requests durations.", Buckets: []float64{0.01, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 30, 60, 150, 300, 600}, - }, []string{"type", "remote_address", "summary", "status"}), + }, []string{"client_type", "remote_address", "summary", "status"}), } } @@ -63,35 +59,42 @@ type MetricsRoundTripper struct { // Delegate is the next RoundTripper in the chain. Delegate http.RoundTripper - // RequestType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. - RequestType string + // ClientType is a target service. e.g. 'auth-service' + ClientType string // Collector is a metrics collector. Collector MetricsCollector + + // ClassifyRequest does request classification, producing non-parameterized summary for given request. + ClassifyRequest func(r *http.Request, requestType string) string } // MetricsRoundTripperOpts is an HTTP transport that measures requests done. type MetricsRoundTripperOpts struct { - // RequestType is a type of request. e.g. service 'auth-service', an action 'login' or specific information to correlate. - RequestType string + // ClientType is a target service. e.g. 'auth-service' + ClientType string - // Collector is a metrics collector. - Collector MetricsCollector + // ClassifyRequest does request classification, producing non-parameterized summary for given request. + ClassifyRequest func(r *http.Request, requestType string) string } // NewMetricsRoundTripper creates an HTTP transport that measures requests done. -func NewMetricsRoundTripper(delegate http.RoundTripper) http.RoundTripper { - return NewMetricsRoundTripperWithOpts(delegate, MetricsRoundTripperOpts{}) +func NewMetricsRoundTripper(delegate http.RoundTripper, collector MetricsCollector) http.RoundTripper { + return NewMetricsRoundTripperWithOpts(delegate, collector, MetricsRoundTripperOpts{}) } // NewMetricsRoundTripperWithOpts creates an HTTP transport that measures requests done. -func NewMetricsRoundTripperWithOpts(delegate http.RoundTripper, opts MetricsRoundTripperOpts) http.RoundTripper { - requestType := DefaultRequestType - if opts.RequestType == "" { - requestType = opts.RequestType +func NewMetricsRoundTripperWithOpts( + delegate http.RoundTripper, + collector MetricsCollector, + opts MetricsRoundTripperOpts, +) http.RoundTripper { + return &MetricsRoundTripper{ + Delegate: delegate, + ClientType: opts.ClientType, + Collector: collector, + ClassifyRequest: opts.ClassifyRequest, } - - return &MetricsRoundTripper{Delegate: delegate, RequestType: requestType, Collector: opts.Collector} } // RoundTrip measures external requests done. @@ -108,15 +111,11 @@ func (rt *MetricsRoundTripper) RoundTrip(r *http.Request) (*http.Response, error status = strconv.Itoa(resp.StatusCode) } - rt.Collector.RequestDuration(rt.RequestType, r.Host, requestSummary(r, rt.RequestType), status, start) - return resp, err -} - -// requestSummary does request classification, producing non-parameterized summary for given request. -func requestSummary(r *http.Request, requestType string) string { - if ClassifyRequest != nil { - return ClassifyRequest(r, requestType) + summary := fmt.Sprintf("%s %s", r.Method, rt.ClientType) + if rt.ClassifyRequest != nil { + summary = rt.ClassifyRequest(r, rt.ClientType) } - return fmt.Sprintf("%s %s", r.Method, requestType) + rt.Collector.RequestDuration(rt.ClientType, r.Host, summary, status, start) + return resp, err } diff --git a/httpclient/metrics_round_tripper_test.go b/httpclient/metrics_round_tripper_test.go index 97e6704..797b8ea 100644 --- a/httpclient/metrics_round_tripper_test.go +++ b/httpclient/metrics_round_tripper_test.go @@ -8,11 +8,12 @@ package httpclient import ( "context" - "github.com/prometheus/client_golang/prometheus" - "github.com/stretchr/testify/require" "net/http" "net/http/httptest" "testing" + + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/require" ) func TestNewMetricsRoundTripper(t *testing.T) { @@ -24,9 +25,8 @@ func TestNewMetricsRoundTripper(t *testing.T) { collector := NewPrometheusMetricsCollector("") defer collector.Unregister() - metricsRoundTripper := NewMetricsRoundTripperWithOpts(http.DefaultTransport, MetricsRoundTripperOpts{ - RequestType: "test-request", - Collector: collector, + metricsRoundTripper := NewMetricsRoundTripperWithOpts(http.DefaultTransport, collector, MetricsRoundTripperOpts{ + ClientType: "test-client-type", }) client := &http.Client{Transport: metricsRoundTripper} req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, server.URL, nil) diff --git a/httpclient/request_id_round_tripper.go b/httpclient/request_id_round_tripper.go index 0b653c8..477b0ee 100644 --- a/httpclient/request_id_round_tripper.go +++ b/httpclient/request_id_round_tripper.go @@ -8,8 +8,9 @@ package httpclient import ( "context" - "github.com/acronis/go-appkit/httpserver/middleware" "net/http" + + "github.com/acronis/go-appkit/httpserver/middleware" ) // RequestIDRoundTripper for X-Request-ID header to the request. @@ -59,7 +60,7 @@ func (rt *RequestIDRoundTripper) RoundTrip(r *http.Request) (*http.Response, err } requestID := rt.getRequestIDProvider()(r.Context()) - r = CloneHTTPRequest(r) + r = r.Clone(r.Context()) r.Header.Set("X-Request-ID", requestID) return rt.Delegate.RoundTrip(r) } diff --git a/httpclient/request_id_round_tripper_test.go b/httpclient/request_id_round_tripper_test.go index 438c924..1ea1d99 100644 --- a/httpclient/request_id_round_tripper_test.go +++ b/httpclient/request_id_round_tripper_test.go @@ -8,11 +8,13 @@ package httpclient import ( "context" - "github.com/acronis/go-appkit/httpserver/middleware" - "github.com/stretchr/testify/require" "net/http" "net/http/httptest" "testing" + + "github.com/stretchr/testify/require" + + "github.com/acronis/go-appkit/httpserver/middleware" ) func TestNewRequestIDRoundTripper(t *testing.T) { diff --git a/httpclient/retryable_round_tripper.go b/httpclient/retryable_round_tripper.go index 1ba605b..98ce90c 100644 --- a/httpclient/retryable_round_tripper.go +++ b/httpclient/retryable_round_tripper.go @@ -190,7 +190,7 @@ func (rt *RetryableRoundTripper) RoundTrip(req *http.Request) (*http.Response, e if curRetryAttemptNum > 0 { if !reqCloned { - req, reqCloned = CloneHTTPRequest(req), true // Per RoundTripper contract. + req, reqCloned = req.Clone(req.Context()), true // Per RoundTripper contract. } req.Header.Set(RetryAttemptNumberHeader, strconv.Itoa(curRetryAttemptNum)) } diff --git a/httpclient/user_agent_round_tripper.go b/httpclient/user_agent_round_tripper.go index ff56393..20471c0 100644 --- a/httpclient/user_agent_round_tripper.go +++ b/httpclient/user_agent_round_tripper.go @@ -69,7 +69,7 @@ func (rt *UserAgentRoundTripper) RoundTrip(req *http.Request) (*http.Response, e } } - req = CloneHTTPRequest(req) // Per RoundTripper contract. + req = req.Clone(req.Context()) // Per RoundTripper contract. req.Header.Set("User-Agent", userAgent) return rt.Delegate.RoundTrip(req) } From e1d7fc99f771f237c3b1617540cf380da8c94368 Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Fri, 10 Jan 2025 14:07:42 +0000 Subject: [PATCH 11/16] Client type and logging mode comments --- httpclient/config.go | 4 ++-- httpclient/httpclient.go | 2 +- httpclient/logging_round_tripper.go | 8 ++++---- httpclient/metrics_round_tripper.go | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/httpclient/config.go b/httpclient/config.go index 431484d..605dcfa 100644 --- a/httpclient/config.go +++ b/httpclient/config.go @@ -141,7 +141,7 @@ type RetriesConfig struct { // MaxAttempts is the maximum number of attempts to retry the request. MaxAttempts int - // Policy of a retry: [exponential, constant]. default is exponential. + // Policy of a retry: [exponential, constant]. Policy RetryPolicy // ExponentialBackoff is the configuration for exponential backoff. @@ -264,7 +264,7 @@ type LogConfig struct { // SlowRequestThreshold is a threshold for slow requests. SlowRequestThreshold time.Duration - // Mode of logging: all or failed. + // Mode of logging: [all, failed]. Mode LoggingMode } diff --git a/httpclient/httpclient.go b/httpclient/httpclient.go index 88622b9..513714f 100644 --- a/httpclient/httpclient.go +++ b/httpclient/httpclient.go @@ -36,7 +36,7 @@ type Opts struct { // UserAgent is a user agent string. UserAgent string - // ClientType is a target service. e.g. 'auth-service' + // ClientType represents a type of client, it's a service component reference. e.g. 'auth-service' ClientType string // Delegate is the next RoundTripper in the chain. diff --git a/httpclient/logging_round_tripper.go b/httpclient/logging_round_tripper.go index f460f86..f5ffc45 100644 --- a/httpclient/logging_round_tripper.go +++ b/httpclient/logging_round_tripper.go @@ -38,10 +38,10 @@ type LoggingRoundTripper struct { // Delegate is the next RoundTripper in the chain. Delegate http.RoundTripper - // ClientType is a target service. e.g. 'auth-service' + // ClientType represents a type of client, it's a service component reference. e.g. 'auth-service' ClientType string - // Mode of logging: all or failed. + // Mode of logging: [all, failed]. Mode LoggingMode // SlowRequestThreshold is a threshold for slow requests. @@ -54,10 +54,10 @@ type LoggingRoundTripper struct { // LoggingRoundTripperOpts represents an options for LoggingRoundTripper. type LoggingRoundTripperOpts struct { - // ClientType is a target service. e.g. 'auth-service' + // ClientType represents a type of client, it's a service component reference. e.g. 'auth-service' ClientType string - // Mode of logging: all or failed. + // Mode of logging: [all, failed]. Mode LoggingMode // SlowRequestThreshold is a threshold for slow requests. diff --git a/httpclient/metrics_round_tripper.go b/httpclient/metrics_round_tripper.go index 66bd3cf..a941269 100644 --- a/httpclient/metrics_round_tripper.go +++ b/httpclient/metrics_round_tripper.go @@ -59,7 +59,7 @@ type MetricsRoundTripper struct { // Delegate is the next RoundTripper in the chain. Delegate http.RoundTripper - // ClientType is a target service. e.g. 'auth-service' + // ClientType represents a type of client, it's a service component reference. e.g. 'auth-service' ClientType string // Collector is a metrics collector. @@ -71,7 +71,7 @@ type MetricsRoundTripper struct { // MetricsRoundTripperOpts is an HTTP transport that measures requests done. type MetricsRoundTripperOpts struct { - // ClientType is a target service. e.g. 'auth-service' + // ClientType represents a type of client, it's a service component reference. e.g. 'auth-service' ClientType string // ClassifyRequest does request classification, producing non-parameterized summary for given request. From dcabeab357aaa1c7d177c46e2b6f3d64676f261a Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Mon, 13 Jan 2025 12:17:42 +0000 Subject: [PATCH 12/16] HTTP client must new and metrics required collector --- httpclient/config.go | 2 +- httpclient/config_test.go | 2 +- httpclient/httpclient.go | 14 +++++----- httpclient/httpclient_test.go | 6 ++--- httpclient/logging_round_tripper.go | 8 +++--- httpclient/logging_round_tripper_test.go | 6 ++--- httpclient/metrics_round_tripper.go | 29 +++++++++++---------- httpclient/metrics_round_tripper_test.go | 16 ++++++++++++ httpclient/request_id_round_tripper.go | 4 +-- httpclient/request_id_round_tripper_test.go | 8 ++++-- 10 files changed, 57 insertions(+), 38 deletions(-) diff --git a/httpclient/config.go b/httpclient/config.go index 605dcfa..c9ae88a 100644 --- a/httpclient/config.go +++ b/httpclient/config.go @@ -295,7 +295,7 @@ func (c *LogConfig) Set(dp config.DataProvider) error { } loggingMode := LoggingMode(mode) if !loggingMode.IsValid() { - return dp.WrapKeyErr(cfgKeyLogMode, errors.New("choose one of: [none, all, failed]")) + return dp.WrapKeyErr(cfgKeyLogMode, errors.New("choose one of: [all, failed]")) } c.Mode = loggingMode diff --git a/httpclient/config_test.go b/httpclient/config_test.go index 5c7b72a..1214a6b 100644 --- a/httpclient/config_test.go +++ b/httpclient/config_test.go @@ -91,7 +91,7 @@ func TestConfigLogger(t *testing.T) { actualConfig = &Config{} err = config.NewDefaultLoader("").LoadFromReader(bytes.NewReader(yamlData), config.DataTypeYAML, actualConfig) require.Error(t, err) - require.Equal(t, "log.mode: choose one of: [none, all, failed]", err.Error()) + require.Equal(t, "log.mode: choose one of: [all, failed]", err.Error()) } func TestConfigRetriesPolicy(t *testing.T) { diff --git a/httpclient/httpclient.go b/httpclient/httpclient.go index 513714f..78a2d65 100644 --- a/httpclient/httpclient.go +++ b/httpclient/httpclient.go @@ -20,9 +20,9 @@ func New(cfg *Config) (*http.Client, error) { return NewWithOpts(cfg, Opts{}) } -// Must wraps delegate transports with logging, rate limiting, retryable, request id +// MustNew wraps delegate transports with logging, rate limiting, retryable, request id // and panics if any error occurs. -func Must(cfg *Config) *http.Client { +func MustNew(cfg *Config) *http.Client { client, err := New(cfg) if err != nil { panic(err) @@ -48,8 +48,8 @@ type Opts struct { // RequestIDProvider is a function that provides a request ID. RequestIDProvider func(ctx context.Context) string - // Collector is a metrics collector. - Collector MetricsCollector + // MetricsCollector is a metrics collector. + MetricsCollector MetricsCollector } // NewWithOpts wraps delegate transports with options @@ -95,7 +95,7 @@ func NewWithOpts(cfg *Config, opts Opts) (*http.Client, error) { } if cfg.Metrics.Enabled { - delegate = NewMetricsRoundTripperWithOpts(delegate, opts.Collector, MetricsRoundTripperOpts{ + delegate = NewMetricsRoundTripperWithOpts(delegate, opts.MetricsCollector, MetricsRoundTripperOpts{ ClientType: opts.ClientType, }) } @@ -110,10 +110,10 @@ func NewWithOpts(cfg *Config, opts Opts) (*http.Client, error) { return &http.Client{Transport: delegate, Timeout: cfg.Timeout}, nil } -// MustWithOpts wraps delegate transports with options +// MustNewWithOpts wraps delegate transports with options // logging, metrics, rate limiting, retryable, user agent, request id // and panics if any error occurs. -func MustWithOpts(cfg *Config, opts Opts) *http.Client { +func MustNewWithOpts(cfg *Config, opts Opts) *http.Client { client, err := NewWithOpts(cfg, opts) if err != nil { panic(err) diff --git a/httpclient/httpclient_test.go b/httpclient/httpclient_test.go index 322efc7..2309fa4 100644 --- a/httpclient/httpclient_test.go +++ b/httpclient/httpclient_test.go @@ -51,7 +51,7 @@ func TestMustHTTPClientLoggingRoundTripper(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() cfg.Log.Enabled = true - client := Must(cfg) + client := MustNew(cfg) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, server.URL, nil) require.NoError(t, err) @@ -100,7 +100,7 @@ func TestMustHTTPClientWithOptsRoundTripper(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() cfg.Log.Enabled = true - client := MustWithOpts(cfg, Opts{ + client := MustNewWithOpts(cfg, Opts{ UserAgent: "test-agent", ClientType: "test-client-type", }) @@ -135,7 +135,7 @@ func TestMustHTTPClientWithOptsRoundTripperPolicy(t *testing.T) { Multiplier: 1.1, } - client := MustWithOpts(cfg, Opts{ + client := MustNewWithOpts(cfg, Opts{ UserAgent: "test-agent", ClientType: "test-client-type", }) diff --git a/httpclient/logging_round_tripper.go b/httpclient/logging_round_tripper.go index f5ffc45..538ae5f 100644 --- a/httpclient/logging_round_tripper.go +++ b/httpclient/logging_round_tripper.go @@ -41,7 +41,7 @@ type LoggingRoundTripper struct { // ClientType represents a type of client, it's a service component reference. e.g. 'auth-service' ClientType string - // Mode of logging: [all, failed]. + // Mode of logging: [all, failed]. 'all' by default. Mode LoggingMode // SlowRequestThreshold is a threshold for slow requests. @@ -131,14 +131,12 @@ func (rt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error } if rt.ClientType != "" { - message += " client type %s " - args = append(args, rt.ClientType) + message += fmt.Sprintf(" client type %s ", rt.ClientType) } requestID := r.Header.Get("X-Request-ID") if requestID != "" { - message += " request id %s " - args = append(args, requestID) + message += fmt.Sprintf("request id %s ", requestID) } loggerAtLevel(message, args...) diff --git a/httpclient/logging_round_tripper_test.go b/httpclient/logging_round_tripper_test.go index 06a1666..bf22974 100644 --- a/httpclient/logging_round_tripper_test.go +++ b/httpclient/logging_round_tripper_test.go @@ -56,7 +56,7 @@ func TestMustHTTPClientLoggingRoundTripperError(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() cfg.Log.Enabled = true - client := Must(cfg) + client := MustNew(cfg) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) require.NoError(t, err) @@ -79,7 +79,7 @@ func TestMustHTTPClientLoggingRoundTripperDisabled(t *testing.T) { logger := logtest.NewRecorder() cfg := NewConfig() - client := Must(cfg) + client := MustNew(cfg) ctx := middleware.NewContextWithLogger(context.Background(), logger) req, err := http.NewRequestWithContext(ctx, http.MethodPost, serverURL, nil) require.NoError(t, err) @@ -101,7 +101,7 @@ func TestMustHTTPClientLoggingRoundTripperOpts(t *testing.T) { cfg.Log.Enabled = true var loggerProviderCalled bool - client := MustWithOpts(cfg, Opts{ + client := MustNewWithOpts(cfg, Opts{ UserAgent: "test-agent", ClientType: "test-client-type", LoggerProvider: func(ctx context.Context) log.FieldLogger { diff --git a/httpclient/metrics_round_tripper.go b/httpclient/metrics_round_tripper.go index a941269..61b4872 100644 --- a/httpclient/metrics_round_tripper.go +++ b/httpclient/metrics_round_tripper.go @@ -18,7 +18,7 @@ import ( // MetricsCollector is an interface for collecting metrics for client requests. type MetricsCollector interface { // RequestDuration observes the duration of the request and the status code. - RequestDuration(requestType, remoteAddress, summary, status string, startTime time.Time) + RequestDuration(requestType, remoteAddress, summary, status string, duration float64) } // PrometheusMetricsCollector is a Prometheus metrics collector. @@ -45,8 +45,8 @@ func (p *PrometheusMetricsCollector) MustRegister() { } // RequestDuration observes the duration of the request and the status code. -func (p *PrometheusMetricsCollector) RequestDuration(requestType, host, summary, status string, start time.Time) { - p.Durations.WithLabelValues(requestType, host, summary, status).Observe(time.Since(start).Seconds()) +func (p *PrometheusMetricsCollector) RequestDuration(requestType, host, summary, status string, duration float64) { + p.Durations.WithLabelValues(requestType, host, summary, status).Observe(duration) } // Unregister the Prometheus metrics. @@ -62,8 +62,8 @@ type MetricsRoundTripper struct { // ClientType represents a type of client, it's a service component reference. e.g. 'auth-service' ClientType string - // Collector is a metrics collector. - Collector MetricsCollector + // MetricsCollector is a metrics collector. + MetricsCollector MetricsCollector // ClassifyRequest does request classification, producing non-parameterized summary for given request. ClassifyRequest func(r *http.Request, requestType string) string @@ -90,23 +90,24 @@ func NewMetricsRoundTripperWithOpts( opts MetricsRoundTripperOpts, ) http.RoundTripper { return &MetricsRoundTripper{ - Delegate: delegate, - ClientType: opts.ClientType, - Collector: collector, - ClassifyRequest: opts.ClassifyRequest, + Delegate: delegate, + ClientType: opts.ClientType, + MetricsCollector: collector, + ClassifyRequest: opts.ClassifyRequest, } } // RoundTrip measures external requests done. func (rt *MetricsRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - if rt.Collector == nil { - return rt.Delegate.RoundTrip(r) + if rt.MetricsCollector == nil { + return nil, fmt.Errorf("metrics collector is not provided") } - status := "0" start := time.Now() - resp, err := rt.Delegate.RoundTrip(r) + duration := time.Since(start).Seconds() + status := "0" + if err == nil && resp != nil { status = strconv.Itoa(resp.StatusCode) } @@ -116,6 +117,6 @@ func (rt *MetricsRoundTripper) RoundTrip(r *http.Request) (*http.Response, error summary = rt.ClassifyRequest(r, rt.ClientType) } - rt.Collector.RequestDuration(rt.ClientType, r.Host, summary, status, start) + rt.MetricsCollector.RequestDuration(rt.ClientType, r.Host, summary, status, duration) return resp, err } diff --git a/httpclient/metrics_round_tripper_test.go b/httpclient/metrics_round_tripper_test.go index 797b8ea..095e76f 100644 --- a/httpclient/metrics_round_tripper_test.go +++ b/httpclient/metrics_round_tripper_test.go @@ -49,3 +49,19 @@ func TestNewMetricsRoundTripper(t *testing.T) { require.Equal(t, metricCount, 1) } + +func TestNewMetricsCollectionRequiredRoundTripper(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusTeapot) + })) + defer server.Close() + + metricsRoundTripper := NewMetricsRoundTripper(http.DefaultTransport, nil) + client := &http.Client{Transport: metricsRoundTripper} + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, server.URL, nil) + require.NoError(t, err) + + _, err = client.Do(req) // nolint:bodyclose + require.Error(t, err) + require.Contains(t, err.Error(), "metrics collector is not provided") +} diff --git a/httpclient/request_id_round_tripper.go b/httpclient/request_id_round_tripper.go index 477b0ee..7047a51 100644 --- a/httpclient/request_id_round_tripper.go +++ b/httpclient/request_id_round_tripper.go @@ -55,11 +55,11 @@ func (rt *RequestIDRoundTripper) getRequestIDProvider() func(ctx context.Context // RoundTrip adds X-Request-ID header to the request. func (rt *RequestIDRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - if r.Header.Get("X-Request-ID") != "" { + requestID := rt.getRequestIDProvider()(r.Context()) + if r.Header.Get("X-Request-ID") != "" || requestID == "" { return rt.Delegate.RoundTrip(r) } - requestID := rt.getRequestIDProvider()(r.Context()) r = r.Clone(r.Context()) r.Header.Set("X-Request-ID", requestID) return rt.Delegate.RoundTrip(r) diff --git a/httpclient/request_id_round_tripper_test.go b/httpclient/request_id_round_tripper_test.go index 1ea1d99..699de5e 100644 --- a/httpclient/request_id_round_tripper_test.go +++ b/httpclient/request_id_round_tripper_test.go @@ -19,9 +19,10 @@ import ( func TestNewRequestIDRoundTripper(t *testing.T) { requestID := "12345" + var receivedRequestID string server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - require.Equal(t, requestID, r.Header.Get("X-Request-ID")) + receivedRequestID = r.Header.Get("X-Request-ID") rw.WriteHeader(http.StatusTeapot) })) defer server.Close() @@ -35,14 +36,16 @@ func TestNewRequestIDRoundTripper(t *testing.T) { r, err := client.Do(req) defer func() { _ = r.Body.Close() }() require.NoError(t, err) + require.Equal(t, requestID, receivedRequestID) } func TestNewRequestIDRoundTripperWithOpts(t *testing.T) { requestID := "12345" prefix := "my_custom_request_provider" + var receivedRequestID string server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - require.Equal(t, prefix+requestID, r.Header.Get("X-Request-ID")) + receivedRequestID = prefix + requestID rw.WriteHeader(http.StatusTeapot) })) defer server.Close() @@ -59,4 +62,5 @@ func TestNewRequestIDRoundTripperWithOpts(t *testing.T) { r, err := client.Do(req) defer func() { _ = r.Body.Close() }() require.NoError(t, err) + require.Equal(t, prefix+requestID, receivedRequestID) } From 6f76d99e479be3a91d9c3aa07b7af392ad766c24 Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Mon, 13 Jan 2025 12:20:03 +0000 Subject: [PATCH 13/16] Mode logging all by default comment --- httpclient/config.go | 2 +- httpclient/logging_round_tripper.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/httpclient/config.go b/httpclient/config.go index c9ae88a..1fc131c 100644 --- a/httpclient/config.go +++ b/httpclient/config.go @@ -264,7 +264,7 @@ type LogConfig struct { // SlowRequestThreshold is a threshold for slow requests. SlowRequestThreshold time.Duration - // Mode of logging: [all, failed]. + // Mode of logging: [all, failed]. 'all' by default. Mode LoggingMode } diff --git a/httpclient/logging_round_tripper.go b/httpclient/logging_round_tripper.go index 538ae5f..9b24078 100644 --- a/httpclient/logging_round_tripper.go +++ b/httpclient/logging_round_tripper.go @@ -57,7 +57,7 @@ type LoggingRoundTripperOpts struct { // ClientType represents a type of client, it's a service component reference. e.g. 'auth-service' ClientType string - // Mode of logging: [all, failed]. + // Mode of logging: [all, failed]. 'all' by default. Mode LoggingMode // SlowRequestThreshold is a threshold for slow requests. From 59abf3eeb47d0fda11c63b9b6dbc28cde91c9d8b Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Mon, 13 Jan 2025 12:59:47 +0000 Subject: [PATCH 14/16] Request ID generation and metrics labels test --- httpclient/metrics_round_tripper_test.go | 20 +++++++++---------- httpclient/request_id_round_tripper.go | 9 +++++++-- httpclient/request_id_round_tripper_test.go | 22 ++++++++++++++++++++- 3 files changed, 37 insertions(+), 14 deletions(-) diff --git a/httpclient/metrics_round_tripper_test.go b/httpclient/metrics_round_tripper_test.go index 095e76f..e0e212a 100644 --- a/httpclient/metrics_round_tripper_test.go +++ b/httpclient/metrics_round_tripper_test.go @@ -8,8 +8,10 @@ package httpclient import ( "context" + "github.com/acronis/go-appkit/testutil" "net/http" "net/http/httptest" + "strings" "testing" "github.com/prometheus/client_golang/prometheus" @@ -36,18 +38,14 @@ func TestNewMetricsRoundTripper(t *testing.T) { defer func() { _ = r.Body.Close() }() require.NoError(t, err) - ch := make(chan prometheus.Metric, 1) - go func() { - collector.Durations.Collect(ch) - close(ch) - }() - - var metricCount int - for range ch { - metricCount++ + labels := prometheus.Labels{ + "client_type": "test-client-type", + "remote_address": strings.ReplaceAll(server.URL, "http://", ""), + "summary": "POST test-client-type", + "status": "418", } - - require.Equal(t, metricCount, 1) + hist := collector.Durations.With(labels).(prometheus.Histogram) + testutil.AssertSamplesCountInHistogram(t, hist, 1) } func TestNewMetricsCollectionRequiredRoundTripper(t *testing.T) { diff --git a/httpclient/request_id_round_tripper.go b/httpclient/request_id_round_tripper.go index 7047a51..be830ee 100644 --- a/httpclient/request_id_round_tripper.go +++ b/httpclient/request_id_round_tripper.go @@ -8,6 +8,7 @@ package httpclient import ( "context" + "github.com/rs/xid" "net/http" "github.com/acronis/go-appkit/httpserver/middleware" @@ -55,11 +56,15 @@ func (rt *RequestIDRoundTripper) getRequestIDProvider() func(ctx context.Context // RoundTrip adds X-Request-ID header to the request. func (rt *RequestIDRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - requestID := rt.getRequestIDProvider()(r.Context()) - if r.Header.Get("X-Request-ID") != "" || requestID == "" { + if r.Header.Get("X-Request-ID") != "" { return rt.Delegate.RoundTrip(r) } + requestID := rt.getRequestIDProvider()(r.Context()) + if requestID == "" { + requestID = xid.New().String() + } + r = r.Clone(r.Context()) r.Header.Set("X-Request-ID", requestID) return rt.Delegate.RoundTrip(r) diff --git a/httpclient/request_id_round_tripper_test.go b/httpclient/request_id_round_tripper_test.go index 699de5e..9bfedd7 100644 --- a/httpclient/request_id_round_tripper_test.go +++ b/httpclient/request_id_round_tripper_test.go @@ -45,7 +45,7 @@ func TestNewRequestIDRoundTripperWithOpts(t *testing.T) { var receivedRequestID string server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - receivedRequestID = prefix + requestID + receivedRequestID = r.Header.Get("X-Request-ID") rw.WriteHeader(http.StatusTeapot) })) defer server.Close() @@ -64,3 +64,23 @@ func TestNewRequestIDRoundTripperWithOpts(t *testing.T) { require.NoError(t, err) require.Equal(t, prefix+requestID, receivedRequestID) } + +func TestNewRequestIDGeneratedRoundTripperWithOpts(t *testing.T) { + var receivedRequestID string + + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + receivedRequestID = r.Header.Get("X-Request-ID") + rw.WriteHeader(http.StatusTeapot) + })) + defer server.Close() + + requestIDRoundTripper := NewRequestIDRoundTripper(http.DefaultTransport) + client := &http.Client{Transport: requestIDRoundTripper} + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, server.URL, nil) + require.NoError(t, err) + + r, err := client.Do(req) + defer func() { _ = r.Body.Close() }() + require.NoError(t, err) + require.NotEmpty(t, receivedRequestID) +} From 6b953aaa570d4cec57dae60f9ae7da7326a5cbbd Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Mon, 13 Jan 2025 13:41:46 +0000 Subject: [PATCH 15/16] Fix lint imports for metrics and request id --- httpclient/metrics_round_tripper_test.go | 3 ++- httpclient/request_id_round_tripper.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/httpclient/metrics_round_tripper_test.go b/httpclient/metrics_round_tripper_test.go index e0e212a..d97bc4f 100644 --- a/httpclient/metrics_round_tripper_test.go +++ b/httpclient/metrics_round_tripper_test.go @@ -8,7 +8,6 @@ package httpclient import ( "context" - "github.com/acronis/go-appkit/testutil" "net/http" "net/http/httptest" "strings" @@ -16,6 +15,8 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" + + "github.com/acronis/go-appkit/testutil" ) func TestNewMetricsRoundTripper(t *testing.T) { diff --git a/httpclient/request_id_round_tripper.go b/httpclient/request_id_round_tripper.go index be830ee..a6a376b 100644 --- a/httpclient/request_id_round_tripper.go +++ b/httpclient/request_id_round_tripper.go @@ -8,9 +8,10 @@ package httpclient import ( "context" - "github.com/rs/xid" "net/http" + "github.com/rs/xid" + "github.com/acronis/go-appkit/httpserver/middleware" ) From fb0291598525853e5a171c1754c447abbaabcab2 Mon Sep 17 00:00:00 2001 From: Andrew Esteves Date: Mon, 13 Jan 2025 13:53:14 +0000 Subject: [PATCH 16/16] Fix diff tcp error from os --- httpclient/logging_round_tripper_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/httpclient/logging_round_tripper_test.go b/httpclient/logging_round_tripper_test.go index bf22974..2876969 100644 --- a/httpclient/logging_round_tripper_test.go +++ b/httpclient/logging_round_tripper_test.go @@ -67,7 +67,7 @@ func TestMustHTTPClientLoggingRoundTripperError(t *testing.T) { require.NotEmpty(t, logger.Entries()) loggerEntry := logger.Entries()[0] - require.Contains(t, loggerEntry.Text, "err dial tcp "+ln.Addr().String()+": connect: connection refused") + require.Contains(t, loggerEntry.Text, "err dial tcp "+ln.Addr().String()) require.NotContains(t, loggerEntry.Text, "status code") }