1591880 - Investigate a case where with_ads probe isn't incremented for Google

Status: RESOLVED FIXED

(Firefox :: Search, defect, P2)

Description

[Mark Banner (:standard8)]

When investigating bug 1589972, I noticed that on my main development profile, I was not getting the with_ads probe incremented.

It seemed to be that the page only had relative links which were aclk/, which were resolving to urls of https://www.google.com/aclk this did not match the regexp that we have in search telemetry:

/^https:\/\/www\.googleadservices\.com\/(?:pagead\/)?aclk/

Although the with_ads probe wasn't being counted, the https://www.google.com/aclk links were redirecting to https://www.googleadservices.com/aclk and hence were being counted as ad_clicks.

My other profile was loading the page with the links as googleadservices, which is slightly strange. However, we probably do want to investigate updating the regexp (probably at some stage after bug 1589972) is rolled out, to ensure that we get clear data.

Comment 1

[Adrian Florinescu [:aflorinescu]]

[Environment]

71.0a1 20191020214712
72.0a1 20191027212548
Ubuntu 16.04/Windows 10 (doubt it's a OS specific issue)

[Steps;]

Use case scenario for browser.search.with_ads not being initialized, I can reproduce reliably on a new profile and more intermittent on a regular use - didn't catch a clear scenario for which I could reproduce:

1. Open Firefox with a new profile.
2. In address bar type Iphone 11.
3. Check that the search results contain add results.
4. Open about:telemetry#keyed-scalars-tab_search=browser.search.with_ads

[Actual Result:]

The probe is not initialzed.

[Expected Result:]

about:telemetry#keyed-scalars-tab_search=browser.search.with_ads value should be 1

[Logs:]

72.0a1 https://pastebin.com/grWdvgyg
71.0a1 https://pastebin.com/rpQSNgUX

[Note:]

The behavior for this particular case is the same in both cases: before bug 1589972 fix and after bug 1589972 fix

Comment 2

[Adrian Florinescu [:aflorinescu]]

Attachment: iphone 11 search results.zip

Comment 3

[Mark Banner (:standard8)]

Thank you for the attachment, I've just looked at it, and similar to what I was seeing with the ad_click probe, this page looks to be https://www.google.com/aclk vs https://www.googleadservices.com/aclk.

I'll check what we want to do for getting this rolled out.

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1591880 - Simplify test_urlTelemetry.js to not duplicate the same code all the time.

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1591880 - WIP, Expand Regexp for better matching but breaks because of double-counting

Depends on D50938

Comment 6

[Mark Banner (:standard8)]

Mike's currently looking at finishing this off.

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1591880 - Expand the RegExp to match better and prevent double-counting clicks by ignoring beacon requests. r?Standard8!

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1591880 - Simplify test_urlTelemetry.js to not duplicate the same code all the time. r?Standard8!

Depends on D51400

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1591880 - Move the Bing cookie codes to be powered by the configuration dictionary in SearchTelemetry and add a unit test. r?Standard8!

Depends on D51401

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1591880 - Add test coverage for proper redirect handling by SearchTelemetry. r?Standard8!

Depends on D51402

Comment 11

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6edc312c447ffd6e56a9ee9a51bf23f4aa4cdb9a

Comment 12

[Mike de Boer [:mikedeboer]]

Adrian, can you check one of the builds from comment 11 when they're done ready for download? I'm curious to learn if you can still reproduce the bug with the patches applied!

Comment 13

[Adrian Florinescu [:aflorinescu]]

(In reply to Mike de Boer [:mikedeboer] from comment #12)

Adrian, can you check one of the builds from comment 11 when they're done ready for download? I'm curious to learn if you can still reproduce the bug with the patches applied!

Let me redirect this to Cristian, since I'll be off until 11.05.

Comment 14

[Adrian Florinescu [:aflorinescu]]

Tested the try-build from comment 12 (72.0a1 20191031132313) on Ubuntu 16.04, and from the browser.search.with_ads (about:telemetry#keyed-scalars-tab_search=browser.search.with_ads) point of view, the fix looks fine to me: all searches that returned at least one search results would increment the probe. Also, the pages that didn't contain any ads would not increment the probe.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Do you want to land this on m-c now? And, is it something you might want in 70.0.2 (since we are prepping one)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4745a05b58b
https://hg.mozilla.org/mozilla-central/rev/517b2dd99425
https://hg.mozilla.org/mozilla-central/rev/f7ba73a67232
https://hg.mozilla.org/mozilla-central/rev/c44a784fc74a

Comment 17

[Mike de Boer [:mikedeboer]]

(In reply to Liz Henry (:lizzard) from comment #15)

Do you want to land this on m-c now? And, is it something you might want in 70.0.2 (since we are prepping one)

Thanks for asking Liz! We don't need this on 70.0.2, unless it's really no trouble! I mean, obviously it'd be nice to have improved and more reliable metrics earlier, but everything has a cost...

Comment 18

[Mike de Boer [:mikedeboer]]

(In reply to Liz Henry (:lizzard) from comment #15)

Do you want to land this on m-c now? And, is it something you might want in 70.0.2 (since we are prepping one)

Thanks for asking Liz! We don't need this on 70.0.2, unless it's really no trouble! I mean, obviously it'd be nice to have improved and more reliable metrics earlier, but everything has a cost...

Comment 19

[Mike de Boer [:mikedeboer]]

Liz, please disregard my comment on wanting an uplift potentially, we'd like this to be fully verified on Nightly and ride the trains regularly. Thanks!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Moving the ESR tracking flag to the next cycle in the event that we want ESR68 to pick up this fix alongside Fx72.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Is this something we want to uplift to ESR68 for the 68.4 release shipping alongside Fx72?

Comment 22

[Mike de Boer [:mikedeboer]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)

Is this something we want to uplift to ESR68 for the 68.4 release shipping alongside Fx72?

Yeah, that sounds like a good plan. Should I check if the patches graft cleanly?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

That would be great (please include bug 1595123 in that testing also).

Comment 24

[Mike de Boer [:mikedeboer]]

Alright, I was able to successfully transplant the patches from central to esr-68, without any changes. Bug 1595123 also applies cleanly.

Comment 25

[Mike de Boer [:mikedeboer]]

Comment on attachment 9105779 [details]
Bug 1591880 - Expand the RegExp to match better and prevent double-counting clicks by ignoring beacon requests. r?Standard8!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Improved search telemetry coverage. Much appreciated if this could make it into the next upcoming ESR build!
• User impact if declined: We would be getting better insight into ad impressions and adclicks for more of our main search partners.
• Fix Landed on Version: 72
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patches transplant cleanly, thus looks like low risk; this bug has caused one regression - bug 1595123 - which I'll also nominate for uplift.
• String or UUID changes made by this patch: n/a

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9105779 [details]
Bug 1591880 - Expand the RegExp to match better and prevent double-counting clicks by ignoring beacon requests. r?Standard8!

Search Telemetry improvements. Approved for 68.4esr.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/10802da34074
https://hg.mozilla.org/releases/mozilla-esr68/rev/3138cbeaca68
https://hg.mozilla.org/releases/mozilla-esr68/rev/c8153b2bfe90
https://hg.mozilla.org/releases/mozilla-esr68/rev/00aed87a7968

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/7eb5f4604f20