-
Notifications
You must be signed in to change notification settings - Fork 1.3k
For #17418 - Added telemetry for Google Default Top Site #17637
For #17418 - Added telemetry for Google Default Top Site #17637
Conversation
@lwa-moz I'm not entirely sure what to write in the data review form. |
@codrut-topliceanu this PR has a good example of a completed data collection review forms. I believe the form inputs in this case are almost identical to those listed in the linked PR. |
9f88586
to
bbe9251
Compare
Codecov Report
@@ Coverage Diff @@
## master #17637 +/- ##
============================================
- Coverage 33.34% 32.56% -0.79%
+ Complexity 1345 1314 -31
============================================
Files 458 455 -3
Lines 19200 19091 -109
Branches 2685 2670 -15
============================================
- Hits 6403 6217 -186
- Misses 12265 12383 +118
+ Partials 532 491 -41 Continue to review full report at Codecov.
|
if (url == SupportUtils.GOOGLE_XX_URL) { | ||
metrics.track(Event.TopSiteOpenGoogleSearchAttribution) | ||
} |
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.
Are there tests for the DefaultSessionControlController
that we can add for this?
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.
There should be
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.
|
if (url == SupportUtils.GOOGLE_XX_URL) { | ||
metrics.track(Event.TopSiteOpenGoogleSearchAttribution) | ||
} |
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.
There should be
I have asked megan from DS to take a look in case we are missing anything that is needed for the search telemetries. |
I added the https://github.com/mozilla-mobile/fenix/blob/master/app/metrics.yaml#L903-L931 That metric will collect counts for engines and the source to identify how the search was initiated. We probbaly need to introduce a |
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, metrics.yaml and metrics.md -
Is there a control mechanism that allows the user to turn the data collection on and off?
Yes, data collection settings -
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 -
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? If unsure: escalate to legal if:
Yes -
Does there need to be a check-in in the future to determine whether to renew the data?
Fenix team will check-in at expiry -
Does the data collection use a third-party collection tool?
No
Thanks everyone for helping me with this patch. I really appreciate it. ❤️ @st3fan I have refactored my patch to use the |
app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt
Outdated
Show resolved
Hide resolved
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.
Can we also add a test for this?
app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt
Outdated
Show resolved
Hide resolved
This is used to track if the `InContentTelemetry` is a result of the user using the Google Top Site. It looks for `&channel=ts` within the uri.
d213c0a
to
b7873fa
Compare
app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlController.kt
Show resolved
Hide resolved
searchAccessPoint.let { sap -> | ||
MetricsUtils.createSearchEvent(searchEngine, store, sap) | ||
} | ||
} |
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.
At the end of the day this code searches the Google SearchEngine
instance only, with the check above. Should we simplify this?
If we want to keep it generic for supporting more search engines in the future then:
- Should we check
resultUrls
instead ofsuggestUrl
since the latter one is the URL we query search suggestions from and doesn't have to be related to the search engine URL. The result URLs may be closer. - Should we also search in
*.state.search.availableEngines
? Otherwise you will not find a search engine if the user decided to hide it. - In
appendSearchAttributionToUrlIfNeeded()
we already change the URL to the one with the search attribution, so we already know the context, it seems complicated to then check this here again. 🤔 This would also allow us to directly lookup the search engine (e.g. Google) and not have to search for it here, I assume?
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.
Very much agreed on this. I filed #17956 for this work so that we can move this patch into production sooner without another round of changes. I sent out an email to explain my thoughts behind that.
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.
Wonderful work - let's get it on Nightly 🤩
Thank you everyone who helped with this!! 🤗 You're all awesome! |
* For #17418 - Adds channel "ts" to TrackKey This is used to track if the `InContentTelemetry` is a result of the user using the Google Top Site. It looks for `&channel=ts` within the uri. * For #17418 - Adds TopSite PerformedSearch back in * For #17418 - Check now looks for equality with GOOGLE_URL * For #17418 - Adds test for topSite changes
mozilla-mobile#17637) * For mozilla-mobile#17418 - Adds channel "ts" to TrackKey This is used to track if the `InContentTelemetry` is a result of the user using the Google Top Site. It looks for `&channel=ts` within the uri. * For mozilla-mobile#17418 - Adds TopSite PerformedSearch back in * For mozilla-mobile#17418 - Check now looks for equality with GOOGLE_URL * For mozilla-mobile#17418 - Adds test for topSite changes
Pull Request checklist
To download an APK when reviewing a PR: