mozilla-vpn-client
mozilla-vpn-client copied to clipboard
VPN-6285: Metrics for bandwidth usage
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:
- 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 uglysed
work was added (along with a comment) to the iOS Glean cmake files to remove these metrics completely for iOS. - I found some potential memory retain bugs in iOS, and left a fix for them as part of this PR.
- 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.
- 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, asgetStatus
called in just 2 places: First,ConnectionHealth
, where it’s called right after confirming that controller is StateOn. And second, inTelemetry
, when we’re recordingdataTransferTelemetry
. Thus, I feel comfortable removing the check for controller state withinController::getStatus
: We need it removed for telemetry’s usage of it, and it doesn’t matter forConnectionHealth
because we already do a check withinConnectionHealth
before callinggetStatus
. - 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. - 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
- 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