diff --git a/docs/content/features/opentelemetry.mdoc b/docs/content/features/opentelemetry.mdoc index 062d298ec..46db3e633 100644 --- a/docs/content/features/opentelemetry.mdoc +++ b/docs/content/features/opentelemetry.mdoc @@ -44,6 +44,17 @@ Number of events successfully delivered. | `type` | Destination type | | `status` | Delivery status (`success`, `failed`) | +### `delivery_connections` + +Outbound HTTP connections used for delivery, split by whether the connection came from the idle pool or had to be opened. + +A high `reused=false` share means the idle connection pool is not holding connections long enough to be reused — expected at low traffic, but under sustained load it indicates the pool ceiling is binding. + +| Dimension | Description | +|-----------|-------------| +| `type` | Destination type (`webhook`, `hookdeck`) | +| `reused` | Whether the connection was reused from the idle pool | + ### `published_events` Number of events published via the Publish API. diff --git a/internal/config/destinations.go b/internal/config/destinations.go index 13fe1bee6..b0f11429e 100644 --- a/internal/config/destinations.go +++ b/internal/config/destinations.go @@ -27,6 +27,7 @@ func (c *DestinationsConfig) ToConfig(cfg *Config) destregistrydefault.RegisterD IncludeMillisecondTimestamp: c.IncludeMillisecondTimestamp, Webhook: c.Webhook.toConfig(), AWSKinesis: c.AWSKinesis.toConfig(), + DeliveryMaxConcurrency: cfg.DeliveryMaxConcurrency, } } diff --git a/internal/config/logging.go b/internal/config/logging.go index 94c625630..aa37d06db 100644 --- a/internal/config/logging.go +++ b/internal/config/logging.go @@ -3,6 +3,7 @@ package config import ( "strings" + "github.com/hookdeck/outpost/internal/destregistry" destregistrydefault "github.com/hookdeck/outpost/internal/destregistry/providers" "github.com/hookdeck/outpost/internal/version" "go.uber.org/zap" @@ -43,6 +44,10 @@ func (c *Config) LogConfigurationSummary() []zap.Field { // DISABLE_* flags. webhookCfg := c.Destinations.Webhook.toConfig() + // Delivery connection pool. Not configurable — derived from the delivery + // worker pool size — so the resolved values are logged. + fanOutPool := destregistry.SizeFanOutPool(c.DeliveryMaxConcurrency) + fields := []zap.Field{ // General zap.String("service", c.Service), @@ -102,6 +107,8 @@ func (c *Config) LogConfigurationSummary() []zap.Field { // Event Delivery zap.Int("max_destinations_per_tenant", c.MaxDestinationsPerTenant), zap.Int("delivery_timeout_seconds", c.DeliveryTimeoutSeconds), + zap.Int("delivery_max_idle_conns", fanOutPool.MaxIdleConns), + zap.Int("delivery_max_idle_conns_per_host", fanOutPool.MaxIdleConnsPerHost), // Idempotency zap.Int("publish_idempotency_key_ttl", c.PublishIdempotencyKeyTTL), diff --git a/internal/destregistry/connpool.go b/internal/destregistry/connpool.go new file mode 100644 index 000000000..65e89f08b --- /dev/null +++ b/internal/destregistry/connpool.go @@ -0,0 +1,97 @@ +package destregistry + +// Connection pool sizing for outbound delivery clients. +// +// Go's defaults are wrong for this workload in both directions: two idle +// connections per host means reuse collapses above two concurrent deliveries +// to a destination, and a 100 connection total means destinations evict each +// other in any fan-out deployment. The pool is sized from the delivery worker +// pool (DELIVERY_MAX_CONCURRENCY) as a sane default that scales with the +// deployment; an explicit config knob can be exposed later if a workload +// needs one. +// +// MaxIdleConns bounds only parked (idle) connections, never active ones — +// exceeding it causes connection churn, not errors. That makes oversizing +// cheap (idle FDs) and undersizing merely a reuse-rate loss. + +const ( + // IdleConnsPerConcurrency scales the total idle pool with the delivery + // worker count: at ~3s per delivery a worker revisits roughly 32 distinct + // destinations within the 90s IdleConnTimeout, and slow destinations are + // where reuse matters most. + IdleConnsPerConcurrency = 32 + + // MinTotalIdleConns floors the total for low-concurrency fanout. + // Concurrency bounds simultaneous requests, not distinct hosts touched + // over time — a single worker at ~100ms per delivery still cycles through + // ~900 destinations per idle window. + MinTotalIdleConns = 512 + + // MaxTotalIdleConns caps the parked-FD/memory cost where the reuse hit + // rate decays. The cap never binds below the concurrency level itself — + // see SizeFanOutPool. + MaxTotalIdleConns = 4096 + + // MinIdleConnsPerHost is the floor for per-host depth, matching Go's + // default. DELIVERY_MAX_CONCURRENCY defaults to 1, which would otherwise + // size us below stock behavior. + MinIdleConnsPerHost = 2 +) + +// PoolSizing is the resolved connection pool configuration. +type PoolSizing struct { + // MaxIdleConns bounds breadth — how many distinct destinations can hold a + // warm connection at all. + MaxIdleConns int + + // MaxIdleConnsPerHost bounds depth — how many warm connections a single + // destination keeps. + MaxIdleConnsPerHost int +} + +// SizeFanOutPool sizes a pool for a client that talks to arbitrarily many +// destination hosts (the webhook providers). Depth comes from the delivery +// worker pool — it caps how many deliveries can be in flight, so it caps how +// many connections one destination could need. Breadth scales with the same +// number: total = clamp(32×C, 512, max(4096, C)), the ceiling raised to C so +// the cap never undersizes the pool below the concurrency level. +// +// deliveryMaxConcurrency <= 0 means "unknown"; the floors apply. +func SizeFanOutPool(deliveryMaxConcurrency int) PoolSizing { + total := deliveryMaxConcurrency * IdleConnsPerConcurrency + if total < MinTotalIdleConns { + total = MinTotalIdleConns + } + if ceiling := max(MaxTotalIdleConns, deliveryMaxConcurrency); total > ceiling { + total = ceiling + } + + perHost := deliveryMaxConcurrency + if perHost < MinIdleConnsPerHost { + perHost = MinIdleConnsPerHost + } + // Depth can't exceed the total pool — a per-host limit above the total + // would silently never be reachable. + if perHost > total { + perHost = total + } + + return PoolSizing{ + MaxIdleConns: total, + MaxIdleConnsPerHost: perHost, + } +} + +// SizeSingleHostPool sizes a pool for a client that talks to one host (the +// hookdeck provider). It needs depth, not breadth, so the total is the +// per-host value rather than a fan-out ceiling. +func SizeSingleHostPool(deliveryMaxConcurrency int) PoolSizing { + perHost := deliveryMaxConcurrency + if perHost < MinIdleConnsPerHost { + perHost = MinIdleConnsPerHost + } + return PoolSizing{ + MaxIdleConns: perHost, + MaxIdleConnsPerHost: perHost, + } +} diff --git a/internal/destregistry/connpool_test.go b/internal/destregistry/connpool_test.go new file mode 100644 index 000000000..f66b8776b --- /dev/null +++ b/internal/destregistry/connpool_test.go @@ -0,0 +1,70 @@ +package destregistry_test + +import ( + "testing" + + "github.com/hookdeck/outpost/internal/destregistry" + "github.com/stretchr/testify/assert" +) + +func TestSizeFanOutPool_PerHostDerivesFromConcurrency(t *testing.T) { + t.Parallel() + + // Per-host depth tracks the delivery worker pool, floored at Go's default + // so DELIVERY_MAX_CONCURRENCY=1 never sizes us below stock behavior. + assert.Equal(t, destregistry.MinIdleConnsPerHost, destregistry.SizeFanOutPool(0).MaxIdleConnsPerHost, + "unknown concurrency should use the floor") + assert.Equal(t, destregistry.MinIdleConnsPerHost, destregistry.SizeFanOutPool(1).MaxIdleConnsPerHost, + "concurrency below the floor should use the floor") + assert.Equal(t, 64, destregistry.SizeFanOutPool(64).MaxIdleConnsPerHost) +} + +func TestSizeFanOutPool_TotalScalesWithConcurrency(t *testing.T) { + t.Parallel() + + // total = clamp(32×C, 512, max(4096, C)). + for _, tc := range []struct { + concurrency int + total int + }{ + {0, destregistry.MinTotalIdleConns}, // unknown → floor + {1, destregistry.MinTotalIdleConns}, // 32 → floor + {16, destregistry.MinTotalIdleConns}, // 512 → exactly the floor + {64, 2048}, // 32×64, between floor and cap + {128, destregistry.MaxTotalIdleConns}, // 4096 → exactly the cap + {1000, destregistry.MaxTotalIdleConns}, + } { + assert.Equal(t, tc.total, destregistry.SizeFanOutPool(tc.concurrency).MaxIdleConns, + "concurrency %d", tc.concurrency) + } +} + +func TestSizeFanOutPool_CapNeverBindsBelowConcurrency(t *testing.T) { + t.Parallel() + + // Above the cap, the total rises to the concurrency level itself so the + // per-host depth (== concurrency) is always reachable. + pool := destregistry.SizeFanOutPool(10_000) + assert.Equal(t, 10_000, pool.MaxIdleConns) + assert.Equal(t, 10_000, pool.MaxIdleConnsPerHost) + + for _, concurrency := range []int{0, 1, 16, 512, 100_000} { + pool := destregistry.SizeFanOutPool(concurrency) + assert.LessOrEqual(t, pool.MaxIdleConnsPerHost, pool.MaxIdleConns, + "a per-host limit above the total would never be reachable (concurrency %d)", concurrency) + } +} + +func TestSizeSingleHostPool_IsDepthOnly(t *testing.T) { + t.Parallel() + + // One host: the total is the per-host value. No breadth needed, so this + // stays small regardless of the fan-out formula. + pool := destregistry.SizeSingleHostPool(32) + assert.Equal(t, 32, pool.MaxIdleConnsPerHost) + assert.Equal(t, 32, pool.MaxIdleConns) + + floored := destregistry.SizeSingleHostPool(1) + assert.Equal(t, destregistry.MinIdleConnsPerHost, floored.MaxIdleConnsPerHost) + assert.Equal(t, destregistry.MinIdleConnsPerHost, floored.MaxIdleConns) +} diff --git a/internal/destregistry/httpclient.go b/internal/destregistry/httpclient.go index a1bfc580f..1bcc10789 100644 --- a/internal/destregistry/httpclient.go +++ b/internal/destregistry/httpclient.go @@ -3,6 +3,7 @@ package destregistry import ( "fmt" "net/http" + "net/http/httptrace" "net/url" "time" ) @@ -18,6 +19,25 @@ type HTTPClientConfig struct { // underlying transport plus the parsed proxy URL; returns the // RoundTripper to use thereafter. WrapTransport func(*http.Transport, *url.URL) http.RoundTripper + + // Pool sizes the transport's idle connection pool. The zero value leaves + // Go's defaults in place (2 idle per host, 100 total), which is only + // appropriate for clients outside the delivery path. Use SizeFanOutPool + // or SizeSingleHostPool to derive it. + Pool PoolSizing + + // OnConnection, if set, is invoked once per request with whether the + // underlying connection was reused. This is the signal that the pool + // ceiling is binding. + OnConnection func(reused bool) +} + +func (c HTTPClientConfig) needsTransport() bool { + return c.ProxyURL != nil || + c.UserAgent != nil || + c.OnConnection != nil || + c.Pool.MaxIdleConns > 0 || + c.Pool.MaxIdleConnsPerHost > 0 } // NewHTTPClient builds an *http.Client from config. Free function — no @@ -29,11 +49,17 @@ func NewHTTPClient(config HTTPClientConfig) (*http.Client, error) { client.Timeout = *config.Timeout } - if config.ProxyURL == nil && config.UserAgent == nil { + if !config.needsTransport() { return client, nil } transport := http.DefaultTransport.(*http.Transport).Clone() + if config.Pool.MaxIdleConns > 0 { + transport.MaxIdleConns = config.Pool.MaxIdleConns + } + if config.Pool.MaxIdleConnsPerHost > 0 { + transport.MaxIdleConnsPerHost = config.Pool.MaxIdleConnsPerHost + } var rt http.RoundTripper = transport @@ -55,10 +81,34 @@ func NewHTTPClient(config HTTPClientConfig) (*http.Client, error) { } } + if config.OnConnection != nil { + rt = &connTraceTransport{onConnection: config.OnConnection, transport: rt} + } + client.Transport = rt return client, nil } +// connTraceTransport reports whether each request got a pooled connection or +// had to open a new one. Outermost wrapper so the trace is installed before +// any other RoundTripper runs. +type connTraceTransport struct { + onConnection func(reused bool) + transport http.RoundTripper +} + +func (t *connTraceTransport) RoundTrip(req *http.Request) (*http.Response, error) { + trace := &httptrace.ClientTrace{ + GotConn: func(info httptrace.GotConnInfo) { + t.onConnection(info.Reused) + }, + } + // httptrace composes with any trace already on the context rather than + // replacing it. + req = req.WithContext(httptrace.WithClientTrace(req.Context(), trace)) + return t.transport.RoundTrip(req) +} + // userAgentTransport wraps an http.RoundTripper to inject a User-Agent header type userAgentTransport struct { userAgent string diff --git a/internal/destregistry/httpclient_pool_test.go b/internal/destregistry/httpclient_pool_test.go new file mode 100644 index 000000000..57595c5c1 --- /dev/null +++ b/internal/destregistry/httpclient_pool_test.go @@ -0,0 +1,221 @@ +package destregistry_test + +import ( + "context" + "fmt" + "io" + "net" + "net/http" + "net/http/httptest" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/hookdeck/outpost/internal/destregistry" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// countingServer is an httptest server that counts how many TCP connections +// were opened against it. That count is the thing under test: with a correctly +// sized idle pool it should track the concurrency level, not the request count. +type countingServer struct { + *httptest.Server + opened atomic.Int64 +} + +func newCountingServer(t *testing.T, latency time.Duration) *countingServer { + t.Helper() + cs := &countingServer{} + cs.Server = httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if latency > 0 { + time.Sleep(latency) + } + w.WriteHeader(http.StatusOK) + })) + cs.Server.Config.ConnState = func(_ net.Conn, state http.ConnState) { + if state == http.StateNew { + cs.opened.Add(1) + } + } + cs.Server.Start() + t.Cleanup(cs.Server.Close) + return cs +} + +// drive sends requestsPerWorker requests from each of `concurrency` workers, +// reading and closing every body so the connection is returned to the idle +// pool before the next request. +func drive(t *testing.T, client *http.Client, url string, concurrency, requestsPerWorker int) { + t.Helper() + var wg sync.WaitGroup + errs := make(chan error, concurrency*requestsPerWorker) + for range concurrency { + wg.Add(1) + go func() { + defer wg.Done() + for range requestsPerWorker { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, url, nil) + if err != nil { + errs <- err + return + } + resp, err := client.Do(req) + if err != nil { + errs <- err + return + } + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + } + }() + } + wg.Wait() + close(errs) + for err := range errs { + require.NoError(t, err) + } +} + +// TestHTTPClientPool_ConnectionsTrackConcurrency is the core verification for +// the shared-client change: connections opened must stay near the concurrency +// level rather than near the request count. +// +// Both destination speeds are covered because the two regimes fail differently +// — against a fast destination the cost of a fresh connection is latency (the +// handshake dwarfs the request), against a slow one it is ephemeral ports +// piling up in TIME_WAIT on the sender. +func TestHTTPClientPool_ConnectionsTrackConcurrency(t *testing.T) { + t.Parallel() + + const requestsPerWorker = 10 + + for _, dest := range []struct { + name string + latency time.Duration + }{ + {"fast_destination", 0}, + {"slow_destination", 20 * time.Millisecond}, + } { + for _, concurrency := range []int{1, 4, 16} { + t.Run(fmt.Sprintf("%s/concurrency_%d", dest.name, concurrency), func(t *testing.T) { + t.Parallel() + + server := newCountingServer(t, dest.latency) + client, err := destregistry.NewHTTPClient(destregistry.HTTPClientConfig{ + Pool: destregistry.SizeFanOutPool(concurrency), + }) + require.NoError(t, err) + + drive(t, client, server.URL, concurrency, requestsPerWorker) + + // Go's transport can start dialing a new connection while an + // idle one is being returned, so the count sits at or slightly + // above the concurrency level rather than exactly on it. The + // claim under test is the order of magnitude: it tracks + // concurrency, not request count. + total := concurrency * requestsPerWorker + opened := int(server.opened.Load()) + assert.LessOrEqual(t, opened, 2*concurrency, + "connections opened should track concurrency (%d), not the %d requests sent", + concurrency, total) + assert.Less(t, opened, total/2, + "connections opened should be far below the request count") + }) + } + } +} + +// TestHTTPClientPool_BeatsStockDefaults pins the behavior the change exists to +// fix, so the test above can't pass trivially: with Go's stock per-host limit +// of two, reuse collapses as soon as more than two deliveries to a destination +// overlap. Same workload, two identical servers, only the pool differs. +func TestHTTPClientPool_BeatsStockDefaults(t *testing.T) { + t.Parallel() + + const concurrency = 32 + const requestsPerWorker = 10 + + sizedServer := newCountingServer(t, 0) + sized, err := destregistry.NewHTTPClient(destregistry.HTTPClientConfig{ + Pool: destregistry.SizeFanOutPool(concurrency), + }) + require.NoError(t, err) + drive(t, sized, sizedServer.URL, concurrency, requestsPerWorker) + + stockServer := newCountingServer(t, 0) + stock, err := destregistry.NewHTTPClient(destregistry.HTTPClientConfig{}) + require.NoError(t, err) + drive(t, stock, stockServer.URL, concurrency, requestsPerWorker) + + // Only a strict inequality is asserted — the exact ratio varies with + // scheduling, and pinning a multiplier makes the test flaky. + assert.Greater(t, stockServer.opened.Load(), sizedServer.opened.Load(), + "stock defaults should open more connections than a pool sized for the concurrency level") +} + +func TestHTTPClientPool_OnConnectionReportsReuse(t *testing.T) { + t.Parallel() + + const concurrency = 4 + const requestsPerWorker = 10 + + server := newCountingServer(t, 0) + + var fresh, reused atomic.Int64 + client, err := destregistry.NewHTTPClient(destregistry.HTTPClientConfig{ + Pool: destregistry.SizeFanOutPool(concurrency), + OnConnection: func(wasReused bool) { + if wasReused { + reused.Add(1) + } else { + fresh.Add(1) + } + }, + }) + require.NoError(t, err) + + drive(t, client, server.URL, concurrency, requestsPerWorker) + + assert.Equal(t, int64(concurrency*requestsPerWorker), fresh.Load()+reused.Load(), + "every request should report exactly one connection acquisition") + // Every non-reused acquisition needed a connection the server saw opened. + // The reverse doesn't hold: a speculative dial that loses the race to a + // returning idle connection is opened and then discarded without ever + // serving a request. + assert.LessOrEqual(t, fresh.Load(), server.opened.Load()) + assert.Positive(t, reused.Load()) +} + +// TestHTTPClientPool_MultipleHostsKeepWarmConnections covers breadth: fan-out +// across many destinations, each with almost no concurrency, is the normal +// shape for this product. Go's 100-connection total would have destinations +// evicting each other. +func TestHTTPClientPool_MultipleHostsKeepWarmConnections(t *testing.T) { + t.Parallel() + + const hosts = 20 + const rounds = 5 + + servers := make([]*countingServer, hosts) + for i := range servers { + servers[i] = newCountingServer(t, 0) + } + + client, err := destregistry.NewHTTPClient(destregistry.HTTPClientConfig{ + Pool: destregistry.SizeFanOutPool(1), + }) + require.NoError(t, err) + + for range rounds { + for _, server := range servers { + drive(t, client, server.URL, 1, 1) + } + } + + for i, server := range servers { + assert.Equal(t, int64(1), server.opened.Load(), + "host %d should have kept its connection warm across all %d rounds", i, rounds) + } +} diff --git a/internal/destregistry/providers/default.go b/internal/destregistry/providers/default.go index 5388c6831..179330373 100644 --- a/internal/destregistry/providers/default.go +++ b/internal/destregistry/providers/default.go @@ -1,6 +1,8 @@ package destregistrydefault import ( + "context" + "github.com/hookdeck/outpost/internal/destregistry" "github.com/hookdeck/outpost/internal/destregistry/providers/destawskinesis" "github.com/hookdeck/outpost/internal/destregistry/providers/destawss3" @@ -12,6 +14,7 @@ import ( "github.com/hookdeck/outpost/internal/destregistry/providers/destrabbitmq" "github.com/hookdeck/outpost/internal/destregistry/providers/destwebhook" "github.com/hookdeck/outpost/internal/destregistry/providers/destwebhookstandard" + "github.com/hookdeck/outpost/internal/emetrics" ) // WebhookHeaderConfig is the resolved directive for a single webhook system @@ -48,6 +51,11 @@ type RegisterDefaultDestinationOptions struct { IncludeMillisecondTimestamp bool Webhook *DestWebhookConfig AWSKinesis *DestAWSKinesisConfig + + // DeliveryMaxConcurrency is the delivery worker pool size. It bounds how + // many deliveries can be in flight, and therefore how many connections a + // single destination could need. 0 leaves Go's per-host default in place. + DeliveryMaxConcurrency int } // RegisterDefault registers the default destination providers with the registry. @@ -62,6 +70,21 @@ func RegisterDefault(registry destregistry.Registry, opts RegisterDefaultDestina basePublisherOpts = append(basePublisherOpts, destregistry.WithMillisecondTimestamp(opts.IncludeMillisecondTimestamp)) } + // Webhook destinations fan out across arbitrarily many hosts, so their pool + // needs breadth as well as depth. The hookdeck provider talks to one host. + fanOutPool := destregistry.SizeFanOutPool(opts.DeliveryMaxConcurrency) + singleHostPool := destregistry.SizeSingleHostPool(opts.DeliveryMaxConcurrency) + + emeter, err := emetrics.New() + if err != nil { + return err + } + connObserver := func(destinationType string) func(bool) { + return func(reused bool) { + emeter.DeliveryConnection(context.Background(), reused, destinationType) + } + } + // Register webhook provider based on mode if opts.Webhook != nil && opts.Webhook.Mode == "standard" { // Standard Webhooks mode - register webhook_standard as "webhook" @@ -70,6 +93,8 @@ func RegisterDefault(registry destregistry.Registry, opts RegisterDefaultDestina destwebhookstandard.WithProxyURL(opts.Webhook.ProxyURL), destwebhookstandard.WithHeaderPrefix(opts.Webhook.HeaderPrefix), destwebhookstandard.WithMaxResponseBodyBytes(opts.Webhook.MaxResponseBodyBytes), + destwebhookstandard.WithConnectionPool(fanOutPool), + destwebhookstandard.WithConnectionObserver(connObserver("webhook")), } webhookStandard, err := destwebhookstandard.New(loader, basePublisherOpts, webhookStandardOpts...) if err != nil { @@ -80,6 +105,8 @@ func RegisterDefault(registry destregistry.Registry, opts RegisterDefaultDestina // Default mode - register customizable webhook as "webhook" webhookOpts := []destwebhook.Option{ destwebhook.WithUserAgent(opts.UserAgent), + destwebhook.WithConnectionPool(fanOutPool), + destwebhook.WithConnectionObserver(connObserver("webhook")), } if opts.Webhook != nil { webhookOpts = append(webhookOpts, @@ -105,7 +132,9 @@ func RegisterDefault(registry destregistry.Registry, opts RegisterDefaultDestina } hookdeck, err := desthookdeck.New(loader, basePublisherOpts, - desthookdeck.WithUserAgent(opts.UserAgent)) + desthookdeck.WithUserAgent(opts.UserAgent), + desthookdeck.WithConnectionPool(singleHostPool), + desthookdeck.WithConnectionObserver(connObserver("hookdeck"))) if err != nil { return err } diff --git a/internal/destregistry/providers/desthookdeck/desthookdeck.go b/internal/destregistry/providers/desthookdeck/desthookdeck.go index af9b01d1c..3dfb32e01 100644 --- a/internal/destregistry/providers/desthookdeck/desthookdeck.go +++ b/internal/destregistry/providers/desthookdeck/desthookdeck.go @@ -34,8 +34,13 @@ func WithHTTPClient(client *http.Client) ProviderOption { // Provider implementation type HookdeckProvider struct { *destregistry.BaseProvider - userAgent string - httpClient *http.Client + userAgent string + // httpClient is shared by every publisher this provider creates. This + // provider talks to a single host, so it is sized for depth rather than + // breadth — see destregistry.SizeSingleHostPool. + httpClient *http.Client + pool destregistry.PoolSizing + onConnection func(reused bool) } // Ensure our provider implements the Provider interface @@ -48,6 +53,23 @@ func WithUserAgent(userAgent string) ProviderOption { } } +// WithConnectionPool sizes the shared client's idle connection pool. Ignored +// when a client is injected with WithHTTPClient. +func WithConnectionPool(pool destregistry.PoolSizing) ProviderOption { + return func(p *HookdeckProvider) { + p.pool = pool + } +} + +// WithConnectionObserver registers a callback invoked once per request with +// whether the underlying connection was reused. Ignored when a client is +// injected with WithHTTPClient. +func WithConnectionObserver(fn func(reused bool)) ProviderOption { + return func(p *HookdeckProvider) { + p.onConnection = fn + } +} + // Constructor func New(loader metadata.MetadataLoader, basePublisherOpts []destregistry.BasePublisherOption, opts ...ProviderOption) (*HookdeckProvider, error) { base, err := destregistry.NewBaseProvider(loader, "hookdeck", basePublisherOpts...) @@ -64,6 +86,18 @@ func New(loader metadata.MetadataLoader, basePublisherOpts []destregistry.BasePu opt(provider) } + if provider.httpClient == nil { + client, err := destregistry.NewHTTPClient(destregistry.HTTPClientConfig{ + UserAgent: &provider.userAgent, + Pool: provider.pool, + OnConnection: provider.onConnection, + }) + if err != nil { + return nil, err + } + provider.httpClient = client + } + return provider, nil } @@ -110,11 +144,23 @@ func NewPublisher(tokenString string, opts ...PublisherOption) (*HookdeckPublish // Note: This NewPublisher is called from CreatePublisher which has access to BaseProvider // For now, we create a default BasePublisher here - this will be refactored + timeout := 30 * time.Second + client, err := destregistry.NewHTTPClient(destregistry.HTTPClientConfig{ + Timeout: &timeout, + // Single host — depth only. Concurrency is unknown here, so the + // floor applies; callers wanting a deeper pool inject a client via + // PublisherWithClient (as CreatePublisher effectively does by + // sharing the provider's client). + Pool: destregistry.SizeSingleHostPool(0), + }) + if err != nil { + return nil, err + } publisher := &HookdeckPublisher{ BasePublisher: destregistry.NewBasePublisher(), tokenString: tokenString, parsedToken: parsedToken, - client: &http.Client{Timeout: 30 * time.Second}, + client: client, } // Apply custom options @@ -144,26 +190,12 @@ func (p *HookdeckProvider) CreatePublisher(ctx context.Context, destination *mod }) } - // Determine HTTP client - var client *http.Client - if p.httpClient != nil { - client = p.httpClient - } else { - var err error - client, err = destregistry.NewHTTPClient(destregistry.HTTPClientConfig{ - UserAgent: &p.userAgent, - }) - if err != nil { - return nil, err - } - } - // Create publisher with base publisher from provider publisher := &HookdeckPublisher{ BasePublisher: p.BaseProvider.NewPublisher(destregistry.WithDeliveryMetadata(destination.DeliveryMetadata)), tokenString: tokenString, parsedToken: parsedToken, - client: client, + client: p.httpClient, } return publisher, nil diff --git a/internal/destregistry/providers/destwebhook/destwebhook.go b/internal/destregistry/providers/destwebhook/destwebhook.go index ef470c0c3..8602c4c49 100644 --- a/internal/destregistry/providers/destwebhook/destwebhook.go +++ b/internal/destregistry/providers/destwebhook/destwebhook.go @@ -116,6 +116,21 @@ type WebhookDestination struct { rawSigningSecretTemplate string signingSecretTemplate *template.Template maxResponseBodyBytes int + + // httpClient is shared by every publisher this provider creates. Nothing + // in its configuration varies per destination — user agent, proxy and the + // transport wrapper are all provider-level — so a client per destination + // would just give each one its own two-connection idle pool. See + // destregistry.PoolSizing. + // + // Only connection-level concerns justify a separate client. Headers, auth, + // timeouts and body limits are per-request and belong on the request. If + // per-destination proxy settings or client certificates are ever needed, + // transports should be keyed by that configuration and shared within a key + // — not built per destination. + httpClient *http.Client + pool destregistry.PoolSizing + onConnection func(reused bool) } type WebhookDestinationConfig struct { @@ -170,6 +185,22 @@ func WithMaxResponseBodyBytes(maxBytes int) Option { } } +// WithConnectionPool sizes the shared client's idle connection pool. The zero +// value leaves Go's defaults in place. +func WithConnectionPool(pool destregistry.PoolSizing) Option { + return func(w *WebhookDestination) { + w.pool = pool + } +} + +// WithConnectionObserver registers a callback invoked once per request with +// whether the underlying connection was reused. +func WithConnectionObserver(fn func(reused bool)) Option { + return func(w *WebhookDestination) { + w.onConnection = fn + } +} + // WithEventIDHeader sets the event ID header directive. A non-empty name pins // the exact header name (bypassing "event-id"); disabled omits the // header. The name is trimmed of whitespace. @@ -335,6 +366,22 @@ func New(loader metadata.MetadataLoader, basePublisherOpts []destregistry.BasePu } destination.signingSecretTemplate = tmpl + var proxyURL *string + if destination.proxyURL != "" { + proxyURL = &destination.proxyURL + } + httpClient, err := destregistry.NewHTTPClient(destregistry.HTTPClientConfig{ + UserAgent: &destination.userAgent, + ProxyURL: proxyURL, + WrapTransport: WrapTransport, + Pool: destination.pool, + OnConnection: destination.onConnection, + }) + if err != nil { + return nil, err + } + destination.httpClient = httpClient + return destination, nil } @@ -427,23 +474,9 @@ func (d *WebhookDestination) CreatePublisher(ctx context.Context, destination *m WithAlgorithm(d.signingAlgorithm), ) - var proxyURL *string - if d.proxyURL != "" { - proxyURL = &d.proxyURL - } - - httpClient, err := destregistry.NewHTTPClient(destregistry.HTTPClientConfig{ - UserAgent: &d.userAgent, - ProxyURL: proxyURL, - WrapTransport: WrapTransport, - }) - if err != nil { - return nil, err - } - return &WebhookPublisher{ BasePublisher: d.BaseProvider.NewPublisher(destregistry.WithDeliveryMetadata(destination.DeliveryMetadata)), - httpClient: httpClient, + httpClient: d.httpClient, url: config.URL, headerPrefix: d.headerPrefix, eventIDHeader: d.eventIDHeader, diff --git a/internal/destregistry/providers/destwebhook/destwebhook_connpool_test.go b/internal/destregistry/providers/destwebhook/destwebhook_connpool_test.go new file mode 100644 index 000000000..4d3daf508 --- /dev/null +++ b/internal/destregistry/providers/destwebhook/destwebhook_connpool_test.go @@ -0,0 +1,104 @@ +package destwebhook_test + +import ( + "context" + "fmt" + "net" + "net/http" + "net/http/httptest" + "sync" + "sync/atomic" + "testing" + + "github.com/hookdeck/outpost/internal/destregistry" + "github.com/hookdeck/outpost/internal/destregistry/providers/destwebhook" + "github.com/hookdeck/outpost/internal/util/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestWebhookProvider_PublishersShareOneConnectionPool is the provider-level +// half of the pooling change. Before, each destination got its own client with +// its own two-connection idle pool, so concurrent deliveries to one destination +// opened roughly one connection each. Now the client — and therefore the pool — +// is built once per provider. +func TestWebhookProvider_PublishersShareOneConnectionPool(t *testing.T) { + t.Parallel() + + const destinations = 8 + const requestsPerDestination = 5 + const concurrency = 4 + + var opened, received atomic.Int64 + server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + received.Add(1) + w.WriteHeader(http.StatusOK) + })) + server.Config.ConnState = func(_ net.Conn, state http.ConnState) { + if state == http.StateNew { + opened.Add(1) + } + } + server.Start() + defer server.Close() + + provider := NewTestProvider(t, + destwebhook.WithConnectionPool(destregistry.SizeFanOutPool(concurrency)), + ) + + ctx := context.Background() + + // Every destination points at the same host — the pool is per-host, so + // these all draw from the same set of connections once the client is + // shared. + publishers := make([]destregistry.Publisher, destinations) + for i := range publishers { + dest := testutil.DestinationFactory.Any( + testutil.DestinationFactory.WithType("webhook"), + testutil.DestinationFactory.WithConfig(map[string]string{"url": server.URL}), + testutil.DestinationFactory.WithCredentials(map[string]string{ + "secret": fmt.Sprintf("whsec_test_%d", i), + }), + ) + publisher, err := provider.CreatePublisher(ctx, &dest) + require.NoError(t, err) + publishers[i] = publisher + t.Cleanup(func() { publisher.Close() }) + } + + // Drive `concurrency` workers over the publishers round-robin. + var wg sync.WaitGroup + errs := make(chan error, destinations*requestsPerDestination) + work := make(chan destregistry.Publisher) + for range concurrency { + wg.Add(1) + go func() { + defer wg.Done() + for publisher := range work { + event := testutil.EventFactory.Any() + if _, err := publisher.Publish(ctx, &event); err != nil { + errs <- err + } + } + }() + } + for range requestsPerDestination { + for _, publisher := range publishers { + work <- publisher + } + } + close(work) + wg.Wait() + close(errs) + for err := range errs { + require.NoError(t, err) + } + + require.Equal(t, int64(destinations*requestsPerDestination), received.Load(), + "every publish should have reached the server") + // Slack of one concurrency level: Go's transport can start dialing while + // an idle connection is being returned. + assert.LessOrEqual(t, int(opened.Load()), 2*concurrency, + "publishers should share one pool: %d requests across %d destinations should track concurrency (%d), not destination count", + destinations*requestsPerDestination, destinations, concurrency) +} diff --git a/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go b/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go index b677999ee..ad6ab2d47 100644 --- a/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go +++ b/internal/destregistry/providers/destwebhookstandard/destwebhookstandard.go @@ -78,6 +78,11 @@ type StandardWebhookDestination struct { headerPrefix string // Prefix for metadata headers (defaults to "webhook-") maxResponseBodyBytes int + // httpClient is shared by every publisher this provider creates — see the + // note on destwebhook.WebhookDestination.httpClient. + httpClient *http.Client + pool destregistry.PoolSizing + onConnection func(reused bool) // Standard Webhooks templates are fixed by the spec, so the formatters are // built once and shared by every publisher rather than per destination. signatureFormatter destwebhook.SignatureFormatter @@ -124,6 +129,22 @@ func WithMaxResponseBodyBytes(maxBytes int) Option { } } +// WithConnectionPool sizes the shared client's idle connection pool. The zero +// value leaves Go's defaults in place. +func WithConnectionPool(pool destregistry.PoolSizing) Option { + return func(d *StandardWebhookDestination) { + d.pool = pool + } +} + +// WithConnectionObserver registers a callback invoked once per request with +// whether the underlying connection was reused. +func WithConnectionObserver(fn func(reused bool)) Option { + return func(d *StandardWebhookDestination) { + d.onConnection = fn + } +} + // WithHeaderPrefix sets the prefix for metadata headers. // The prefix is trimmed of whitespace. An empty string disables the prefix entirely. // Config is responsible for providing the appropriate default ("webhook-" for standard mode). @@ -148,6 +169,22 @@ func New(loader metadata.MetadataLoader, basePublisherOpts []destregistry.BasePu // But the caller must have explicitly set it via WithHeaderPrefix. // Config is responsible for providing the appropriate default ("webhook-"). + var proxyURL *string + if destination.proxyURL != "" { + proxyURL = &destination.proxyURL + } + httpClient, err := destregistry.NewHTTPClient(destregistry.HTTPClientConfig{ + UserAgent: &destination.userAgent, + ProxyURL: proxyURL, + WrapTransport: destwebhook.WrapTransport, + Pool: destination.pool, + OnConnection: destination.onConnection, + }) + if err != nil { + return nil, err + } + destination.httpClient = httpClient + destination.signatureFormatter, err = destwebhook.NewSignatureFormatter(signatureContentTemplate) if err != nil { return nil, err @@ -250,23 +287,9 @@ func (d *StandardWebhookDestination) CreatePublisher(ctx context.Context, destin destwebhook.WithAlgorithm(destwebhook.GetAlgorithm("hmac-sha256")), ) - var proxyURL *string - if d.proxyURL != "" { - proxyURL = &d.proxyURL - } - - httpClient, err := destregistry.NewHTTPClient(destregistry.HTTPClientConfig{ - UserAgent: &d.userAgent, - ProxyURL: proxyURL, - WrapTransport: destwebhook.WrapTransport, - }) - if err != nil { - return nil, err - } - return &StandardWebhookPublisher{ BasePublisher: d.BaseProvider.NewPublisher(destregistry.WithDeliveryMetadata(destination.DeliveryMetadata)), - httpClient: httpClient, + httpClient: d.httpClient, url: config.URL, secrets: secrets, sm: sm, diff --git a/internal/emetrics/emetrics.go b/internal/emetrics/emetrics.go index 5b2f32238..6401659a6 100644 --- a/internal/emetrics/emetrics.go +++ b/internal/emetrics/emetrics.go @@ -17,6 +17,7 @@ type OutpostMetrics interface { EventEligbible(ctx context.Context, event *models.Event) APIResponseLatency(ctx context.Context, latency time.Duration, opts APIResponseLatencyOpts) APICalls(ctx context.Context, opts APICallsOpts) + DeliveryConnection(ctx context.Context, reused bool, destinationType string) } type DeliveryLatencyOpts struct { @@ -48,6 +49,7 @@ type emetricsImpl struct { eventEligibleCounter metric.Int64Counter apiResponseLatency metric.Int64Histogram apiCallsCounter metric.Int64Counter + deliveryConnCounter metric.Int64Counter } func New() (OutpostMetrics, error) { @@ -92,6 +94,12 @@ func New() (OutpostMetrics, error) { return nil, err } + if impl.deliveryConnCounter, err = meter.Int64Counter("outpost.delivery_connections", + metric.WithDescription("Outbound delivery connections, split by whether the connection was reused from the idle pool"), + ); err != nil { + return nil, err + } + return &impl, nil } @@ -127,6 +135,17 @@ func (e *emetricsImpl) APIResponseLatency(ctx context.Context, latency time.Dura )) } +// DeliveryConnection records one connection acquisition on the delivery path. +// The ratio of reused=false to the total is the signal that the idle pool +// ceiling is binding: it should stay near the concurrency level, not track +// request count. +func (e *emetricsImpl) DeliveryConnection(ctx context.Context, reused bool, destinationType string) { + e.deliveryConnCounter.Add(ctx, 1, metric.WithAttributes( + attribute.String("type", destinationType), + attribute.Bool("reused", reused), + )) +} + func (e *emetricsImpl) APICalls(ctx context.Context, opts APICallsOpts) { e.apiCallsCounter.Add(ctx, 1, metric.WithAttributes( attribute.String("method", opts.Method),