828228 - Instrument nsSearchService initialization with Telemetry

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Before proceeding to further refactorings in nsSearchService, we should add some Telemetry probes.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: First prototype

As I'm not sure exactly what we want to measure, let's start with a first prototype.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 699849 [details] [diff] [review]
First prototype

Seems fine to me. I can't think of other things to measure offhand, but I haven't thought of it much.

The descriptions in the histogram definitions should be unique :)

One other nit: I don't like using constants for short strings, like TELEMETRY_SERVICE_INIT/TELEMETRY_BUILD_CACHE - it's a level of indirection that makes the code harder to read/grep, and doesn't really serve a purpose (typos would be caught otherwise, and we don't need to change these values often).

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: Some telemetry measurements for nsSearchService initialization

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 700264 [details] [diff] [review]
Some telemetry measurements for nsSearchService initialization

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>@@ -2639,16 +2642,17 @@ SearchService.prototype = {
>         null,
>         function onError(e) {
>           LOG("_buildCache: failure during writeAtomic: " + e);
>         }
>       );
>     } catch (ex) {
>       LOG("_buildCache: Could not write to cache file: " + ex);
>     }
>+    TelemetryStopwatch.finish("SEARCH_SERVICE_BUILD_CACHE_MS");

This doesn't actually measure writing the file to disk. Should you put this stop() in the writeAtomic callback instead? Depends on what the intent is I guess.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

I assumed that we didn't need that measure, because it's not on the critical path. We can always add a further measure later, I guess.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Some telemetry measurements for nsSearchService initialization, v2

Moving the patch before bug 828223.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=05aba2d53945

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eca602d98d14

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/eca602d98d14