Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

FxA WebChannels integration #4931

Merged
merged 4 commits into from
Sep 17, 2019
Merged

Conversation

grigoryk
Copy link
Contributor

@grigoryk grigoryk commented Aug 26, 2019

This patch includes:

@grigoryk
Copy link
Contributor Author

Let's add missing FxA telemetry. Closes #4971.
cc @boek @liuche as mobile data stewards.

Request for data collection review form

All questions are mandatory. You must receive review from a data steward peer on your responses to these questions before shipping new data collection.

  1. What questions will you answer with this data?

How users are interacting with FxA in Fenix - creating an account, pairing, using auto-login. In addition, "system health" metrics were added to monitor general health of the implementation ("recovered", "other_external").

  1. Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements? Some example responses:

We need to provide our users with a seamless, robust and easy-to-use FxA experience, and these metrics help us understand if our implementation is working as expected, and which kinds of authentication flows users are following.

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

An alternative could be to rely FxA metrics, but these do not allow us to complete a coherent picture of what's going on in a local authentication funnel, and some of the key local events (account recovery, auto-login) are not visible outside of the local client.

  1. Can current instrumentation answer these questions?

No.

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

All data is Category 2.

  1. How long will this data be collected? Choose one of the following:

This is scoped to a time-limited experiment/project until date "2020-03-01", at which point it'll be reviewed for renewal.

  1. What populations will you measure?

All release, beta, and nightly users with telemetry enabled.

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

This is part of events telemetry, so can be disabled via Data Collection settings.

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

Dashboards in redash (and amplitude?) from glean data.

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

Internally with the mobile teams, and on redash/aplitude.

  1. Is there a third-party tool (i.e. not Telemetry) that you are proposing to use for this data collection? If so:

N/A

@sblatz sblatz added the pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on label Sep 10, 2019
@codecov-io
Copy link

codecov-io commented Sep 11, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4931   +/-   ##
=========================================
  Coverage          ?   12.55%           
  Complexity        ?      258           
=========================================
  Files             ?      248           
  Lines             ?    10294           
  Branches          ?     1520           
=========================================
  Hits              ?     1292           
  Misses            ?     8913           
  Partials          ?       89
Impacted Files Coverage Δ Complexity Δ
.../java/org/mozilla/fenix/browser/BrowserFragment.kt 0% <ø> (ø) 0 <0> (?)
...n/java/org/mozilla/fenix/IntentReceiverActivity.kt 0% <ø> (ø) 0 <0> (?)
...main/java/org/mozilla/fenix/components/Services.kt 0% <0%> (ø) 0 <0> (?)
...in/java/org/mozilla/fenix/AppRequestInterceptor.kt 0% <0%> (ø) 0 <0> (?)
...rc/main/java/org/mozilla/fenix/FenixApplication.kt 3.84% <0%> (ø) 2 <0> (?)
...la/fenix/components/metrics/GleanMetricsService.kt 0.8% <0%> (ø) 1 <0> (?)
...a/org/mozilla/fenix/browser/BaseBrowserFragment.kt 0% <0%> (ø) 0 <0> (?)
app/src/main/java/org/mozilla/fenix/Experiments.kt 100% <100%> (ø) 3 <1> (?)
...in/java/org/mozilla/fenix/components/Components.kt 81.81% <100%> (ø) 1 <0> (?)
...va/org/mozilla/fenix/components/metrics/Metrics.kt 24.74% <100%> (ø) 0 <0> (?)
... and 1 more

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 a61391e...2977e8e. Read the comment docs.

@grigoryk grigoryk marked this pull request as ready for review September 11, 2019 00:12
@grigoryk grigoryk requested review from a team as code owners September 11, 2019 00:12
@grigoryk grigoryk changed the title WIP Use WebChannels for FxA integration FxA WebChannels integration Sep 11, 2019
@grigoryk grigoryk added needs:data-review PR is awaiting a data review implementation review and removed pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on labels Sep 11, 2019
@grigoryk grigoryk force-pushed the webChannels branch 2 times, most recently from 1b8820f to 72ec700 Compare September 11, 2019 00:27
@grigoryk
Copy link
Contributor Author

Manual testing that I've done with various local builds of this:

  • signup, signin and pairing flows with WebChannels enabled
  • signup, signin and pairing flows with WebChannels disabled
  • making sure that correct telemetry events fire for every one of those cases above

@grigoryk
Copy link
Contributor Author

Note: if you're testing this locally, it won't work until mozilla-mobile/android-components#4375 is available in the snapshot that Fenix is using. Easiest way is a local a-c build.

@grigoryk
Copy link
Contributor Author

grigoryk commented Sep 11, 2019

0% of diff hit (target 9.46%) - yeah, thanks codecov :P Let's add some tests here...

@grigoryk grigoryk force-pushed the webChannels branch 3 times, most recently from b7e1c7d to ab230e8 Compare September 11, 2019 03:40
@grigoryk
Copy link
Contributor Author

grigoryk commented Sep 11, 2019

All right, codecov is better be happy now!

For reviewing, I highly recommend reading commit with tests separately, as it does some refactoring to make things test-friendly.

@liuche liuche self-requested a review September 12, 2019 20:47
Copy link
Contributor

@liuche liuche left a comment

Choose a reason for hiding this comment

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

A few comments in-line, as long as they are addressed r+

Data-review only, not code review

Data Review Form (to be filled by Data Stewards)

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

Yes, added to metrics.yaml and now that #5101 has landed, no longer needs updating to metrics.md

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

Yes, glean telemetry is controlled by the Fenix data controls

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

No, expires 3/2020

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

Type 2 user interactions w/ FxA auth, Type 1 for auto-auth

  1. Is the data collection request for default-on or default-off?

Default on

  1. 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

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

Yes

  1. 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)**

No, has expiry

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

No

app/metrics.yaml Outdated Show resolved Hide resolved
@liuche liuche removed the needs:data-review PR is awaiting a data review label Sep 13, 2019
@grigoryk
Copy link
Contributor Author

Thank you, @liuche. This just needs a code review, and a minor change now that mozilla/fxa#2491 landed and we can fully disable a "choose what to sync" selection during sign-up.

@grigoryk
Copy link
Contributor Author

Depends on mozilla-mobile/android-components#4412

@liuche
Copy link
Contributor

liuche commented Sep 14, 2019

Thank you, @liuche. This just needs a code review, and a minor change now that mozilla/fxa#2491 landed and we can fully disable a "choose what to sync" selection during sign-up.

Just making sure you're planning on addressing my two comments.

@grigoryk
Copy link
Contributor Author

grigoryk commented Sep 17, 2019

mozilla-mobile/android-components#4412 landed. "choose what to sync" capability defaults to false if not specified. This means we can land this PR to enable WebChannel support without showing users CWTS UI during sign-up, and once sync manager support lands we would need to list CWTS as an FxA capability in a small Fenix PR in order to enable full CWTS UI.

@grigoryk
Copy link
Contributor Author

@ekager i've addressed your comment RE BaseBrowserFragment.kt - seems like I did something stupid the first time around.

Grisha Kruglov and others added 4 commits September 16, 2019 18:08
This patch includes:
- WebChannels support enabled by default, with ability to disable it via remote flag
- expanded FxA telemetry (closes mozilla-mobile#4971)

Co-authored-by: Arturo Mejia <arturomejiamarmol@gmail.com>
Copy link
Contributor

@ekager ekager left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comment!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add telemetry for FxA sign up
7 participants