From 991ed81c0ec17c49a932e495fe6d9bf9d9c52b69 Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Fri, 5 Jul 2024 15:12:41 +0200 Subject: [PATCH 1/6] Adds pixels to track connection tester failure and recovery --- .../NetworkProtectionConnectionTester.swift | 5 ++--- .../PacketTunnelProvider.swift | 21 ++++++++++++++++++- Sources/PixelKit/PixelKit+Parameters.swift | 1 + 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionConnectionTester.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionConnectionTester.swift index ec09ba5ba..98653e1c8 100644 --- a/Sources/NetworkProtection/Diagnostics/NetworkProtectionConnectionTester.swift +++ b/Sources/NetworkProtection/Diagnostics/NetworkProtectionConnectionTester.swift @@ -32,7 +32,7 @@ import Common final class NetworkProtectionConnectionTester { enum Result { case connected - case reconnected + case reconnected(failureCount: Int) case disconnected(failureCount: Int) } @@ -267,9 +267,8 @@ final class NetworkProtectionConnectionTester { if failureCount == 0 { resultHandler(.connected) } else if failureCount > 0 { + resultHandler(.reconnected(failureCount: failureCount)) failureCount = 0 - - resultHandler(.reconnected) } } diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 730c044e7..b0decacee 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -32,6 +32,8 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { public enum Event { case userBecameActive + case connectionTesterStatusChange(_ status: ConnectionTesterStatus) + case connectionTesterLongStatusChange(_ status: ConnectionTesterStatus) case reportConnectionAttempt(attempt: ConnectionAttempt) case tunnelStartAttempt(_ step: TunnelStartAttemptStep) case tunnelStopAttempt(_ step: TunnelStopAttemptStep) @@ -64,6 +66,11 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { case failure } + public enum ConnectionTesterStatus { + case failed + case recovered(failureCount: Int) + } + // MARK: - Error Handling public enum TunnelError: LocalizedError, CustomNSError, SilentErrorConvertible { @@ -321,11 +328,23 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { self.tunnelHealth.isHavingConnectivityIssues = false self.updateBandwidthAnalyzerAndRekeyIfExpired() - case .reconnected: + case .reconnected(let failureCount): + providerEvents.fire(.connectionTesterStatusChange(.recovered(failureCount: failureCount))) + + if failureCount >= 8 { + providerEvents.fire(.connectionTesterLongStatusChange(.recovered(failureCount: failureCount))) + } + self.tunnelHealth.isHavingConnectivityIssues = false self.updateBandwidthAnalyzerAndRekeyIfExpired() case .disconnected(let failureCount): + if failureCount == 1 { + providerEvents.fire(.connectionTesterStatusChange(.failed)) + } else if failureCount == 8 { + providerEvents.fire(.connectionTesterLongStatusChange(.failed)) + } + self.tunnelHealth.isHavingConnectivityIssues = true self.bandwidthAnalyzer.reset() } diff --git a/Sources/PixelKit/PixelKit+Parameters.swift b/Sources/PixelKit/PixelKit+Parameters.swift index b69227170..377e56c33 100644 --- a/Sources/PixelKit/PixelKit+Parameters.swift +++ b/Sources/PixelKit/PixelKit+Parameters.swift @@ -21,6 +21,7 @@ import Foundation public extension PixelKit { enum Parameters: Hashable { + public static let count = "count" public static let duration = "duration" public static let test = "test" public static let appVersion = "appVersion" From d27b9a59e72aff2dd14e6e019b2553f93306328f Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Fri, 5 Jul 2024 16:02:27 +0200 Subject: [PATCH 2/6] Makes several improvements to connection tester pixels --- Sources/NetworkProtection/PacketTunnelProvider.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index b0decacee..06baed2df 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -33,7 +33,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { public enum Event { case userBecameActive case connectionTesterStatusChange(_ status: ConnectionTesterStatus) - case connectionTesterLongStatusChange(_ status: ConnectionTesterStatus) + case connectionTesterExtendedStatusChange(_ status: ConnectionTesterStatus) case reportConnectionAttempt(attempt: ConnectionAttempt) case tunnelStartAttempt(_ step: TunnelStartAttemptStep) case tunnelStopAttempt(_ step: TunnelStopAttemptStep) @@ -332,7 +332,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { providerEvents.fire(.connectionTesterStatusChange(.recovered(failureCount: failureCount))) if failureCount >= 8 { - providerEvents.fire(.connectionTesterLongStatusChange(.recovered(failureCount: failureCount))) + providerEvents.fire(.connectionTesterExtendedStatusChange(.recovered(failureCount: failureCount))) } self.tunnelHealth.isHavingConnectivityIssues = false @@ -342,7 +342,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { if failureCount == 1 { providerEvents.fire(.connectionTesterStatusChange(.failed)) } else if failureCount == 8 { - providerEvents.fire(.connectionTesterLongStatusChange(.failed)) + providerEvents.fire(.connectionTesterExtendedStatusChange(.failed)) } self.tunnelHealth.isHavingConnectivityIssues = true From f22ca61dd866f7561d790ade09463f2b73bef16c Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Fri, 5 Jul 2024 16:29:58 +0200 Subject: [PATCH 3/6] Unifies connection tester status changes by kind --- .../PacketTunnelProvider.swift | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 06baed2df..820ffd21a 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -33,7 +33,6 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { public enum Event { case userBecameActive case connectionTesterStatusChange(_ status: ConnectionTesterStatus) - case connectionTesterExtendedStatusChange(_ status: ConnectionTesterStatus) case reportConnectionAttempt(attempt: ConnectionAttempt) case tunnelStartAttempt(_ step: TunnelStartAttemptStep) case tunnelStopAttempt(_ step: TunnelStopAttemptStep) @@ -67,8 +66,13 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { } public enum ConnectionTesterStatus { - case failed - case recovered(failureCount: Int) + case failed(kind: Kind) + case recovered(kind: Kind, failureCount: Int) + + public enum Kind: String { + case immediate + case extended + } } // MARK: - Error Handling @@ -329,10 +333,10 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { self.updateBandwidthAnalyzerAndRekeyIfExpired() case .reconnected(let failureCount): - providerEvents.fire(.connectionTesterStatusChange(.recovered(failureCount: failureCount))) + providerEvents.fire(.connectionTesterStatusChange(.recovered(kind: .immediate, failureCount: failureCount))) if failureCount >= 8 { - providerEvents.fire(.connectionTesterExtendedStatusChange(.recovered(failureCount: failureCount))) + providerEvents.fire(.connectionTesterStatusChange(.recovered(kind: .extended, failureCount: failureCount))) } self.tunnelHealth.isHavingConnectivityIssues = false @@ -340,9 +344,9 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { case .disconnected(let failureCount): if failureCount == 1 { - providerEvents.fire(.connectionTesterStatusChange(.failed)) + providerEvents.fire(.connectionTesterStatusChange(.failed(kind: .immediate))) } else if failureCount == 8 { - providerEvents.fire(.connectionTesterExtendedStatusChange(.failed)) + providerEvents.fire(.connectionTesterStatusChange(.failed(kind: .extended))) } self.tunnelHealth.isHavingConnectivityIssues = true From 2da7466efc1ffc84c80d06f914ca2b104417d33a Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Fri, 5 Jul 2024 16:35:43 +0200 Subject: [PATCH 4/6] Some additional changes to improve clarity in connection tester events --- .../NetworkProtection/PacketTunnelProvider.swift | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 820ffd21a..6073de1e7 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -66,10 +66,10 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { } public enum ConnectionTesterStatus { - case failed(kind: Kind) - case recovered(kind: Kind, failureCount: Int) + case failed(duration: Duration) + case recovered(duration: Duration, failureCount: Int) - public enum Kind: String { + public enum Duration: String { case immediate case extended } @@ -333,10 +333,10 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { self.updateBandwidthAnalyzerAndRekeyIfExpired() case .reconnected(let failureCount): - providerEvents.fire(.connectionTesterStatusChange(.recovered(kind: .immediate, failureCount: failureCount))) + providerEvents.fire(.connectionTesterStatusChange(.recovered(duration: .immediate, failureCount: failureCount))) if failureCount >= 8 { - providerEvents.fire(.connectionTesterStatusChange(.recovered(kind: .extended, failureCount: failureCount))) + providerEvents.fire(.connectionTesterStatusChange(.recovered(duration: .extended, failureCount: failureCount))) } self.tunnelHealth.isHavingConnectivityIssues = false @@ -344,9 +344,9 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { case .disconnected(let failureCount): if failureCount == 1 { - providerEvents.fire(.connectionTesterStatusChange(.failed(kind: .immediate))) + providerEvents.fire(.connectionTesterStatusChange(.failed(duration: .immediate))) } else if failureCount == 8 { - providerEvents.fire(.connectionTesterStatusChange(.failed(kind: .extended))) + providerEvents.fire(.connectionTesterStatusChange(.failed(duration: .extended))) } self.tunnelHealth.isHavingConnectivityIssues = true From 144878218c99d175d359e000cbe547fc26d06adb Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Sat, 6 Jul 2024 13:39:03 +0200 Subject: [PATCH 5/6] Adds a server parameter for the connection tester failure pixels --- .../PacketTunnelProvider.swift | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 6073de1e7..aa85efbab 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -32,7 +32,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { public enum Event { case userBecameActive - case connectionTesterStatusChange(_ status: ConnectionTesterStatus) + case connectionTesterStatusChange(_ status: ConnectionTesterStatus, server: String) case reportConnectionAttempt(attempt: ConnectionAttempt) case tunnelStartAttempt(_ step: TunnelStartAttemptStep) case tunnelStopAttempt(_ step: TunnelStopAttemptStep) @@ -327,16 +327,24 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { NetworkProtectionConnectionTester(timerQueue: timerQueue, log: .networkProtectionConnectionTesterLog) { @MainActor [weak self] result in guard let self else { return } + let serverName = lastSelectedServerInfo?.name ?? "Unknown" + switch result { case .connected: self.tunnelHealth.isHavingConnectivityIssues = false self.updateBandwidthAnalyzerAndRekeyIfExpired() case .reconnected(let failureCount): - providerEvents.fire(.connectionTesterStatusChange(.recovered(duration: .immediate, failureCount: failureCount))) + providerEvents.fire( + .connectionTesterStatusChange( + .recovered(duration: .immediate, failureCount: failureCount), + server: serverName)) if failureCount >= 8 { - providerEvents.fire(.connectionTesterStatusChange(.recovered(duration: .extended, failureCount: failureCount))) + providerEvents.fire( + .connectionTesterStatusChange( + .recovered(duration: .extended, failureCount: failureCount), + server: serverName)) } self.tunnelHealth.isHavingConnectivityIssues = false @@ -344,9 +352,15 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { case .disconnected(let failureCount): if failureCount == 1 { - providerEvents.fire(.connectionTesterStatusChange(.failed(duration: .immediate))) + providerEvents.fire( + .connectionTesterStatusChange( + .failed(duration: .immediate), + server: serverName)) } else if failureCount == 8 { - providerEvents.fire(.connectionTesterStatusChange(.failed(duration: .extended))) + providerEvents.fire( + .connectionTesterStatusChange( + .failed(duration: .extended), + server: serverName)) } self.tunnelHealth.isHavingConnectivityIssues = true From 96eee91dec5ffcf1078997763f096d092c66f660 Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Sat, 6 Jul 2024 13:45:27 +0200 Subject: [PATCH 6/6] Turns a magic value into a constant --- Sources/NetworkProtection/PacketTunnelProvider.swift | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index aa85efbab..1111d62f5 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -320,6 +320,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { // MARK: - Connection tester + private static let connectionTesterExtendedFailuresCount = 8 private var isConnectionTesterEnabled: Bool = true @MainActor @@ -340,7 +341,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { .recovered(duration: .immediate, failureCount: failureCount), server: serverName)) - if failureCount >= 8 { + if failureCount >= Self.connectionTesterExtendedFailuresCount { providerEvents.fire( .connectionTesterStatusChange( .recovered(duration: .extended, failureCount: failureCount),