Improves underlying error pixel information #2543

diegoreymendez · 2024-04-04T11:10:04Z

Task/Issue URL: https://app.asana.com/0/0/1206999233882390/f

Description

Some errors have more than 1 level of underlying error information. I've added code to handle this specifically in a way that we support including multiple levels of underlying errors, to get more detailed information.

Additionally I'm doing some cleanup work to remove unnecessary code and start reducing debt.

Testing

Make sure CI is green - I've added a unit test to report multiple levels of underlying error info.

Internal references:

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

diegoreymendez · 2024-04-04T11:10:41Z

...orkProtectionMac/Sources/NetworkProtectionProxy/Pixels/TransparentProxyControllerPixel.swift

@@ -19,15 +19,17 @@
 import Foundation
 import PixelKit

-extension TransparentProxyController.StartError: PixelKitEventErrorDetails {


PixelKitEventErrorDetails was never needed and is now replaced by the more standard CustomNSError.

diegoreymendez · 2024-04-04T11:11:53Z

LocalPackages/PixelKit/Sources/PixelKit/PixelKit+Parameters.swift

-            params[PixelKit.Parameters.underlyingErrorDomain] = underlyingError.domain
+        let underlyingErrorParameters = self.underlyingErrorParameters(for: nsError)
+        params.merge(underlyingErrorParameters) { first, _ in
+            return first


Instead of supporting only 1 level of underlying error info, we now support multiple in order to get all the detail.

diegoreymendez · 2024-04-04T11:12:33Z

LocalPackages/PixelKit/Sources/PixelKit/PixelKit+Parameters.swift

+            let errorCodeParameterName = PixelKit.Parameters.underlyingErrorCode + (level == 0 ? "" : String(level + 1))
+            let errorDomainParameterName = PixelKit.Parameters.underlyingErrorDomain + (level == 0 ? "" : String(level + 1))


First underlying error params are named as usual "ue" and "ud". Follow up underlying errors are named "ue2", "ue3", etc.

diegoreymendez · 2024-04-04T11:13:12Z

LocalPackages/PixelKit/Sources/PixelKit/PixelKit.swift

-        let nsError = error as NSError
-
-        self[PixelKit.Parameters.errorCode] = "\(nsError.code)"
-        self[PixelKit.Parameters.errorDomain] = nsError.domain
-
-        if let error = error as? PixelKitEventErrorDetails,
-           let underlyingError = error.underlyingError {
-
-            let underlyingNSError = underlyingError as NSError
-            self[PixelKit.Parameters.underlyingErrorCode] = "\(underlyingNSError.code)"
-            self[PixelKit.Parameters.underlyingErrorDomain] = underlyingNSError.domain
-        } else if let underlyingError = nsError.userInfo["NSUnderlyingError"] as? NSError {
-            self[PixelKit.Parameters.underlyingErrorCode] = "\(underlyingError.code)"
-            self[PixelKit.Parameters.underlyingErrorDomain] = underlyingError.domain
-        } else if let sqlErrorCode = nsError.userInfo["NSSQLiteErrorDomain"] as? NSNumber {
-            self[PixelKit.Parameters.underlyingErrorCode] = "\(sqlErrorCode.intValue)"
-            self[PixelKit.Parameters.underlyingErrorDomain] = "NSSQLiteErrorDomain"
+        self.merge(error.pixelParameters) { _, second in
+            return second


This was duplicated code, so now I've unified to use error.pixelParameters.

diegoreymendez · 2024-04-04T11:13:39Z

LocalPackages/PixelKit/Sources/PixelKit/PixelKitEventV2.swift

-extension PixelKitEventV2 {
-    var pixelParameters: [String: String] {
-        guard let error else {
-            return [:]
-        }
-
-        let nsError = error as NSError
-        var parameters = [
-            PixelKit.Parameters.errorCode: "\(nsError.code)",
-            PixelKit.Parameters.errorDomain: nsError.domain,
-        ]
-
-        if let error = error as? PixelKitEventErrorDetails {
-            parameters.merge(error.underlyingErrorParameters, uniquingKeysWith: { $1 })
-        }
-
-        return parameters
-    }
-}


This was duplicated code, we're now using error.pixelParameters.

diegoreymendez · 2024-04-04T11:13:57Z

LocalPackages/PixelKit/Sources/PixelKitTestingUtilities/PixelFireExpectations.swift

    var customFields: [String: String]?

    /// Convenience initializer for cleaner semantics
    ///
-    public static func expect(pixelName: String, error: Error? = nil, underlyingError: Error? = nil, customFields: [String: String]? = nil) -> PixelFireExpectations {
+    public static func expect(pixelName: String, error: Error? = nil, underlyingErrors: [Error] = [], customFields: [String: String]? = nil) -> PixelFireExpectations {


Changing this test utility to support multiple levels of underlying errors.

diegoreymendez · 2024-04-04T11:18:49Z

UnitTests/NetworkProtection/NetworkProtectionPixelEventTests.swift

@@ -78,7 +78,7 @@ final class NetworkProtectionPixelEventTests: XCTestCase {
        fire(NetworkProtectionPixelEvent.networkProtectionControllerStartFailure(TestError.testError),
             and: .expect(pixelName: "m_mac_netp_controller_start_failure",
                          error: TestError.testError,
-                          underlyingError: TestError.underlyingError),
+                          underlyingErrors: [TestError.underlyingError]),


Just adapting existing tests to the new testing utility changes.

samsymons

Really nice changes here, LGTM!

diegoreymendez · 2024-04-08T07:57:23Z

Thank you, sir!

* main: (22 commits) Removes last instance of NETWORK_PROTECTION flag (#2573) Bye bye NETWORK_PROTECTION (#2509) QWD: Enable Hide/Show for Autofill Credit Card Number and CVV (#2539) Removed the VPN waitlist beta pixels (#2555) VPN: Cleanup authorize call (#2565) Revert "VPN: Cleanup authorize call (#2553)" VPN: Cleanup authorize call (#2553) Improves underlying error pixel information (#2543) Fixes the VPN restarting logic on update (#2545) fix download save panel disappearing on navigation (#2549) Add CI support for handling installation attribution (#2502) Fix usertext comment to ensure it matches localizable string (#2546) Update Neighbor Report (#2542) DBP: Compare by url and not name (#2544) Adds a series of UI tests for Bookmarks Bar visibility Improve Handling of noData Import Errors (#2494) Use the default action button style for VPN onboarding (#2529) Closing empty tabs after download (#2510) Add Web UI loading state pixels (#2531) fix localization warnings (#2288) ...

…flagger # By Diego Rey Mendez (7) and others # Via GitHub * main: Automatically close VPN popover when the app goes into the background (#2562) Fix last known VPN crash and missing IPC registration (#2579) Create a new window when making a feedback form if necessary (#2563) Improve VPN uninstallation reliability (#2560) Removes last instance of NETWORK_PROTECTION flag (#2573) Bye bye NETWORK_PROTECTION (#2509) QWD: Enable Hide/Show for Autofill Credit Card Number and CVV (#2539) Removed the VPN waitlist beta pixels (#2555) VPN: Cleanup authorize call (#2565) Revert "VPN: Cleanup authorize call (#2553)" VPN: Cleanup authorize call (#2553) Improves underlying error pixel information (#2543) # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

Improves underlying error pixel information

a586fa5

diegoreymendez self-assigned this Apr 4, 2024

github-actions bot added the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Apr 4, 2024

diegoreymendez commented Apr 4, 2024

View reviewed changes

Removes some unnecessary code

1c7b29e

diegoreymendez commented Apr 4, 2024

View reviewed changes

Removes some unnecessary code

02ab234

diegoreymendez commented Apr 4, 2024

View reviewed changes

Adds some documentation

dec1ff4

diegoreymendez marked this pull request as ready for review April 4, 2024 11:22

Fixes swiftlint warnings

e96a764

diegoreymendez removed the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Apr 4, 2024

diegoreymendez requested a review from samsymons April 4, 2024 11:24

Swiftlint fixes

a683c8a

samsymons approved these changes Apr 7, 2024

View reviewed changes

diegoreymendez merged commit 553d3ee into main Apr 8, 2024
17 checks passed

diegoreymendez deleted the diego/multilevel-underlying-error-information branch April 8, 2024 07:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improves underlying error pixel information #2543

Improves underlying error pixel information #2543

diegoreymendez commented Apr 4, 2024

diegoreymendez Apr 4, 2024

diegoreymendez Apr 4, 2024

diegoreymendez Apr 4, 2024

samsymons Apr 7, 2024

diegoreymendez Apr 4, 2024

diegoreymendez Apr 4, 2024

diegoreymendez Apr 4, 2024

diegoreymendez Apr 4, 2024

samsymons left a comment

diegoreymendez commented Apr 8, 2024

		let errorCodeParameterName = PixelKit.Parameters.underlyingErrorCode + (level == 0 ? "" : String(level + 1))
		let errorDomainParameterName = PixelKit.Parameters.underlyingErrorDomain + (level == 0 ? "" : String(level + 1))

Improves underlying error pixel information #2543

Improves underlying error pixel information #2543

Conversation

diegoreymendez commented Apr 4, 2024

Description

Testing

Internal references:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsymons left a comment

Choose a reason for hiding this comment

diegoreymendez commented Apr 8, 2024