1371198 - Enable telemetry within the follow-on search system add-on

Status: RESOLVED FIXED

(Firefox :: Search, enhancement, P1)

Description

[Mark Banner (:standard8)]

In bug 1369028 we're landing the follow-on search system add-on with the telemetry parts disabled. This is due to tabpaint currently showing a performance regression.

Currently tabpaint is potentially unreliable and that is being investigated in bug 1369662.

Once that is complete, we need to re-evaluate the performance tests and sort out any issues before re-enabling the telemetry recording in the add-on.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

In bug 1369662, I landed a patch that should hopefully make the tabpaint Talos test more accurate, and less sensitive to things that the test isn't actually supposed to measure.

I've taken the liberty of pushing two patches to try: both with the the tabpaint test fix, but one that turns the follow-on search telemetry back on.

We can look at comparisons coming in here:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5bf757cfb0a4&newProject=try&newRevision=230f435493a7&framework=1&showOnlyImportant=0

Comment 2

[Mike Conley (:mconley) (:⚙️)]

With the fixes to the test, enabling the Telemetry (unsurprisingly) has no affect on tabpaint. \o/

Once bug 1369662 lands on central (it's on autoland right now), you should be good to go.

Comment 3

[Mark Banner (:standard8)]

I'll need to double check that, as there is currently a 10% limit on whether it is enabled or not. So it might not have been enabled for the tests. I'll push something tomorrow to confirm.

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1371198 - Update follow-on search telemetry to v0.9.0, and enable its telemetry reporting for most users.

Review commit: https://reviewboard.mozilla.org/r/147638/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147638/

Comment 5

[Mark Banner (:standard8)]

There's fresh try pushes with the patch from latest master, now that everything has merged:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=d17ab3d1a03f4ac1273fdeb5afe145b8ed69c44f&newProject=try&newRevision=1226ed8e7085de0e20bf8b1f8f12776503132e27&framework=1&showOnlyImportant=0

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8876195 [details]
Bug 1371198 - Update follow-on search telemetry to v0.9.0, and enable its telemetry reporting for most users.

https://reviewboard.mozilla.org/r/147638/#review151902

Comment 7

[Mark Banner (:standard8)]

Unfortunately there's still tabpaint regressions. I'll do some more investigation today as to possible causes - I have some ideas.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8876195 [details]
Bug 1371198 - Update follow-on search telemetry to v0.9.0, and enable its telemetry reporting for most users.

https://reviewboard.mozilla.org/r/147638/#review152532

::: browser/extensions/followonsearch/bootstrap.js:95
(Diff revision 1)
>    Services.mm.addMessageListener(kSaveTelemetryMsg, handleSaveTelemetryMsg);
>    Services.mm.loadFrameScript(frameScript, true);
>  
>    // Record the fact we're saving the extra data as a telemetry environment
>    // value.
>    TelemetryEnvironment.setExperimentActive(kExtensionID, "active");

Dave suggested that we should remove the telemetry experiment annotations (here and elsewhere) from the final version that goes out to release as it will increase the load on the data pipeline without any material benefit (all users will be part of the active population).

Comment 9

[Mark Banner (:standard8)]

I've just reworked the frame script to not do a lot of for-loops-expanding-objects when it is loaded. I think this will solve the tabpaint regressions, I've pushed it to try to find out:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b955595fdcb5feccc518e8a9a2925a38cb9a218c&newProject=try&newRevision=441bbb3edfd22925cbf62c17c3669faba45635c1&framework=1&showOnlyImportant=0

I've also created a PR with these changes in case anyone wants to look ahead of the results:

https://github.com/mozilla/followonsearch/pull/5

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8876195 [details]
Bug 1371198 - Update follow-on search telemetry to v0.9.0, and enable its telemetry reporting for most users.

I've also created a PR to do the changes to 100% and drop the experiment annotations:

https://github.com/mozilla/followonsearch/pull/6

Once all of these are complete, I'll post a new patch which will be a new release from the repo.

Comment 11

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #9)
> I've just reworked the frame script to not do a lot of
> for-loops-expanding-objects when it is loaded. I think this will solve the
> tabpaint regressions, I've pushed it to try to find out:
> 
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=b955595fdcb5feccc518e8a9a2925a38
> cb9a218c&newProject=try&newRevision=441bbb3edfd22925cbf62c17c3669faba45635c1&
> framework=1&showOnlyImportant=0

It seems to have worked. Linux/Mac/Windows 64 are showing ~1% or less increase. Windows 32 shows a 1.44% decrease. Which makes me wonder if the test is still a bit dodgy.

In any case, I think we're probably good to go, so I'll get the new release up tomorrow.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8876195 [details]
Bug 1371198 - Update follow-on search telemetry to v0.9.0, and enable its telemetry reporting for most users.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147638/diff/1-2/

Comment 13

[Mark Banner (:standard8)]

The latest patch is an export from the github repo of the latest changes for enabling for all users & fixing the perf issues. Details here:

https://github.com/mozilla/followonsearch/releases/tag/v0.9.0

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8876195 [details]
Bug 1371198 - Update follow-on search telemetry to v0.9.0, and enable its telemetry reporting for most users.

Looks good!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1a64d1a6920
Update follow-on search telemetry to v0.9.0, and enable its telemetry reporting for most users. r=past

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a1a64d1a6920

Comment 17

[Mark Banner (:standard8)]

Justin: can you re-verify the add-on in today's nightly (once it is out)?

Note that the cohort pref is no longer - it is enabled for all users except distributions. Additionally the experiment flag won't be set.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8876195 [details]
Bug 1371198 - Update follow-on search telemetry to v0.9.0, and enable its telemetry reporting for most users.

Approval Request Comment
[Feature/Bug causing the regression]: Follow-on search telemetry
[User impact if declined]: We are enabling this telemetry to get a better understanding of search behaviour.
[Is this code covered by automated tests?]: Not yet, these will be coming in 
[Has the fix been verified in Nightly?]: I've requested Justin to test
[Needs manual test from QE? If yes, steps to reproduce]: Justin has these.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simple well-contained changes to the existing add-on.
[String changes made/needed]: None

Comment 19

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #18)
> [Is this code covered by automated tests?]: Not yet, these will be coming in 

I forgot to complete this.

These will be coming in bug 1371294. The separate repository has its own unit tests which passed for the add-on, but I need to do something m-c compatible.

Additionally, release notes in https://github.com/mozilla/followonsearch/releases/tag/v0.9.0

Comment 20

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

Comment on attachment 8876195 [details]
Bug 1371198 - Update follow-on search telemetry to v0.9.0, and enable its telemetry reporting for most users.

Sounds like we have manual testing for this change, for now. Let's get the changes in for beta 55.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ee70f9647f29

Comment 22

[Justin [:JW_SoftvisionQA]]

I have completed testing for the follow-on search system add-on. It is Green to ship. One edge case noted:

Steps to reproduce:

1. Be in a UK/US Proxy
2. Use any search engine
3. Search from either the awesomebar or search bar
4. Open a new tab before the webpage has loaded

The search is registered twice. This happens on Win 7, Mac 10.12(intermittently), and Ubuntu 16.04. This does not happen on Win 10.