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

usage segmentation #3263

Merged
merged 45 commits into from
Sep 3, 2024
Merged

usage segmentation #3263

merged 45 commits into from
Sep 3, 2024

Conversation

brindy
Copy link
Contributor

@brindy brindy commented Aug 21, 2024

Task/Issue URL: https://app.asana.com/0/392891325557410/1206955287217877/f
Tech Design URL: https://app.asana.com/0/392891325557410/1208090005627335/f
CC:

Description:
This PR ads usage segmentation calculations to the app so that we can monitor usage across segments in a private way, without having to send granular usage information to the server.

Deviation from tech design:

  • Added "Calculation" class / protocol in order to encapsulate the calculation logic exclusively and code it so that it's similar to the python implementation.

Steps to test this PR:

First run, new user:

  1. Reset the simulator (so that the keychain is cleared)
  2. Run the app
  3. No m_retention_segments pixel should be fired

Subsequent run a few days later

  1. Reset the simulator (so that the keychain is cleared)

  2. Modify StatisticsLoader add the following line at the start of the load function:

         self.statisticsStore.atb = "445-1"
    
  3. Run the app

  4. An assertion failure should crash the app saying this is not a valid ATB.

  5. Update the above line as follows and reset the simulator again:

         self.statisticsStore.atb = "v445-1"
    
  6. Ensure that m_retention_segments pixel is fired. There will be various parameters but one should indicate "app_use" activity

  7. Re-launch the app

  8. No additional pixel should be fired

  9. Perform a search

  10. The pixel will be fired again and there should be a parameter indicating search activity.

  11. Perform another search

  12. No additional pixel should be fired

Return user test

  1. Repeat the above test bit with the following atb. The pixels sent should include reinstaller in the segments parameter.

         self.statisticsStore.atb = "v445-1ru"
    

Definition of Done (Internal Only):

Copy link

github-actions bot commented Aug 21, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against ad4bda3

@brindy brindy changed the title retention segmentation usage segmentation Aug 22, 2024
@brindy brindy marked this pull request as ready for review September 2, 2024 16:55
@brindy brindy requested a review from aataraxiaa September 2, 2024 16:55
Copy link
Contributor

@aataraxiaa aataraxiaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good @brindy , left some comments. Thanks for the useful documentation - it really helped in understanding, particularly for UsageSegmentationCalculator.

Also, tested and validated. One unit test is failing but I believe it’s unrelated.

private extension String {

func substring(_ range: ClosedRange<Int>) -> String {
let startIndex = self.index(self.startIndex, offsetBy: range.lowerBound)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we account for out-of-bounds here with something like:

guard range.lowerBound >= 0, 
              range.upperBound < self.count else {
     return nil
}   

and then return an Optional String.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably using min(self.count, range.upperBound) is better and more like you'd expect from a substring function. Annoying swift just doesn't provide these tbh

Core/StatisticsLoader.swift Outdated Show resolved Hide resolved
Core/UsageSegmentation.swift Outdated Show resolved Hide resolved
Core/UsageSegmentation.swift Outdated Show resolved Hide resolved
Core/UsageSegmentation.swift Show resolved Hide resolved
}

/// This implementation is based on https://dub.duckduckgo.com/flawrence/felix-jupyter-modules/blob/master/segments_reference.py and has been written to try and
/// resemble the original code as closely as possible.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

Core/UsageSegmentationCalculator.swift Show resolved Hide resolved
Core/UsageSegmentationCalculator.swift Outdated Show resolved Hide resolved
Core/UsageSegmentationCalculator.swift Outdated Show resolved Hide resolved
@brindy brindy requested a review from aataraxiaa September 3, 2024 11:09
@@ -115,7 +115,7 @@ private extension String {

func substring(_ range: ClosedRange<Int>) -> String {
let startIndex = self.index(self.startIndex, offsetBy: range.lowerBound)
let endIndex = self.index(self.startIndex, offsetBy: range.upperBound + 1)
let endIndex = self.index(self.startIndex, offsetBy: min(self.count, range.upperBound + 1))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

@aataraxiaa
Copy link
Contributor

LGTM @brindy . Still a unit test failing - SyncManagementViewModelTests.testWhenSaveRecoveryPDFPressed_recoveryMethodShown(), which I assume is unrelated.

@brindy brindy merged commit 933e11e into main Sep 3, 2024
13 checks passed
@brindy brindy deleted the brindy/retention-segmentation branch September 3, 2024 12:19
brindy added a commit that referenced this pull request Sep 4, 2024
Task/Issue URL:
https://app.asana.com/0/392891325557410/1208213671229670/f
Tech Design URL:
CC:

**Description**:
Do not send any default parameters with this pixel. 

**Steps to test this PR**:
1. Run the app and get the usage segmentation pixel to fire, see #3263 
2. Ensure that pixel does not include the atb or app version parameters

**Definition of Done (Internal Only)**:

* [ ] Does this PR satisfy our [Definition of
Done](https://app.asana.com/0/1202500774821704/1207634633537039/f)?
samsymons added a commit that referenced this pull request Sep 9, 2024
# By Christopher Brind (4) and others
# Via Alessandro Boron (1) and others
* main: (27 commits)
  Bump C-S-S to 6.14.1 (#3331)
  DuckPlayer Launch Experiment for iOS (#3328)
  defer loading the tab switcher button until view did load (#3326)
  Release 7.136.0-3 (#3324)
  Release PR: Check for the negative attribution case (#3311)
  fix tab switcher crashes (speculative fix) (#3319)
  Onboarding highlights feature flag setup (#3308)
  Release 7.136.0-2 (#3320)
  Attempt to fix dissapearing privacy icon (#3317)
  Fix bookmarks toolbar behaviour with Sync Promo on iOS 15 (#3313)
  Bump BSK with C-S-S to 6.14.0 (#3314)
  ensure no atb or app version sent with pixel (#3315)
  Fix #3298: Add support for Xcode 16 (#3299)
  Fix Keychain Debug view controller segue (#3310)
  Release 7.136.0-1 (#3309)
  Fix an issue that causes the fire dialog to show multiple times for the same website after dismissing it (#3305)
  [DuckPlayer] 28. Open in Youtube -> Youtube App (#3290)
  usage segmentation (#3263)
  Fix wrong URL displayed for auth dialog (#3307)
  iOS Integration of BSK Onboarding (#3282)
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
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

Successfully merging this pull request may close these issues.

2 participants