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

Network Protection Error Pixels #2019

Merged
merged 15 commits into from
Sep 21, 2023
Merged

Network Protection Error Pixels #2019

merged 15 commits into from
Sep 21, 2023

Conversation

graeme
Copy link
Contributor

@graeme graeme commented Sep 18, 2023

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

Description:

We want to report errors when the user tries to activate Network Protection on macOS, or when Network Protection encounters an unexpected failure. This includes:

  • When we fail to read a stored network configuration file from the Keychain
  • When we fail to write a server list file to the application support directory
  • When the WireGuard framework reports an error back to the macOS client
  • When the Network Extension receives an error during activation

We would like to append a suffix (_d | _c) on each pixel to represent whether or not this was the first time the pixel was fired each day for the user.
This is to let us determine whether a spike in pixels is distributed across many devices, or isolated to a small number of devices that are in a bad state and sending pixels repeatedly.

Steps to test this PR:
Testing all the Pixels may be somewhat overkill and, in some cases, not even possible.

  1. Select a few of the Pixels that should be reproducible (e.g m_netp_network_extension_error_activation_request_failed_ which should fire if the user denies VPN permissions)
  2. Run the PacketTunnelProvider in the debugger
  3. Put a breakpoint in the fire(pixelNamed function
  4. Check that it fires as expected with all the correct parameters
Internal references:

Software Engineering Expectations
Technical Design Template

@graeme graeme requested a review from samsymons September 18, 2023 16:29
@github-actions
Copy link

github-actions bot commented Sep 19, 2023

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

Generated by 🚫 dangerJS against 0edad7e

@graeme graeme mentioned this pull request Sep 19, 2023
13 tasks
@graeme
Copy link
Contributor Author

graeme commented Sep 20, 2023

@samsymons I fixed a couple of issues I noticed. See my two commits from today.

case DispatchSource.MemoryPressureEvent.warning:
Pixel.fire(pixel: .networkProtectionMemoryWarning)
case DispatchSource.MemoryPressureEvent.critical:
Pixel.fire(pixel: .networkProtectionMemoryCritical)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll be curious to see if these ones ever come through - will the extension have enough time to send the pixel before it gets killed? If we start getting warning pixels but never critical ones then that would be my theory, and we can look at implementing an offline pixel cache to catch situations like this.

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.

As far as I can see, this is working really nicely 👍

@graeme graeme merged commit 3f455ec into develop Sep 21, 2023
8 checks passed
@graeme graeme deleted the graeme/netp-error-pixels branch September 21, 2023 13:47
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