Land the follow-on search counts system add-on in m-c

RESOLVED FIXED in Firefox 55

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: past, Assigned: standard8)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55+ fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(3 attachments, 3 obsolete attachments)

The system add-on is on track to ship for 53 users, but due to the upcoming 54 release it needs to be in the tree as well in order to not lose study data after the upgrade.
Assignee

Comment 1

2 years ago
Posted patch Build system patch (obsolete) — Splinter Review
This is a draft build system patch. With this applied, and then doing:

```
python scripts/export_mc.py -m "Test export in-content search telemetry" --no-switch-branch
```

from the latest repository, you can get the add-on into mozilla-central and it will be part of the build.

I've just added a "extensions.firefoxcontentsearch.override" string pref which can be set to "nightly" (or beta / release) to fake the channel (developer builds are "default" and won't currently report results).
Since the nightly and beta channel populations are not as big as release, we can just remove the code that selects just 10% of the population for this version. This way all nightly users will be eligible, and when 55 goes to beta in a couple of weeks, all beta users as well.

We will still need to push an add-on update via Go Faster in a few weeks to increase the eligible population of release 54 users to 100%.

Another decision made today was that we will not uplift this patch to 54 due to the substantial risk this late in the cycle.
Assignee

Comment 3

2 years ago
Posted patch Build system patch v2 (obsolete) — Splinter Review
When we're ready, we'll couple this with an export patch of the latest version, but getting review ahead of time anyway.
Attachment #8873859 - Flags: review?(dtownsend)
Comment on attachment 8873859 [details] [diff] [review]
Build system patch v2

Review of attachment 8873859 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/followonsearch/moz.build
@@ +4,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +with Files("**"):
> +    BUG_COMPONENT = ("Firefox", "General")

Shouldn't this be search?
Attachment #8873859 - Flags: review?(dtownsend) → review+
Assignee: nobody → standard8
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [fxsearch]
Assignee

Updated

2 years ago
Attachment #8873083 - Attachment is obsolete: true
Assignee

Comment 5

2 years ago
Posted patch Export of v0.8.0 (obsolete) — Splinter Review
Assignee

Comment 6

2 years ago
I've been testing the export on try server. Currently I'm seeing a small tabpaint regression on Windows (and slightly smaller on Linux). This is with the add-on enabled (including it submitting telemetry data to the telemetry subsystem):

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ccdf98ac1c0dfb57f4bfcb8a872427f652a2738f&newProject=try&newRevision=cc688fd96abfb0eb36482632683a1424d5ac71d6&framework=1

5.95% on Windows 32
2.41% on linux 64

I don't have Mac numbers, the new runs should bring those in.

Here's the comparison with just the add-on loaded, and it disabled internally (no frame script, telemetry etc):

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ccdf98ac1c0dfb57f4bfcb8a872427f652a2738f&newProject=try&newRevision=420914d5d6da3958aeabf57cb52245a7ac696711&framework=1&showOnlyImportant=0

No regressions on that run.
Assignee

Comment 8

2 years ago
So the newest try runs are confusing. The re-compare shows the following for tabpaint:

Linux64 +6.22%
Mac -7.75%
Windows32 +3.18%

Positive is bad, negative is good.

Disabling various things does seem to reduce the negative impact slightly on Linux/Windows, as well as reducing the positive impact on Mac.

I think next I'll try and get a baseline with an empty-ish frame script. Though if folks have any idea what's going on here (especially re Mac), that would be useful. I'm starting to wonder if this is a dodgy test or something.
Flags: needinfo?(jmaher)
Flags: needinfo?(ehsan)

Comment 9

2 years ago
I don't have any context about what the search count system add-on is, or why this bug is private, and what the try push in question is testing, so I'm not quite sure what you're asking besides the abstract question of "do you know anything about tabpaint"?  My answer to that question would be very little besides a) knowing that mconley added it in bug 1253382 and that b) he is helping me understand in bug 1369662 why a patch that should have made opening tabs faster has regressed that test suite.  He has a hypothesis on that bug about the test being dodgy a bit.

If you are trying to test the impact of an add-on on something specific (such as opening tabs) have you tried profiling it and whether seeing any code from the add-on shows up in the profile?
Flags: needinfo?(ehsan)
Group: mozilla-employee-confidential
Summary: Land the in-content search counts system add-on in m-c → Land the follow-on search counts system add-on in m-c
I see that tabpaint on osx is fairly stable as a test as of May 23rd (oddly linux64 follows the same pattern):
https://treeherder.mozilla.org/perf.html#/graphs?timerange=5184000&series=%5Bautoland,9271d84f3b0efdd755e9d36244506e0dce3a9483,1,1%5D&highlightedRevisions=83157e5965b6&highlightedRevisions=3e0d738cfd0f

as we are focused on windows performance, possibly we consider taking some wins and 1 loss on our less popular OS?
Flags: needinfo?(jmaher)
Assignee

Comment 11

2 years ago
(In reply to Joel Maher ( :jmaher) from comment #10)
> as we are focused on windows performance, possibly we consider taking some
> wins and 1 loss on our less popular OS?

Unfortunately Windows & Linux are the ones making the loss, and Mac has the win.
Assignee

Comment 12

2 years ago
Mike, can you help us here?

We're trying to add a fairly simple add-on that adds some telemetry via a frame script (attachment 8874475 [details] [diff] [review]), also on github at https://github.com/mozilla/followonsearch/tree/master/add-on

Running the full add-on through Talos, shows issues with tabpaint:

Linux64 6.22% (loss)
Mac -7.75% (win)
Windows32 +3.18% (loss)
Windows64 -0.5% (win)

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=83157e5965b6f883cbbb3a66131fda0afb62a8bf&newProject=try&newRevision=690a2865f203b418b535b96a6c371a2e75d404c4&framework=1

I've done some debugging via try server, and taking out virtually all of the code within the frame script "resolves" the wins and losses.

However, I don't understand why we're loosing on Linux/Windows, and winning on Mac. That feels like there's something wrong with the test.

I tried running the profiler on a build with the add-on installed, however, I could only see a few ms attributed to the frame script.

Any ideas?
Flags: needinfo?(mconley)
The tabpaint test seems to be sensitive to event queue length and ordering. I suspect you haven't caused a regression, but have caused the test to go over a single frame "cliff", due to changes in the events.

I've been working on a more accurate version for a separate tabpaint "regression" in bug 1369662. I should hopefully have that landed soon. We should then re-test the add-on with that test fix to ensure there's no actual regression.
Flags: needinfo?(mconley)
Assignee

Comment 14

2 years ago
Thanks Mike. I think what we'll do here is to land the add-on with the telemetry parts disabled, and then get them enabled after we resolve what's up with tabpaint.

Doing that will make it easier for me & others to do a bit more work on the add-on (like adding a few mochitests).
Assignee

Updated

2 years ago
Blocks: 1371198
Assignee

Updated

2 years ago
Attachment #8873859 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8874475 - Attachment is obsolete: true
Comment on attachment 8875649 [details] [diff] [review]
Import the Follow-on search telemetry system add-on v0.8.0.

Review of attachment 8875649 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds like a fine plan.
Attachment #8875649 - Flags: review?(past) → review+
Assignee

Updated

2 years ago
Blocks: 1371294
Comment on attachment 8875650 [details] [diff] [review]
Disable the extra telemetry reporting from follow-on search until the perf issues are fixed.

Review of attachment 8875650 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/followonsearch/bootstrap.js
@@ +170,5 @@
> +    log("Not enabling extra telemetry due to bug 1371198");
> +
> +    // if (cohortSample <= REPORTING_THRESHOLD[updateChannel]) {
> +    //   log("Enabling telemetry for user");
> +    //   this.enableForUser = true;

Might look cleaner if you just commented out this one line, but I'm fine either way.
Attachment #8875650 - Flags: review?(past) → review+
Assignee

Comment 21

2 years ago
(In reply to Panos Astithas [:past] (please needinfo?) from comment #20)
> Comment on attachment 8875650 [details] [diff] [review]
> Disable the extra telemetry reporting from follow-on search until the perf
> issues are fixed.
> 
> Review of attachment 8875650 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/extensions/followonsearch/bootstrap.js
> @@ +170,5 @@
> > +    log("Not enabling extra telemetry due to bug 1371198");
> > +
> > +    // if (cohortSample <= REPORTING_THRESHOLD[updateChannel]) {
> > +    //   log("Enabling telemetry for user");
> > +    //   this.enableForUser = true;
> 
> Might look cleaner if you just commented out this one line, but I'm fine
> either way.

Hopefully only temporary, so I think I'll leave it as it is.

Comment 22

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1272c6ebb3f
Add build infrastructure for follow-on search telemetry system add-on. r=Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0d544cc5ca
Import the Follow-on search telemetry system add-on v0.8.0. r=past
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2a4436a4349
Disable the extra telemetry reporting from follow-on search until the perf issues are fixed. r=past

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d1272c6ebb3f
https://hg.mozilla.org/mozilla-central/rev/6e0d544cc5ca
https://hg.mozilla.org/mozilla-central/rev/d2a4436a4349
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1425977
You need to log in before you can comment on or make changes to this bug.