Closed
Bug 1353295
Opened 8 years ago
Closed 8 years ago
Remove addonHistograms from main ping
Categories
(Toolkit :: Telemetry, enhancement, P1)
Toolkit
Telemetry
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: gfritzsche, Assigned: chutten)
References
Details
(Whiteboard: [measurement:client])
Attachments
(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
Assignee | ||
Comment 1•8 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•8 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Updated•8 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 2•8 years ago
|
||
Analysis up for review: https://github.com/mozilla/mozilla-reports/pull/50
Reporter | ||
Comment 3•8 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)
Updated•8 years ago
|
Whiteboard: [measurement:client] → [qf][measurement:client]
Comment hidden (mozreview-request) |
Reporter | ||
Comment 5•8 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•8 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+
Comment hidden (mozreview-request) |
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cfe47bace342
Remove addonHistograms from Telemetry r=Dexter
Updated•8 years ago
|
Whiteboard: [qf][measurement:client] → [qf-][measurement:client]
Comment 9•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 10•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/890a29e0bd94
Remove addonHistograms from Telemetry r=Dexter
Comment 14•8 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•7 years ago
|
Summary: Check usage of addon histograms → Remove addonHistograms from main ping
Updated•4 years ago
|
Flags: needinfo?(ydelendik)
Updated•3 years ago
|
Performance Impact: --- → -
Whiteboard: [qf-][measurement:client] → [measurement:client]
You need to log in
before you can comment on or make changes to this bug.
Description
•