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

VPN: Specific TunnelController start failure reporting #2714

Merged

Conversation

graeme
Copy link
Contributor

@graeme graeme commented Apr 11, 2024

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

Background
We’re seeing a rise in tunnel controller start failures but are lacking which specific function calls to the SDK are causing the failure.

Objective
Add more specific errors that can be sent with the m.netp.controller.start.failure.ios pixel to help with diagnosing the issue.

Steps to test this PR:

  • Sort of hard to test without changing code.

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 14
  • iOS 15
  • iOS 16

Theme Testing:

  • Light theme
  • Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

@graeme graeme requested a review from diegoreymendez April 11, 2024 16:38
@graeme graeme force-pushed the graeme/add-more-specific-error-to-controller-failure branch from 57e4cc5 to e82f18b Compare April 11, 2024 16:43
@graeme graeme force-pushed the graeme/add-more-specific-error-to-controller-failure branch from 3557a3c to 6588a90 Compare April 11, 2024 17:00
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.

Approved with a few appreciation comments, and one small observation. Great change.

@@ -37,9 +37,24 @@ final class NetworkProtectionTunnelController: TunnelController {

// MARK: - Starting & Stopping the VPN

enum StartError: LocalizedError {
case connectionStatusInvalid
enum StartError: LocalizedError, CustomNSError {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet!

case startVPNFailed(Error)
case fetchAuthTokenFailed(Error)

public static let errorDomain = "com.duckduckgo.NetworkProtectionTunnelController.StartError.domain"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note that I believe you can omit this, and the system will fill it in with a nice string.


public static let errorDomain = "com.duckduckgo.NetworkProtectionTunnelController.StartError.domain"

public var errorCode: Int {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great to have explicit codes. Thanks :)

@diegoreymendez diegoreymendez self-requested a review April 12, 2024 10:25
Comment on lines 57 to 69
public var errorUserInfo: [String: Any] {
switch self {
case
.simulateControllerFailureError:
return [:]
case
.loadFromPreferencesFailed(let error),
.saveToPreferencesFailed(let error),
.startVPNFailed(let error),
.fetchAuthTokenFailed(let error):
return ["NSUnderlyingError": error]
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, my bad I missed this.

.saveToPreferencesFailed(let error),
.startVPNFailed(let error),
.fetchAuthTokenFailed(let error):
return ["NSUnderlyingError": error]
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a constant NSUnderlyingErrorKey.

@graeme graeme changed the base branch from main to release/7.115.0 April 12, 2024 10:48
@graeme graeme merged commit 7e9d9bb into release/7.115.0 Apr 12, 2024
18 checks passed
@graeme graeme deleted the graeme/add-more-specific-error-to-controller-failure branch April 12, 2024 10:54
samsymons added a commit that referenced this pull request Apr 15, 2024
# By Daniel Bernal (33) and others
# Via Mariusz Śpiewak (5) and others
* main: (245 commits)
  Fix inconsistent bars state when scrolling (#2733)
  fix tests (#2732)
  Release 7.115.0-4 (#2729)
  Fix VPN denial prompt loop (#2728)
  Small UI Fixes for subscriptions (#2690)
  SPM updated: SwiftSoup, Lottie, ZIPFoundation (#2724)
  Release 7.115.0-3 (#2727)
  VPN: Specific TunnelController start failure reporting (#2714)
  update bsk dependency (#2725)
  Subscriptions: Fix thread issue on Subscription Restore (#2719)
  Manage ‘Stale’ PRs (#2723)
  maestro: hide dax dialogs if visible and cancel keyboard after fireproof (#2695)
  Remove timezone offset from the VPN server object (#2701)
  Reverting accidental push to main (#2718)
  Add SubscriptionContainerViewModel and
  Manually hide loader + Pixel (#2687)
  Release 7.115.0-2 (#2712)
  soft revert history suggestions (#2711)
  Bring back accessibility identifiers for onboarding buttons (#2709)
  BSK release 133.1.0 (#2708)
  ...

# Conflicts:
#	Core/Pixel.swift
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