Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
zhenlan opened this issue Mar 14, 2024 · 5 comments
Milestone

Comments

@zhenlan
Copy link
Member

zhenlan commented Mar 14, 2024

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.)

@zhenlan zhenlan added this to the v4.0.0 milestone Mar 23, 2024
@rossgrambo
Copy link
Contributor

rossgrambo commented Mar 27, 2024

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants