PixelKit adoption #2557

federicocappelli · 2024-04-05T13:23:57Z

Task/Issue URL:https://app.asana.com/0/1205842942115003/1206999268603553/f
Tech Design URL:https://app.asana.com/0/1205842942115003/1206989773504216/f
CC: @diegoreymendez

Description:

90% of this PR is changes from pixel.fire() to PixelKit.fire() with related name and frequency changes. The few logic changes are concentrated in PixelKit.

Old Pixel implementation removed
PixelKit adopted for every Pixel
PixelKit code improved
Debug menu "remove daily pixels" changed in "Remove Pixels Storage" (reset all unique and daily pixels)

Steps to test this PR:

For each area of responsibility as discussed in https://app.asana.com/0/0/1206999268603553/1207035044172102/f:

Review the code with focus on the Pixel's "frequency"
Run the app and try to replicate as much pixels as possible

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo/DataImport/Model/DataImportViewModel.swift # DuckDuckGo/Preferences/View/PreferencesRootView.swift # UnitTests/Statistics/ATB/StatisticsLoaderTests.swift

github-actions · 2024-04-05T13:58:32Z

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

Generated by 🚫 dangerJS against 1c17e9a

Task/Issue URL: https://app.asana.com/0/0/1207002879349167/f **Description**: Migrates the Installation Attribution Pixel to use PixelKit. <img width="1289" alt="Screenshot 2024-04-08 at 4 07 47 PM" src="https://github.com/duckduckgo/macos-browser/assets/1089358/fcaf4fc2-f4b8-4d9e-a9b7-ef80f96cff97"> **Steps to test this PR:** Download the Review App [here](https://github.com/duckduckgo/macos-browser/actions/runs/8595167779/artifacts/1393075355) Make sure to clean up the app's data directory by running sh clean-app.sh review. Run the App. Search for pixel:m.mac.install in Kibana. **Extra:** Create the Origin.txt file manually and add it to the Bundle of the App (.app/Contents/Resources) to test that the pixel fires and has the origin field set. This requires to re-sign the App again in order to run it. Steps to re-sign the App [here](https://github.com/duckduckgo/macos-browser/blob/988a3ef69aaf3c69fa2b56b274de5fa113d73809/.github/workflows/create_variants.yml#L142) In terminal: codesign -d --entitlements :- DuckDuckGo.app > entitlements.plist Copy result of security find-certificate -a -c "Developer ID Application" -Z | grep ^SHA-1 | cut -d " " -f3 | uniq Code sign the App using the below: --force \ --sign <paste value generated from #2 here> \ --options runtime \ --entitlements entitlements.plist \ --generate-entitlement-der “DuckDuckGo.app” Once the App is re-signed you need to open it by right-click on the icon and select Open from the context menu otherwise you won’t be able to open it.

…Storage" debug menu

# Conflicts: # DuckDuckGo/Application/AppDelegate.swift # DuckDuckGo/MainWindow/MainViewController.swift # DuckDuckGo/NavigationBar/View/MoreOptionsMenu.swift # DuckDuckGo/NavigationBar/View/NavigationBarViewController.swift # DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionNavBarButtonModel.swift # DuckDuckGo/Waitlist/Waitlist.swift # DuckDuckGo/Waitlist/WaitlistTermsAndConditionsActionHandler.swift

federicocappelli · 2024-04-08T14:02:00Z

DuckDuckGo/Application/AppDelegate.swift

@@ -97,6 +97,15 @@ final class AppDelegate: NSObject, NSApplicationDelegate {
    var updateController: UpdateController!
 #endif

+    static private var aMonthAgo = Calendar.current.date(byAdding: .month, value: -1, to: Date())! // Temporary for init


Code moved from Pixel.swift to AppDelegate, I'm sure this isn't the right place or approach but it's comparable to the previous one.

we have Date.monthAgo in BSK.Common.DateExtensions, worths removing this

Much better, thanks, fixed

DuckDuckGo/Common/Localizables/UserText.swift

…cos-browser into fcappelli/PixelKit

federicocappelli · 2024-04-08T14:36:21Z

UnitTests/Statistics/PixelExperiment/PixelExperimentTests.swift

 @testable import DuckDuckGo_Privacy_Browser

+/*
+ WARNING: This component doesn't work anymore, re-implementation needed
+ https://app.asana.com/0/0/1207002879349166/f


There's no active experiments in macOS atm, the re-implementation is scheduled as part of this refactoring.

# Conflicts: # DuckDuckGo/HomePage/Model/HomePageContinueSetUpModel.swift # DuckDuckGo/NavigationBar/View/MoreOptionsMenu.swift # DuckDuckGo/NavigationBar/View/NavigationBarViewController.swift # DuckDuckGo/Tab/Model/Tab.swift

federicocappelli · 2024-04-09T09:22:48Z

DuckDuckGo/PrivacyDashboard/View/PrivacyDashboardViewController.swift

@@ -22,6 +22,7 @@ import Combine
 import BrowserServicesKit
 import PrivacyDashboard
 import Common
+import PixelKit


Reviewed and tested by me

# Conflicts: # DuckDuckGo/Tab/Model/Tab.swift

# Conflicts: # DuckDuckGo/DBP/DBPHomeViewController.swift # DuckDuckGo/History/Services/EncryptedHistoryStore.swift # DuckDuckGo/NetworkProtection/NetworkExtensionTargets/NetworkExtensionTargets/MacPacketTunnelProvider.swift # LocalPackages/PixelKit/Sources/PixelKit/PixelKit.swift # LocalPackages/PixelKit/Tests/PixelKitTests/PixelKitTests.swift

DuckDuckGo.xcodeproj/xcshareddata/xcschemes/sandbox-test-tool.xcscheme

aataraxiaa

Narrow +1 on the /PasswordManager code, which LGTM 🚀. However, I left a comment on an unrelated part of the code.

DuckDuckGo/Application/AppDelegate.swift

diegoreymendez

Added some more comments, will be testing meanwhile.

LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/XCTestCase+PixelKit.swift

DuckDuckGo/DBP/DataBrokerProtectionDebugMenu.swift

DuckDuckGo/MainWindow/MainView.swift

DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugMenu.swift

DuckDuckGo/Preferences/View/PreferencesRootView.swift

mallexxx · 2024-04-17T12:16:07Z

DuckDuckGo/Statistics/Experiment/PixelExperiment.swift

@@ -16,7 +16,13 @@
 //  limitations under the License.
 //

+/*
+ WARNING: This component doesn't work anymore, re-implementation needed


why not to drop it then?

The reason is that the overall implementation is probably correct but it will need to be migrated to PixelKit, I wanted to make the re-implementation easier avoiding the need to recover the files. Anyway, I added all the info in https://app.asana.com/0/0/1207002879349166/f so I have no preferences, I can remove it if needed.

mallexxx · 2024-04-17T12:16:39Z

DuckDuckGo/Statistics/GeneralPixel.swift

        case .protectionToggledOffBreakageReport: return "m_mac_protection-toggled-off-breakage-report"
        case .toggleProtectionsDailyCount: return "m_mac_toggle-protections-daily-count"
        case .toggleReportDoNotSend: return "m_mac_toggle-report-do-not-send"
        case .toggleReportDismiss: return "m_mac_toggle-report-dismiss"

-        // Password Import Keychain Prompt
+            // Password Import Keychain Prompt


indentation

that's apparently the correct indentation in the context, all other comments are the same and Xcode re-intend it like that

mallexxx · 2024-04-17T12:17:44Z

DuckDuckGo/Statistics/GeneralPixel.swift

-        case .bitwardenRespondedCannotDecryptUnique:
-            return "bitwarden_responded_cannot_decrypt_unique"
+        case .bitwardenRespondedCannotDecrypt:
+            return "bitwarden_responded_cannot_decrypt_d"


just making sure, is this change reflected in grafana?

yes! https://grafana.duckduckgo.com/d/HVWVaOh7k/macos-errors?orgId=1&from=now-6M&to=now&viewPanel=38&editPanel=38 (at the bottom)

mallexxx

The changes look good to me, see minor styling comments inline;
One note: as I remember we were going to drop pixel CoreData storage in favor of User Defaults storage, and here it‘s renamed…

# Conflicts: # DuckDuckGo/Tab/Model/Tab.swift

diegoreymendez

Approved.

@diegoreymendez

Task/Issue URL:https://app.asana.com/0/1205842942115003/1206999268603553/f Tech Design URL:https://app.asana.com/0/1205842942115003/1206989773504216/f CC: @diegoreymendez **Description**: Migrating the entire app to PixelKit changing pixel.fire() to PixelKit.fire() with related name and frequency changes. The few logic changes are concentrated in PixelKit. - Old Pixel implementation removed - PixelKit adopted for every Pixel - PixelKit code improved - Debug menu "remove daily pixels" changed in "Remove Pixels Storage" (reset all unique and daily pixels)

Pixels fired for email autofill use-cases are intentionally not prefixed with `m.mac.`, though the historical reason isn't clear to me. The migration to PixelKit for pixel management[^1] caused those email-related pixels to be prefixed. Wrap email-related JS pixels in `NonStandardEvent`s to prevent that prefixing, to restore their original intended names. [^1]: d885fd0 (PixelKit adoption (#2557), 2024-04-17)

Task/Issue URL: https://app.asana.com/0/1206682621538333/1207380134103391/f Tech Design URL: N/A Pixels fired for email autofill use-cases are intentionally not prefixed with `m.mac.`, though the historical reason isn't clear to me. The migration to PixelKit for pixel management[^1] caused those email-related pixels to be prefixed. Wrap email-related JS pixels in `NonStandardEvent`s to prevent that prefixing, to restore their original intended names. [^1]: d885fd0 (PixelKit adoption (#2557), 2024-04-17)

federicocappelli added 9 commits April 4, 2024 13:41

DailyPixels moved to PixelKit

2490e15

many more pixels migrated to PixelKit

a08d287

all pixels migrated to PixelKit

227c2b5

more pixels migrated and unit tests now build

afbb6a6

Unit tests fixed and PixelExperiment removed

c2b6453

lint and comments added

6eb1b98

PixelKit API cleanup

a8a62ac

PixelKit safety checks

04de217

Merge branch 'main' into fcappelli/PixelKit

2262709

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo/DataImport/Model/DataImportViewModel.swift # DuckDuckGo/Preferences/View/PreferencesRootView.swift # UnitTests/Statistics/ATB/StatisticsLoaderTests.swift

alessandroboron and others added 3 commits April 8, 2024 11:07

clearFrequencyHistoryForAllPixels implemented with new "reset Pixels …

54c0cf0

…Storage" debug menu

federicocappelli commented Apr 8, 2024

View reviewed changes

Update sandbox-test-tool.xcscheme

1e15a9b

federicocappelli commented Apr 8, 2024

View reviewed changes

DuckDuckGo/Common/Localizables/UserText.swift Show resolved Hide resolved

federicocappelli added 2 commits April 8, 2024 15:33

reverting unrelated changes

069183f

Merge branch 'fcappelli/PixelKit' of https://github.com/duckduckgo/ma…

e6a73d6

…cos-browser into fcappelli/PixelKit

federicocappelli commented Apr 8, 2024

View reviewed changes

federicocappelli added 4 commits April 8, 2024 15:57

pixel logs fired even in dry mode

a452537

.legacyDaily pixel frequency added for support daily

746a29d

PixelKit Unit tests fixed

6de49c3

Merge branch 'main' into fcappelli/PixelKit

3e8a24c

# Conflicts: # DuckDuckGo/HomePage/Model/HomePageContinueSetUpModel.swift # DuckDuckGo/NavigationBar/View/MoreOptionsMenu.swift # DuckDuckGo/NavigationBar/View/NavigationBarViewController.swift # DuckDuckGo/Tab/Model/Tab.swift

federicocappelli commented Apr 9, 2024

View reviewed changes

federicocappelli marked this pull request as ready for review April 9, 2024 10:07

federicocappelli added 3 commits April 9, 2024 12:01

Merge branch 'main' into fcappelli/PixelKit

245c603

# Conflicts: # DuckDuckGo/Tab/Model/Tab.swift

unit tests fix

88d0f27

federicocappelli commented Apr 9, 2024

View reviewed changes

DuckDuckGo.xcodeproj/xcshareddata/xcschemes/sandbox-test-tool.xcscheme Outdated Show resolved Hide resolved

aataraxiaa approved these changes Apr 9, 2024

View reviewed changes

DuckDuckGo/Application/AppDelegate.swift Show resolved Hide resolved

federicocappelli added 3 commits April 17, 2024 12:32

Merge branch 'main' into fcappelli/PixelKit

e1c789f

Date code duplication removed

6ff3e5e

Legacy Pixel class and related files removed

aa7adb4

diegoreymendez reviewed Apr 17, 2024

View reviewed changes

LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/XCTestCase+PixelKit.swift Show resolved Hide resolved