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.

Sign up for GitHub

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

Jump to bottom

Update login item failure pixel #2024

Merged
merged 5 commits into from
Jan 8, 2024
Merged

Update login item failure pixel #2024

merged 5 commits into from
Jan 8, 2024

Conversation

samsymons
Copy link
Collaborator

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

Description:

This PR updates the m.mac.login-item.update-error pixel to include additional parameters. Specifically:

  1. The agent bundle ID
  2. The action that failed (e.g. restart)
  3. The build type (e.g. appstore vs dmg)

Steps to test this PR:

  1. Launch the app and turn pixel logging on via Debug -> Logging -> Pixel
  2. Add a forced failure to the login item registration flow. You can do this by adding throw NSError(domain: "domain", code: 1) to LoginItem.swift anywhere that we're doing something that throws; enable, disable, and restart functions should cover it
  3. Launch the app again, and you should see a pixel fire in the console that includes the appropriate parameters. If you don't, activate NetP or DBP to trigger a login item to be enabled
Internal references:

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

samsymons
samsymons commented Jan 8, 2024
View reviewed changes
DuckDuckGo/Statistics/PixelParameters.swift
@@ -168,6 +172,122 @@ extension Pixel.Event {

}

extension Pixel.Event.Debug {

var parameters: [String: String]? {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We didn't have the ability to do this before AFAIK, so I've added support for it here. This code avoids using default, so that any time someone adds a new debug event they're forced to consider whether parameters need adding.

samsymons
samsymons commented Jan 8, 2024
View reviewed changes
DuckDuckGo/LoginItems/LoginItemsManager.swift
buildType: AppVersion.buildType
)

DailyPixel.fire(pixel: .debug(event: event, error: error), frequency: .dailyAndCount, includeAppVersionParameter: true)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This technically changes the name of the pixel to add _d and _c at the end depending whether it's a daily or count pixel. Any dashboards that are using this pixel will need to be updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is something we can do after merging. Would you let me know when you're doing this so I can follow along?

@diegoreymendez diegoreymendez requested review from diegoreymendez and removed request for THISISDINOSAUR January 8, 2024 13:30
diegoreymendez
diegoreymendez approved these changes Jan 8, 2024
View reviewed changes
Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

Works as described. I'll merge once the CI allows.

DuckDuckGo.xcodeproj/project.pbxproj
Comment on lines +2026 to +2028
4BA71EDA2B4B81E80002EBCE /* AppVersionExtension.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4BA71ED92B4B81E80002EBCE /* AppVersionExtension.swift */; };
4BA71EDB2B4B81E80002EBCE /* AppVersionExtension.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4BA71ED92B4B81E80002EBCE /* AppVersionExtension.swift */; };
4BA71EDC2B4B81E80002EBCE /* AppVersionExtension.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4BA71ED92B4B81E80002EBCE /* AppVersionExtension.swift */; };
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should hold back the PR but I'd consider adding these files to a common package under NetworkProtectionMac.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is useful to all parts of the codebase so I'd avoid keeping it out of a NetworkProtection directory if possible

DuckDuckGo/LoginItems/LoginItemsManager.swift
buildType: AppVersion.buildType
)

DailyPixel.fire(pixel: .debug(event: event, error: error), frequency: .dailyAndCount, includeAppVersionParameter: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is something we can do after merging. Would you let me know when you're doing this so I can follow along?

@diegoreymendez diegoreymendez merged commit e777097 into main Jan 8, 2024
14 checks passed
@diegoreymendez diegoreymendez deleted the sam/expand-login-item-failure-pixel branch January 8, 2024 14:42
samsymons added a commit that referenced this pull request Jan 8, 2024
@samsymons
Merge branch 'main' into sam/netp-system-extension-crash-reports
145ee1e 
* main: (35 commits)
  Update login item failure pixel (#2024)
  fix turn off sync error message (#2025)
  DBP: macOS - Scheduler Progress Notifications (#2023)
  DBP: Implement sign-out flow for DBP (#2009)
  Switch CI to Xcode 15.1 (#2022)
  fix(duckplayer): bump CSS for duckplayer nav loop fix (#1982)
  Bump Submodules/privacy-reference-tests from `a3acc21` to `6b7ad1e` (#2006)
  Bump version to 1.69.0 (99)
  DBP: Respect foreign constraints when deleting all user's data (#2014)
  DBP: Respect foreign constraints when deleting all user's data (#2014)
  Don't force reload tab when restoring state (#2016)
  Bump version to 1.69.0 (98)
  Update embedded files
  Update NetP launch agent logic to include build number (#2015)
  Always use 'sandbox' Application Support directory for Favicons Fetcher (#2013)
  Allow calculations in the address bar (#2012)
  Fix clickable area for buttons in 'Sync with Another Device' view (#2011)
  Fix for empty autofill state displayed on top of existing password items (#1998)
  Update error messages (#1999)
  Autofill never save for site (#1991)
  ...
samsymons added a commit that referenced this pull request Jan 9, 2024
@samsymons
Merge branch 'main' into sam/data-broker-remote-messaging
6646d37 
# By Dax the Duck (6) and others
# Via Fernando Bunn (2) and others
* main: (36 commits)
  Send VPN system extension crashes to Sentry (#2002)
  Update login item failure pixel (#2024)
  fix turn off sync error message (#2025)
  DBP: macOS - Scheduler Progress Notifications (#2023)
  DBP: Implement sign-out flow for DBP (#2009)
  Switch CI to Xcode 15.1 (#2022)
  fix(duckplayer): bump CSS for duckplayer nav loop fix (#1982)
  Bump Submodules/privacy-reference-tests from `a3acc21` to `6b7ad1e` (#2006)
  Bump version to 1.69.0 (99)
  DBP: Respect foreign constraints when deleting all user's data (#2014)
  DBP: Respect foreign constraints when deleting all user's data (#2014)
  Don't force reload tab when restoring state (#2016)
  Bump version to 1.69.0 (98)
  Update embedded files
  Update NetP launch agent logic to include build number (#2015)
  Always use 'sandbox' Application Support directory for Favicons Fetcher (#2013)
  Allow calculations in the address bar (#2012)
  Fix clickable area for buttons in 'Sync with Another Device' view (#2011)
  Fix for empty autofill state displayed on top of existing password items (#1998)
  Update error messages (#1999)
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo/Statistics/PixelEvent.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diegoreymendez diegoreymendez diegoreymendez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@samsymons @diegoreymendez