Skip to content

Commit

Permalink
Remove data inconsistency reporting pixels (#3795)
Browse files Browse the repository at this point in the history
<!--
Note: This checklist is a reminder of our shared engineering
expectations. Feel free to change it, although assigning a GitHub
reviewer and the items in bold are required.

⚠️ If you're an external contributor, please file an issue first before
working on a PR, as we can't guarantee that we will accept your changes
if they haven't been discussed ahead of time. Thanks!
-->

Task/Issue URL:
https://app.asana.com/0/1205278999335242/1208659901469299/f
Tech Design URL:
CC:

**Description**:

Removes temporary pixels used for monitoring UserDefaults data
inconsistencies.

**Steps to test this PR**:
1. Compare with original state of
#3510

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

* [ ] Does this PR satisfy our [Definition of
Done](https://app.asana.com/0/1202500774821704/1207634633537039/f)?

**Copy Testing**:

* [ ] Use of correct apostrophes in new copy, ie `’` rather than `'`

**Orientation Testing**:

* [ ] Portrait
* [ ] Landscape

**Device Testing**:

* [ ] iPhone SE (1st Gen)
* [ ] iPhone 8
* [ ] iPhone X
* [ ] iPhone 14 Pro
* [ ] iPad

**OS Testing**:

* [ ] iOS 15
* [ ] iOS 16
* [ ] iOS 17

**Theme Testing**:

* [ ] Light theme
* [ ] Dark theme

---
###### Internal references:
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
  • Loading branch information
dus7 authored Jan 15, 2025
1 parent c8fef6e commit f8c1604
Show file tree
Hide file tree
Showing 11 changed files with 6 additions and 166 deletions.
10 changes: 0 additions & 10 deletions Core/PixelEvent.swift
Original file line number Diff line number Diff line change
Expand Up @@ -947,11 +947,6 @@ extension Pixel {
// MARK: WebView Error Page Shown
case webViewErrorPageShown

// MARK: UserDefaults incositency monitoring
case protectedDataUnavailableWhenBecomeActive
case statisticsLoaderATBStateMismatch
case adAttributionReportStateMismatch

// MARK: Browsing
case stopPageLoad

Expand Down Expand Up @@ -1919,11 +1914,6 @@ extension Pixel.Event {
// MARK: - DuckPlayer FE Application Telemetry
case .duckPlayerLandscapeLayoutImpressions: return "duckplayer_landscape_layout_impressions"

// MARK: UserDefaults incositency monitoring
case .protectedDataUnavailableWhenBecomeActive: return "m_protected_data_unavailable_when_become_active"
case .statisticsLoaderATBStateMismatch: return "m_statistics_loader_atb_state_mismatch"
case .adAttributionReportStateMismatch: return "m_ad_attribution_report_state_mismatch"

// MARK: Browsing
case .stopPageLoad: return "m_stop-page-load"

Expand Down
19 changes: 1 addition & 18 deletions Core/StatisticsLoader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,38 +35,26 @@ public class StatisticsLoader {
private let returnUserMeasurement: ReturnUserMeasurement
private let usageSegmentation: UsageSegmenting
private let parser = AtbParser()
private let atbPresenceFileMarker = BoolFileMarker(name: .isATBPresent)
private let inconsistencyMonitoring: StatisticsStoreInconsistencyMonitoring
private let fireSearchExperimentPixels: () -> Void
private let fireAppRetentionExperimentPixels: () -> Void
private let pixelFiring: PixelFiring.Type

init(statisticsStore: StatisticsStore = StatisticsUserDefaults(),
returnUserMeasurement: ReturnUserMeasurement = KeychainReturnUserMeasurement(),
usageSegmentation: UsageSegmenting = UsageSegmentation(),
inconsistencyMonitoring: StatisticsStoreInconsistencyMonitoring = StorageInconsistencyMonitor(),
fireAppRetentionExperimentPixels: @escaping () -> Void = PixelKit.fireAppRetentionExperimentPixels,
fireSearchExperimentPixels: @escaping () -> Void = PixelKit.fireSearchExperimentPixels,
pixelFiring: PixelFiring.Type = Pixel.self) {
self.statisticsStore = statisticsStore
self.returnUserMeasurement = returnUserMeasurement
self.usageSegmentation = usageSegmentation
self.inconsistencyMonitoring = inconsistencyMonitoring
self.fireSearchExperimentPixels = fireSearchExperimentPixels
self.fireAppRetentionExperimentPixels = fireAppRetentionExperimentPixels
self.pixelFiring = pixelFiring
}

public func load(completion: @escaping Completion = {}) {
let hasFileMarker = atbPresenceFileMarker?.isPresent ?? false
let hasInstallStatistics = statisticsStore.hasInstallStatistics

inconsistencyMonitoring.statisticsDidLoad(hasFileMarker: hasFileMarker, hasInstallStatistics: hasInstallStatistics)

if hasInstallStatistics {
// Synchronize file marker with current state
createATBFileMarker()

if statisticsStore.hasInstallStatistics {
completion()
return
}
Expand Down Expand Up @@ -109,7 +97,6 @@ public class StatisticsLoader {
self.statisticsStore.installDate = Date()
self.statisticsStore.atb = atb.version
self.returnUserMeasurement.installCompletedWithATB(atb)
self.createATBFileMarker()
completion()
}
}
Expand All @@ -128,10 +115,6 @@ public class StatisticsLoader {
})
}

private func createATBFileMarker() {
atbPresenceFileMarker?.mark()
}

public func refreshSearchRetentionAtb(completion: @escaping Completion = {}) {
fireSearchExperimentPixels()
guard let url = StatisticsDependentURLFactory(statisticsStore: statisticsStore).makeSearchAtbURL() else {
Expand Down
66 changes: 0 additions & 66 deletions Core/StorageInconsistencyMonitor.swift

This file was deleted.

6 changes: 1 addition & 5 deletions DuckDuckGo.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,6 @@
6F03CB052C32EFCC004179A8 /* MockPixelFiring.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6F03CB032C32EFA8004179A8 /* MockPixelFiring.swift */; };
6F03CB072C32F173004179A8 /* PixelFiring.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6F03CB062C32F173004179A8 /* PixelFiring.swift */; };
6F03CB092C32F331004179A8 /* PixelFiringAsync.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6F03CB082C32F331004179A8 /* PixelFiringAsync.swift */; };
6F04224D2CD2A3AD00729FA6 /* StorageInconsistencyMonitor.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6F98573F2CD2933B001BE9A0 /* StorageInconsistencyMonitor.swift */; };
6F0FEF6B2C516D540090CDE4 /* NewTabPageSettingsStorage.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6F0FEF6A2C516D540090CDE4 /* NewTabPageSettingsStorage.swift */; };
6F0FEF6D2C52639E0090CDE4 /* ReorderableForEach.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6F0FEF6C2C52639E0090CDE4 /* ReorderableForEach.swift */; };
6F3529FF2CDCEDFF00A59170 /* OmniBarLoadingStateBearerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6F3529FE2CDCEDF700A59170 /* OmniBarLoadingStateBearerTests.swift */; };
Expand Down Expand Up @@ -1741,7 +1740,6 @@
6F934F852C58DB00008364E4 /* NewTabPageSettingsPersistentStorageTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NewTabPageSettingsPersistentStorageTests.swift; sourceTree = "<group>"; };
6F96FF0F2C2B128500162692 /* NewTabPageCustomizeButtonView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NewTabPageCustomizeButtonView.swift; sourceTree = "<group>"; };
6F9857332CD27F98001BE9A0 /* BoolFileMarker.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BoolFileMarker.swift; sourceTree = "<group>"; };
6F98573F2CD2933B001BE9A0 /* StorageInconsistencyMonitor.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StorageInconsistencyMonitor.swift; sourceTree = "<group>"; };
6F9FFE252C579BCD00A238BE /* NewTabPageShortcutsSettingsStorage.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NewTabPageShortcutsSettingsStorage.swift; sourceTree = "<group>"; };
6F9FFE272C579DEA00A238BE /* NewTabPageSectionsSettingsStorage.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NewTabPageSectionsSettingsStorage.swift; sourceTree = "<group>"; };
6F9FFE292C57ADB100A238BE /* EditableShortcutsView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EditableShortcutsView.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -6390,9 +6388,7 @@
F143C3191E4A99DD00CFDE3A /* Utilities */ = {
isa = PBXGroup;
children = (
6F98573F2CD2933B001BE9A0 /* StorageInconsistencyMonitor.swift */,
6F9857332CD27F98001BE9A0 /* BoolFileMarker.swift */,
6F395BB92CD2C84300B92FC3 /* BoolFileMarkerTests.swift */,
9FCFCD7D2C6AF52A006EB7A0 /* LaunchOptionsHandler.swift */,
B603974829C19F6F00902A34 /* Assertions.swift */,
CBAA195927BFE15600A4BD49 /* NSManagedObjectContextExtension.swift */,
Expand Down Expand Up @@ -6572,6 +6568,7 @@
1CB7B82223CEA28300AA24EA /* DateExtensionTests.swift */,
4BC21A2C272388BD00229F0E /* RunLoopExtensionTests.swift */,
9F16230A2CA0F0190093C4FC /* DebouncerTests.swift */,
6F395BB92CD2C84300B92FC3 /* BoolFileMarkerTests.swift */,
);
name = Utilities;
sourceTree = "<group>";
Expand Down Expand Up @@ -8949,7 +8946,6 @@
37CEFCAC2A673B90001EF741 /* CredentialsCleanupErrorHandling.swift in Sources */,
CB2A7EF128410DF700885F67 /* PixelEvent.swift in Sources */,
98B000532915C46E0034BCA0 /* LegacyBookmarksStoreMigration.swift in Sources */,
6F04224D2CD2A3AD00729FA6 /* StorageInconsistencyMonitor.swift in Sources */,
85200FA11FBC5BB5001AF290 /* DDGPersistenceContainer.swift in Sources */,
9FEA22322C3270BD006B03BF /* TimerInterface.swift in Sources */,
1E4DCF4C27B6A4CB00961E25 /* URLFileExtension.swift in Sources */,
Expand Down
34 changes: 2 additions & 32 deletions DuckDuckGo/AdAttribution/AdAttributionPixelReporter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,9 @@ final actor AdAttributionPixelReporter {
private let pixelFiring: PixelFiringAsync.Type
private var isSendingAttribution: Bool = false

private let inconsistencyMonitoring: AdAttributionReporterInconsistencyMonitoring
private let attributionReportSuccessfulFileMarker = BoolFileMarker(name: .isAttrbutionReportSuccessful)

private var shouldReport: Bool {
get async {
if let attributionReportSuccessfulFileMarker {
// If marker is present then report only if data consistent
return await !fetcherStorage.wasAttributionReportSuccessful && !attributionReportSuccessfulFileMarker.isPresent
} else {
return await fetcherStorage.wasAttributionReportSuccessful
}
return await !fetcherStorage.wasAttributionReportSuccessful
}
}

Expand All @@ -52,15 +44,13 @@ final actor AdAttributionPixelReporter {
featureFlagger: FeatureFlagger = AppDependencyProvider.shared.featureFlagger,
privacyConfigurationManager: PrivacyConfigurationManaging = ContentBlocking.shared.privacyConfigurationManager,
variantManager: VariantManager = AppDependencyProvider.shared.variantManager,
pixelFiring: PixelFiringAsync.Type = Pixel.self,
inconsistencyMonitoring: AdAttributionReporterInconsistencyMonitoring = StorageInconsistencyMonitor()) {
pixelFiring: PixelFiringAsync.Type = Pixel.self) {
self.fetcherStorage = fetcherStorage
self.attributionFetcher = attributionFetcher
self.featureFlagger = featureFlagger
self.privacyConfigurationManager = privacyConfigurationManager
self.variantManager = variantManager
self.pixelFiring = pixelFiring
self.inconsistencyMonitoring = inconsistencyMonitoring
}

@discardableResult
Expand All @@ -69,8 +59,6 @@ final actor AdAttributionPixelReporter {
return false
}

await checkStorageConsistency()

guard await shouldReport else {
return false
}
Expand Down Expand Up @@ -112,24 +100,6 @@ final actor AdAttributionPixelReporter {

private func markAttributionReportSuccessful() async {
await fetcherStorage.markAttributionReportSuccessful()
attributionReportSuccessfulFileMarker?.mark()
}

private func checkStorageConsistency() async {

guard let attributionReportSuccessfulFileMarker else { return }

let wasAttributionReportSuccessful = await fetcherStorage.wasAttributionReportSuccessful

inconsistencyMonitoring.addAttributionReporter(
hasFileMarker: attributionReportSuccessfulFileMarker.isPresent,
hasCompletedFlag: wasAttributionReportSuccessful
)

// Synchronize file marker with current state (in case we have updated from previous version)
if wasAttributionReportSuccessful && !attributionReportSuccessfulFileMarker.isPresent {
attributionReportSuccessfulFileMarker.mark()
}
}

private func pixelParametersForAttribution(_ attribution: AdServicesAttributionResponse, isReinstall: Bool, attributionToken: String?) -> [String: String] {
Expand Down
1 change: 0 additions & 1 deletion DuckDuckGo/AppLifecycle/AppStates/Active.swift
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ struct Active: AppState {

// MARK: handle applicationDidBecomeActive(_:) logic here
private func activateApp(isTesting: Bool = false) {
StorageInconsistencyMonitor().didBecomeActive(isProtectedDataAvailable: application.isProtectedDataAvailable)
appDependencies.syncService.initializeIfNeeded()
appDependencies.syncDataProviders.setUpDatabaseCleanersIfNeeded(syncService: appDependencies.syncService)

Expand Down
1 change: 0 additions & 1 deletion DuckDuckGo/OldAppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,6 @@ final class OldAppDelegate: NSObject, UIApplicationDelegate, DDGApp {
}
}

StorageInconsistencyMonitor().didBecomeActive(isProtectedDataAvailable: application.isProtectedDataAvailable)
syncService.initializeIfNeeded()
syncDataProviders.setUpDatabaseCleanersIfNeeded(syncService: syncService)

Expand Down
20 changes: 1 addition & 19 deletions DuckDuckGoTests/AdAttributionPixelReporterTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -62,17 +62,6 @@ final class AdAttributionPixelReporterTests: XCTestCase {
XCTAssertTrue(result)
}

func testDoesNotReportIfOnlyFileMarkerIsPresent() async throws {
let sut = createSUT()
fileMarker.mark()
attributionFetcher.fetchResponse = ("example", AdServicesAttributionResponse(attribution: true))

let result = await sut.reportAttributionIfNeeded()

XCTAssertNil(PixelFiringMock.lastPixelName)
XCTAssertFalse(result)
}

func testReportsOnce() async {
let sut = createSUT()
attributionFetcher.fetchResponse = ("example", AdServicesAttributionResponse(attribution: true))
Expand Down Expand Up @@ -241,8 +230,7 @@ final class AdAttributionPixelReporterTests: XCTestCase {
featureFlagger: featureFlagger,
privacyConfigurationManager: privacyConfigurationManager,
variantManager: MockVariantManager(isSupportedReturns: false, currentVariant: variant),
pixelFiring: PixelFiringMock.self,
inconsistencyMonitoring: MockAdAttributionReporterInconsistencyMonitoring())
pixelFiring: PixelFiringMock.self)
}
}

Expand All @@ -264,12 +252,6 @@ private class AdAttributionFetcherMock: AdAttributionFetcher {
}
}

private struct MockAdAttributionReporterInconsistencyMonitoring: AdAttributionReporterInconsistencyMonitoring {
func addAttributionReporter(hasFileMarker: Bool, hasCompletedFlag: Bool) {

}
}

private extension AdServicesAttributionResponse {
init(attribution: Bool) {
self.init(
Expand Down
File renamed without changes.
7 changes: 0 additions & 7 deletions DuckDuckGoTests/StatisticsLoaderTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ class StatisticsLoaderTests: XCTestCase {
mockUsageSegmentation = MockUsageSegmentation()
testee = StatisticsLoader(statisticsStore: mockStatisticsStore,
usageSegmentation: mockUsageSegmentation,
inconsistencyMonitoring: MockStatisticsStoreInconsistencyMonitoring(),
fireAppRetentionExperimentPixels: { self.fireAppRetentionExperimentPixelsCalled = true },
fireSearchExperimentPixels: { self.fireSearchExperimentPixelsCalled = true },
pixelFiring: mockPixelFiring)
Expand Down Expand Up @@ -337,9 +336,3 @@ class StatisticsLoaderTests: XCTestCase {
}

}

private struct MockStatisticsStoreInconsistencyMonitoring: StatisticsStoreInconsistencyMonitoring {
func statisticsDidLoad(hasFileMarker: Bool, hasInstallStatistics: Bool) {

}
}
8 changes: 1 addition & 7 deletions IntegrationTests/AtbServerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class AtbServerTests: XCTestCase {
super.setUp()
PixelKit.configureExperimentKit(featureFlagger: MockFeatureFlagger())
store = MockStatisticsStore()
loader = StatisticsLoader(statisticsStore: store, inconsistencyMonitoring: MockStatisticsStoreInconsistencyMonitoring())
loader = StatisticsLoader(statisticsStore: store)

}

Expand Down Expand Up @@ -133,12 +133,6 @@ class MockStatisticsStore: StatisticsStore {
var variant: String?
}

private struct MockStatisticsStoreInconsistencyMonitoring: StatisticsStoreInconsistencyMonitoring {
func statisticsDidLoad(hasFileMarker: Bool, hasInstallStatistics: Bool) {

}
}

class MockFeatureFlagger: FeatureFlagger {
func isFeatureOn<Flag>(for featureFlag: Flag, allowOverride: Bool) -> Bool where Flag: FeatureFlagDescribing {
return false
Expand Down

0 comments on commit f8c1604

Please sign in to comment.