search

microsoft / FeatureManagement-Dotnet

Microsoft.FeatureManagement provides standardized APIs for enabling feature flags within applications. Utilize this library to secure a consistent experience when developing applications that use patterns such as beta access, rollout, dark deployments, and more.
MIT License
1.03k stars 111 forks source link

The TargetingId should be included in the FeatureEvaluation event even if it's null/empty. #399

Closed zhenlan closed 2 months ago

zhenlan commented 6 months ago

From what I can see in the telemetry data, TargetingId appears to be skipped if it's null/empty, but we do send a FeatureEvaluation event for it. We should include it even if it's null/empty.

(We've discussed this. Open an issue to track it.)

rossgrambo commented 5 months ago

Currently null and empty string are treated as different targets. Should we continue that? I believe python is treating them as the same. If we're going to emit it, the languages will need to do the same thing. cc; @mrm9084

jimmyca15 commented 5 months ago

Currently null and empty string are treated as different targets

Can you explain what you mean here? Do you mean if someone sets TargetingContext.UserId to null vs "", there may be a different variant returned?

rossgrambo commented 5 months ago

They get the same variant right now, but if we removed the null or empty check on the outgoing telemetry- we can emit events where the userId is null and other events where the userId is "".

Python would never emit an event for null, as it defaults to "". So a null userId in C# would have a differently shaped impression event than that same "null" user in python.

tldr; should we commit to "" as the null/empty targetingId?

jimmyca15 commented 5 months ago

As far as I'm concerned, there is no such thing as a null user id. Our system should effectively treat it as empty string.

rossgrambo commented 2 months ago

Turns out both null and empty are ignored by Application Insights SDK and the field won't be added. So unfortunately we can't include this on the impression event.

We decided to have the ActivityEvent optionally include these as well to follow suit.