-
Notifications
You must be signed in to change notification settings - Fork 117
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-5547: Implement subscription management telemetry #8838
Conversation
Request for data collection review form
This collection is Glean so is documented in the Glean Dictionary.
This collection will be collected permanently.
All channels, countries, and locales. No filters.
These collections are Glean. The opt-out can be found in the product's preferences.
No. |
const isMonthlyPlanTest = testCase === "monthly plan" | ||
|
||
//Check if we are testing the monthly plan so we can mock the guardian response | ||
beforeEach(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brizental do you know of any good way to do this before each item in testCases
instead of before each test (it)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you want to use before
instead of beforeEach
? That will be applied before all test cases inside a describe block. See: https://mochajs.org/#hooks
tests/functional/testSubscription.js
Outdated
assert.equal(subscriptionManagementScreenTelemetryId, manageSubscriptionSelectedEventsExtras.screen); | ||
}); | ||
|
||
it('homeSelected interaction event is recorded with correct screen', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brizental do you think it is worth testing the navbar telemetry with the appropriate screen
extra key like I did here for every different potential screen? Or should we just test it once somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think since that is happening in the C++ code you can just test it in a unit test and skip this telemetry piece in functional tests completely. We can pair on that if you'd like to see how it would go.
Otherwise, I think it's also not a big deal to just leave it like this and no need to test for each screen. The main logic is tested here. Testing for each screen would just be testing that the telemetryScreenId
of some screen was set correctly, but for that there are other test cases already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Matt, this is looking great! Question, I see there is instrumentation on the "Sign out" button spec'd in the Miro board but I don't see it here. Is it already there or mabe coming in a follow up PR?
const isMonthlyPlanTest = testCase === "monthly plan" | ||
|
||
//Check if we are testing the monthly plan so we can mock the guardian response | ||
beforeEach(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you want to use before
instead of beforeEach
? That will be applied before all test cases inside a describe block. See: https://mochajs.org/#hooks
tests/functional/testSubscription.js
Outdated
}); | ||
|
||
it('accountScreen impression event is recorded', async () => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The newline at the start of each test is bothering me 😅
tests/functional/testSubscription.js
Outdated
assert.equal(subscriptionManagementScreenTelemetryId, manageSubscriptionSelectedEventsExtras.screen); | ||
}); | ||
|
||
it('homeSelected interaction event is recorded with correct screen', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think since that is happening in the C++ code you can just test it in a unit test and skip this telemetry piece in functional tests completely. We can pair on that if you'd like to see how it would go.
Otherwise, I think it's also not a big deal to just leave it like this and no need to test for each screen. The main logic is tested here. Testing for each screen would just be testing that the telemetryScreenId
of some screen was set correctly, but for that there are other test cases already.
- https://mozilla-hub.atlassian.net/browse/VPN-5547 | ||
data_reviews: | ||
- https://github.com/mozilla-mobile/mozilla-vpn-client/pull/8838#issuecomment-1862135964 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to point to the data-review response. Same applies to all other places where this link is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh good call. Will fix after it has been reviewed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request for data collection review form
1. What questions will you answer with this data? * As a Product Manager … I want to understand how often users on Monthly plans display interest in upgrading to an Annual plan by virtue of using the in-app Profile screen CTA. * As a Product Manager … I want to understand how often users who come from the in-app Profile screen CTA end up upgrading to an Annual Plan. 2. Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements? * This data is necessary for the Mozilla VPN product team to understand the behavior of the subscription management and subscription upgrading experience and how to make that flow better for users. 3. What alternative methods did you consider to answer these questions? Why were they not sufficient? * None 4. Can current instrumentation answer these questions? * No 5. List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories found on the Mozilla wiki.
Measurement Name Measurement Description Data Collection Category Tracking Bug
impression.account_screen
The user has viewed the subscription management screen impression mozilla-hub.atlassian.net/browse/VPN-5547
interaction.edit_selected
The user has clicked the "edit account" button interaction mozilla-hub.atlassian.net/browse/VPN-5547
interaction.manage_subscription_selected
The user has clicked the “manage subscription” button from the subscription management view interaction mozilla-hub.atlassian.net/browse/VPN-5547
interaction.change_plan_selected
The user has clicked the “change plan” button from the subscription management view (monthly web plan) interaction mozilla-hub.atlassian.net/browse/VPN-55476. Please provide a link to the documentation for this data collection which describes the ultimate data set in a public, complete, and accurate way.
This collection is Glean so is documented in the Glean Dictionary.
7. How long will this data be collected?
This collection will be collected permanently. [email protected], [email protected] will be responsible for the permanent collections.
8. What populations will you measure?
All channels, countries, and locales. No filters.
9. If this data collection is default on, what is the opt-out mechanism for users?
These collections are Glean. The opt-out can be found in the product's preferences.
10. Please provide a general description of how you will analyze this data. * Dashboards that product managers and others will consult on a regular basis. 11. Where do you intend to share the results of your analysis? * Within the Mozilla VPN team and Security and Privacy Products team. 12. Is there a third-party tool (i.e. not Glean or Telemetry) that you are proposing to use for this data collection?
No.
Data Review
- Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way?
Yes, through the metrics.yaml file and the Glean Dictionary.
- Is there a control mechanism that allows the user to turn the data collection on and off?
Yes, through the data preferences in the application settings.
- If the request is for permanent data collection, is there someone who will monitor the data over time?
Permanent collection to be monitored by [email protected] and vpn-team
- Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?
Category 2, Interaction data
- Is the data collection request for default-on or default-off?
Default-on
- Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?
No
- Is the data collection covered by the existing Firefox privacy notice?
Yes
- Does the data collection use a third-party collection tool?
No
Result
data-review+
@brizental Moving the "sign out" button from the settings menu to the subscription management view will come as part of VPN-5696: Implement new Settings menu & IA |
src/frontend/navigator.cpp
Outdated
// screens) | ||
if (layers.length() < 1 || layers.last() | ||
.m_layer->property("telemetryScreenId") | ||
.toString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this crash if the property is not there at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just though of this, I think it's worth adding a test case for when the property is not there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will not. if the property doesn't exist, the QVariant will be invalid (aka it's userType
will be QMetaType::UnknownType
which is 0
). performing QVariant::toString
on a QVariant with an unknown meta type will return an empty string
src/frontend/navigator.cpp
Outdated
} | ||
|
||
QString telemetryScreenId = | ||
layers.last().m_layer->property("telemetryScreenId").toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Move this declaration before the conditional and use this variable instead of repeating layers.last().m_layer->property("telemetryScreenId").toString()
.
mozilla::glean::interaction::SettingsSelectedExtra{ | ||
._screen = telemetryScreenId, | ||
}); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a default case here, just in case?
tests/unit_tests/testnavigator.h
Outdated
private slots: | ||
void init(); | ||
|
||
void navbar_button_telemetry(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
void navbar_button_telemetry(); | |
void testNavbarButtonTelemetry(); |
I know this is not standardized right now... So feel free to ignore.
Description
Reference
VPN-5547: Implement Telemetry
[Telemetry] Miro board