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

Add WireGuard NetP Error Pixels #508

Merged
merged 8 commits into from
Sep 21, 2023
Merged

Add WireGuard NetP Error Pixels #508

merged 8 commits into from
Sep 21, 2023

Conversation

graeme
Copy link
Contributor

@graeme graeme commented Sep 19, 2023

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/0/1205524329218224/f
iOS PR: duckduckgo/iOS#2023
macOS PR: duckduckgo/macos-browser#1641
What kind of version bump will this require?: Major/Minor/Patch

Description:

When implementing error pixels for NetP on iOS, I noticed that macOS was missing the Pixels (that were listed in its privacy triage) for WireGuard error callbacks. Furthermore, we didn’t have the hook to respond to them from the NetworkProtection library. This task adds this capability so we can get a better idea of why WireGuard is failing in the wild.

Steps to test this PR:

  • These may be a bit hard to reproduce, but it’s good too have them here just in case something is happening in the wild.

OS Testing:

  • iOS 14
  • iOS 15
  • iOS 16
  • macOS 10.15
  • macOS 11
  • macOS 12

Internal references:

Software Engineering Expectations
Technical Design Template

@graeme graeme self-assigned this Sep 19, 2023
@graeme graeme requested a review from samsymons September 19, 2023 15:42
@graeme graeme marked this pull request as ready for review September 19, 2023 15:43
// This is probably not the most elegant error to show to a user but
// it's a great way to get detailed reports for those cases we haven't
// provided good descriptions for yet.
return "NetworkProtectionError.\(String(describing: self))"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Sounds fair to me

Copy link
Contributor

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

Looks great! 👏

@graeme graeme merged commit ec5dc59 into main Sep 21, 2023
4 checks passed
@graeme graeme deleted the graeme/netp-wireguard-errors branch September 21, 2023 10:48
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