add experiment logic #1081
add experiment logic #1081
Conversation
| randomizer: randomizer) | ||
| } | ||
|
|
||
| public func stateFor(_ subfeature: any PrivacySubfeature, |
There was a problem hiding this comment.
looking at this API, I see an important issue with the naming. I understand the urge to be backward compatible, but I dislike that a method named like a simple getter actually mutates the internal state underneath (this is new behavior). can you think of proposing something here? e.g. resolveState(for subfeature?
|
|
||
| private var store: ExperimentsDataStoring | ||
| private let randomizer: (Range<Double>) -> Double | ||
| private let queue = DispatchQueue(label: "com.ExperimentCohortsManager.queue") |
There was a problem hiding this comment.
this queue is not needed, it gives a false sense of security but it doesn't protect from anything;
instead you should clarify in the documentation for this manager that it is inherently not thread-safe, particularly for the method that assigns a cohort (as it mutates the state), and so should never be used elsewhere except for as an instance inside AppPrivacyConfig
| cohortID: CohortID?, | ||
| versionProvider: AppVersionProvider, | ||
| randomizer: (Range<Double>) -> Double) -> PrivacyConfigurationFeatureState { | ||
| Self.experimentManagerQueue.sync { |
There was a problem hiding this comment.
You should limit the sync queue block to only what truly requires synchronization. Avoid including any dependencies inside it (!). For example, the InternalUserDecider should handle its responsibilities elsewhere because if someone adds main queue synchronization code to it ever, it will lead to potential deadlock
| (target.localeLanguage == nil || target.localeLanguage == locale.languageCode) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
What I dislike the most is that AppPrivacyConfiguration is turning into a MassiveAppPrivacyConfiguration. Can we plan a follow-up project to streamline and clean this up, possibly?
|
Just glancing over this, made me think that we should add @samsymons and @diegoreymendez as stakeholders, as Config space is now used both in Host app and the Extensions - so potentially we could be using Feature experiments in all of these targets, while current implementation doesn't really solve the potential problem of cross-process data synchronization. I'm not saying we need to solve for this now, but rather should be aware of that any any potential limitations. Other than this, I see some good discussions in the comments, @SabrinaTardio and @jaceklyp :-) If you would feel like I could help speed up things by jumping on a call to try talking through some concepts brought here with both you, feel free to ping me. Other than that I was thinking to have a look (overview, not detailed) at this (as it's pretty interesting problem!), but once you are closer to actual solution to not add noise to your current flow. |
Just for transparency we had a sync discussion about this and have a TD as a result (https://app.asana.com/0/1204186595873227/1208823859841917/f) |
|
We had a sync discussion about this and have a TD as a result (https://app.asana.com/0/1204186595873227/1208823859841917/f) |
Please review the release process for BrowserServicesKit here.
Required:
Task/Issue URL: https://app.asana.com/0/1204186595873227/1208687542689225/f
iOS PR: duckduckgo/iOS#3582
macOS PR: duckduckgo/macos-browser#3559
What kind of version bump will this require?: Major
Optional:
Tech Design URL: https://app.asana.com/0/481882893211075/1208680035673275/f
CC:
Description: updates stateFor and isSubfeatureEnabled api to manage remote experiments and also adds getActiveExperimentsAPI
Steps to test this PR:
<!—
Before submitting a PR, please ensure you have tested the combinations you expect the reviewer to test, then delete configurations you know do not need explicit testing.
Using a simulator where a physical device is unavailable is acceptable.
—>
OS Testing:
—
Internal references:
Software Engineering Expectations
Technical Design Template