1168653 - B2G telemetry needs to ignore search service

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[[:fabrice] Fabrice Desré]

Not sure why that didn't show up when fixing bug 1166897, but we now have this error:

E/GeckoConsole(16883): [JavaScript Error: "TypeError: Services.search is undefined" {file: "resource://gre/modules/TelemetryEnvironment.jsm" line: 802}]

That's because the search service is not implemented on b2g.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: telemetry-search-service.patch

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8610910 [details] [diff] [review]
telemetry-search-service.patch

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +876,5 @@
> +      if (!Services.search.isInitialized) {
> +        return;
> +      }
> +    } catch(e) {
> +      // Just ignore cases where the search service is not implemented.

I'd rather log.error() here, search is an important metric for us.
How about an early-return on |!("search" in Services)|?

Comment 3

[[:fabrice] Fabrice Desré]

Attachment: telemetry-search-service.patch v2

I didn't add log.error() since it doesn't seem like an error to me. But it you really want it I'll add something like "No search service implemented for telemetry" so that it doesn't look like a scary issue to people looking at logcat ;)

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8611294 [details] [diff] [review]
telemetry-search-service.patch v2

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

This looks fine.
Above i was only worried about hiding errors with the search service usage when it is available.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/8d2ec500cf97

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d2ec500cf97

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8611294 [details] [diff] [review]
telemetry-search-service.patch v2

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry
This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2.
[User impact if declined]:
Data & measurement insight projects delayed or blocked with direct impact on projects depending on this.
[Describe test coverage new/current, TreeHerder]:
We have good automation coverage of the feature.
We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here.
[Risks and why]:
Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports.
We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements.
[String/UUID change made/needed]:
The only string changes were to the about:telemetry page.
We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8611294 [details] [diff] [review]
telemetry-search-service.patch v2

Unified telemetry is an important new feature. It is blocking some other projects. Taking it.

Comment 9

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c8f4915ec89c