Closed Bug 1168653 Opened 9 years ago Closed 9 years ago

B2G telemetry needs to ignore search service

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

(Whiteboard: [uplift])

Attachments

(1 file, 1 obsolete file)

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.
Summary: Make telemetry more B2G friendly → B2G telemetry needs to ignore search service
Attached patch telemetry-search-service.patch (obsolete) — Splinter Review
Assignee: nobody → fabrice
Attachment #8610910 - Flags: review?(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)|?
Attachment #8610910 - Flags: review?(gfritzsche)
Status: NEW → ASSIGNED
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 ;)
Attachment #8610910 - Attachment is obsolete: true
Attachment #8611294 - Flags: review?(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.
Attachment #8611294 - Flags: review?(gfritzsche) → review+
https://hg.mozilla.org/mozilla-central/rev/8d2ec500cf97
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Blocks: 1120356
Whiteboard: [uplift]
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.
Attachment #8611294 - Flags: approval-mozilla-aurora?
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.
Attachment #8611294 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.