Remove addonHistograms from main ping

RESOLVED FIXED in Firefox 55
(NeedInfo from)

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gfritzsche, Assigned: chutten, NeedInfo)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf-][measurement:client])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
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
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

2 years ago
Priority: P2 → P1
(Assignee)

Comment 2

2 years ago
Analysis up for review: https://github.com/mozilla/mozilla-reports/pull/50
(Reporter)

Comment 3

2 years ago
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?
Flags: needinfo?(ydelendik)
Whiteboard: [measurement:client] → [qf][measurement:client]
Comment hidden (mozreview-request)
(Reporter)

Comment 5

2 years ago
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

2 years ago
mozreview-review
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)
Attachment #8857143 - Flags: review?(alessio.placitelli) → review+
(Assignee)

Updated

2 years ago
Blocks: 1355882
Comment hidden (mozreview-request)

Comment 8

2 years ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cfe47bace342
Remove addonHistograms from Telemetry r=Dexter
Whiteboard: [qf][measurement:client] → [qf-][measurement:client]

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cfe47bace342
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
Status: RESOLVED → REOPENED
Flags: needinfo?(chutten)
Resolution: FIXED → ---

Comment 11

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/1f1c921f172c
Backed out changeset cfe47bace342 for assertion failures
(Assignee)

Comment 12

2 years ago
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
Flags: needinfo?(chutten)

Comment 13

2 years ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/890a29e0bd94
Remove addonHistograms from Telemetry r=Dexter

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/890a29e0bd94
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
(Reporter)

Updated

2 years ago
Summary: Check usage of addon histograms → Remove addonHistograms from main ping
You need to log in before you can comment on or make changes to this bug.