diff --git a/android/daemon/src/main/java/org/mozilla/firefox/vpn/daemon/VPNService.kt b/android/daemon/src/main/java/org/mozilla/firefox/vpn/daemon/VPNService.kt index 9387c2cac0..39a2ba6339 100644 --- a/android/daemon/src/main/java/org/mozilla/firefox/vpn/daemon/VPNService.kt +++ b/android/daemon/src/main/java/org/mozilla/firefox/vpn/daemon/VPNService.kt @@ -51,8 +51,8 @@ class VPNService : android.net.VpnService() { override fun onTick(millisUntilFinished: Long) { // We want to skip the first onTick, which is called as the timer is started. // (The Start ping is sent then, so there is no need for a Timer ping.) - var wasTimerJustStarted = (millisUntilFinished == mBackgroundPingTimerMSec) - if (!wasTimerJustStarted && isUsingShortTimerSessionPing && shouldRecordTimerAndEndMetrics) { + var wasTimerJustStarted = (millisUntilFinished >= (mBackgroundPingTimerMSec - 1000)) + if (!wasTimerJustStarted && isUsingShortTimerSessionPing && isSuperDooperMetricsActive) { Log.i(tag, "Sending daemon_timer ping (on short timer debug schedule)") Pings.daemonsession.submit( Pings.daemonsessionReasonCodes.daemonTimer, @@ -61,8 +61,7 @@ class VPNService : android.net.VpnService() { } override fun onFinish() { Log.i(tag, "Sending daemon_timer ping") - if (shouldRecordTimerAndEndMetrics) { - recordDataTransferMetrics() + if (isSuperDooperMetricsActive) { Pings.daemonsession.submit( Pings.daemonsessionReasonCodes.daemonTimer, ) @@ -84,11 +83,6 @@ class VPNService : android.net.VpnService() { return (this.mConfig?.optInt("reason") ?: 0) == 1 } - private val shouldRecordTimerAndEndMetrics: Boolean - get() { - return isSuperDooperMetricsActive && !isAppChangingServers - } - private val isUsingShortTimerSessionPing: Boolean get() { return this.mConfig?.optBoolean("isUsingShortTimerSessionPing", false) ?: false @@ -325,6 +319,12 @@ class VPNService : android.net.VpnService() { // now. if (currentTunnelHandle != -1) { Log.i(tag, "Currently have a connection, close old handle") + // Record data transfer metrics, as we're switching servers/configs and this data will get reset + // Data metrics must be recorded prior to wgTurnOff, or we can't get data from the config + if (isSuperDooperMetricsActive) { + recordDataTransferMetrics() + } + wgTurnOff(currentTunnelHandle) } currentTunnelHandle = wgTurnOn("mvpn0", tun.detachFd(), wgConfig) @@ -338,12 +338,14 @@ class VPNService : android.net.VpnService() { protect(wgGetSocketV6(currentTunnelHandle)) mConfig = json - // shouldRecordStartTelemetry must be calculated after mConfig is set (on prior line) // We don't want to record start metrics in several situations: - // - If Super Dooper feature is not active. (Covered by shouldRecordTimerAndEndMetrics) - // - If this is an app-caused server switch. (Covered by shouldRecordTimerAndEndMetrics) - // - If this is a daemon-caused server switch. (Covered by isDaemonChangingServers) - val shouldRecordStartTelemetry = shouldRecordTimerAndEndMetrics && !isDaemonChangingServers + // - If Super Dooper feature is not active. + // - If this is an app-caused server switch. + // - If this is a daemon-caused server switch. + // This must be calculated after mConfig is set (on line above) + val isChangingServers = isDaemonChangingServers || isAppChangingServers + val shouldSkipStartTelemetry = !isSuperDooperMetricsActive || isChangingServers + Log.i(tag, "Is changing server: " + shouldSkipStartTelemetry) // Store the config in case the service gets // asked boot vpn from the OS @@ -363,13 +365,17 @@ class VPNService : android.net.VpnService() { } else { fallbackIpv4 = json.getJSONArray("servers").getJSONObject(0).getString("ipv4AddrIn") } - mConnectionHealth.start( - jServer.getString("ipv4AddrIn"), - jServer.getString("ipv4Gateway"), - json.getString("dns"), - fallbackIpv4, - shouldRecordStartTelemetry, - ) + + // If changing server, connection health is still running + if (!isChangingServers) { + mConnectionHealth.start( + jServer.getString("ipv4AddrIn"), + jServer.getString("ipv4Gateway"), + json.getString("dns"), + fallbackIpv4, + isSuperDooperMetricsActive, + ) + } // For `isGleanDebugTagActive` and `isSuperDooperMetricsActive` to work, // this must be after the mConfig is set to the latest data. @@ -378,7 +384,8 @@ class VPNService : android.net.VpnService() { Log.i(tag, "Setting Glean debug tag for daemon.") Glean.setDebugViewTag(gleanTag) } - if (shouldRecordStartTelemetry) { + if (!shouldSkipStartTelemetry) { + Log.v(tag, "Recording start telmetry") val installationIdString = json.getString("installationId") installationIdString?.let { try { @@ -401,7 +408,10 @@ class VPNService : android.net.VpnService() { Pings.daemonsession.submit( Pings.daemonsessionReasonCodes.daemonStart, ) + } else { + Log.v(tag, "Skipping recording of start telemetry") } + mMetricsTimer.cancel() // if this is a server switch, time is already running and must be reset mMetricsTimer.start() } @@ -455,8 +465,10 @@ class VPNService : android.net.VpnService() { fun turnOff() { Log.v(tag, "Try to disable tunnel") - if (shouldRecordTimerAndEndMetrics) { - // Data metrics must be recorded prior to wgTurnOff, or we can't get data from the cocnfig + // turnOff is not called when switching locations, doing a silent server switch from app, or doing a silent server switch from daemon... + // Thus we always want to record end-of-session metrics here, and send the ping below. + if (isSuperDooperMetricsActive) { + // Data metrics must be recorded prior to wgTurnOff, or we can't get data from the config recordDataTransferMetrics() } @@ -471,7 +483,8 @@ class VPNService : android.net.VpnService() { // Clear the notification message, so the content // is not "disconnected" in case we connect from a non-client. CannedNotification(mConfig)?.let { mNotificationHandler.hide(it) } - if (shouldRecordTimerAndEndMetrics) { + if (isSuperDooperMetricsActive) { + Log.v(tag, "Sending ending metrics") Session.daemonSessionEnd.set() Pings.daemonsession.submit( Pings.daemonsessionReasonCodes.daemonEnd, @@ -483,6 +496,8 @@ class VPNService : android.net.VpnService() { // "flush" reason, which should contain this UUID and no other // metrics. Session.daemonSessionId.generateAndSet() + } else { + Log.v(tag, "Skipping ending metrics") } mMetricsTimer.cancel() } @@ -648,6 +663,7 @@ class VPNService : android.net.VpnService() { } private fun recordDataTransferMetrics() { + Log.v(tag, "Recording data metrics") val rxBytes = getConfigValue("rx_bytes")?.toLong() val txBytes = getConfigValue("tx_bytes")?.toLong() if (rxBytes != null && txBytes != null) { diff --git a/src/connectionhealth.cpp b/src/connectionhealth.cpp index a475909e96..5cf81d0d2a 100644 --- a/src/connectionhealth.cpp +++ b/src/connectionhealth.cpp @@ -45,23 +45,6 @@ constexpr const char* PING_WELL_KNOWN_ANYCAST_DNS = "194.242.2.2"; ConnectionHealth::ConnectionHealth() : m_dnsPingSender(QHostAddress()) { MZ_COUNT_CTOR(ConnectionHealth); -// On mobile, these metrics are recorded in the daemon process. -#ifndef MZ_MOBILE - m_metricsTimer.setSingleShot(false); - - if (SettingsHolder::instance()->shortTimerSessionPing()) { - m_metricsTimer.setInterval( - std::chrono::duration_cast(3min)); - } else { - m_metricsTimer.setInterval( - std::chrono::duration_cast(3h)); - } - connect(&m_metricsTimer, &QTimer::timeout, this, []() { - emit MozillaVPN::instance()->controller()->recordDataTransferTelemetry(); - }); - m_metricsTimer.start(); -#endif - m_noSignalTimer.setSingleShot(true); m_settlingTimer.setSingleShot(true); diff --git a/src/connectionhealth.h b/src/connectionhealth.h index 83eb436557..e232035b57 100644 --- a/src/connectionhealth.h +++ b/src/connectionhealth.h @@ -78,7 +78,6 @@ class ConnectionHealth final : public QObject { QTimer m_settlingTimer; QTimer m_noSignalTimer; QTimer m_healthCheckTimer; - QTimer m_metricsTimer; PingHelper m_pingHelper;