-
Notifications
You must be signed in to change notification settings - Fork 1.3k
For #8172: Add a new "Fenix" Leanplum attribute #8208
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8208 +/- ##
============================================
+ Coverage 18.6% 18.76% +0.16%
- Complexity 452 454 +2
============================================
Files 316 315 -1
Lines 12435 12308 -127
Branches 1634 1617 -17
============================================
- Hits 2313 2310 -3
+ Misses 9923 9798 -125
- Partials 199 200 +1
Continue to review full report at Codecov.
|
Can you please fill out a data review request? :) https://github.com/mozilla/data-review/blob/master/request.md |
Request for data collection review formAll questions are mandatory. You must receive review from a data steward peer on your responses to these questions before shipping new data collection.
|
df1524c
to
392eac1
Compare
As @lwa-moz suggested in this comment I also added a |
Added @jonalmeida and @pocmo as reviewers |
app/src/main/java/org/mozilla/fenix/components/metrics/LeanplumMetricsService.kt
Outdated
Show resolved
Hide resolved
if (state.progress == MigrationProgress.COMPLETED) { | ||
metrics.track(Event.FennecToFenixMigrated) | ||
} | ||
} |
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 should work. As an alternative we could also emit a Fact
from the component. With that you wouldn't need to listen here indefinitely.
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.
@pocmo if that is easy let's do it. I'm going to land this for now. I filed mozilla-mobile/android-components#5929, if that lands I'll factor out this code and use the Fact instead 👍
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've put up a PR for this: mozilla-mobile/android-components#5932
@ValentinTimisica @boek if we want this to land in the last Migration Nightly build (which we will do tomorrow) then this needs a data review and land today. |
468e79e
to
0004cc8
Compare
…lum event This new event will be sent when the user has successfully migrated from Fennec to Fenix. This event will only be sent to Leanplum and not to the other telemetry services like Glean or Adjust. Co-authored-by: ValentinTimisica <valentin.timisica@softvision.ro>
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.
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, mma.md -
Is there a control mechanism that allows the user to turn the data collection on and off?
Yes, under data controls -
If the request is for permanent data collection, is there someone who will monitor the data over time? Yes, the fenix-core team
-
Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?
Type 1 & Type 2 -
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 there need to be a check-in in the future to determine whether to renew the data?
Will check-in at next data review meeting -
Does the data collection use a third-party collection tool?
Yes, Leanplum
Pull Request checklist
After merge
To download an APK when reviewing a PR: