Expose HttpConfig so retry behaviour is user-configurable - #147
Open
MichaelGHSeg wants to merge 6 commits into
Open
MichaelGHSeg wants to merge 6 commits into
MichaelGHSeg wants to merge 6 commits into
Conversation
The retry state machine added in #144 could only ever be configured from CDN settings: RateLimitConfig, BackoffConfig and HttpConfig were all internal and Configuration had no entry point, so a C# consumer could not set retry behaviour at all. Kotlin and Swift both expose this. Kotlin has `Configuration.httpConfig: HttpConfig?` with a public `data class HttpConfig`; Swift has `public func httpConfig(_ config: HttpConfig?) -> Configuration`. This brings C# in line with the SDKs #144 was written to match. - Make RetryBehavior, RateLimitConfig, BackoffConfig and HttpConfig public. RetryConfig stays internal — it is plumbing built from HttpConfig, never supplied by callers. - Add Configuration.HttpConfig, as a trailing optional constructor argument so existing positional callers are unaffected. Defaults to null, preserving today's CDN-only behaviour. - Have EventPipelineProvider and SyncEventPipelineProvider pass it through as the pipeline's starting retry config. CDN settings still override it later via UpdateHttpConfig. - Make the pipeline constructors that take an HttpConfig public, so a custom IEventPipelineProvider can pass one on rather than only read it. 216 tests pass, including 6 new ones covering that a config set on Configuration reaches both pipelines' retry state machines.
This was referenced Sep 3, 2026
Merged
Two problems that only matter once these types are public: - BackoffConfig stored a reference to the shared static DefaultStatusCodeOverrides whenever no map was supplied. With StatusCodeOverrides exposed as a public property, a caller doing the natural thing — cfg.StatusCodeOverrides[500] = Drop — corrupted the defaults for every BackoffConfig constructed afterwards in the process, including ones parsed from CDN settings, with no way to reset. The constructor now copies the map. - A user-supplied HttpConfig reached the retry state machine unclamped, while the CDN path is validated by HttpConfigParser. Configuration.HttpConfig was therefore the only unvalidated route in, so out-of-range values such as maxRetryInterval: 0 or a negative jitterPercent took effect verbatim. Both pipelines now call Validated() on user-supplied config, matching the CDN path. 218 tests pass, including two new cases covering the copy and the clamping.
The property doc said retry settings come from CDN settings alone when this is null, which reads as 'non-null means yours is used'. It is not: SegmentDestination calls UpdateHttpConfig on every settings refresh carrying an httpConfig key, which replaces the whole config. A CDN payload also counts as enabling a subsystem unless it explicitly says enabled: false, so a payload tuning something unrelated can turn retries back on. Only a payload with no httpConfig key leaves this value in effect. This matches analytics-kotlin (SegmentDestination.kt:133) and analytics-swift (SegmentDestination.swift:83-91), which assign CDN config over the user's the same way and share the enabled-defaults-true rule, so the behaviour is left alone and only the documentation is corrected.
Adding a trailing optional parameter to Configuration's constructor is source compatible but not binary compatible: the compiler bakes optional defaults into the call site, so the assembly loses the old 13-parameter .ctor and anything compiled against it fails with MissingMethodException. That is fine for NuGet consumers, who recompile, but this SDK also ships Unity and Xamarin samples where DLLs are dropped in. #144 never touched Configuration.cs, so the break would have been new here. Making HttpConfig a settable property is purely additive, leaves the existing constructor signature untouched, and is closer to analytics-kotlin, which uses a mutable 'var httpConfig' rather than a constructor argument. new Configuration("writeKey") { HttpConfig = new HttpConfig(...) } 218 tests pass.
Cut the before/after narration from the comments added with the HttpConfig work. The copy of StatusCodeOverrides and the Validated() calls now state why they are needed rather than what the code did without them.
wenxi-zeng
previously approved these changes
Sep 21, 2026
…eneric Retry-After (529) (#148) * Handle Retry-After on every retryable status, including 529 Route any retryable response carrying a valid Retry-After header through the rate-limit path (no retry-budget cost) instead of special-casing 429. Retryable statuses without Retry-After continue to use counted exponential backoff. Adds 529 to the retryable set and covers both paths with tests. Matches the behaviour already shipped in analytics-java 3.5.5 and the generic-retry-after conformance suite in sdk-e2e-tests. * Keep the batch when Retry-After routes it to the rate-limit path Routing any retryable status with Retry-After to the rate-limit path left ShouldDeleteBatch inconsistent with HandleResponse. With rate limiting on and backoff off, a 503 or 529 carrying Retry-After would rate-limit the pipeline (WaitUntilTime set, uploads blocked) while ShouldDeleteBatch still reported true, so the batch file was deleted and the pipeline then stalled waiting to retry events that no longer existed. That configuration is reachable from CDN settings and directly from Configuration.HttpConfig — it is the config ConfigurationHttpConfigTest builds. ShouldDeleteBatch now keeps a retryable batch whenever rate limiting is enabled, matching swift's shouldDropBatch ("Rate limit config handles retryable codes that carry Retry-After — don't drop"). Non-retryable statuses are still dropped, and a retryable status with neither rate limiting nor backoff enabled is still dropped since nothing would retry it. * Base the keep-or-delete decision on Retry-After, not just config The previous commit kept a retryable batch whenever rate limiting was enabled, which was too broad: a 500 with no Retry-After and backoff disabled was also kept, so the file was re-uploaded even though nothing had scheduled a retry. The sdk-e2e-tests "backoffConfig.enabled: false" case caught this — it expects exactly one request and saw two. ShouldDeleteBatch now takes the same retryAfterSeconds value handed to HandleResponse, so the two agree on whether the response actually took the rate-limit path. A retryable status keeps its batch only when it carries a usable Retry-After and rate limiting is on; otherwise only backoff can retry it, and with backoff off the batch is dropped as before. The single-argument overload is retained. 232 tests pass. * Treat 3xx as success, per spec item 1 Analytics-CSharp-plan.md states 'Spec item 1: 2xx and 3xx are success', but IsSuccessStatusCode and the two status checks in RetryStateMachine were 2xx-only, so a 3xx fell through to the retry classifier. analytics-go, analytics-python and analytics-php already follow the spec here; this brings C# into line with them and with its own plan. 236 tests pass, including new cases covering 200, 201, 301 and 304. * Send the Authorization header, and drop 511 Two Key Agreements from the HTTP response design doc that this SDK did not meet. The doc requires every SDK to send the write key in the Authorization header, and TAPI authenticates and routes on it instead of parsing the payload — which is the performance reason the header exists. This SDK sent no Authorization at all; it relied solely on the writeKey embedded in the batch body by Storage. Upload requests now carry Basic credentials built from the write key with an empty password, matching analytics-python, -go, -ruby, -php and -java, all of which send base64("<writeKey>:"). The value is exposed as a protected BasicAuthorization on HTTPClient rather than by widening _apiKey, so a custom IHTTPClientProvider can send the same header; the Unity sample, which overrides DoPost, now does. The writeKey stays in the payload, so nothing depends on the header alone yet. Separately, 511 Network Authentication Required was retryable here. The doc makes it conditional — "Authenticate, then retry if library supports OAuth" — and this SDK has no OAuth, so a 511 could never be satisfied and retrying only spent the budget. It joins 501 and 505 as an explicit Drop. analytics-python, the one SDK with OAuth, correctly retries 511 only when an OauthManager is configured; go, ruby, php and java exclude it as this now does. 240 tests pass, including new coverage of the header value, and all 79 e2e tests still pass. * Opt in to the e2e Authorization check The header assertion in sdk-e2e-tests is opt-in per SDK, since analytics-kotlin and analytics-swift do not send it yet. This SDK does, so it runs the check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The retry state machine added in #144 can only be configured from CDN settings.
RateLimitConfig,BackoffConfigandHttpConfigare allinternal, andConfigurationhas no entry point — so a C# consumer currently cannot set retry behaviour at all.Kotlin and Swift both expose this, as a single config object:
Configuration.httpConfig: HttpConfig? = null(publicdata class HttpConfig)public func httpConfig(_ config: HttpConfig?) -> ConfigurationThis brings C# in line with the two SDKs #144 was explicitly written to match.
What
RetryBehavior,RateLimitConfig,BackoffConfigandHttpConfigpublic.RetryConfigstaysinternal— it's plumbing built fromHttpConfig, never supplied by callers.Configuration.HttpConfig, as a trailing optional constructor argument so existing positional callers are unaffected. Defaults tonull, preserving today's CDN-only behaviour exactly.EventPipelineProvider/SyncEventPipelineProviderpass it through as the pipeline's starting retry config. CDN settings still override it later viaUpdateHttpConfig.HttpConfigpublic, so a customIEventPipelineProvidercan pass one on rather than only read it.Usage
Testing
216 tests pass, including 6 new cases in
Tests/Retry/ConfigurationHttpConfigTest.csasserting that a config set onConfigurationreaches both pipelines' retry state machines (and that omitting it still yields legacy mode).Notes
This supersedes the
Configurationportion of #143, which added individualMaxRetries/MaxTotalBackoffDuration/MaxRateLimitDurationknobs. That shape matches analytics-python but not Kotlin/Swift; #143 and #144 were independent branches off85f025eand never shared history, so the divergence was never reconciled.