diff --git a/.changeset/noop-api-failures.md b/.changeset/noop-api-failures.md new file mode 100644 index 0000000..987a543 --- /dev/null +++ b/.changeset/noop-api-failures.md @@ -0,0 +1,5 @@ +--- +"PostHog": patch +--- + +Return no-op results instead of throwing from public APIs when PostHog API calls fail. diff --git a/src/PostHog/PostHogClient.cs b/src/PostHog/PostHogClient.cs index 2fe3f74..68370e1 100644 --- a/src/PostHog/PostHogClient.cs +++ b/src/PostHog/PostHogClient.cs @@ -23,7 +23,6 @@ public sealed class PostHogClient : IPostHogClient readonly IFeatureFlagCache _featureFlagsCache; readonly MemoryCache _featureFlagCalledEventCache; static readonly ApiResult NoOpApiResult = new(0); - static readonly Task NoOpApiResultTask = Task.FromResult(NoOpApiResult); static readonly IReadOnlyDictionary EmptyFeatureFlags = new Dictionary(0); readonly TimeProvider _timeProvider; @@ -173,7 +172,15 @@ public async Task AliasAsync( return NoOpApiResult; } - return await _apiClient.AliasAsync(previousId, newId, cancellationToken); + try + { + return await _apiClient.AliasAsync(previousId, newId, cancellationToken); + } + catch (Exception e) when (e is not ArgumentException and not NullReferenceException and not OperationCanceledException) + { + _logger.LogErrorApiCallFailed(e, nameof(AliasAsync)); + return NoOpApiResult; + } } /// @@ -188,15 +195,23 @@ public async Task IdentifyAsync( return NoOpApiResult; } - return await _apiClient.IdentifyAsync( - distinctId, - personPropertiesToSet, - personPropertiesToSetOnce, - cancellationToken); + try + { + return await _apiClient.IdentifyAsync( + distinctId, + personPropertiesToSet, + personPropertiesToSetOnce, + cancellationToken); + } + catch (Exception e) when (e is not ArgumentException and not NullReferenceException and not OperationCanceledException) + { + _logger.LogErrorApiCallFailed(e, nameof(IdentifyAsync)); + return NoOpApiResult; + } } /// - public Task GroupIdentifyAsync( + public async Task GroupIdentifyAsync( string type, StringOrValue key, Dictionary? properties, @@ -204,14 +219,22 @@ public Task GroupIdentifyAsync( { if (CheckDisabledAndLog(nameof(GroupIdentifyAsync))) { - return NoOpApiResultTask; + return NoOpApiResult; } - return _apiClient.GroupIdentifyAsync(type, key, properties, cancellationToken); + try + { + return await _apiClient.GroupIdentifyAsync(type, key, properties, cancellationToken); + } + catch (Exception e) when (e is not ArgumentException and not NullReferenceException and not OperationCanceledException) + { + _logger.LogErrorApiCallFailed(e, nameof(GroupIdentifyAsync)); + return NoOpApiResult; + } } /// - public Task GroupIdentifyAsync( + public async Task GroupIdentifyAsync( string distinctId, string type, StringOrValue key, @@ -220,10 +243,18 @@ public Task GroupIdentifyAsync( { if (CheckDisabledAndLog(nameof(GroupIdentifyAsync))) { - return NoOpApiResultTask; + return NoOpApiResult; } - return _apiClient.GroupIdentifyAsync(type, key, properties, cancellationToken, distinctId); + try + { + return await _apiClient.GroupIdentifyAsync(type, key, properties, cancellationToken, distinctId); + } + catch (Exception e) when (e is not ArgumentException and not NullReferenceException and not OperationCanceledException) + { + _logger.LogErrorApiCallFailed(e, nameof(GroupIdentifyAsync)); + return NoOpApiResult; + } } /// @@ -510,7 +541,7 @@ public async Task IsFeatureEnabledAsync( return null; } - LocalEvaluator? localEvaluator; + LocalEvaluator? localEvaluator = null; try { localEvaluator = await _featureFlagsLoader.GetFeatureFlagsForLocalEvaluationAsync(cancellationToken); @@ -520,6 +551,10 @@ public async Task IsFeatureEnabledAsync( _logger.LogWarningQuotaExceeded(e); return null; } + catch (Exception e) when (e is not ArgumentException and not NullReferenceException and not OperationCanceledException) + { + _logger.LogErrorFailedToLoadFeatureFlags(e); + } FeatureFlag? response = null; if (localEvaluator is not null && localEvaluator.TryGetLocalFeatureFlag(featureKey, out var localFeatureFlag)) @@ -928,6 +963,12 @@ public async Task EvaluateFlagsAsync( errors.Add(FeatureFlagError.QuotaLimited); fallbackToRemote = false; } + catch (Exception e) when (e is not ArgumentException and not NullReferenceException and not OperationCanceledException) + { + _logger.LogErrorFailedToLoadFeatureFlags(e); + errors.Add(FeatureFlagError.UnknownError); + fallbackToRemote = options is not { OnlyEvaluateLocally: true }; + } } // 2. Remote pass — only if we still need it. @@ -1040,6 +1081,14 @@ public async Task> GetAllFeatureFlagsAs _logger.LogWarningQuotaExceeded(e); return EmptyFeatureFlags; } + catch (Exception e) when (e is not ArgumentException and not NullReferenceException and not OperationCanceledException) + { + _logger.LogErrorFailedToLoadFeatureFlags(e); + if (options is { OnlyEvaluateLocally: true }) + { + return EmptyFeatureFlags; + } + } } try @@ -1121,12 +1170,10 @@ public async Task LoadFeatureFlagsAsync(CancellationToken cancellationToken) catch (ApiException e) when (e.ErrorType is "quota_limited") { _logger.LogWarningQuotaExceeded(e); - throw; } catch (Exception e) when (e is not ArgumentException and not NullReferenceException and not OperationCanceledException) { _logger.LogErrorFailedToLoadFeatureFlags(e); - throw; } } @@ -1138,7 +1185,14 @@ public async Task FlushAsync() return; } - await _asyncBatchHandler.FlushAsync(); + try + { + await _asyncBatchHandler.FlushAsync(); + } + catch (Exception e) when (e is not ArgumentException and not NullReferenceException and not OperationCanceledException) + { + _logger.LogErrorApiCallFailed(e, nameof(FlushAsync)); + } } /// @@ -1163,6 +1217,11 @@ public async Task FlushAsync() _logger.LogWarningQuotaExceeded(e); return null; } + catch (Exception e) when (e is not ArgumentException and not NullReferenceException and not OperationCanceledException) + { + _logger.LogErrorFailedToLoadFeatureFlags(e); + return null; + } } /// @@ -1361,4 +1420,10 @@ public static partial void LogErrorUnableToGetRemoteConfigPayload( Level = LogLevel.Warning, Message = "[FEATURE FLAGS] distinctId is required to evaluate feature flags. Pass a distinctId explicitly or use PostHog request context for the current request.")] public static partial void LogWarningMissingFeatureFlagDistinctId(this ILogger logger); + + [LoggerMessage( + EventId = 24, + Level = LogLevel.Error, + Message = "PostHog API call failed in {MethodName}; returning a no-op result.")] + public static partial void LogErrorApiCallFailed(this ILogger logger, Exception exception, string methodName); } diff --git a/tests/UnitTests/Features/ETagSupportTests.cs b/tests/UnitTests/Features/ETagSupportTests.cs index 07b014c..582e066 100644 --- a/tests/UnitTests/Features/ETagSupportTests.cs +++ b/tests/UnitTests/Features/ETagSupportTests.cs @@ -154,9 +154,8 @@ public async Task QuotaLimitedErrorClearsETag() await client.LoadFeatureFlagsAsync(CancellationToken.None); // Stores ETag - // This should throw quota_limited error and clear ETag - await Assert.ThrowsAsync( - () => client.LoadFeatureFlagsAsync(CancellationToken.None)); + // Quota-limited errors should be swallowed by the public SDK method, while still clearing the ETag. + await client.LoadFeatureFlagsAsync(CancellationToken.None); // Next request should not have If-None-Match header (ETag was cleared) await client.LoadFeatureFlagsAsync(CancellationToken.None); diff --git a/tests/UnitTests/PostHogClientTests.cs b/tests/UnitTests/PostHogClientTests.cs index 6752e24..b2241a6 100644 --- a/tests/UnitTests/PostHogClientTests.cs +++ b/tests/UnitTests/PostHogClientTests.cs @@ -1,4 +1,5 @@ #pragma warning disable CS0618 // Tests/samples retain coverage of the deprecated single-flag API surface. +using System.Net; using System.Reflection; using System.Runtime.InteropServices; using System.Runtime.Loader; @@ -1102,52 +1103,115 @@ public async Task CaptureExceptionWhenNoStackTrace() [Fact] public async Task CaptureExceptionCauseIOFailureEmptyContext() { - FileStream? lockHandle = null; - var (container, requestHandler, client) = CreateClient(); + var (_, requestHandler, client) = CreateClient(); + var compiledThrower = await CreateDivideByZeroExceptionWithTempSourceFileAsync(); try { - try + // Lock the source file exclusively so File.ReadAllLines(sourcePath) will throw IOException + // and as result frames will not contain source code context. Use a temp file so parallel + // target-framework test runs do not contend over this test source file. + await using var lockHandle = new FileStream( + compiledThrower.SourcePath, + FileMode.Open, + FileAccess.Read, + FileShare.None); + + client.CaptureException(compiledThrower.Exception, "some-distinct-id"); + await client.FlushAsync(); + + var (_, batchItem, props) = ParseSingleEvent(requestHandler.GetReceivedRequestBody(indented: true)); + var divideByZeroException = GetExceptionOfType(props, "System.DivideByZeroException"); + var frames = GetStackFrames(divideByZeroException); + + Assert.True(File.Exists(compiledThrower.SourcePath)); + Assert.Equal("$exception", batchItem.GetProperty("event").GetString()); + + var sourceFrame = frames.FirstOrDefault(f => + f.TryGetProperty("abs_path", out var absPath) && + string.Equals(absPath.GetString(), compiledThrower.SourcePath, StringComparison.Ordinal)); + + // In Release builds, stack frames may not include source file paths due + // to JIT optimizations, making this scenario impossible to reproduce. + if (sourceFrame.ValueKind is JsonValueKind.Undefined) { - var zero = 0; - var result = 1 / zero; + return; } - catch (DivideByZeroException ex) + + AssertContextEmpty(sourceFrame); + } + finally + { + File.Delete(compiledThrower.SourcePath); + } + } + + private static async Task<(string SourcePath, DivideByZeroException Exception, AssemblyLoadContext LoadContext)> + CreateDivideByZeroExceptionWithTempSourceFileAsync() + { + var sourcePath = Path.Combine(Path.GetTempPath(), $"PostHogClientTests-{Guid.NewGuid():N}.cs"); + var code = + """ + using System; + public static class Thrower { - var st = new System.Diagnostics.StackTrace(ex, true); - var path = st.GetFrames() - .Select(f => f?.GetFileName()) - .FirstOrDefault(p => !string.IsNullOrEmpty(p) && File.Exists(p)); - - // In Release builds, stack frames may not include source file paths due - // to JIT optimizations, making this scenario impossible to reproduce. - if (path is null) + public static void Boom() { - return; + int zero = 0; + var _ = 1 / zero; } + } + """; + + await File.WriteAllTextAsync(sourcePath, code, Encoding.UTF8); + + var shouldDeleteSource = true; + try + { + var parse = CSharpParseOptions.Default; + var tree = CSharpSyntaxTree.ParseText(SourceText.From(code, Encoding.UTF8), parse, path: sourcePath); + var trustedPlatformAssemblies = ((string)AppContext.GetData("TRUSTED_PLATFORM_ASSEMBLIES")!).Split(Path.PathSeparator); + + var refs = trustedPlatformAssemblies + .Where(p => p.EndsWith(".dll", StringComparison.OrdinalIgnoreCase)) + .GroupBy(Path.GetFileName, StringComparer.OrdinalIgnoreCase) + .Select(g => MetadataReference.CreateFromFile(g.First())); + + var comp = CSharpCompilation.Create( + $"ThrowerAsm{Guid.NewGuid():N}", + [tree], + refs, + new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary, optimizationLevel: OptimizationLevel.Debug)); - // Lock the source file exclusively so File.ReadAllLines(path) will throw IOException - // and as result frames will not contain source code context - lockHandle = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.None); + using var pe = new MemoryStream(); + using var pdb = new MemoryStream(); + var emit = comp.Emit(pe, pdb, options: new EmitOptions(debugInformationFormat: DebugInformationFormat.PortablePdb)); + Assert.True(emit.Success, string.Join(Environment.NewLine, emit.Diagnostics)); - client.CaptureException(ex, "some-distinct-id"); - await client.FlushAsync(); + pe.Position = 0; + pdb.Position = 0; - var received = requestHandler.GetReceivedRequestBody(indented: true); - var (_, batchItem, props) = ParseSingleEvent(received); - var divideByZeroException = GetExceptionOfType(props, "System.DivideByZeroException"); - var frames = GetStackFrames(divideByZeroException); + var assemblyLoadContext = new AssemblyLoadContext($"ThrowerCtx{Guid.NewGuid():N}", isCollectible: true); + var assembly = assemblyLoadContext.LoadFromStream(pe, pdb); + var boom = assembly.GetType("Thrower")!.GetMethod("Boom", BindingFlags.Public | BindingFlags.Static)!; - Assert.True(File.Exists(path)); - Assert.Equal("$exception", batchItem.GetProperty("event").GetString()); - AssertContextEmpty(frames[0]); + try + { + boom.Invoke(null, null); } + catch (TargetInvocationException tie) when (tie.InnerException is DivideByZeroException ex) + { + shouldDeleteSource = false; + return (sourcePath, ex, assemblyLoadContext); + } + + throw new InvalidOperationException("Expected Thrower.Boom to throw a DivideByZeroException."); } finally { - if (lockHandle != null) + if (shouldDeleteSource) { - await lockHandle.DisposeAsync(); + File.Delete(sourcePath); } } } @@ -1600,6 +1664,84 @@ static void AssertNoProjectTokenRequiredLog(TestContainer container) } } +public class TheApiFailureNoOpBehavior +{ + [Fact] + public async Task DirectIngestionMethodsReturnNoOpResultOnApiError() + { + var container = new TestContainer(); + var aliasHandler = container.FakeHttpMessageHandler.AddResponse( + new Uri("https://us.i.posthog.com/capture"), + HttpMethod.Post, + UnauthorizedResponse()); + var identifyHandler = container.FakeHttpMessageHandler.AddResponse( + new Uri("https://us.i.posthog.com/capture"), + HttpMethod.Post, + UnauthorizedResponse()); + var groupHandler = container.FakeHttpMessageHandler.AddResponse( + new Uri("https://us.i.posthog.com/capture"), + HttpMethod.Post, + UnauthorizedResponse()); + var groupWithDistinctIdHandler = container.FakeHttpMessageHandler.AddResponse( + new Uri("https://us.i.posthog.com/capture"), + HttpMethod.Post, + UnauthorizedResponse()); + var client = container.Activate(); + + var aliasResult = await client.AliasAsync("previous-id", "new-id", CancellationToken.None); + var identifyResult = await client.IdentifyAsync("distinct-id"); + var groupResult = await client.GroupIdentifyAsync("organization", "id:5", null, CancellationToken.None); + var groupWithDistinctIdResult = await client.GroupIdentifyAsync("distinct-id", "organization", "id:5", null, CancellationToken.None); + + Assert.Equal(0, aliasResult.Status); + Assert.Equal(0, identifyResult.Status); + Assert.Equal(0, groupResult.Status); + Assert.Equal(0, groupWithDistinctIdResult.Status); + Assert.Single(aliasHandler.ReceivedRequests); + Assert.Single(identifyHandler.ReceivedRequests); + Assert.Single(groupHandler.ReceivedRequests); + Assert.Single(groupWithDistinctIdHandler.ReceivedRequests); + } + + [Fact] + public async Task FlushAsyncDoesNotThrowOnApiError() + { + var container = new TestContainer(); + var batchHandler = container.FakeHttpMessageHandler.AddResponse( + new Uri("https://us.i.posthog.com/batch"), + HttpMethod.Post, + UnauthorizedResponse()); + var client = container.Activate(); + + Assert.True(client.Capture("distinct-id", "some-event")); + await client.FlushAsync(); + + Assert.Single(batchHandler.ReceivedRequests); + } + + [Fact] + public async Task LoadFeatureFlagsAsyncDoesNotThrowOnApiError() + { + var container = new TestContainer("fake-personal-api-key"); + var localEvaluationHandler = container.FakeHttpMessageHandler.AddResponse( + FakeHttpMessageHandlerExtensions.LocalEvaluationUrl, + HttpMethod.Get, + UnauthorizedResponse()); + var client = container.Activate(); + + await client.LoadFeatureFlagsAsync(); + + Assert.Single(localEvaluationHandler.ReceivedRequests); + } + + static HttpResponseMessage UnauthorizedResponse() => new(HttpStatusCode.Unauthorized) + { + Content = new StringContent(""" + {"detail":"Invalid project token"} + """, Encoding.UTF8, "application/json") + }; +} + public class ThePersonalApiKeyProtectedMethods { [Fact]