Telemetry: stop retrying into 429s, honour Retry-After, fix userAgent

[samikshya-db]

Summary

After v1.11.0 enabled telemetry by default via the server feature flag, high-QPS workloads produced excessive 429s on /telemetry-ext. Three issues compounded:

1. Double-retry. The telemetry exporter ran its own retry loop on top of the retryablehttp-wrapped HTTP client (internal/client.RetryableClient), which already retries 429/5xx with Retry-After. Result: up to RetryMax × (MaxRetries+1) HTTP attempts per export, all collapsed into a single circuit-breaker outcome — so the breaker barely opened against persistent throttling.
2. Untraceable in access logs. Telemetry POSTs and feature-flag GETs sent no User-Agent, so 429s landed in access logs tagged as Go-http-client/1.1 and could not be attributed to godatabrickssqlconnector by driver version.
3. High request volume. FlushInterval=5s / BatchSize=100.

Changes

Retry behavior

• telemetry/exporter.go — Removed doExport's retry loop entirely. doExport now makes a single HTTP request; transient retries (429/5xx, Retry-After) are owned by the underlying retryablehttp client. Each export() call now corresponds to exactly one HTTP transaction = one breaker outcome.
• telemetry/config.go, telemetry/driver_integration.go — Removed MaxRetries / RetryDelay from telemetry.Config and TelemetryInitOptions. telemetry_retry_count / telemetry_retry_delay DSN params still parse without error for backwards compatibility but are no-ops.

Identifiability

• connector.go — New buildUserAgent helper mirroring internal/client/client.go:295-302 exactly: DriverName/DriverVersion + optional UserAgentEntry + agent product.
• telemetry/exporter.go, telemetry/featureflag.go — Set User-Agent on telemetry POST and feature-flag GET. Plumbed via TelemetryInitOptions.UserAgent.

Cadence and breaker tuning

• telemetry/config.go — FlushInterval 5s → 30s, BatchSize 100 → 200.
• telemetry/circuitbreaker.go — minimumNumberOfCalls 20 → 10 (so low-traffic clients can trip the breaker now that each export is one signal), waitDurationInOpenState 30s → 60s (respect typical Retry-After).

Tests

• Removed obsolete retry/backoff tests (TestExport_RetryOn5xx, TestExport_ExponentialBackoff, TestIsRetryableStatus, retry-config parsing tests).
• Added TestExport_SingleAttemptPerExport covering 4xx/429/5xx, asserting the exporter never retries.
• Added TestExport_SetsUserAgent and TestFetchFeatureFlag_SetsUserAgent.

Mitigation

While this rolls out, users can opt out via DSN: enableTelemetry=false. Server-side: disable enableTelemetryForGoDriver for affected workspaces.

Test plan

• go test ./... — all green locally.
• Exporter never retries (4xx, 429, 500, 503).
• User-Agent set on telemetry POST and feature-flag GET.
• Verify in Lumberjack post-deploy that /telemetry-ext and /api/2.0/connector-service/feature-flags/GOLANG/... requests carry godatabrickssqlconnector/<version> in http_user_agent.
• Confirm 429 rate against /telemetry-ext drops after rollout.

This pull request and its description were written by Isaac.

[gopalldb]

Must-fix

1. "One breaker signal per export" is only true for 429. WithSkipRateLimitRetry(ctx) in exporter.go suppresses retryablehttp's 429 retries, but 502/503/5xx still retry internally per
   isRetryableServerResponse in internal/client/client.go. A 503 storm will go through N internal retries before surfacing one error to the breaker — same problem the PR claims to fix, just one code path
   narrower. Either pass a stricter "skip all retries" context flag from telemetry, or accept the asymmetry and document it explicitly.
2. Retry-After is honored twice on 429. retryablehttp's backoff() (client.go ~L749) already waits Retry-After before re-attempting; the exporter then calls extendOpenStateAtLeast(...) with the same value, so
   the breaker sleeps Retry-After + waitDurationInOpenState. With both ~60s the effective pause is ~120s. Either move Retry-After handling exclusively to the breaker (don't retry at the transport layer for
   telemetry) or skip the breaker extension when retryablehttp owned the wait.
3. Retry-After not parsed on 503. Exporter only extracts the header when resp.StatusCode == 429. A 503 with Retry-After is silently ignored — breaker stays open the default 60s regardless of what the server
   asked for. Apply the same Retry-After parse to 503 (and 429), not just 429.

Should-fix

4. DSN backcompat is silent. telemetry_retry_count / telemetry_retry_delay are extracted and discarded (_, _ = params.extract(...) style). Users with legacy DSNs won't know their config is ignored. Log a
   one-time DEBUG (or WARN) when these params are present.
5. Breaker math change isn't documented. minimumNumberOfCalls 20→10 with FlushInterval 5s→30s means worst-case ~5 minutes to detect a sustained outage at default cadence. State this in a code comment so
   future maintainers don't accidentally regress it.
6. Sliding-window size didn't change with minimumNumberOfCalls. Window stayed at 30 while min dropped to 10. Verify intentional — the half-open recovery window now spans a smaller fraction, which shifts the
   trip-to-recover ratio.
7. buildUserAgent is duplicated, not extracted. PR body says it "mirrors internal/client/client.go:295-302 exactly" — two copies of the same logic will drift. Extract to a shared helper (e.g.,
   internal/agent/useragent.go) and have both call sites use it.

Test coverage

8. TestExport_SingleAttemptPerExport uses a bare *http.Client, not the retryablehttp-wrapped one. That correctly proves the exporter layer doesn't loop, but doesn't catch issue Add License file #1 above (transport-layer
   retry on 503). Add an integration-flavor test that uses the real RetryableClient() and asserts the combined behavior matches "one breaker signal per export" for all status codes the PR cares about.
9. TestIsRetryableStatus was deleted, not relocated. isRetryableStatus moved from exporter to the transport layer (isRetryableServerResponse); the predicate test should move with it into
   internal/client/client_test.go.
10. Breaker edge cases. New circuitbreaker_test.go (+118) covers happy paths for the Retry-After hint. Missing:
    - Two consecutive extendOpenStateAtLeast calls with different hints — which wins?
    - Half-open → fail → re-open while a prior hint is pending — does the hint persist? Reading the code, hints only clear on Close, so a stale hint can linger.

[samikshya-db]

Thanks for the thorough review. All ten items addressed in a follow-up commit on this branch:

Must-fix

1. 5xx asymmetry fixed, not documented away. Renamed the context flag WithSkipRateLimitRetry → WithSkipTransientRetries and broadened it to suppress retries for both 429 and 503 (everything isRetryableServerResponse matches). Telemetry sets this, so the transport now collapses to exactly one HTTP transaction per export for 429, 503, 5xx, and transport errors — one breaker signal per call, no asymmetry. (internal/client/client.go, telemetry/exporter.go, telemetry/featureflag.go)

2. Double Retry-After eliminated. With Add License file #1 in place, retryablehttp never honours Retry-After internally on telemetry calls — the response flows directly back to the exporter. The exporter parses Retry-After once and feeds it to the breaker via extendOpenStateAtLeast. Also: the breaker already takes max(waitDurationInOpenState, retryAfterHint), not a sum, so no stacking even before this fix.

   While wiring this up I caught a related production bug: with retryablehttp's StandardClient adapter, when CheckRetry returns (false, err) net/http silently discards the response (logs "RoundTripper returned a response & error; ignoring response"). The exporter was therefore never seeing the actual 429/503 response in production. RetryPolicy now returns (false, nil) on the skip path so the response is preserved through net/http.

3. 503 Retry-After now honoured. Exporter parses Retry-After on 429 || 503 (was 429-only). Added TestExport_503RecordsRetryAfter to lock this in.

Should-fix
4. Legacy DSN params now logged. telemetry_retry_count / telemetry_retry_delay emit a one-time WARN on extraction noting they're deprecated and ignored. (internal/config/config.go)
5. Breaker timing documented. Expanded the comment on defaultCircuitBreakerConfig to spell out the minimumNumberOfCalls=10 × FlushInterval=30s = ~5min detection time, and why that's intentional (slow trip avoids flapping on transient blips for low-traffic clients; open-state interval owns the recovery cadence).
6. Sliding-window vs minimum calls clarified. Same comment: minimumNumberOfCalls gates when evaluation starts; slidingWindowSize caps how many outcomes participate once it does. Intentional — recent successes get to offset transient failures inside a larger memory window.
7. buildUserAgent deduplicated. Extracted to internal/client/useragent.go as client.BuildUserAgent(cfg). Both connector.go (telemetry/feature-flag path) and client.InitThriftClient (Thrift path) call it now — single source of truth.

Test coverage
8. Integration test through real RetryableClient. Added TestExport_SingleAttemptThroughRetryableClient which routes the export through client.RetryableClient(cfg) with a noop authenticator and asserts exactly 1 attempt for 429, 503, and 500 — catches transport-layer retry leaks for every status code the PR cares about. This is what surfaced the response-discarding bug fixed in #2.
9. TestIsRetryableStatus relocated. Added TestIsRetryableServerResponse in internal/client/client_test.go against the new predicate. Note the predicate semantics narrowed (only 429 + 503 now — 500/502/504 take the generic 5xx branch governed by ClientMethod); the test reflects that.
10. Breaker edge cases covered.
- TestCircuitBreaker_RetryAfterHintTakesMax: consecutive extendOpenStateAtLeast calls — larger wins, smaller is ignored, non-positive is a no-op.
- TestCircuitBreaker_HintClearedBeforeHalfOpenFailRecycle: Open(hint=10s) → HalfOpen (hint cleared) → fail → Open. Verifies the second Open uses waitDurationInOpenState, not the stale 10s hint — i.e. no stale hint persistence on the recycle path.

All existing tests still pass; go vet ./... clean. Diff stat: 9 files changed, +336 / −107.

---

This comment was generated with GitHub MCP.

[gopalldb]

Minor comment on host-key used in circuit breaker