1353295 - Remove addonHistograms from main ping

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Georg Fritzsche [:gfritzsche]]

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

Comment 1

[Chris H-C :chutten]

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.

Comment 2

[Chris H-C :chutten]

Analysis up for review: https://github.com/mozilla/mozilla-reports/pull/50

Comment 3

[Georg Fritzsche [:gfritzsche]]

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:
https://github.com/mozilla/pdf.js/pull/8239

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

@Yury:
Does that sound fine to you?
Or do you think we still need to do a PR for Shumway as well?

Comment 4

[Chris H-C :chutten]

Attachment: bug 1353295 - Remove addonHistograms from Telemetry

addonHistograms isn't being used and has started getting in the way.
So it goes.

Review commit: https://reviewboard.mozilla.org/r/129076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129076/

Comment 5

[Georg Fritzsche [:gfritzsche]]

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 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8857143 [details]
bug 1353295 - Remove addonHistograms from Telemetry

https://reviewboard.mozilla.org/r/129076/#review131896

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 <https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/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)

Comment 7

[Chris H-C :chutten]

Comment on attachment 8857143 [details]
bug 1353295 - Remove addonHistograms from Telemetry

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129076/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfe47bace342
Remove addonHistograms from Telemetry r=Dexter

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 10

[Carsten Book [:Tomcat]]

had to back this out for assertion failures that happened on m-c after this merged like https://treeherder.mozilla.org/logviewer.html#?job_id=91115044&repo=mozilla-central

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/1f1c921f172c
Backed out changeset cfe47bace342 for assertion failures

Comment 12

[Chris H-C :chutten]

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.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=948d2447699c7a02fd418e83041a7f470d8eed44

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/890a29e0bd94
Remove addonHistograms from Telemetry r=Dexter

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/890a29e0bd94