Closed Bug 1371198 Opened 3 years ago Closed 2 years ago

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

Categories

(Firefox :: Search, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

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.
Status: NEW → ASSIGNED
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
Flags: needinfo?(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.
Flags: needinfo?(mconley)
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 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
Attachment #8876195 - Flags: review?(past) → review+
Unfortunately there's still tabpaint regressions. I'll do some more investigation today as to possible causes - I have some ideas.
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).
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 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.
Attachment #8876195 - Attachment is obsolete: true
(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.
Attachment #8876195 - Flags: review+ → review?(past)
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 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!
Attachment #8876195 - Flags: review?(past) → review+
Pushed by mbanner@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/a1a64d1a6920
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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.
Flags: qe-verify?
Flags: needinfo?(jwilliams)
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
Attachment #8876195 - Flags: approval-mozilla-beta?
(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 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.
Attachment #8876195 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Flags: needinfo?(jwilliams)
Flags: qe-verify?
You need to log in before you can comment on or make changes to this bug.