For #4456: Adds total_uri_count to metrics core ping #6003
Conversation
| @@ -307,6 +307,11 @@ class Settings private constructor( | |||
| default = 0 | |||
| ) | |||
|
|
|||
| var totalUriCount by longPreference( | |||
| appContext.getPreferenceKey(R.string.pref_key_total_uri), | |||
| default = 0 | |||
There was a problem hiding this comment.
Is there a security risk here? We don't normally store telemetry data in prefs. 😬
There was a problem hiding this comment.
I don't think there's a security risk in this information here - number of urls opened is not PII or anything else.
There was a problem hiding this comment.
Do we ever reset this number? It's not clear to me that we do, and if we don't, this number will only be useful if we normalize it (e.g. 100 urls / days opened).
sblatz
force-pushed
the
total-uri-telem
branch
from
283cb59
to
e8ddf9c
Compare
Codecov Report
@@ Coverage Diff @@
## master #6003 +/- ##
============================================
+ Coverage 14.49% 14.53% +0.03%
Complexity 323 323
============================================
Files 272 272
Lines 11039 11046 +7
Branches 1593 1593
============================================
+ Hits 1600 1605 +5
- Misses 9311 9313 +2
Partials 128 128
Continue to review full report at Codecov.
|
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.
Until 03/01/2020
|
| @@ -307,6 +307,11 @@ class Settings private constructor( | |||
| default = 0 | |||
| ) | |||
|
|
|||
| var totalUriCount by longPreference( | |||
| appContext.getPreferenceKey(R.string.pref_key_total_uri), | |||
| default = 0 | |||
There was a problem hiding this comment.
I don't think there's a security risk in this information here - number of urls opened is not PII or anything else.
| @@ -307,6 +307,11 @@ class Settings private constructor( | |||
| default = 0 | |||
| ) | |||
|
|
|||
| var totalUriCount by longPreference( | |||
| appContext.getPreferenceKey(R.string.pref_key_total_uri), | |||
| default = 0 | |||
There was a problem hiding this comment.
Do we ever reset this number? It's not clear to me that we do, and if we don't, this number will only be useful if we normalize it (e.g. 100 urls / days opened).
When should we reset this? Do we reset it currently in any way for the other metric? My understanding is that it's a counter that always grows. @boek Looks like Chenxia's suggestion is to do this:
Are we wanting total_uri_count to be reset each session? |
|
Good question! Quick glance shows me 3 increments in that file: two of them are counters so we stop showing a hint after a certain number of times, which I agree with - that one won't grow unbounded. The other counter is "number of time PB has been opened". I really do think that this one is also suspect - just having a huge number doesn't really make sense, and it's hard to understand what we could actually do with that number! Like would we do something different if we knew a user EVER opened PB 100x vs 10000x, vs opened PB in a session 2x vs 10x? I'd argue that "total number of urls opened even" is much less useful of a metric than "number of urls opened in a session". |
|
@baron-severin also a related total_uri_count |
|
@fbertsch from a data perspective, is there an easy way to look at total_uri_count for the lifetime of the app vs. per session? Do you have any opinions on how much information each of those provides? (Would we need to normalize lifetime uri count for instance, and if so is that easy to do?) |
@liuche It looks like this is sent from the same place as the code I was looking at, so unless I've made a mistake, it will suffer from #6126 as well. EDIT: and #3676 too |
|
@liuche almost certainly we want |
|
@sblatz I'd go with Frank's suggestion here! It sounds like it will both give us the information that is useful for data analysis, as well as be aggregated to give the total lifetime count. |
liuche
added
the
pr:needs-changes
sblatz
force-pushed
the
total-uri-telem
branch
from
e8ddf9c
to
ab062d2
Compare
|
@liuche I am now zeroing this value out inside of |
There was a problem hiding this comment.
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 metrics.yaml and the generated metrics.md
- Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available.
Yes, coreping is also controlled by Fenix data controls
- If the request is for permanent data collection, is there someone who will monitor the data over time?
Has expiry
- Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?
Type 2, number of url loads and reloads made by the user
- 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, just a count of uris browsed.
- 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)**
Has expiry
- Does the data collection use a third-party collection tool?
No
Pull Request checklist
After merge
To download an APK when reviewing a PR: