Add default PostHog SDK client facade#190
Conversation
posthog-dotnet Compliance ReportDate: 2026-05-07 11:11:41 UTC
|
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ❌ | 34ms |
| Request Payload.Flags Request Uses V2 Query Param | ❌ | 17ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ❌ | 4ms |
| Request Payload.Flags Request Omits Authorization Header | ❌ | 4ms |
| Request Payload.Token In Flags Body Matches Init | ❌ | 3ms |
| Request Payload.Groups Round Trip | ❌ | 4ms |
| Request Payload.Groups Default To Empty Object | ❌ | 4ms |
| Request Payload.Person Properties Distinct Id Auto Populated When Caller Omits It | ❌ | 3ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ❌ | 4ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ❌ | 4ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ❌ | 4ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 2ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 167ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ❌ | 5ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ❌ | 4ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ❌ | 3ms |
Failures
request_payload.request_with_person_properties_device_id
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.flags_request_uses_v2_query_param
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.flags_request_hits_flags_path_not_decide
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.flags_request_omits_authorization_header
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.token_in_flags_body_matches_init
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.groups_round_trip
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.groups_default_to_empty_object
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.person_properties_distinct_id_auto_populated_when_caller_omits_it
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.disable_geoip_false_propagates_as_geoip_disable_false
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.disable_geoip_omitted_defaults_to_false
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.flag_keys_to_evaluate_contains_only_requested_key
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_lifecycle.two_flag_calls_produce_two_remote_requests
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_lifecycle.mock_response_value_is_returned_to_caller
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
side_effect_events.get_feature_flag_captures_feature_flag_called_event
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
|
@haacked wdyt? i noticed some SDKs don't have a global facade, and you have to pass it around, and sometimes passing it around is tricky, you have to change a lot of method and ctor signatures, and make your code even more coupled to PostHog |
I'm not sure this is needed, but I'm not totally opposed to it. For context, this repo has two packages:
Dependency Injection (aka DI) is very popular in the .NET community. So many developers would be familiar with the approach this library takes which is:
In particular, this is because certain environments (such as AspNetCore) have specific requirements on how the client is configured. Using a global facade in AspNetCore would almost certainly be wired up wrong. Our docs explain how to do it: https://posthog.com/docs/libraries/dotnet using PostHog;
var builder = WebApplication.CreateBuilder(args);
// Add PostHog to the dependency injection container as a singleton.
builder.AddPostHog();And then inject the client anywhere you needed it. There may be scenarios where a global Facade makes sense, but I'd rather create a package for that specific scenario. For example, if we wanted to support Maui client apps, we could create a |
This is not about Maui, as the PR desc says:
I'd agree with a Maui package for sure, but for console apps, scripts, etc., then it's not scalable and maintainable having way too many packages just for setting up a conventional DI where a global facade would do, some other SDKs do this already btw |
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/PostHog/PostHogSdk.cs:248-258
**`ShutdownAsync` triggers spurious warning when no client is configured**
When `_defaultClient` is `null`, `Interlocked.Exchange(ref _defaultClient, null)` returns `null`, so `?? Current` is evaluated. `Current` then calls `NoOpPostHogClient.LogNoDefaultClient()`, emitting the "PostHogSdk.DefaultClient is not configured" warning. This contradicts the XML doc which promises the method shuts down "if one is configured." The `Dispose()` teardown in the test suite also hits this path for every test that doesn't set a client.
The fix is to take the client atomically and return early when nothing was configured:
```csharp
public static async ValueTask ShutdownAsync()
{
var client = Interlocked.Exchange(ref _defaultClient, null);
if (client is null)
{
return;
}
try
{
await client.FlushAsync();
}
finally
{
await client.DisposeAsync();
}
}
```
```suggestion
public static async ValueTask ShutdownAsync()
{
var client = Interlocked.Exchange(ref _defaultClient, null);
if (client is null)
{
return;
}
```
### Issue 2 of 3
src/PostHog/README.md:38-39
**README example flushes twice**
`ShutdownAsync` internally calls `FlushAsync` before disposing the client, so calling `FlushAsync` just before `ShutdownAsync` in the example results in two sequential flushes. The standalone `FlushAsync` call is redundant and may confuse readers into thinking both calls are always necessary. Consider replacing both lines with just `await PostHogSdk.ShutdownAsync();`.
```suggestion
await PostHogSdk.ShutdownAsync();
```
### Issue 3 of 3
tests/UnitTests/PostHogSdkTests.cs:57-66
**Prefer parameterised tests for the no-op assertions**
`AsyncCallsAreNoOpWithoutDefaultClient` exercises three independent methods in a single `[Fact]`, meaning a failure in the first assertion can silently mask the others. The project's coding standard prefers `[Theory]` / `[InlineData]` parameterised tests for this kind of repetitive-assertion pattern; splitting these into a `[Theory]` would also make the test name self-documenting for each case.
Reviews (1): Last reviewed commit: "Merge branch 'main' into feat/add-defaul..." | Re-trigger Greptile |
Fair point on Maui not being the analogue. Console/scripts is its own thing. Two concerns I want to separate: Namespace collision. Options:
Separate
With those, the package earns its keep. Without them, agreed it's overkill. Either way, the namespace collision is my blocker. Can we sort that out first? |
sure |
|
@haacked done |
💡 Motivation and Context
Adds an optional process-wide default PostHog client facade for console apps, scripts, and other places where passing an
IPostHogClientinstance around is inconvenient.💚 How did you test it?
dotnet build src/PostHog/PostHog.csproj --no-restore -v:minimaldotnet test tests/UnitTests/UnitTests.csproj --framework net8.0 --no-restore --filter PostHogSdkTests -v:minimal📝 Checklist
If releasing new changes
releaselabel to the PRbump-patch,bump-minor, orbump-major