Skip to content

Use RustDecider#choose() in for_identifier*() api #84

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

Merged

Conversation

mrlevitas
Copy link
Contributor

@mrlevitas mrlevitas commented Dec 12, 2022

Summary

Utilize rust_decider.RustDecider#choose() in get_variant_for_identifier() & get_variant_for_identifier_without_expose()
(instead of rust_decider.make_ctx()/rust_decider.choose() functions)
to emit prometheus metrics.

Notes

1)
If a ctx contains the field that a Feature config, called through get_variant_for_identifier(), needs for bucketing, it will get utilized, since there's no concept of identifier_type in RustDecider#choose() forcing the bucketing to override (we just set ctx[identifier_type] = identifier).

e.g. if a Feature "exp_1" with bucket_val: device_id gets called via:
get_variant_for_identifier("exp_1", identifier="identifier.com", identifier_type="canonical_url")
BUT has the device_id field set on ctx, it will use device_id for bucketing, not
"identifier.com", identifier_type="canonical_url" params.

The only way to establish similar behavior as before would be to clear the other two identifiers in ctx that aren't of the identifier_type param, e.g. clear device_id/user_id from ctx and set canonical_url: "identifier.com" on it.

This is why I deleted 2 test cases..but maybe it is worth clearing the other bucketing fields in ctx?

2)
Removed the overwrite_identifier: bool param/logic in _send_expose()/_send_expose_if_holdout, since we should always use/set the bucket_val: bucketing_value in the v2 exposure event based on the decider event string, not just in some cases, when we had a for_identifier*() "override" .

@mrlevitas
Copy link
Contributor Author

after discussion, we decided it's ok for the identifier_type/identifier params to be ignored if identifier_type doesn't correspond to the experiment's 'bucket_val' field, and instead lookup the field corresponding to bucket_val in the context and use that for bucketing.
essentially, identifier_type/identifier params are added to the context, but if they don't correspond to the experiment's bucket_val, they will be ignored, since they shouldn't act like an override.

cc @gshipilov

@mrlevitas mrlevitas force-pushed the use_RustDecider_choose_in_get_variant_for_identifier branch from d2e38fe to bad81fc Compare December 12, 2022 22:27
@mrlevitas mrlevitas changed the title Use RustDecider#choose() in get_variant_for_identifier*() Use RustDecider#choose() in for_identifier*() api Dec 12, 2022
@mrlevitas mrlevitas merged commit 46598e5 into develop Dec 12, 2022
@mrlevitas mrlevitas deleted the use_RustDecider_choose_in_get_variant_for_identifier branch December 12, 2022 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants