Closed Bug 1505411 Opened Last year Closed 11 months ago

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

Categories

(Firefox :: Search, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 65
Tracking Status
firefox65 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fxsearch])

Attachments

(2 files)

This is the initial implementation bug for bug 1495548 - see that bug for more information.
Depends on: 1505456
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
Flags: needinfo?(mozilla)
Flags: needinfo?(adw)
This looks good to me. I really like the bug 1505456 changes.
Flags: needinfo?(mozilla)
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.
Flags: needinfo?(adw)
Status: NEW → ASSIGNED
Blocks: 1510570
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
Also simplify regexps as we don't no longer need to return match details.
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.
(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.
Flags: needinfo?(mozilla)
I did a quick glance and it seems to look good to me. I'm going to build locally and test.
Attachment #9024463 - Attachment description: Bug 1505411 - Add basic monitoring for partner search pages with ads and clicks. → Bug 1505411 - Add basic monitoring for partner search pages with ads and clicks. Depends on D11188
Depends on: 1510957
Blocks: 1511065
Blocks: 1511799
Pushed by mbanner@mozilla.com:
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
Flags: needinfo?(mozilla)
Blocks: 1512463
https://hg.mozilla.org/mozilla-central/rev/78fddbcccf74
https://hg.mozilla.org/mozilla-central/rev/5b5a072c0628
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Depends on: 1544077
You need to log in before you can comment on or make changes to this bug.