feat: add EvaluateFlagsAsync() API for single-call flag evaluation#187
feat: add EvaluateFlagsAsync() API for single-call flag evaluation#187
Conversation
Adds a new EvaluateFlagsAsync(distinctId, options) method on the client that returns a FeatureFlagEvaluations snapshot. The snapshot powers IsEnabled / GetFlag / GetFlagPayload calls, fires $feature_flag_called lazily (deduped against the existing per-distinct-id cache), and can be forwarded to a new Capture(..., flags: snapshot) overload to attach $feature/<key> and $active_feature_flags to events without a second /flags request. Mirrors PostHog/posthog-js#3476 and PostHog/posthog-python#539. Also fixes a long-standing bug where the legacy single-flag path hard-coded locally_evaluated=false on every $feature_flag_called event. Locally-evaluated flags now correctly carry locally_evaluated=true, $feature_flag_reason="Evaluated locally", and a new $feature_flag_definitions_loaded_at timestamp surfaced via LocalFeatureFlagsLoader. The existing IsFeatureEnabledAsync / GetFeatureFlagAsync / Capture(..., sendFeatureFlags, ...) APIs are unchanged in this PR; a follow-up minor will mark them deprecated in favor of the snapshot API. Generated-By: PostHog Code Task-Id: 494d1c64-1b39-421a-9317-7ccd5992aa40
posthog-dotnet Compliance ReportDate: 2026-04-29 22:16:01 UTC
|
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ❌ | 43ms |
Failures
request_payload.request_with_person_properties_device_id
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| { | ||
| readonly IFeatureFlagEvaluationsHost _host; | ||
| readonly IReadOnlyDictionary<string, EvaluatedFlagRecord> _records; | ||
| readonly HashSet<string> _accessed; |
There was a problem hiding this comment.
_accessed is not thread-safe
_accessed is a plain HashSet<string> with no synchronization. RecordAccess calls _accessed.Add() without a lock, so if a caller shares a snapshot across threads — e.g., parallel Task.WhenAll branches each calling IsEnabled() on the same snapshot — the HashSet can be corrupted or throw. FeatureFlagEvaluations is a public type with no documented single-thread constraint.
Consider replacing _accessed with a ConcurrentDictionary<string, byte> (used as a set) or wrapping the mutation in a lock.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/PostHog/Features/FeatureFlagEvaluations.cs
Line: 18
Comment:
**`_accessed` is not thread-safe**
`_accessed` is a plain `HashSet<string>` with no synchronization. `RecordAccess` calls `_accessed.Add()` without a lock, so if a caller shares a snapshot across threads — e.g., parallel `Task.WhenAll` branches each calling `IsEnabled()` on the same snapshot — the `HashSet` can be corrupted or throw. `FeatureFlagEvaluations` is a public type with no documented single-thread constraint.
Consider replacing `_accessed` with a `ConcurrentDictionary<string, byte>` (used as a set) or wrapping the mutation in a `lock`.
How can I resolve this? If you propose a fix, please make it concise.- FeatureFlagEvaluations._accessed: HashSet<string> -> ConcurrentDictionary<string, byte> so callers may share a snapshot across parallel branches without corrupting it. - ToRecord: leave EvaluatedFlagRecord.Reason null for locally-evaluated flags; the "Evaluated locally" string is hardcoded inside BuildFeatureFlagCalledProperties and the host gates record.Reason with !LocallyEvaluated, so it was unread. - Collapse IsEnabledReturnsFalseForUnknownKey + GetFlagReturnsNullForUnknownKey into a parameterized [Theory] over the accessor under test. - Replace the brittle substring match on $active_feature_flags with a parsed, order-independent comparison; Dictionary iteration order isn't a guarantee. Generated-By: PostHog Code Task-Id: 494d1c64-1b39-421a-9317-7ccd5992aa40
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Love seeming my friends write C# code! There's a couple minor blocking issues and a few other suggestions to make this nice idiomatic and non-breaking C#.
| /// <param name="flags">A snapshot of feature flag evaluations. When non-null, <c>$feature/<key></c> and <c>$active_feature_flags</c> are attached from the snapshot — no <c>/flags</c> call is made.</param> | ||
| /// <param name="timestamp">Optional: Custom timestamp when the event occurred. If not provided, uses current time.</param> | ||
| /// <returns><c>true</c> if the event was successfully enqueued. Otherwise <c>false</c>.</returns> | ||
| bool Capture( |
There was a problem hiding this comment.
blocking: Adding three abstract members to a public interface is a source-and-binary break for any external implementer (custom decorators, hand-rolled fakes). LangVersion 13.0 is set on this repo, so default interface methods are available. Default each new member to a sensible no-op and the change becomes additive.
| bool Capture( | |
| bool Capture( | |
| string distinctId, | |
| string eventName, | |
| Dictionary<string, object>? properties, | |
| GroupCollection? groups, | |
| FeatureFlagEvaluations? flags, | |
| DateTimeOffset? timestamp = null) | |
| => Capture(distinctId, eventName, properties, groups, sendFeatureFlags: false, timestamp); |
Same shape on the CaptureException overload at line 155 (delegate to the sendFeatureFlags: false version) and on EvaluateFlagsAsync at line 231 (throw new NotSupportedException(...)). PostHogClient's explicit implementations remain and shadow the defaults; in-box behavior is unchanged.
| <Project> | ||
| <PropertyGroup> | ||
| <Version>2.5.0</Version> | ||
| <Version>2.6.0</Version> |
There was a problem hiding this comment.
blocking: If the default-interface-methods change on IPostHogClient lands, 2.6.0 is fine. Without it, this should be 3.0.0. Adding non-default abstract members to a shipped public interface is a SemVer major break, and pinning to [2.5.0,3.0.0) shouldn't silently pick up a compile-breaking change.
| <Version>2.6.0</Version> | |
| <Version>3.0.0</Version> |
| groups: null, | ||
| errors: null); | ||
|
|
||
| EvaluatedFlagRecord? RecordAccess(string key) |
There was a problem hiding this comment.
suggestion: Every IsEnabled/GetFlag call hits _host.TryCaptureFeatureFlagCalledEventIfNeeded, which allocates a List<string>, a synthetic FeatureFlag (when record is null), a property Dictionary, and acquires the MemoryCache lock — even when the snapshot already saw this key. _accessed.TryAdd returns false on repeats; gate the host call on that. The MemoryCache still serves cross-snapshot dedup, so this is purely an in-snapshot fast path.
| EvaluatedFlagRecord? RecordAccess(string key) | |
| EvaluatedFlagRecord? RecordAccess(string key) | |
| { | |
| var keyChecked = NotNull(key); | |
| var firstAccess = _accessed.TryAdd(keyChecked, 0); | |
| _records.TryGetValue(keyChecked, out var record); | |
| if (!firstAccess || string.IsNullOrEmpty(DistinctId)) | |
| { | |
| // Already accessed in this snapshot, or empty-distinct-id safety fallback: | |
| // skip the per-call dedup cache hit and property allocation. | |
| return record; | |
| } | |
| _host.TryCaptureFeatureFlagCalledEventIfNeeded( | |
| distinctId: DistinctId, | |
| featureKey: keyChecked, | |
| record: record, | |
| groups: _groups, | |
| requestId: RequestId, | |
| evaluatedAt: EvaluatedAt, | |
| flagDefinitionsLoadedAt: FlagDefinitionsLoadedAt, | |
| errors: _errors); | |
| return record; | |
| } |
For a request that calls IsEnabled 1000 times in a loop, this drops 1000 dictionary allocations and 1000 cache-lookup lock acquisitions to 1 of each.
| locallyEvaluated: record?.LocallyEvaluated ?? false, | ||
| flagDefinitionsLoadedAt: record?.LocallyEvaluated == true ? flagDefinitionsLoadedAt : null); | ||
|
|
||
| // For locally-evaluated flags from a snapshot we still want id/version/reason metadata |
There was a problem hiding this comment.
suggestion: For non-locally-evaluated flags, BuildFeatureFlagCalledProperties already writes $feature_flag_id/$feature_flag_version/$feature_flag_reason from the FeatureFlagWithMetadata branch. Then this block writes them again from record.Id/record.Version/record.Reason — same source (the record's Flag is the same FeatureFlagWithMetadata whose metadata seeded the record), so this is dead code. A reader has to trace both paths to convince themselves the values can't diverge.
If the intent is to handle a record that carries id/version without a FeatureFlagWithMetadata flag, make that intent explicit in a comment and have BuildFeatureFlagCalledProperties skip the metadata branch when called from this path. Otherwise drop lines 692–705.
|
|
||
| public int? Id { get; init; } | ||
| public int? Version { get; init; } | ||
| public string? Reason { get; init; } |
There was a problem hiding this comment.
suggestion: Reason semantics are split across three locations: this field holds the remote reason; BuildFeatureFlagCalledProperties hardcodes "Evaluated locally" for the local path; EvaluationsHost.TryCaptureFeatureFlagCalledEventIfNeeded then re-applies record.Reason only if (!record.LocallyEvaluated). Three places to keep in sync.
Cleaner: store the resolved reason on the record at construction time. In ToRecord, set reason = locallyEvaluated ? "Evaluated locally" : (flag as FeatureFlagWithMetadata)?.Reason. Drop the local-vs-remote branch in BuildFeatureFlagCalledProperties and the !record.LocallyEvaluated guard in EvaluationsHost. The legacy single-flag path doesn't have a record so it would still hardcode the string inline — already the case today. The record becomes self-describing.
If you keep the current shape, at minimum document the asymmetry on the field:
| public string? Reason { get; init; } | |
| /// <summary> | |
| /// The evaluation reason from <see cref="FeatureFlagWithMetadata"/>. Intentionally <c>null</c> | |
| /// for locally-evaluated flags — <c>BuildFeatureFlagCalledProperties</c> hardcodes | |
| /// <c>"Evaluated locally"</c> for that path so the two reason sources cannot drift. | |
| /// </summary> | |
| public string? Reason { get; init; } |
| catch (ApiException e) when (e.ErrorType is "quota_limited") | ||
| { | ||
| _logger.LogWarningQuotaExceeded(e); | ||
| return FeatureFlagEvaluations.Empty(_evaluationsHost, distinctId); |
There was a problem hiding this comment.
question: Local-pass quota_limited returns an empty snapshot here — discarding any flags evaluated up to this point and skipping the remote pass. But remote-pass quota_limited (around line 793–798) goes via flagsResult.QuotaLimited.Contains("feature_flags") → adds FeatureFlagError.QuotaLimited to the errors list and otherwise keeps the locally-evaluated records.
Local quota → empty snapshot, no error annotation. Remote quota → snapshot with whatever local produced, plus a $feature_flag_error: "quota_limited" annotation. The remote behavior is arguably the better one (preserves local work, surfaces the error). Is the discard on local-quota intentional?
| /// Fires a <c>$feature_flag_called</c> event for the given access, deduplicated against the | ||
| /// per-distinct-id cache that the legacy single-flag path also writes to. | ||
| /// </summary> | ||
| void TryCaptureFeatureFlagCalledEventIfNeeded( |
There was a problem hiding this comment.
question: Did you consider just taking an internal PostHogClient reference on the snapshot directly? Only EvaluationsHost implements this interface, the interface is internal, and no test substitutes a fake host. With a direct reference, EvaluationsHost and this file go away (~30 lines).
Defensible argument exists for the seam — keeps the snapshot honest about what state it depends on, future tests stay cheap to mock — but "future tests might want a mock" is the textbook YAGNI smell.
If you keep it, the method name itself is overstuffed: Try, Capture, IfNeeded are three uncertainty modifiers, and the method has no return value to make Try meaningful.
| void TryCaptureFeatureFlagCalledEventIfNeeded( | |
| void CaptureFeatureFlagCalled( |
| /// <param name="flags">A snapshot of feature flag evaluations. When non-null, <c>$feature/<key></c> and <c>$active_feature_flags</c> are attached from the snapshot — no <c>/flags</c> call is made.</param> | ||
| /// <param name="timestamp">Optional: Custom timestamp when the event occurred.</param> | ||
| /// <returns><c>true</c> if the exception event was successfully enqueued. Otherwise <c>false</c>.</returns> | ||
| bool CaptureException( |
There was a problem hiding this comment.
suggestion: Consider landing [Obsolete(error: false)] on the legacy methods (IsFeatureEnabledAsync, GetFeatureFlagAsync, the bool sendFeatureFlags overload of Capture/CaptureException) in this PR rather than a follow-up. Non-breaking change, ~3 lines per method, lets users discover the migration path the moment they update the package.
[Obsolete("Prefer EvaluateFlagsAsync(...) which fetches all flags in one call. See PR #187.", error: false)]
The PR description mentions this is deferred to a follow-up; nothing about adding [Obsolete] blocks the rest of the deprecation cleanup that follow-up would do (README, examples, etc.).
| /// </param> | ||
| /// <param name="cancellationToken">The cancellation token that can be used to cancel the operation.</param> | ||
| /// <returns>A snapshot of feature flag evaluations.</returns> | ||
| Task<FeatureFlagEvaluations> EvaluateFlagsAsync( |
There was a problem hiding this comment.
suggestion: Worth adding a <remarks> paragraph cross-referencing Only(...) so users see the distinction at the entry point too: AllFeatureFlagsOptions.FlagKeysToEvaluate scopes the underlying /flags request body. FeatureFlagEvaluations.Only(keys) filters an already-evaluated snapshot in memory. Two helpers, two costs — easy to confuse.
| var matches = System.Text.RegularExpressions.Regex.Matches(body, "\\$feature_flag_called"); | ||
| Assert.Single(matches); | ||
| } | ||
| } |
There was a problem hiding this comment.
blocking: Three coverage gaps for new code paths:
-
Mixed local+remote merge.
OnlyEvaluateLocallyDoesNotHitRemoteonly exercises local. Nothing pins the merge inEvaluateFlagsAsync(PostHogClient.cs:800–806) where local fills some records and the remote pass fills the rest withlocallyEvaluated: false. A regression that flips theif (!records.ContainsKey(key))guard wouldn't be caught. -
Error propagation.
flagsResult.ErrorsWhileComputingFlagsaddsFeatureFlagError.ErrorsWhileComputingFlagsto the snapshot errors, andEvaluationsHostappendsFeatureFlagError.FlagMissingwhen an unknown key is accessed. Neither path surfaces in any test, so the wiring through to$feature_flag_erroron$feature_flag_calledis unverified. -
CaptureException(..., flags: snapshot). Brand-new public overload, zero tests. A wiring mistake (e.g., accidentally passingflags: nullinCaptureExceptionCore) would silently drop$feature/<key>props on exception events.
Skeleton for the merge test:
[Fact]
public async Task MixedLocalAndRemoteEvaluationMergesRecordsAndTagsSourceCorrectly()
{
var container = new TestContainer(personalApiKey: "fake-personal-api-key");
container.FakeHttpMessageHandler.AddLocalEvaluationResponse(
"""{"flags": [
{"id": 1, "key": "local-only", "active": true, "rollout_percentage": 100,
"filters": {"groups": [{"properties": [], "rollout_percentage": 100}]}},
{"id": 2, "key": "needs-remote",
"filters": {"groups": [{"properties": [{"key": "country", "value": "US"}],
"rollout_percentage": 100}]}}
]}""");
container.FakeHttpMessageHandler.AddFlagsResponse(
"""{"featureFlags": {"needs-remote": "variant-x", "remote-only": true}}""");
var batchHandler = container.FakeHttpMessageHandler.AddBatchResponse();
var client = container.Activate<PostHogClient>();
var snapshot = await client.EvaluateFlagsAsync("user-1", options: null, CancellationToken.None);
Assert.True(snapshot.IsEnabled("local-only"));
Assert.Equal("variant-x", snapshot.GetFlag("needs-remote")?.VariantKey);
Assert.True(snapshot.IsEnabled("remote-only"));
await client.FlushAsync();
using var doc = JsonDocument.Parse(batchHandler.GetReceivedRequestBody(indented: false));
var byFlag = doc.RootElement.GetProperty("batch").EnumerateArray()
.ToDictionary(
e => e.GetProperty("properties").GetProperty("$feature_flag").GetString()!,
e => e.GetProperty("properties").GetProperty("locally_evaluated").GetBoolean());
Assert.True(byFlag["local-only"]);
Assert.False(byFlag["needs-remote"]);
Assert.False(byFlag["remote-only"]);
}Skeleton for FlagMissing propagation:
[Fact]
public async Task UnknownKeyAccessAppendsFlagMissingError()
{
var container = new TestContainer();
container.FakeHttpMessageHandler.AddFlagsResponse("""{"featureFlags": {"known": true}}""");
var batchHandler = container.FakeHttpMessageHandler.AddBatchResponse();
var client = container.Activate<PostHogClient>();
var snapshot = await client.EvaluateFlagsAsync("user-1", options: null, CancellationToken.None);
snapshot.IsEnabled("does-not-exist");
await client.FlushAsync();
using var doc = JsonDocument.Parse(batchHandler.GetReceivedRequestBody(indented: false));
var props = doc.RootElement.GetProperty("batch").EnumerateArray().Single().GetProperty("properties");
Assert.Contains("FlagMissing",
props.GetProperty("$feature_flag_error").GetString(), StringComparison.Ordinal);
}Skeleton for CaptureException:
[Fact]
public async Task CaptureExceptionAttachesFeatureFlagsFromSnapshot()
{
var container = new TestContainer();
container.FakeHttpMessageHandler.AddFlagsResponse(
"""{"featureFlags": {"flag-a": true, "flag-b": "variant-x"}}""");
var batchHandler = container.FakeHttpMessageHandler.AddBatchResponse();
var client = container.Activate<PostHogClient>();
var snapshot = await client.EvaluateFlagsAsync("user-1", options: null, CancellationToken.None);
client.CaptureException(new InvalidOperationException("boom"), "user-1",
properties: null, groups: null, flags: snapshot);
await client.FlushAsync();
using var doc = JsonDocument.Parse(batchHandler.GetReceivedRequestBody(indented: false));
var exceptionEvent = doc.RootElement.GetProperty("batch").EnumerateArray()
.Single(e => e.GetProperty("event").GetString() == "$exception");
var props = exceptionEvent.GetProperty("properties");
Assert.True(props.GetProperty("$feature/flag-a").GetBoolean());
Assert.Equal("variant-x", props.GetProperty("$feature/flag-b").GetString());
}Address PR feedback: - IPostHogClient: add default interface implementations for the new Capture(flags:), CaptureException(flags:), and EvaluateFlagsAsync members so external implementers don't see a source break. Conditionally compiled — DIMs only on netstandard2.1+ (the runtime requirement); netstandard2.0 keeps abstract members. - FeatureFlagEvaluations.RecordAccess: early-return on repeat access, dropping per-call dedup-cache lookups + property allocation when a key has already been seen by this snapshot. Cross-snapshot dedup still flows through the MemoryCache. - AddFeatureFlagsToCapturedEvent (snapshot path): single-pass enumeration over Records, skip the LINQ Where/Select/ToArray for $active_feature_flags. - FeatureFlagEvaluations._records: tighten field type to Dictionary so Keys is a clean expression-bodied getter (no IReadOnlyDictionary cast). - FeatureFlagEvaluations.Only(...): lazy missing-keys list — no allocation when every requested key is present. - EvaluationsHost: drop the redundant id/version/reason copy block — the values it would write are already populated by BuildFeatureFlagCalledProperties via the FeatureFlagWithMetadata pattern match. - EvaluatedFlagRecord: remove the now-unused Id/Version/Reason fields. The property dict is built from record.Flag (typed as FeatureFlagWithMetadata when present) rather than from duplicated record-level state. - EvaluateFlagsAsync: local-pass quota_limited preserves locally-evaluated records and surfaces FeatureFlagError.QuotaLimited (matches remote-pass behavior); previously it discarded local results entirely. Add a comment on the local-wins merge clarifying the divergence from GetAllFeatureFlagsAsync. - IPostHogClient.EvaluateFlagsAsync: <remarks> contrasting FlagKeysToEvaluate (request-body scoping) with FeatureFlagEvaluations.Only(...) (in-memory). - IFeatureFlagEvaluationsHost.TryCaptureFeatureFlagCalledEventIfNeeded -> CaptureFeatureFlagCalled (no return value, no try semantics). Tests added: - MixedLocalAndRemoteEvaluationMergesRecordsAndTagsSourceCorrectly: pins the local-wins merge with locally_evaluated tagged correctly per source. - UnknownKeyAccessAppendsFlagMissingErrorOnFeatureFlagCalled: pins the $feature_flag_error wiring through to the emitted event. - CaptureExceptionAttachesFeatureFlagsFromSnapshot: pins the new CaptureException(flags:) overload so a CaptureExceptionCore wiring mistake would be caught. Generated-By: PostHog Code Task-Id: 494d1c64-1b39-421a-9317-7ccd5992aa40
There was a problem hiding this comment.
Pull request overview
Adds a new “single /flags call” feature-flag snapshot API (EvaluateFlagsAsync) and wires it through capture flows, while fixing locally-evaluated exposure event metadata.
Changes:
- Introduces
FeatureFlagEvaluationssnapshot with lazy/deduped$feature_flag_calledemission and filtering helpers (OnlyAccessed,Only). - Adds
Capture(..., flags: FeatureFlagEvaluations)/CaptureException(..., flags: ...)overloads to attach$feature/<key>+$active_feature_flagswithout an extra/flagscall. - Fixes
$feature_flag_calledproperties for locally-evaluated flags (locally_evaluated=true, reason, and definitions-loaded timestamp) and adds warning-log controls.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/UnitTests/Features/FeatureFlagsTests.cs | Updates assertions for corrected locally-evaluated $feature_flag_called properties. |
| tests/UnitTests/Features/FeatureFlagEvaluationsTests.cs | Adds unit tests for the new snapshot API, filtering helpers, dedup behavior, and capture wiring. |
| src/PostHog/PostHogClient.cs | Implements EvaluateFlagsAsync, snapshot-host bridge, new capture overloads, and updated exposure event property building. |
| src/PostHog/IPostHogClient.cs | Extends public interface with snapshot evaluation + snapshot-based capture overloads. |
| src/PostHog/Generated/VersionConstants.cs | Bumps library version constant to 2.6.0. |
| src/PostHog/Features/LocalFeatureFlagsLoader.cs | Tracks and exposes “flag definitions loaded at” timestamp for local evaluation. |
| src/PostHog/Features/IFeatureFlagEvaluationsHost.cs | Introduces internal host seam for snapshot → client interactions (deduped capture + warnings). |
| src/PostHog/Features/FeatureFlagExtensions.cs | Adds convenience extension overloads for EvaluateFlagsAsync. |
| src/PostHog/Features/FeatureFlagEvaluations.cs | Adds the new snapshot type (access tracking, filtering, lazy exposure capture). |
| src/PostHog/Features/EvaluatedFlagRecord.cs | Adds internal record type used by the snapshot for event/property attachment and exposure capture. |
| src/PostHog/Config/PostHogOptions.cs | Adds FeatureFlagsLogWarnings option for snapshot filter-helper warnings. |
| Directory.Build.props | Bumps package version to 2.6.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| catch (Exception e) when (e is not ArgumentException and not NullReferenceException) | ||
| { | ||
| _logger.LogErrorUnableToGetFeatureFlagsAndPayloads(e); | ||
| errors.Add(FeatureFlagError.UnknownError); | ||
| } |
There was a problem hiding this comment.
This catch-all will also swallow OperationCanceledException/TaskCanceledException when the provided CancellationToken is canceled, causing EvaluateFlagsAsync to ignore cancellation and instead log/return an UnknownError snapshot. Exclude OperationCanceledException (and/or TaskCanceledException) in the filter so cancellation propagates like GetFeatureFlagAsync does.
| using System.Text.Json; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.Logging; | ||
| using Microsoft.Extensions.Options; |
There was a problem hiding this comment.
Microsoft.Extensions.Options is not used in this file. With TreatWarningsAsErrors enabled, the resulting unused-using warning (CS8019) will fail the build; remove the unused using directive.
| using Microsoft.Extensions.Options; |
| GroupCollection? groups, | ||
| FeatureFlagEvaluations? flags, | ||
| DateTimeOffset? timestamp = null) | ||
| => CaptureCore(distinctId, eventName, properties, groups, sendFeatureFlags: false, flags, timestamp); |
There was a problem hiding this comment.
This call mixes a named argument (sendFeatureFlags: false) with subsequent positional arguments (flags, timestamp), which is a C# compile-time error. Use either all positional arguments or name the remaining arguments (e.g., flags: flags, timestamp: timestamp).
| => CaptureCore(distinctId, eventName, properties, groups, sendFeatureFlags: false, flags, timestamp); | |
| => CaptureCore(distinctId, eventName, properties, groups, sendFeatureFlags: false, flags: flags, timestamp: timestamp); |
| GroupCollection? groups, | ||
| bool sendFeatureFlags, | ||
| DateTimeOffset? timestamp = null) | ||
| => CaptureCore(distinctId, eventName, properties, groups, sendFeatureFlags, flags: null, timestamp); |
There was a problem hiding this comment.
This call passes a named argument (flags: null) followed by a positional argument (timestamp), which is not allowed in C# and will not compile. Name the timestamp argument as well (timestamp: timestamp) or make the entire call positional.
| => CaptureCore(distinctId, eventName, properties, groups, sendFeatureFlags, flags: null, timestamp); | |
| => CaptureCore(distinctId, eventName, properties, groups, sendFeatureFlags, flags: null, timestamp: timestamp); |
| GroupCollection? groups, | ||
| bool sendFeatureFlags, | ||
| DateTimeOffset? timestamp = null) | ||
| => CaptureExceptionCore(exception, distinctId, properties, groups, sendFeatureFlags, flags: null, timestamp); |
There was a problem hiding this comment.
This call mixes positional arguments with a later named argument (flags: null) and then a positional argument (timestamp), which is a compile-time error. Use all positional arguments or name all arguments after the first named one (e.g., flags: null, timestamp: timestamp).
| GroupCollection? groups, | ||
| FeatureFlagEvaluations? flags, | ||
| DateTimeOffset? timestamp = null) | ||
| => CaptureExceptionCore(exception, distinctId, properties, groups, sendFeatureFlags: false, flags, timestamp); |
There was a problem hiding this comment.
This call mixes a named argument (sendFeatureFlags: false) with subsequent positional arguments (flags, timestamp), which is a C# compile-time error. Use all positional arguments or name the remaining arguments (flags: flags, timestamp: timestamp).
| => CaptureExceptionCore(exception, distinctId, properties, groups, sendFeatureFlags: false, flags, timestamp); | |
| => CaptureExceptionCore(exception, distinctId, properties, groups, sendFeatureFlags: false, flags: flags, timestamp: timestamp); |
| FeatureFlagEvaluations? flags, | ||
| DateTimeOffset? timestamp = null) | ||
| #if !NETSTANDARD2_0 | ||
| => Capture(distinctId, eventName, properties, groups, sendFeatureFlags: false, timestamp) |
There was a problem hiding this comment.
Default interface implementation: the call uses a named argument (sendFeatureFlags: false) followed by a positional argument (timestamp), which will not compile. Name the timestamp argument too (timestamp: timestamp) or use all positional arguments.
| => Capture(distinctId, eventName, properties, groups, sendFeatureFlags: false, timestamp) | |
| => Capture(distinctId, eventName, properties, groups, sendFeatureFlags: false, timestamp: timestamp) |
| => CaptureException(exception, distinctId, properties, groups, sendFeatureFlags: false, timestamp) | ||
| #endif |
There was a problem hiding this comment.
Default interface implementation: the call uses a named argument (sendFeatureFlags: false) followed by a positional argument (timestamp), which is a compile-time error. Use timestamp: timestamp (or make the whole invocation positional).
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Nice! We'll make a .NET developer out of you!
Just some nits/suggestions. But nothing blocking.
| GroupCollection? groups, | ||
| FeatureFlagEvaluations? flags, | ||
| DateTimeOffset? timestamp = null) | ||
| #if !NETSTANDARD2_0 |
There was a problem hiding this comment.
question: Worth flagging in the release notes that the DIM fix is partial. The #if !NETSTANDARD2_0 guard wraps the default body but not the member itself, so on the netstandard2.0 build the new methods are still abstract. A .NET Framework consumer who hand-implements IPostHogClient (test fakes, decorators) will hit CS0535 on rebuild. DIMs need IL features netstandard2.0 doesn't have, so the conditional is unavoidable here.
Given .NET Framework is the least supported audience, keeping 2.6.0 and calling this out in the release notes seems right. Something like:
⚠️ Breaking for netstandard2.0 implementers. Three new methods onIPostHogClient(Capture(flags:),CaptureException(flags:),EvaluateFlagsAsync) ship as abstract on the netstandard2.0 build because default interface methods aren't supported on that runtime. .NET Framework consumers who hand-implementIPostHogClientwill need to add stubs (or upgrade to netstandard2.1+/net8.0 builds, where the members are default-implemented). netstandard2.1 and net8.0 consumers see this as an additive change.
| /// The internal per-flag records. Used by <see cref="PostHogClient"/>'s capture path to attach | ||
| /// <c>$feature/<key></c> properties. | ||
| /// </summary> | ||
| internal Dictionary<string, EvaluatedFlagRecord> Records => _records; |
There was a problem hiding this comment.
suggestion: Tightening the field to Dictionary<> was the right call for the Keys getter, but Records still hands the inner dictionary out by reference. The one call site (PostHogClient.AddFeatureFlagsToCapturedEvent) only iterates, so dropping to IReadOnlyDictionary<> here costs nothing and prevents a future flags.Records.Add(...) from silently corrupting the snapshot.
| internal Dictionary<string, EvaluatedFlagRecord> Records => _records; | |
| internal IReadOnlyDictionary<string, EvaluatedFlagRecord> Records => _records; |
| } | ||
| catch (ApiException e) when (e.ErrorType is "quota_limited") | ||
| { | ||
| // Quota-limited at the local-evaluation endpoint: keep whatever local records we |
There was a problem hiding this comment.
nit: "keep whatever local records we computed before the failure" reads as if a partial set could be preserved, but quota_limited is only ever thrown by _featureFlagsLoader.GetFeatureFlagsForLocalEvaluationAsync, the first call in the try, before any evaluation runs. So records is always empty when this catch fires. Worth saying what the code actually does:
| // Quota-limited at the local-evaluation endpoint: keep whatever local records we | |
| // Quota-limited at /flags/definitions: no local records were produced (the failure | |
| // happens during the definitions fetch, before evaluation), so the snapshot ends | |
| // up empty. Still surface the error on $feature_flag_called and skip the remote | |
| // pass to avoid double-charging quota. |
Mark the four legacy paths replaced by EvaluateFlagsAsync + snapshot [Obsolete(error: false)] so users see migration guidance the moment they update the package: - IPostHogClient.IsFeatureEnabledAsync / .GetFeatureFlagAsync - IPostHogClient.Capture(..., bool sendFeatureFlags, ...) - IPostHogClient.CaptureException(..., bool sendFeatureFlags, ...) Cascade [Obsolete] to wrapper extensions: - FeatureFlagExtensions: 5 IsFeatureEnabledAsync + 5 GetFeatureFlagAsync overloads - CaptureExtensions: bool-sendFeatureFlags overloads of Capture / CapturePageView / CaptureScreenView - CaptureExceptionExtensions: bool-sendFeatureFlags overloads Each extension delegates internally; suppress CS0618 inside the body so the warning surfaces at the user call site, not at the SDK call into itself. Internal call sites that always passed sendFeatureFlags: false migrate to the new Capture(..., flags: null, ...) overload — no behavioral change, but stops the SDK from internally calling its own deprecated path. Tests and samples that intentionally exercise the deprecated surface get a file-level #pragma warning disable CS0618. The new FeatureFlagEvaluationsTests cross-path dedup test wraps a single IsFeatureEnabledAsync call in a per-call pragma so the rest of the file still catches accidental new uses. PostHog.AI's OpenAI handler keeps the legacy Capture(..., sendFeatureFlags: false, ...) call with a #pragma + TODO; its tests assert the legacy mock shape and migrating them is its own change. PostHog.AspNetCore's PostHogVariantFeatureManager suppresses with a #pragma + TODO; the FeatureManager API is per-flag so a snapshot rewrite is non-trivial. All 781 unit tests, 26 AspNetCore tests, and 19 AI tests pass. Generated-By: PostHog Code Task-Id: 494d1c64-1b39-421a-9317-7ccd5992aa40
- sdk_compliance_adapter Program.cs: switch the lone Capture(..., sendFeatureFlags: false, ...) call to the new flags: null overload so the Docker publish in the SDK-compliance CI job no longer hits CS0618 → exit 1. - EvaluateFlagsAsync: exclude OperationCanceledException from the catch-all so cancellation propagates instead of being logged as UnknownError. Matches GetFeatureFlagAsync's filter. - FeatureFlagEvaluations.Records: typed as IReadOnlyDictionary so the one consumer (PostHogClient.AddFeatureFlagsToCapturedEvent) can iterate but cannot mutate the snapshot's underlying state. - Local-quota comment in EvaluateFlagsAsync: clarify that `records` is always empty when the catch fires (the throwing call is the first inside the try). - Capture / CaptureException / DIM bodies: name every trailing argument (timestamp:, flags:) so non-trailing-named-argument call sites don't trip future IDE/bot warnings even though the C# 7.2+ rules accept them. - FeatureFlagEvaluationsTests: drop the unused Microsoft.Extensions.Options using directive. Generated-By: PostHog Code Task-Id: 494d1c64-1b39-421a-9317-7ccd5992aa40
Generated-By: PostHog Code Task-Id: 494d1c64-1b39-421a-9317-7ccd5992aa40
Summary
EvaluateFlagsAsync(distinctId, options)returns aFeatureFlagEvaluationssnapshot — one/flagscall powers all flag branching for the rest of the request.IsEnabled/GetFlag/GetFlagPayloadplusOnlyAccessed()andOnly(...)filter helpers, with$feature_flag_calledfired lazily on first access and deduped against the existing per-distinct-id cache.Capture(..., FeatureFlagEvaluations? flags, ...)overload (and matchingCaptureException) attaches$feature/<key>and$active_feature_flagsfrom the snapshot — no second/flagsrequest.locally_evaluated=falseon$feature_flag_called. They now correctly carrylocally_evaluated=true,$feature_flag_reason="Evaluated locally", and a new$feature_flag_definitions_loaded_attimestamp.IsFeatureEnabledAsync,GetFeatureFlagAsync, and thebool sendFeatureFlagsoverloads ofCapture/CaptureException— is now[Obsolete(error: false)]. Users see a non-breaking migration warning on update.RFC: PostHog/requests-for-comments-internal#1020 · Reference implementations: posthog-python#539, posthog-js#3476.
Design decisions
GetFlagreturns the existing publicFeatureFlag?(with implicitbool/stringconversions) rather than mirroring Node's barebool|string|null— keeps a single canonical .NET flag type and gives callers the payload too.Capture(flags:)is a separate overload, not a new optional parameter on the existing one — adding a defaulted parameter to a positional public signature is source-breaking.AllFeatureFlagsOptions.FlagKeysToEvaluate(already present) is reused as the request-body scoper; in-memory filtering uses the snapshot's ownOnly(keys)so the two concerns stay distinct.IFeatureFlagEvaluationsHost(fire-deduped-event + log-warning), not the full client — keeps the snapshot easy to test in isolation.OnlyAccessed/Only) get a freshConcurrentDictionaryfor accessed keys so child access doesn't back-propagate to the parent and the snapshot is safe to read across parallel branches.OnlyAccessed()with no prior access logs a warning and returns all flags as a fallback (rather than silently dropping exposure data); silenceable via a newPostHogOptions.FeatureFlagsLogWarningsknob.IPostHogClientimplementers see an additive change. Netstandard2.0 keeps abstract members because the ns2.0 runtime can't host DIMs — .NET Framework 4.x consumers who hand-implementIPostHogClient(test fakes, custom decorators) will need to add the three new members on rebuild. The release notes should call this out.Phase 2 (separate PR)
PostHog.AspNetCore'sPostHogVariantFeatureManagerstill calls the deprecatedGetFeatureFlagAsync(per-flag) — migrate toEvaluateFlagsAsync+ per-request snapshot.PostHog.AI'sPostHogOpenAIHandlerstill calls the deprecatedCapture(..., sendFeatureFlags: false, ...); switching to the snapshot overload also requires updating the NSubstitute test mocks.Examples.csand thesamples/projects to demonstrate the snapshot pattern in docs.All UnitTests + UnitTests.AspNetCore + PostHog.AI.Tests pass; 18 new tests cover the snapshot, filter helpers, capture wiring, mixed local+remote merge, FlagMissing propagation,
CaptureException(flags:), and cross-path dedup.Created with PostHog Code