Closed Bug 1353295 Opened 5 years ago Closed 5 years ago

Remove addonHistograms from main ping


(Toolkit :: Telemetry, enhancement, P1)




Performance -
Tracking Status
firefox55 --- fixed


(Reporter: gfritzsche, Assigned: chutten)



(Whiteboard: [measurement:client])


(1 file)

Addon histograms shouldn't be really used anymore by any active projects.

We should verify if that is the case.
Depending on the results we can either:
- completely remove the code for them in current versions or
- make the APIs print console warnings only and inform any project that still uses them
As of some back-of-napkin analysis I did yesterday, pings with addonHistograms in their payloads are vanishingly rare. About a hundredth of a percent of main pings on release, for instance.

I'll look today to see what data's actually in those few pings, and to clean up the analysis for review. Preliminary results: nothing but pdfjs, shumway, and firebug.
Assignee: nobody → chutten
Priority: P2 → P1
Blocks: 1350765
For those:
- Firebug is abandoned.
- Shumway still exists but seems effectively dead.
- pdf.js is the only one still seeing some development.

I put up a PR for pdf.js:

With that we should be able to just remove the addon histogram APIs from our code.

Does that sound fine to you?
Or do you think we still need to do a PR for Shumway as well?
Flags: needinfo?(ydelendik)
Whiteboard: [measurement:client] → [qf][measurement:client]
Quick drive-by:
- we still need the about:telemetry code for addon histograms for one more cycle (to allow inspecting archived ping data)
- we should send a heads-up e-mail to fhr-dev after this lands
Comment on attachment 8857143 [details]
bug 1353295 - Remove addonHistograms from Telemetry

r+wc, this looks good but the following need to be addressed before landing, especially the about:telemetry part.

::: toolkit/components/telemetry/Telemetry.cpp:2929
(Diff revision 1)
>  size_t
>  TelemetryImpl::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf)
>  {
>    size_t n = aMallocSizeOf(this);
>    // Ignore the hashtables in mAddonMap; they are not significant.

nit: drop this comment, as it's no longer relevant

::: toolkit/components/telemetry/docs/data/main-ping.rst:150
(Diff revision 1)
>  The recorded events are defined in the `Events.yaml <>`_. The ``info.revision`` field indicates the revision of the file that describes the reported events.
>  childPayloads
>  -------------
>  The Telemetry payloads sent by child processes, recorded on child process shutdown (event ``content-child-shutdown`` observed). They are reduced session payloads, only available with e10s. Among some other things, they don't contain histograms, keyed histograms, addon details, addon histograms, or UI Telemetry.

nit: remove "addon histograms," from this line.

::: toolkit/components/telemetry/nsITelemetry.idl:318
(Diff revision 1)
>     * data). This is true on official, non-debug builds with built in support for Mozilla
>     * Telemetry reporting.
>     */
>    readonly attribute boolean isOfficialTelemetry;
>    /** Addon telemetry hooks */

nit: I think you can get rid of this line too.

::: toolkit/content/aboutTelemetry.js
(Diff revision 1)
>      setHasData("keyed-histograms-section", hasData || keyedHgramsSelect.options.length);
>    }
>    // Show event data.
>    Events.render(payload);
> -

As Georg mentioned, let's keep the stuff in about:telemetry for at least a cycle. Let's file a follow-up bug about removing stuff from about:telemetry so we don't forget.

::: toolkit/content/aboutTelemetry.xhtml
(Diff revision 1)
>          <span class="empty-caption">&aboutTelemetry.emptySection;</span>
>          <div id="addon-details" class="data">
>          </div>
>        </section>
> -      <section id="addon-histograms-section" class="data-section">

Let's keep this (about:telemetry)

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.dtd
(Diff revision 1)
>  <!ENTITY aboutTelemetry.sessionInfoSection "
>    Session Information
>  ">
> -<!ENTITY aboutTelemetry.addonHistogramsSection "

Let's keep this (about:telemetry)
Attachment #8857143 - Flags: review?(alessio.placitelli) → review+
Blocks: 1355882
Pushed by
Remove addonHistograms from Telemetry r=Dexter
Whiteboard: [qf][measurement:client] → [qf-][measurement:client]
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
had to back this out for assertion failures that happened on m-c after this merged like
Flags: needinfo?(chutten)
Resolution: FIXED → ---
Backout by
Backed out changeset cfe47bace342 for assertion failures
Hrm. Running the changes locally on my Win10 box (wow, Debug builds take a long time on Windows) results in no problems. A rerun of try results in no problems (well, an intermittent orange in websockets)[1].

I wonder if this was somehow multifactorial with other changes, or the result of some temporary nonsense.

I'm going to try landing this again.

Flags: needinfo?(chutten)
Pushed by
Remove addonHistograms from Telemetry r=Dexter
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Summary: Check usage of addon histograms → Remove addonHistograms from main ping
Flags: needinfo?(ydelendik)
Performance: --- → -
Whiteboard: [qf-][measurement:client] → [measurement:client]
You need to log in before you can comment on or make changes to this bug.