1359031 - Don't trigger early search service init in Telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Georg Fritzsche [:gfritzsche]]

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

Comment 1

[Georg Fritzsche [:gfritzsche]]

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.

Comment 2

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

Hey florian, should we mark this as blocking bug 1355956?

Comment 3

[Florian Quèze [:florian]]

(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.

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1359031 - Don't trigger early search service init in Telemetry.

This patch will make TelemetryEnvironment wait for the
"browser-search-service" topic with "init-complete" before attempting
to query search data.

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

Comment 5

[Alessio Placitelli [:Dexter]]

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?

Comment 6

[Florian Quèze [: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.

Comment 7

[Florian Quèze [:florian]]

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.

Comment 8

[Georg Fritzsche [:gfritzsche]]

(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".

Comment 9

[Florian Quèze [: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.

Comment 10

[Georg Fritzsche [:gfritzsche]]

(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?

Comment 11

[Florian Quèze [: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.

Comment 12

[Alessio Placitelli [:Dexter]]

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 13

[Alessio Placitelli [:Dexter]]

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

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

Comment 14

[Florian Quèze [:florian]]

(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.

Comment 15

[Alessio Placitelli [:Dexter]]

(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).

Comment 16

[Alessio Placitelli [:Dexter]]

I filed bug 1367762 to investigate the Android crashes. I'm disabling the offending test on Android.

Comment 17

[Alessio Placitelli [:Dexter]]

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

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5772650731f5
Don't trigger early search service init in Telemetry. r=florian

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5772650731f5