mozilla-vpn-client icon indicating copy to clipboard operation
mozilla-vpn-client copied to clipboard

VPN-6285: Metrics for bandwidth usage

Open mcleinman opened this issue 5 months ago • 2 comments

Description

This is a continuation of https://github.com/mozilla-mobile/mozilla-vpn-client/pull/9318, which Bea had mostly completed before she left. Data review has been completed on that PR.

Notes:

  1. Per conversation w/ Santiago, we are not going to record these metrics on iOS. The custom distribution metric does not exist on iOS stock Glean (which is used in the network extension), and thus we can’t use this metric as designed for other platforms. We'd either have to use a different metric type on iOS (or on all platforms), and Santiago and I believe we can extrapolate the data on iOS effectively enough to not make this work necessary. a. Unfortunately, this doesn't mean that iOS takes no work. If we don't remove the metric, we get a compile error as the non-existant metric type is mentioned in VPNMetrics.swift. Thus, some ugly sed work was added (along with a comment) to the iOS Glean cmake files to remove these metrics completely for iOS.
  2. I found some potential memory retain bugs in iOS, and left a fix for them as part of this PR.
  3. Bea wrote the MemoryDistribution scaffolding, before it was decided to use a CustomDistribution for this work instead. Her intention was to leave this work in the PR, to prevent having to re-do this work later. (There is a bit of work required for each new metric type we use in the client, due to our custom Glean work.) I'm split on whether to leave this untested (and currently unused) code in (I did leave a note), or pull it out. For now, I've left it in - but I'm very open to removing it.
  4. I removed a check from Controller::getStatus, as without it we couldn't get the data transfer data as the VPN was being turned off (without a decent sized refactor). This felt very safe, as getStatus called in just 2 places: First, ConnectionHealth, where it’s called right after confirming that controller is StateOn. And second, in Telemetry, when we’re recording dataTransferTelemetry. Thus, I feel comfortable removing the check for controller state within Controller::getStatus: We need it removed for telemetry’s usage of it, and it doesn’t matter for ConnectionHealth because we already do a check within ConnectionHealth before calling getStatus.
  5. Similar to Controller::getStatus, I updated some Android-specific code to remove a check. That check, similarly, was blocking us from getting data as the tunnel turns off. And I can't find a code path where that check is necessary.
  6. Important for testing: The short session timer debug option is broken on Android. I filed a ticket: https://mozilla-hub.atlassian.net/browse/VPN-6629
  7. I tested this on Android (manually setting a short debug timer option in the code) and macOS, and made sure the app continues to work on iOS.

Reference

VPN-6285

Checklist

  • [x] My code follows the style guidelines for this project
  • [x] I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
  • [x] I have performed a self review of my own code
  • [x] I have commented my code PARTICULARLY in hard to understand areas
  • [x] I have added thorough tests where needed

mcleinman avatar Sep 27 '24 16:09 mcleinman