Closed Bug 828228 Opened 7 years ago Closed 7 years ago

Instrument nsSearchService initialization with Telemetry


(Firefox :: Search, defect)

Not set



Firefox 21


(Reporter: Yoric, Assigned: Yoric)




(1 file, 2 obsolete files)

Before proceeding to further refactorings in nsSearchService, we should add some Telemetry probes.
Attached patch First prototype (obsolete) — Splinter Review
As I'm not sure exactly what we want to measure, let's start with a first prototype.
Assignee: nobody → dteller
Attachment #699849 - Flags: feedback?(
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).
Attachment #699849 - Flags: feedback?( → feedback+
Attachment #699849 - Attachment is obsolete: true
Attachment #700264 - Flags: review?(
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.
Attachment #700264 - Flags: review?( → review+
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.
Moving the patch before bug 828223.
Attachment #700264 - Attachment is obsolete: true
Attachment #702754 - Flags: review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
You need to log in before you can comment on or make changes to this bug.