Repository navigation
Bug 2088185 - Give RecordedContext the client's enrollment state - #7777
Draft
yashikakhurana wants to merge 3 commits into
Draft
yashikakhurana wants to merge 3 commits into
yashikakhurana wants to merge 3 commits into
Conversation
Desktop records which experiments the client is enrolled in, but mobile could not: the SDK knows the enrollment state, yet the recorded context only ever received event query results, so the apps had nothing to record. Add set_enrollment_state to RecordedContext, with a RecordedEnrollmentState carrying active_experiments, enrollments and enrollments_map. The SDK calls it just before record(), alongside the existing event query hand-over. is_already_enrolled is left out: it is only meaningful while one experiment is being evaluated, so it has no single value to record. These keys also feed targeting, so drop them from whatever to_json returns before it is stored, and a recorded copy can never shadow the real values. This is a breaking change for every implementer of RecordedContext, in the same way adding the event query methods was.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Desktop records which experiments the client is enrolled in. Mobile can't, because the SDK never gives the app's recorded context that information. This lets population sizing handle targeting that depends on enrollment on Fenix and iOS.
This adds
set_enrollment_statetoRecordedContext. The SDK calls it just beforerecord(), passingactive_experiments,enrollmentsandenrollments_map.is_already_enrolledis left out because it only applies while one experiment is being evaluated.The SDK also drops these keys from whatever
to_jsonreturns, so an app can never override the real values used for targeting.Breaking change
Every
RecordedContextimplementer must add the method. This is a draft until the consumer PRs are ready:metrics.yamland record them inRecordedNimbusContext.ktRecordedNimbusContext.swiftTesting
cargo test -p nimbus-sdk, rustfmt and clippy pass.Pull Request checklist