2493 reauth flow toolbar #2780

severinrudie · 2019-09-03T18:53:52Z

Checklist

Confirm the acceptance criteria is fully satisfied in the issue(s) this PR will close
Add thorough tests or an explanation of why it does not
Add a CHANGELOG entry if applicable
Add QA labels on the associated issue (not this PR; qa-ready or qa-notneeded)

severinrudie · 2019-09-03T18:54:08Z

@liuche

Request for data collection review form

What questions will you answer with this data?

How many users are in a “fax needs reauth” state at any given time?

Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements?

To determine how many people remain in this state. In part, this will help answer how annoying it is to reauth on FFTV

What alternative methods did you consider to answer these questions? Why were they not sufficient?

This is the only way to determine this information.

Can current instrumentation answer these questions?

No telemetry currently exists to answer these questions.

List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories on the Mozilla wiki.

Measurement Description	Data Collection Category	Tracking Bug #
Whenever a users FxA state changes, send whether or not they need to reauthenticate	2	Issue 2493
Reauthenticate button clicks	2	Issue 2493

How long will this data be collected?

@athomasmoz will permanently monitor this data.

What populations will you measure?

All users with telemetry enabled, on release channel.

If this data collection is default on, what is the opt-out mechanism for users?

There is an option in settings to stop sending usage data.

Please provide a general description of how you will analyze this data.

The data will be analyzed through Redash queries and dashboards.

Where do you intend to share the results of your analysis?

On Redash and with mobile teams.

mcomella · 2019-09-03T21:31:09Z

@liuche please data review #2780 (comment)

mcomella

I'm concerned that we have the potential to block the main thread: let's discuss.

CHANGELOG.md

app/src/main/java/org/mozilla/tv/firefox/telemetry/TelemetrySettingsProvider.kt

mcomella

lgtm w/ nits.

app/src/main/java/org/mozilla/tv/firefox/fxa/FxaRepo.kt

app/src/main/java/org/mozilla/tv/firefox/navigationoverlay/NavigationOverlayFragment.kt

app/src/main/java/org/mozilla/tv/firefox/fxa/FxaRepo.kt

docs/telemetry.md

app/src/main/java/org/mozilla/tv/firefox/fxa/FxaRepo.kt

codecov-io · 2019-09-03T22:59:05Z

Codecov Report

❗ No coverage uploaded for pull request base (master@12ec1a0). Click here to learn what that means.
The diff coverage is 45%.

@@            Coverage Diff            @@
##             master    #2780   +/-   ##
=========================================
  Coverage          ?   52.71%           
  Complexity        ?      467           
=========================================
  Files             ?      117           
  Lines             ?     3428           
  Branches          ?      446           
=========================================
  Hits              ?     1807           
  Misses            ?     1473           
  Partials          ?      148

Impacted Files	Coverage Δ	Complexity Δ
...fox/navigationoverlay/NavigationOverlayFragment.kt	`66.97% <0%> (ø)`	`27 <0> (?)`
...ox/navigationoverlay/NavigationOverlayViewModel.kt	`48.93% <0%> (ø)`	`8 <0> (?)`
...rc/main/java/org/mozilla/tv/firefox/fxa/FxaRepo.kt	`91.86% <100%> (ø)`	`6 <1> (?)`
...zilla/tv/firefox/telemetry/TelemetryIntegration.kt	`22.54% <75%> (ø)`	`15 <1> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12ec1a0...2e97cbb. Read the comment docs.

…state

It was noticed that we had no good guarantee about when our settings ping would be sent (especially after we eventually migrate to Glean), meaning we could miss important information. For example, if sent on startup, 0% of users would be authenticated because the call would not yet be completed. Instead we chose to send all FxA state changes, but to narrow them down to 'Needs Reauth == true/false', to be as lean as possible. We are also debouncing this ping, to cut down on redundant information. See FxaRepo#setupTelemetry for details

This had not yet been merged to master.

… telemetry

liuche

Data Review Form (to be filled by Data Stewards)

Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way?

Yes, in telemetry.md

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, FFTV data settings

If the request is for permanent data collection, is there someone who will monitor the data over time?

@athomasmoz will monitor, and we will check in every 6mo

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Type 2, Type 1: interaction with the Accounts button, as well as the account state

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, click event and boolean of account state

Is the data collection covered by the existing Firefox privacy notice?

Yes

Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)**

6mo

Does the data collection use a third-party collection tool?

No

liuche · 2019-09-04T17:31:28Z

docs/telemetry.md

@@ -104,6 +106,8 @@ This value existed from v3.2+, but is intended for removal. For versions after v

 (***)The tile_id is only collected for bundled tiles.

+(****)This probe is sent any time the user's FxA sign in state changes (debounced to avoid sending redundant pings). `boolean` will be `true` if the user requires reauthentication, or `false` otherwise


Might be more clear if we list what states could lead to false - though that could also change. Maybe explain it's FxA states, which include X, Y, Z?

Oh whoops, I merged before I noticed this. I'll open a small PR to make that change and tag you.

Opened #2795

severinrudie requested a review from liuche September 3, 2019 18:53

severinrudie requested review from mcomella and removed request for liuche September 3, 2019 21:13

mcomella requested a review from liuche September 3, 2019 21:30

mcomella suggested changes Sep 3, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

app/src/main/java/org/mozilla/tv/firefox/telemetry/TelemetrySettingsProvider.kt Outdated Show resolved Hide resolved

mcomella approved these changes Sep 3, 2019

View reviewed changes

severinrudie added 12 commits September 4, 2019 10:11

Issue mozilla-mobile#2493: import fxa needs reauth icon

7f65de1

Issue mozilla-mobile#2493: update fxa icon / text when needs reauth

b28ac4c

Issue mozilla-mobile#2493: add reauth button click telemetry

238904f

Issue mozilla-mobile#2493: send telemetry when reauth is clicked

3aa5ef9

Closes mozilla-mobile#2493: track number of users in 'reauth needed' …

8f06f08

…state

Closes mozilla-mobile#2493: changelog for fxa reauth

ec19c80

Closes mozilla-mobile#2493: nits for fxa reauth svg

ba77466

Closes mozilla-mobile#2493: fix reauthenticate button ping

867d352

This had not yet been merged to master.

Closes mozilla-mobile#2493: changelog update per nit on fxa reauth

a8654eb

Closes mozilla-mobile#2493: nits for FxA reauth

49dcf86

Closes mozilla-mobile#2493: wrote test for fxa state reauthentication…

2e97cbb

… telemetry

severinrudie force-pushed the 2493-reauth-flow-toolbar branch from b9e5ddf to 2e97cbb Compare September 4, 2019 17:19

liuche approved these changes Sep 4, 2019

View reviewed changes

severinrudie merged commit c3353d0 into mozilla-mobile:master Sep 4, 2019

severinrudie deleted the 2493-reauth-flow-toolbar branch September 4, 2019 21:37

severinrudie mentioned this pull request Sep 4, 2019

Issue #2493: update telemetry comment for fxa reauth ping #2795

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2493 reauth flow toolbar #2780

2493 reauth flow toolbar #2780

severinrudie commented Sep 3, 2019

severinrudie commented Sep 3, 2019 •

edited

mcomella commented Sep 3, 2019

mcomella left a comment

mcomella left a comment

codecov-io commented Sep 3, 2019 •

edited

liuche left a comment

liuche Sep 4, 2019

severinrudie Sep 4, 2019

severinrudie Sep 4, 2019

		@@ -104,6 +106,8 @@ This value existed from v3.2+, but is intended for removal. For versions after v

		(***)The tile_id is only collected for bundled tiles.

		(****)This probe is sent any time the user's FxA sign in state changes (debounced to avoid sending redundant pings). `boolean` will be `true` if the user requires reauthentication, or `false` otherwise

2493 reauth flow toolbar #2780

2493 reauth flow toolbar #2780

Conversation

severinrudie commented Sep 3, 2019

Checklist

severinrudie commented Sep 3, 2019 • edited

Request for data collection review form

mcomella commented Sep 3, 2019

mcomella left a comment

Choose a reason for hiding this comment

mcomella left a comment

Choose a reason for hiding this comment

codecov-io commented Sep 3, 2019 • edited

Codecov Report

liuche left a comment

Choose a reason for hiding this comment

Data Review Form (to be filled by Data Stewards)

liuche Sep 4, 2019

Choose a reason for hiding this comment

severinrudie Sep 4, 2019

Choose a reason for hiding this comment

severinrudie Sep 4, 2019

Choose a reason for hiding this comment

severinrudie commented Sep 3, 2019 •

edited

codecov-io commented Sep 3, 2019 •

edited