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

For #17418 - Add event ping telemetry for the Google Top Site click #17862

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

gabrielluong
Copy link
Member

@gabrielluong gabrielluong commented Feb 5, 2021

This adds an event ping telemetry for whenever the user clicks and opens a Google top site.

Original data-review @ #17637 (comment)

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

@gabrielluong gabrielluong requested review from a team as code owners February 5, 2021 16:35
@gabrielluong gabrielluong added the needs:review PRs that need to be reviewed label Feb 5, 2021
Copy link
Contributor

@st3fan st3fan left a comment

Choose a reason for hiding this comment

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

Approved with one question to confirm.

@@ -372,6 +372,10 @@ class DefaultSessionControlController(
TopSite.Type.PINNED -> metrics.track(Event.TopSiteOpenPinned)
}

if (url == SupportUtils.GOOGLE_URL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm - the tile always has GOOGLE_URL and then after the click we determine where it should actually go (GOOGLE_US, GOOGLE_XX) so at this point in thime url is really always GOOGLE_URL ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

Copy link
Member Author

Choose a reason for hiding this comment

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

You will see url = appendSearchAttributionToUrlIfNeeded(url), on line 384 below.

@gabrielluong gabrielluong changed the title For #17418 - Added telemetry for Google Default Top Site For #17418 - Add event ping telemetry for the Google Top Site click Feb 5, 2021
@codecov-io
Copy link

Codecov Report

Merging #17862 (f2628c4) into master (50e66b4) will increase coverage by 0.00%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #17862   +/-   ##
=========================================
  Coverage     32.53%   32.54%           
  Complexity     1324     1324           
=========================================
  Files           458      458           
  Lines         19250    19255    +5     
  Branches       2697     2699    +2     
=========================================
+ Hits           6263     6266    +3     
- Misses        12471    12473    +2     
  Partials        516      516           
Impacted Files Coverage Δ Complexity Δ
...la/fenix/components/metrics/GleanMetricsService.kt 11.90% <0.00%> (-0.05%) 5.00 <0.00> (ø)
...java/org/mozilla/fenix/components/metrics/Event.kt 34.96% <100.00%> (+0.20%) 0.00 <0.00> (ø)
...ix/home/sessioncontrol/SessionControlController.kt 75.00% <100.00%> (+0.28%) 0.00 <0.00> (ø)

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 50e66b4...f2628c4. Read the comment docs.

@gabrielluong gabrielluong merged commit a3d401a into mozilla-mobile:master Feb 5, 2021
@gabrielluong gabrielluong deleted the telemetry branch February 5, 2021 17:06
st3fan pushed a commit that referenced this pull request Feb 5, 2021
…17862)

Co-authored-by: codrut.topliceanu <codrut.topliceanu@softvision.ro>
Co-authored-by: Gabriel Luong <gabriel.luong@gmail.com>
st3fan pushed a commit that referenced this pull request Feb 5, 2021
…17862)

Co-authored-by: codrut.topliceanu <codrut.topliceanu@softvision.ro>
Co-authored-by: Gabriel Luong <gabriel.luong@gmail.com>
st3fan added a commit that referenced this pull request Feb 8, 2021
…17862) (#17866)

Co-authored-by: codrut.topliceanu <codrut.topliceanu@softvision.ro>
Co-authored-by: Gabriel Luong <gabriel.luong@gmail.com>

Co-authored-by: Gabriel Luong <gabriel.luong@gmail.com>
Co-authored-by: codrut.topliceanu <codrut.topliceanu@softvision.ro>
st3fan added a commit that referenced this pull request Feb 8, 2021
…17862) (#17867)

Co-authored-by: codrut.topliceanu <codrut.topliceanu@softvision.ro>
Co-authored-by: Gabriel Luong <gabriel.luong@gmail.com>

Co-authored-by: Gabriel Luong <gabriel.luong@gmail.com>
Co-authored-by: codrut.topliceanu <codrut.topliceanu@softvision.ro>
pkirakosyan pushed a commit to gexsi/user-agent-android that referenced this pull request Aug 4, 2021
…p Site click (mozilla-mobile#17862)

Co-authored-by: codrut.topliceanu <codrut.topliceanu@softvision.ro>
Co-authored-by: Gabriel Luong <gabriel.luong@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs:review PRs that need to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants