1505411 - Add basic monitoring for SERP pages with ads / SERP ad clicks

Status: RESOLVED FIXED

(Firefox :: Search, enhancement, P1)

Description

[Mark Banner (:standard8)]

This is the initial implementation bug for bug 1495548 - see that bug for more information.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1505411 - Add basic monitoring for partner search pages with ads and clicks. Depends on D11188

Depends on D11188

Comment 2

[Mark Banner (:standard8)]

Drew, Mike: I still want to add some tests for this (and a lot of documentation), but I think this is ready for any feedback you have on the general architecture.

It depends on the patches in bug 1505456 (though `arc patch --nobranch D11656` should get them all automatically).

I'm think we could probably go ahead and land bug 1505456 sooner if the architecture here is alright.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3228cbad36f43422033076056f8f3af8442cf621

Comment 3

[Mike Kaply [:mkaply]]

This looks good to me. I really like the bug 1505456 changes.

Comment 4

[Drew Willcoxon :adw]

The architecture looks good to me.  I didn't look at the diff too in depth, but the structure looks good.  The only comment I have is that it would be nice to separate the tab/content-related methods of the TelemetryHandler class into their own/class module.  Not that you need a separate file (although feel free IMO), but I think it would be easier to read and modify (and be, well, more modular) if the parent side of the architecture were in a module/class by itself.

Comment 5

[Mark Banner (:standard8)]

I've just finished doing an update:

- Some slight reworking of the architecture,
- lots of documentation
- added some tests

I think this is in good enough shape that we can head towards review now.

Try push of the latest patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56cfdd8242a7a389bcb5fb15e8fb026abf9bad55

Comment 6

[Mark Banner (:standard8)]

Attachment: Bug 1505411 - Add a basic test to check telemetry results for visits to SERP urls.

Also simplify regexps as we don't no longer need to return match details.

Comment 7

[Mark Banner (:standard8)]

Mike: although I've requested review from Drew, if you could look over and check the new ad matching appears to be working correctly, that would be useful.

Comment 8

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #7)
> Mike: although I've requested review from Drew, if you could look over and
> check the new ad matching appears to be working correctly, that would be
> useful.

Comment 9

[Mike Kaply [:mkaply]]

I did a quick glance and it seems to look good to me. I'm going to build locally and test.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78fddbcccf74
Add a basic test to check telemetry results for visits to SERP urls. r=adw
https://hg.mozilla.org/integration/autoland/rev/5b5a072c0628
Add basic monitoring for partner search pages with ads and clicks. Depends on D11188 r=adw,Felipe,chutten

Comment 11

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/78fddbcccf74
https://hg.mozilla.org/mozilla-central/rev/5b5a072c0628