HTTP client round trippers logging, metrics and request id #18

andrewesteves · 2024-12-19T09:18:03Z

No description provided.

vasayxtx · 2024-12-20T11:38:13Z

httpclient/config_test.go

+func TestConfigWithLoader(t *testing.T) {
+	yamlData := []byte(`
+retries:
+  max_retry_attempts: 30


Configuration parameters should follow camelCase naming conventions to maintain consistency with other packages in go-appkit. Let’s rename them accordingly.

Additionally, the current implementation doesn’t allow enabling the retry mechanism without also enabling rate limiting. I suggest introducing an enabled: true|false flag for both features to provide more flexibility. Alternative option: use rate-limiting round tripper only if the rate limit > 0 and retryable round tripper - only if max retry attempts > 0.

vasayxtx · 2024-12-20T11:41:43Z

httpclient/httpclient.go

+
+// 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 {


Let’s provide both methods:

NewHTTPClient: This method returns an error, allowing callers to handle it gracefully.

MustNewHTTPClient: This method panics if an error occurs, providing a convenient option for cases where error handling is not necessary.

This approach ensures flexibility for different use cases.

Additionally, I propose introducing two more methods:

NewHTTPClientWithOpts

MustNewHTTPClientWithOpts

In these methods, the delegate can be within the HTTPClientOpts struct for better extensibility and clarity in configuration.

vasayxtx · 2024-12-20T11:44:56Z

httpclient/httpclient.go

+// and panics if any error occurs.
+func MustHTTPClient(cfg *Config, userAgent, reqType string, delegate http.RoundTripper) *http.Client {
+	if delegate == nil {
+		delegate = http.DefaultTransport


Please use http.DefaultTransport.(*http.Transport).Clone() to avoid any potential side effects.

vasayxtx · 2024-12-20T12:02:55Z

httpclient/config.go

+
+const (
+	// DefaultLimit is the default limit for rate limiting.
+	DefaultLimit = 10


The name DefaultLimit seems too generic for a package-level constant.

Additionally, I’m not convinced that default values are necessary in this case. Please refer to the second part of my next comment for further details.

vasayxtx · 2024-12-20T12:03:58Z

httpclient/config.go

+	DefaultWaitTimeout = 10 * time.Second
+
+	// configuration properties
+	cfgKeyRetriesMax            = "retries.max_retry_attempts"


How about renaming retries.max_retry_attempts to retries.maxAttempts? The word "retry" in the middle seems redundant and can be omitted for better readability.

Additionally, consider supporting multiple retry backoff types, such as exponential and constant. You can refer to the BackoffPolicy field in the RetryableRoundTripperOpts struct for implementation details.

vasayxtx · 2024-12-20T12:04:40Z

httpclient/config.go

+	if err != nil {
+		return err
+	}
+	c.Limit = limit


Let's do more validation and return an error for example if the limit is negative.

vasayxtx · 2024-12-20T12:16:12Z

httpclient/config.go

+}
+
+// Config represents options for HTTP client configuration.
+type Config struct {


Please consider the following suggestions:

We usually store internal config structs by value, not by pointer. I.e.:

type Config struct { Retries RetriesConfig `mapstructure:"retries"` RateLimits RateLimitConfig `mapstructure:"rate_limits"` Timeout time.Duration `mapstructure:"timeout"` }

All existing Config structs have 2 constructors: NewConfig and NewConfigWithKeyPrefix. Let's do the same for the httpclient.Config too.

Let's add a compile-time check that httpclient.Config implements config.Config and config.KeyPrefixProvider interfaces.

var _ config.Config = (*Config)(nil) var _ config.KeyPrefixProvider = (*Config)(nil)

vasayxtx · 2024-12-20T12:23:58Z

httpclient/httpclient.go

+	delegate = NewMetricsRoundTripper(delegate, reqType)
+
+	delegate, err := NewRateLimitingRoundTripperWithOpts(
+		delegate, cfg.RateLimits.Limit, cfg.RateLimits.TransportOpts(),


It would be a good idea to accept LoggerProvider func(ctx context.Context) log.FieldLogger as an optional parameter in the constructors (NewHTTPClient and MustNewHTTPClient). This provider can then be passed as an option to NewRateLimitingRoundTripperWithOpts.

By default, the LoggerProvider should be set to middleware.GetLoggerFromContext.

Additionally, this LoggerProvider should also be utilized for the LoggingRoundTripper.

vasayxtx · 2024-12-20T13:24:53Z

httpclient/httpclient.go

+		delegate = http.DefaultTransport
+	}
+
+	delegate = NewLoggingRoundTripper(delegate, reqType)


I’m not sure it’s a good idea to log all requests unconditionally without providing a way to configure it.

Could we consider adding a configuration like the following?

log: slowRequestThreshold: 1s # Log requests taking 1s or longer (warn level). mode: none | failed_requests | all_requests # Log modes: # - none: Disable logging entirely. # - failed_requests: Log only failed requests (4xx/5xx) at error level. # - all_requests: Log all requests (2xx at info level, 4xx/5xx at error level). # Default: failed_requests or none (to be decided).

@andrewesteves, what’s your opinion on this approach?

@vasayxtx i liked the approach and implemented it, just the namings i suggest none, failed and all since it's a logger round tripper for http clients the "requests" suffix could be omitted. is it okay for you?

vasayxtx · 2024-12-20T13:27:22Z

httpclient/logging_round_tripper.go

+
+		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"


Let’s differentiate logs for successfully completed and failed requests. Logging them separately will make it much easier to search for errors in the logs.

For additional suggestions, please refer to my previous comment.

vasayxtx · 2024-12-20T13:31:34Z

httpclient/metrics_round_tripper.go

+	ExternalHTTPRequestDuration = prometheus.NewHistogramVec(
+		prometheus.HistogramOpts{
+			Namespace: namespace,
+			Name:      "external_http_request_duration",


The name external_http_request_duration could be ambiguous, as "external" might imply either incoming requests from the internet or outgoing requests. I suggest renaming the Prometheus metric to http_client_request_duration_seconds for better clarity.

Additionally, this aligns with the naming convention already used in go-authkit.

vasayxtx · 2024-12-20T13:34:33Z

httpclient/metrics_round_tripper.go

+	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)


I suggest avoiding log production in the MetricsRoundTripper that is responsible for collecting metrics. Instead, we should use a separate LoggingRoundTripper specifically for logging purposes. This separation of concerns will keep the implementation cleaner and more modular.

vasayxtx · 2024-12-20T13:42:10Z

httpclient/request_id_round_tripper.go

+
+// RoundTrip adds X-Request-ID header to the request.
+func (rt *RequestIDRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
+	requestID := middleware.GetRequestIDFromContext(r.Context())


Let’s add support for a RequestIDProvider func(ctx context.Context) string to enable customization of how request IDs are generated or retrieved. This should be included as part of the HTTPClientOpts struct, with a default implementation of middleware.GetRequestIDFromContext.

Use case: This is particularly useful for scenarios like outgoing requests from background workers, where the context is not tied to an incoming HTTP request.

HTTP client round trippers logging, metrics and request id

4b50866

andrewesteves requested review from MikeYast and vasayxtx as code owners December 19, 2024 09:18

vasayxtx reviewed Dec 20, 2024

View reviewed changes

andrewesteves added 2 commits December 24, 2024 18:18

Client providers, configurations and validations

6d7fa55

Retries with policy options

2d02ab6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP client round trippers logging, metrics and request id #18

HTTP client round trippers logging, metrics and request id #18

andrewesteves commented Dec 19, 2024

vasayxtx Dec 20, 2024 •

edited

Loading

vasayxtx Dec 20, 2024 •

edited

Loading

vasayxtx Dec 20, 2024 •

edited

Loading

vasayxtx Dec 20, 2024

vasayxtx Dec 20, 2024 •

edited

Loading

vasayxtx Dec 20, 2024

vasayxtx Dec 20, 2024

vasayxtx Dec 20, 2024

vasayxtx Dec 20, 2024

andrewesteves Dec 24, 2024

vasayxtx Dec 20, 2024

vasayxtx Dec 20, 2024 •

edited

Loading

vasayxtx Dec 20, 2024

vasayxtx Dec 20, 2024

HTTP client round trippers logging, metrics and request id #18

Are you sure you want to change the base?

HTTP client round trippers logging, metrics and request id #18

Conversation

andrewesteves commented Dec 19, 2024

vasayxtx Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

vasayxtx Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

vasayxtx Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasayxtx Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasayxtx Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasayxtx Dec 20, 2024 •

edited

Loading

vasayxtx Dec 20, 2024 •

edited

Loading

vasayxtx Dec 20, 2024 •

edited

Loading

vasayxtx Dec 20, 2024 •

edited

Loading

vasayxtx Dec 20, 2024 •

edited

Loading