Don't trigger early search service init in Telemetry

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Telemetry
P1
normal
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: gfritzsche, Assigned: Dexter)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [measurement:client] [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
Currently we trigger search service init pretty early in TelemetryEnvironment [1].
We should instead wait for SEARCH_SERVICE_TOPIC with "init-complete" before accessing it at all.

1: https://dxr.mozilla.org/mozilla-central/rev/8e969cc9aff49f845678cba5b35d9dd8aa340f16/toolkit/components/telemetry/TelemetryEnvironment.jsm#794
(Reporter)

Comment 1

4 months ago
Florian, before we do any changes here:
Can you confirm that the search service will always initialize regardless of Telemetry accessing it?
Or do we need to still make sure that this happens?

I'm worried about data regressions and search data is rather important to us.
Flags: needinfo?(florian)
status-firefox55: --- → affected
status-firefox57: affected → ---
(Reporter)

Updated

3 months ago
Whiteboard: [measurement:client] → [measurement:client] [qf]
Hey florian, should we mark this as blocking bug 1355956?
Blocks: 1348289
Whiteboard: [measurement:client] [qf] → [measurement:client] [qf:p1]
(In reply to Mike Conley (:mconley) from comment #2)
> Hey florian, should we mark this as blocking bug 1355956?

Yes, this is definitely visible during startup profiles.
Blocks: 1355956
No longer blocks: 1348289
(Assignee)

Updated

3 months ago
Assignee: nobody → alessio.placitelli
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 5

3 months ago
Comment on attachment 8868549 [details]
Bug 1359031 - Don't trigger early search service init in Telemetry.

Florian, would you take a look at this patch? It prevents querying the search service before the init complete signal is received.

I'm mainly concerned about what Georg pointed at in comment 2:

> Can you confirm that the search service will always initialize regardless of Telemetry accessing it?
> Or do we need to still make sure that this happens?
Attachment #8868549 - Flags: review?(florian)
(In reply to Alessio Placitelli [:Dexter] from comment #5)

> > Can you confirm that the search service will always initialize regardless of Telemetry accessing it?
> > Or do we need to still make sure that this happens?

There's still http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/browser/modules/BrowserUITelemetry.jsm#297 that initializes it unconditionally after we are done restoring windows, so nothing to worry about for now. In the future, I may want to add some explicit search service initialization that doesn't depend on telemetry code, so that if we remove the searchbar by default and the user has a customized home page, we are still ready to use search immediately the first time the awesomebar receives input.
Flags: needinfo?(florian)

Comment 7

3 months ago
mozreview-review
Comment on attachment 8868549 [details]
Bug 1359031 - Don't trigger early search service init in Telemetry.

https://reviewboard.mozilla.org/r/140164/#review145242

Thanks for the patch, looks good to me.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1046
(Diff revision 1)
>        case SEARCH_SERVICE_TOPIC:
>          if (aData != "init-complete") {
>            return;
>          }
>          // Now that the search engine init is complete, record the default search choice.
> +        this._canQuerySearch = true;

I wonder if we should remove the observer here, like the DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC case does a few lines later.
Attachment #8868549 - Flags: review?(florian) → review+
(Reporter)

Comment 8

3 months ago
(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> (In reply to Alessio Placitelli [:Dexter] from comment #5)
> 
> > > Can you confirm that the search service will always initialize regardless of Telemetry accessing it?
> > > Or do we need to still make sure that this happens?
> 
> There's still
> http://searchfox.org/mozilla-central/rev/
> 79f625ac56d936ef5d17ebe13a123b25efee4241/browser/modules/BrowserUITelemetry.
> jsm#297 that initializes it unconditionally after we are done restoring
> windows, so nothing to worry about for now.

That looks a little fragile.
Do we have test coverage that fails if this gets removed or breaks?
Do we have a more exposed place to initialize this?
Does that get triggered on Fennec?

I'm a bit worried that seemingly small changes (like not initializing BrowserUITelemetry in some specific cases) start breaking search Telemetry.

Maybe we should just update TelemetryEnvironment to call `Services.search.init()` from "sessionstore-windows-restored".
Flags: needinfo?(florian)
(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> (In reply to Florian Quèze [:florian] [:flo] from comment #6)
> > (In reply to Alessio Placitelli [:Dexter] from comment #5)
> > 
> > > > Can you confirm that the search service will always initialize regardless of Telemetry accessing it?
> > > > Or do we need to still make sure that this happens?
> > 
> > There's still
> > http://searchfox.org/mozilla-central/rev/
> > 79f625ac56d936ef5d17ebe13a123b25efee4241/browser/modules/BrowserUITelemetry.
> > jsm#297 that initializes it unconditionally after we are done restoring
> > windows, so nothing to worry about for now.
> 
> That looks a little fragile.
> Do we have test coverage that fails if this gets removed or breaks?

I don't know.

> Do we have a more exposed place to initialize this?

Search service initialization is currently triggered during or near startup by a few different things:
- telemetry, as discussed here.
- the searchbar constructor (surprisingly, even if the searchbar has been customized away, because the <searchbar> element is constructed before the UI customization code removes it)
- ContentSearch.jsm for about:home (if the user hasn't customized the home page), or for about:newtab (the first time a new tab is opened)
- the location bar when the user starts typing in it.

In this specific bug, I'm trying to get the search service initialization to happen after we first paint. In the future, I would like to push it even further, to happen off a requestIdleCallback after we start accepting user events. At that point, I think the best place to trigger the search service initialization will be from nsBrowserGlue.js.

> Does that get triggered on Fennec?

I don't know.

> I'm a bit worried that seemingly small changes (like not initializing
> BrowserUITelemetry in some specific cases) start breaking search Telemetry.

Do we really need search telemetry when the search service hasn't been initialized at all during a session? That seems to strongly indicate that the search UI has been customized away and that whatever data we would be collecting about search would be misleading.

> Maybe we should just update TelemetryEnvironment to call
> `Services.search.init()` from "sessionstore-windows-restored".

If you do this, please use requestIdleCallback (or equivalent). Unfortunately, that's harder than it needs to be until bug 1353206 is fixed.
Flags: needinfo?(florian)
(Reporter)

Comment 10

3 months ago
(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> > I'm a bit worried that seemingly small changes (like not initializing
> > BrowserUITelemetry in some specific cases) start breaking search Telemetry.
> 
> Do we really need search telemetry when the search service hasn't been
> initialized at all during a session? That seems to strongly indicate that
> the search UI has been customized away and that whatever data we would be
> collecting about search would be misleading.

Maybe, but this risks changing the recorded data and increases the scope of the bug (to investigate downstream dependencies etc.).
Can we just fix the specific problem here and break this part out?

> > Maybe we should just update TelemetryEnvironment to call
> > `Services.search.init()` from "sessionstore-windows-restored".
> 
> If you do this, please use requestIdleCallback (or equivalent).
> Unfortunately, that's harder than it needs to be until bug 1353206 is fixed.

Using Services.search.init() directly would just mimic what BrowserUITelemetry does now anyway.
Is this enough for the current context of this bug until other uses are fixed?
Flags: needinfo?(florian)
Per the discussion we had in #telemetry yesterday, it's fine with me for now if you want to add a Services.search.init() from a "sessionstore-windows-restored" observer in TelemetryEnvironment. But expect me to revisit this in a couple weeks.
Flags: needinfo?(florian)
(Assignee)

Comment 12

3 months ago
mozreview-review-reply
Comment on attachment 8868549 [details]
Bug 1359031 - Don't trigger early search service init in Telemetry.

https://reviewboard.mozilla.org/r/140164/#review145242

> I wonder if we should remove the observer here, like the DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC case does a few lines later.

Good point! Looks like it also attempts to do so in _removeObservers_, so maybe there's some weird edge case around it. The majority of observer topics seem to be removed in that function, so I'll stick to that for now!
Comment hidden (mozreview-request)
(In reply to Alessio Placitelli [:Dexter] from comment #12)
> Comment on attachment 8868549 [details]
> Bug 1359031 - Don't trigger early search service init in Telemetry.
> 
> https://reviewboard.mozilla.org/r/140164/#review145242
> 
> > I wonder if we should remove the observer here, like the DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC case does a few lines later.
> 
> Good point! Looks like it also attempts to do so in _removeObservers_, so
> maybe there's some weird edge case around it.

I think the edge case is shutdown before we have fully finished starting up (likely to happen in tests, but not much in real life use cases). This seems to be why removeObservers also removes DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC in removeObservers, but with a try/catch to avoid throwing in the general case where it has already been removed after being observed once.

> The majority of observer
> topics seem to be removed in that function, so I'll stick to that for now!

The reason for removing the observer after the first notification is to avoid being pointlessly called for notifications we are not interested in. This search service notification will also be fired at http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/toolkit/components/search/nsSearchService.js#2866 whenever the search service cache file has been written to disk.
(Assignee)

Comment 15

3 months ago
(In reply to Florian Quèze [:florian] [:flo] from comment #14)
> (In reply to Alessio Placitelli [:Dexter] from comment #12)
> > Comment on attachment 8868549 [details]
> > Bug 1359031 - Don't trigger early search service init in Telemetry.
> > 
> > https://reviewboard.mozilla.org/r/140164/#review145242
> > 
> > > I wonder if we should remove the observer here, like the DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC case does a few lines later.
> > 
> > Good point! Looks like it also attempts to do so in _removeObservers_, so
> > maybe there's some weird edge case around it.
> 
> I think the edge case is shutdown before we have fully finished starting up
> (likely to happen in tests, but not much in real life use cases). This seems
> to be why removeObservers also removes
> DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC in removeObservers, but with a
> try/catch to avoid throwing in the general case where it has already been
> removed after being observed once.
> 
> > The majority of observer
> > topics seem to be removed in that function, so I'll stick to that for now!
> 
> The reason for removing the observer after the first notification is to
> avoid being pointlessly called for notifications we are not interested in.
> This search service notification will also be fired at
> http://searchfox.org/mozilla-central/rev/
> 2933592c4a01b634ab53315ce2d0e43fccb82181/toolkit/components/search/
> nsSearchService.js#2866 whenever the search service cache file has been
> written to disk.

Yup, good points. I just found though that making this trivial change now makes telemetry environment tests fail in some non trivial ways (other than the android failures reported on the try push from the review). I'm inclined to leave this change out for now (even though I said the contrary over IRC).
(Assignee)

Updated

3 months ago
Status: NEW → ASSIGNED
(Assignee)

Updated

3 months ago
Depends on: 1367762
(Assignee)

Comment 16

3 months ago
I filed bug 1367762 to investigate the Android crashes. I'm disabling the offending test on Android.
Comment hidden (mozreview-request)

Comment 18

3 months ago
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5772650731f5
Don't trigger early search service init in Telemetry. r=florian

Comment 19

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5772650731f5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.