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.