Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
Hey, this PR has been open for a while. Someone should look at it, or we can just close it if it's no longer relevant |
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Appreciate this PR! I have a few must fix issues and a few nits.
Also, would be nice to add more test coverage: Right now, only one test covers retry behavior. It makes sense to add tests for:
- Success after 1-2 retries (not just exhaustion)
- Both /batch AND /capture endpoints
- 4xx errors should NOT retry
- 5xx errors SHOULD retry
- HttpRequestException handling
- Configuration options (MaxRetries=0, custom delays)
- Cancellation during retry
- Default pipeline construction
| ShouldHandle = args => args.Outcome switch | ||
| { | ||
| { Exception: ApiException } => PredicateResult.True(), | ||
| { Exception: TimeoutRejectedException } => PredicateResult.True(), | ||
| _ => PredicateResult.False() | ||
| } |
There was a problem hiding this comment.
The ShouldHandle predicate here retries all ApiException instances without checking HTTP status codes. This means 400/401/403/404 errors are retried, but these are permanent failures that will never succeed.
Also, I believe we should handle HttpRequestException for network-level failures (DNS, connection refused, socket errors). Maybe something like:
| ShouldHandle = args => args.Outcome switch | |
| { | |
| { Exception: ApiException } => PredicateResult.True(), | |
| { Exception: TimeoutRejectedException } => PredicateResult.True(), | |
| _ => PredicateResult.False() | |
| } | |
| ShouldHandle = args => args.Outcome switch | |
| { | |
| { Exception: ApiException { Status: >= HttpStatusCode.InternalServerError } } => PredicateResult.True(), | |
| { Exception: ApiException { Status: HttpStatusCode.TooManyRequests } } => PredicateResult.True(), | |
| { Exception: TimeoutRejectedException } => PredicateResult.True(), | |
| { Exception: HttpRequestException } => PredicateResult.True(), | |
| _ => PredicateResult.False() | |
| } |
| } | ||
| .AddRetry(new RetryStrategyOptions | ||
| { | ||
| MaxRetryAttempts = _options.Value.MaxRetries, |
There was a problem hiding this comment.
Without UseJitter = true, simultaneous failures could cause thundering herd effects.
| MaxRetryAttempts = _options.Value.MaxRetries, | |
| MaxRetryAttempts = _options.Value.MaxRetries, | |
| UseJitter = true, |
| public Uri HostUrl { get; set; } = new("https://us.i.posthog.com"); | ||
|
|
||
| /// <summary> | ||
| /// Timeout in milliseconds for any calls. Defaults to 10 seconds. |
There was a problem hiding this comment.
Nit: It's a TimeSpan, not milliseconds.
| /// Timeout in milliseconds for any calls. Defaults to 10 seconds. | |
| /// Timeout for any calls. Defaults to 10 seconds. |
| { Exception: TimeoutRejectedException } => PredicateResult.True(), | ||
| _ => PredicateResult.False() | ||
| }, | ||
| OnRetry = args => |
There was a problem hiding this comment.
The retry pipeline here is nearly identical to the one created in PostHogClient. Might make sense to create an internal factory method for creating the pipeline and use that method in both places.
| // This test is pretty expensive because it dynamically compiles and loads an assembly. | ||
| // Consider alternatives. | ||
| [Fact] | ||
| [Fact(Skip= "Takes long time to run because it dynamically compiles and loads an assembly.")] |
There was a problem hiding this comment.
This seems unrelated to this PR.
| /// <summary> | ||
| /// Number of times to retry a failed request. Defaults to 3. | ||
| /// </summary> | ||
| public int MaxRetries { get; set; } = 3; |
There was a problem hiding this comment.
We should probably validate this (and the other new settings) in the PostHogClient ctor.
For example, we may want to have a max value on the allowed retries. Make sure its not negative. Etc.
Same thing for the timespans. Make sure they're not less than TimeSpan.Zero. Etc.
There was a problem hiding this comment.
Pull request overview
This PR adds retry functionality to the PostHog .NET SDK using the Polly resilience library. Failed event requests to the /batch and /capture endpoints will now be automatically retried with exponential backoff to handle transient failures. The default configuration uses 3 retry attempts with a 3-second base delay, capped at 10 seconds, and a 10-second request timeout, matching settings from other PostHog client SDKs.
Key changes:
- Added Polly.Core dependency for resilience patterns (retry, timeout)
- Introduced configurable retry and timeout settings in PostHogOptions
- Wrapped event API calls with a ResiliencePipeline that handles ApiException and TimeoutRejectedException
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/PostHog/PostHog.csproj | Adds Polly.Core package reference for resilience functionality |
| src/PostHog/Config/PostHogOptions.cs | Adds new configuration properties for request timeout, retry attempts, retry delay, and maximum retry delay |
| src/PostHog/PostHogClient.cs | Adds ResiliencePipeline parameter and builds default pipeline with retry and timeout strategies when not provided |
| src/PostHog/Api/PostHogApiClient.cs | Wraps batch and capture endpoint calls with ResiliencePipeline execution for automatic retries |
| tests/TestLibrary/TestContainer.cs | Updates PostHogClient instantiation to pass ResiliencePipeline.Empty for deterministic test behavior |
| tests/TestLibrary/Fakes/FakeHttpMessageHandlerExtensions.cs | Adds helper method to inject exceptions into batch responses for retry testing |
| tests/UnitTests/PostHogClientTests.cs | Adds comprehensive retry behavior test and marks slow compilation test as skipped |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Number of times to retry a failed request. Defaults to 3. | ||
| /// </summary> |
There was a problem hiding this comment.
The documentation should clarify what types of requests are retried. According to the PR description, retries apply to event capture requests (batch and capture endpoints) but not to feature flag or config endpoints. This should be documented here to set clear expectations about which operations will be retried.
| /// Number of times to retry a failed request. Defaults to 3. | |
| /// </summary> | |
| /// Number of times to retry a failed event capture request (batch and capture endpoints). Defaults to 3. | |
| /// </summary> | |
| /// <remarks> | |
| /// Retries apply only to event capture requests (batch and capture endpoints). Feature flag and config endpoint requests are not retried. | |
| /// </remarks> |
| var flushTask2 = client.FlushAsync(); | ||
| await Task.Yield(); | ||
|
|
||
| // Advance fake time past timeout to trigger the retry |
There was a problem hiding this comment.
The comment could be clearer about what's happening. The test advances time by 16 seconds to exceed the 15-second timeout configured on line 705, which causes a TimeoutRejectedException that triggers a retry. Consider revising the comment to: "Advance fake time past the 15-second timeout to cause a TimeoutRejectedException that will be retried" for better clarity.
| // Advance fake time past timeout to trigger the retry | |
| // Advance fake time past the 15-second timeout to cause a TimeoutRejectedException that will be retried |
| .AddRetry(new RetryStrategyOptions | ||
| { | ||
| MaxRetryAttempts = _options.Value.MaxRetries, | ||
| Delay = _options.Value.RetryDelay, | ||
| MaxDelay = _options.Value.MaxRetryDelay, | ||
| BackoffType = DelayBackoffType.Exponential, | ||
| ShouldHandle = args => args.Outcome switch | ||
| { | ||
| { Exception: ApiException } => PredicateResult.True(), | ||
| { Exception: TimeoutRejectedException } => PredicateResult.True(), | ||
| _ => PredicateResult.False() | ||
| } | ||
| }) | ||
| .AddTimeout(_options.Value.RequestTimeout) | ||
| .Build(); |
There was a problem hiding this comment.
The timeout is applied after the retry strategy, which means it applies to the entire retry sequence. With the default configuration (MaxRetries=3, RetryDelay=3s, MaxRetryDelay=10s, exponential backoff), the maximum time for all retries is 3+6+10=19 seconds, but RequestTimeout is only 10 seconds. This means the operation will timeout before exhausting all retry attempts. The order should typically be reversed - add timeout before retry to apply timeout per-request, or adjust the RequestTimeout value to account for the total retry sequence duration (e.g., at least 20+ seconds to allow all retries).
| // This test is pretty expensive because it dynamically compiles and loads an assembly. | ||
| // Consider alternatives. | ||
| [Fact] | ||
| [Fact(Skip= "Takes long time to run because it dynamically compiles and loads an assembly.")] |
There was a problem hiding this comment.
The Skip parameter assignment is missing a space after the equals sign. For consistency with C# conventions and the rest of the codebase, it should be 'Skip = "..."' instead of 'Skip= "..."'.
| [Fact(Skip= "Takes long time to run because it dynamically compiles and loads an assembly.")] | |
| [Fact(Skip = "Takes long time to run because it dynamically compiles and loads an assembly.")] |
| BackoffType = DelayBackoffType.Exponential, | ||
| ShouldHandle = args => args.Outcome switch | ||
| { | ||
| { Exception: ApiException } => PredicateResult.True(), |
There was a problem hiding this comment.
The retry strategy retries all ApiException instances regardless of HTTP status code. This means that client errors like 400 (Bad Request), 401 (Unauthorized), 403 (Forbidden), and 404 (Not Found) will be retried, which typically shouldn't be retried as they won't succeed on retry. Consider filtering ApiException based on the Status property to only retry transient errors (5xx server errors, 429 Too Many Requests, 408 Request Timeout, etc.) and not retry permanent client errors (4xx except certain codes like 429).
| { Exception: ApiException } => PredicateResult.True(), | |
| { Exception: ApiException apiEx } when | |
| (apiEx.Status >= 500 && apiEx.Status < 600) || // 5xx server errors | |
| apiEx.Status == 429 || // Too Many Requests | |
| apiEx.Status == 408 // Request Timeout | |
| => PredicateResult.True(), |
| ShouldHandle = args => args.Outcome switch | ||
| { | ||
| { Exception: ApiException } => PredicateResult.True(), | ||
| { Exception: TimeoutRejectedException } => PredicateResult.True(), | ||
| _ => PredicateResult.False() | ||
| } |
There was a problem hiding this comment.
The retry strategy doesn't handle HttpRequestException, which is commonly thrown for network-related failures like connection timeouts, DNS failures, or connection refused errors. These are typically transient errors that should be retried. Consider adding HttpRequestException to the ShouldHandle predicate to retry network-level failures in addition to ApiException and TimeoutRejectedException.
| /// <param name="httpClientFactory">Creates <see cref="HttpClient"/> for making requests to PostHog's API.</param> | ||
| /// <param name="taskScheduler">Used to run tasks on the background.</param> | ||
| /// <param name="timeProvider">The time provider <see cref="TimeProvider"/> to use to determine time.</param> | ||
| /// <param name="resiliencePipeline">The <see cref="Polly.ResiliencePipeline"/> to use when making event requests to PostHog's API.</param> |
There was a problem hiding this comment.
The XML documentation should clarify that when resiliencePipeline is null, a default pipeline will be created using the retry and timeout settings from PostHogOptions. It should also mention that passing ResiliencePipeline.Empty will disable retry/timeout behavior entirely. This would help users understand the different configuration options available.
| /// <param name="resiliencePipeline">The <see cref="Polly.ResiliencePipeline"/> to use when making event requests to PostHog's API.</param> | |
| /// <param name="resiliencePipeline"> | |
| /// The <see cref="Polly.ResiliencePipeline"/> to use when making event requests to PostHog's API. | |
| /// If <c>null</c>, a default pipeline will be created using the retry and timeout settings from <see cref="PostHogOptions"/>. | |
| /// Passing <see cref="Polly.ResiliencePipeline.Empty"/> disables retry and timeout behavior entirely. | |
| /// </param> |
| public Uri HostUrl { get; set; } = new("https://us.i.posthog.com"); | ||
|
|
||
| /// <summary> | ||
| /// Timeout in milliseconds for any calls. Defaults to 10 seconds. |
There was a problem hiding this comment.
The documentation incorrectly states "Timeout in milliseconds" but the property type is TimeSpan and the default is set using TimeSpan.FromSeconds(10). The documentation should be updated to accurately describe this as a timeout duration or time span, not specifically in milliseconds.
| /// Timeout in milliseconds for any calls. Defaults to 10 seconds. | |
| /// Timeout duration for any calls. Defaults to 10 seconds. |
|
Hey, this PR has been open for a while. Someone should look at it, or we can just close it if it's no longer relevant |
Added Polly to retry failed Event requests, as recommended in .NET docs. As the various client SDKs differ in configuration I've set up a minimal config that can be easily adjusted and extended to more APIs (for example flags, config, etc) if needed. The default settings try to match js/python client configs. We cover
batchandcaptureendpoints for now, but not any of the flags or config endpoints to stay consistent with other clients.We can easily adjust the exceptions or
ApiResultstatuses that are being retried if needed, for now it retriesApiExceptionand timeouts, other exceptions throw directly.A
ResiliencePipelinecan be passed tonew PostHogClient(), ifnullit will create pipeline with default settings fromPostHogOptions, or if no retry needed -ResiliencePipeline.Emptyexecutes the callbacks directly with no extra logic.Honestly, the added test is a bit more of an integration test than unit, but at least helps verify the behavior and executes quickly.
Closes #7