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-6799: Add tests for timeToMainScreen metric #10172

Merged
merged 10 commits into from
Jan 9, 2025

Conversation

oskirby
Copy link
Collaborator

@oskirby oskirby commented Jan 8, 2025

Description

There has been some concern that we might have a regression in our timeToMainScreen performance, which is intended to measure the startup time of the application. To make sure that this isn't some bug in the metric accumulation itself, we should add a functional test to ensure that it's generating sensible data.

Unfortunately, this turned into a bit of a mess. There was a hack in the code that used QML/JS to serialize the metric value for testing, but this fails when handling a histogram metric, like TimingDistributionMetric so this required some refactoring to make it go. In particular:

  • Abandon the QML/JS implementation so that we can write our own metric serialization code in C++.
  • Add a BaseMetric type to try and simplify how metrics are serialized, and give them a unified interface.
  • Add an extra test to make sure that the CustomDistributionMetric we use for RX/TX transfer stats still work.

And after all that is done, we can now check the timeToMainScreen metric as follows:

let timing = await vpn.gleanTestGetValue("performance", "timeToMainScreen", "");

Reference

JIRA issue: VPN-6799
JIRA issue: VPN-6186

Checklist

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

There is some concern that we might have a regression in app loading times
so lets try adding a test to ensure it remains within a reasonable bound.
It turns out that the distribution metrics can't be serialized by the QML/JS
engine, so we'll need to clean up that hack and implement metric serialization
in C++ instead.
@oskirby oskirby requested a review from mcleinman January 8, 2025 20:02
@oskirby oskirby marked this pull request as ready for review January 8, 2025 22:08
Copy link
Collaborator

@mcleinman mcleinman left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@oskirby oskirby merged commit 5291b30 into main Jan 9, 2025
120 checks passed
@oskirby oskirby deleted the vpn-6799-time-to-main-screen-functional-test branch January 9, 2025 23:36
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