Closed
Bug 1320052
Opened 8 years ago
Closed 6 years ago
Stop recording both session & subsession histograms
Categories
(Toolkit :: Telemetry, defect, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: gfritzsche, Assigned: janerik)
References
Details
(Whiteboard: [measurement:client])
Attachments
(7 files)
59 bytes,
text/x-review-board-request
|
chutten
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chutten
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chutten
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chutten
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chutten
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
chutten
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
Dexter
:
review+
|
Details |
With bug 1286868 and bug 1320051 resolved, we shouldn't need the duplicated histograms anymore. There is a remaining question of how to deal with Android though, we need to make sure to not break the existing behavior there. As Android doesn't send main pings right now, we could probably just use the same single histogram storage there and skip clearing it for Android.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jrediger
Priority: P3 → P1
Comment 1•6 years ago
|
||
Current, we store every histogram twice: subsession and full session. The only difference betwen the two are the "clearing" semantics. The full session storage is never cleared for the whole lifetime of the session, the subsession is cleared whenever we take a snapshot and call "clear". Up to now, this difference was only meaningful on Desktop: we used to send both the "main-ping" (which sends subsession measurements) and the "saved-session" ping (which had full session measurements, for legacy reasons). Fennec sends the "saved-session" as well (so full measuements support). Subsession data is currently disabled on Fennec/Android. The code is riddled with things such as [1]. Here comes the fun: GeckoView supports subsessions. One possible approach to fixing this is to drop the "full session" storage for histogram and only allow subsession histograms, with the caveat of never allow to clear the storage on Android/Fennec (vs Android/GV). [1] - https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/toolkit/components/telemetry/Telemetry.cpp#583-587
Comment 2•6 years ago
|
||
Does the proposal from comment 1 make sense?
Flags: needinfo?(jrediger)
Flags: needinfo?(gfritzsche)
Flags: needinfo?(chutten)
Assignee | ||
Comment 3•6 years ago
|
||
That's as I understood it as well. Dexter also verified that we're never requesting data from both storage places at once [1]. For now I'm moving forward with this proposal. [1]: https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/toolkit/components/telemetry/TelemetrySession.jsm#1189-1190,1224-1228
Flags: needinfo?(jrediger)
Comment 4•6 years ago
|
||
Having a single type of storage with semantics that differ based on platform sounds fine to me.
Flags: needinfo?(chutten)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8972821 [details] Bug 1320052 - Unify histogram storage into a single container. https://reviewboard.mozilla.org/r/241384/#review247254 A couple of nits. ::: commit-message-0b0b7:4 (Diff revision 1) > +Bug 1320052 - Unify histogram storage into a single container. r?chutten > + > +On Desktop and GeckoView we only ever need to store histograms for a > +subsession and clear the histograms when a snapshot is done (e.g. a main ping is build). *built ::: toolkit/components/telemetry/TelemetryHistogram.cpp:1233 (Diff revision 1) > { > StaticMutexAutoLock locker(gTelemetryHistogramMutex); > MOZ_ASSERT(internal_IsHistogramEnumId(id)); > > // This is not good standard behavior given that we have histogram instances > // covering multiple processes and two session types. This comment requires updating. ::: toolkit/components/telemetry/nsITelemetry.idl:69 (Diff revision 1) > * counts - array representing contents of the buckets in the histogram > * ranges - array with calculated bucket sizes > * sum - sum of the bucket contents > * > * @param aDataset DATASET_RELEASE_CHANNEL_OPTOUT or DATASET_RELEASE_CHANNEL_OPTIN. > - * @param aSubsession Whether to return the internally-duplicated subsession histograms > + * @param aClear Whether to clear out the histograms after snapshotting Should note it only works if not Fennec ::: toolkit/components/telemetry/nsITelemetry.idl:235 (Diff revision 1) > * Serializes the keyed histograms from the given dataset to a JSON-style object. > * The returned structure looks like: > * { process1: {name1: {histogramData1}, name2:{histogramData2}...}} > * > * @param aDataset DATASET_RELEASE_CHANNEL_OPTOUT or DATASET_RELEASE_CHANNEL_OPTIN. > - * @param aSubsession Whether to return the internally-duplicated subsession keyed histograms > + * @param aClear Whether to clear out the keyed histograms after snapshotting ditto
Attachment #8972821 -
Flags: review?(chutten) → review+
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8972822 [details] Bug 1320052 - Update internal snapshot callers to not deal with subsessions. https://reviewboard.mozilla.org/r/241386/#review247260
Attachment #8972822 -
Flags: review?(chutten) → review+
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8972823 [details] Bug 1320052 - Remove now obsolete full-session/subsession tests for histograms. https://reviewboard.mozilla.org/r/241388/#review247262 Just so long as we still have coverage that clearing subsessions still works.
Attachment #8972823 -
Flags: review?(chutten) → review+
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8972821 [details] Bug 1320052 - Unify histogram storage into a single container. https://reviewboard.mozilla.org/r/241384/#review247264 ::: toolkit/components/telemetry/TelemetryHistogram.cpp:888 (Diff revision 1) > - if (!map.ReflectIntoJS(&KeyedHistogram::ReflectKeyedHistogram, cx, obj)) { > return NS_ERROR_FAILURE; > } > > -#if !defined(MOZ_WIDGET_ANDROID) > - if (subsession && clearSubsession) { > + // Never clear storage on Fennec > + if (GetCurrentPlatform() != SupportedPlatform::Fennec && clearSubsession) { Fly-by: do we need this? We can enforce it from [TelemetrySession](https://searchfox.org/mozilla-central/rev/ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/toolkit/components/telemetry/TelemetrySession.jsm#1189-1190,1224,1226) as well: these files are only used on Fennec/Desktop. Fennec has UNIFIED_TELEMETRY off and GeckoView will never use that file :)
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8972824 [details] Bug 1320052 - Update external snapshot callers to not deal with subsessions. https://reviewboard.mozilla.org/r/241390/#review247274 For the tests that ask for session histogram snapshots, I wonder if they have subsession splits during their tests that we were unintentionally protecting them against... probably not, but we should keep an eye on whether these start growing oranges.
Attachment #8972824 -
Flags: review?(chutten) → review+
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8972825 [details] Bug 1320052 - Clamped count is not double-counted anymore. https://reviewboard.mozilla.org/r/241392/#review247276
Attachment #8972825 -
Flags: review?(chutten) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8972821 [details] Bug 1320052 - Unify histogram storage into a single container. https://reviewboard.mozilla.org/r/241384/#review247254 > Should note it only works if not Fennec We're not checking for Fennec anymore, so in theory it works on Fennec, but the JavaScript code will ensure it's not triggered.
Reporter | ||
Comment 23•6 years ago
|
||
The plan in comment 1 makes sense to me. I'm happy to see this double storage go!
Flags: needinfo?(gfritzsche)
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8973189 [details] Bug 1320052 - Test that clearing subsession data still works. https://reviewboard.mozilla.org/r/241680/#review247978
Attachment #8973189 -
Flags: review?(chutten) → review+
Comment 25•6 years ago
|
||
Pushed by alessio.placitelli@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b0396db0229c Unify histogram storage into a single container. r=chutten https://hg.mozilla.org/integration/autoland/rev/c7b8f6d55a0b Update internal snapshot callers to not deal with subsessions. r=chutten https://hg.mozilla.org/integration/autoland/rev/4749633bd02f Remove now obsolete full-session/subsession tests for histograms. r=chutten https://hg.mozilla.org/integration/autoland/rev/c95a4b0c6642 Update external snapshot callers to not deal with subsessions. r=chutten https://hg.mozilla.org/integration/autoland/rev/7a64a23bf183 Clamped count is not double-counted anymore. r=chutten https://hg.mozilla.org/integration/autoland/rev/07db52945b1f Test that clearing subsession data still works. r=chutten
Comment 26•6 years ago
|
||
Backed out 6 changesets (bug 1320052) on request by Dexter for requently failing test verify dom/base/test/browser_use_counters.js default tip Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=07db52945b1f8bc406eaf2bc6f790b833d06a198&selectedJob=177466737 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=177466737&repo=autoland&lineNumber=2194 Backout: https://hg.mozilla.org/integration/autoland/rev/183c021e1ae07ccd57255dda17e6c9ef2c168cab 14:29:34 INFO - 33 INFO TEST-PASS | dom/base/test/browser_use_counters.js | page counts for PROPERTY_FILL after are correct - 14:29:34 INFO - Buffered messages finished 14:29:34 ERROR - 34 INFO TEST-UNEXPECTED-FAIL | dom/base/test/browser_use_counters.js | document counts for PROPERTY_FILL after are correct - Got 16, expected 17 14:29:34 INFO - Stack trace: 14:29:34 INFO - chrome://mochikit/content/browser-test.js:test_is:1290 14:29:34 INFO - chrome://mochitests/content/browser/dom/base/test/browser_use_counters.js:check_use_counter_img:253 14:29:34 INFO - 35 INFO TEST-PASS | dom/base/test/browser_use_counters.js | top level document counts are correct - 14:29:34 INFO - Not taking screenshot here: see the one that was previously logged 14:29:34 ERROR - 36 INFO TEST-UNEXPECTED-FAIL | dom/base/test/browser_use_counters.js | document counts are correct - 14:29:34 INFO - Stack trace: 14:29:34 INFO - chrome://mochitests/content/browser/dom/base/test/browser_use_counters.js:check_use_counter_img:259 14:29:34 INFO - 37 INFO checking file_use_counter_svg_currentScale.svg as image with histogram PROPERTY_FILL 14:29:34 INFO - GECKO(3132) | ++DOCSHELL 0C513000 == 8 [pid = 4024] [id = {63a6c7dc-b158-4334-bc8c-b903edf06a51}] 14:29:34 INFO - GECKO(3132) | ++DOMWINDOW == 31 (05CBCC60) [pid = 4024] [serial = 101] [outer = 00000000] 14:29:34 INFO - GECKO(3132) | ++DOMWINDOW == 32 (0C515000) [pid = 4024] [serial = 102] [outer = 05CBCC60] 14:29:34 INFO - GECKO(3132) | ++DOMWINDOW == 33 (0BEE6400) [pid = 4024] [serial = 103] [outer = 05CBCC60] 14:29:34 INFO - GECKO(3132) | ++DOCSHELL 0BF1C800 == 9 [pid = 4024] [id = {b8e624c1-7f79-438b-90b5-ab1bb9cc1b24}] 14:29:34 INFO - GECKO(3132) | ++DOMWINDOW == 34 (05CBCD90) [pid = 4024] [serial = 104] [outer = 00000000] 14:29:34 INFO - GECKO(3132) | ++DOMWINDOW == 35 (0BF1D800) [pid = 4024] [serial = 105] [outer = 05CBCD90] 14:29:35 INFO - GECKO(3132) | --DOCSHELL 0BF1DC00 == 8 [pid = 4024] [id = {72ed4433-b232-49bf-aa6c-4ffb58e65d9e}]
Flags: needinfo?(jrediger)
Comment 27•6 years ago
|
||
Note: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=07db52945b1f8bc406eaf2bc6f790b833d06a198&selectedJob=177476205 |browser_remote_navigation_delay_telemetry.js| is failing as well.
Comment 28•6 years ago
|
||
(In reply to Alessio Placitelli [:Dexter] from comment #27) > Note: > https://treeherder.mozilla.org/#/ > jobs?repo=autoland&revision=07db52945b1f8bc406eaf2bc6f790b833d06a198&selected > Job=177476205 > > |browser_remote_navigation_delay_telemetry.js| is failing as well. Ok turns out this test was the only problem, as the failures in |dom/base/test/browser_use_counters.js| are not from this bug.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(jrediger)
Comment 30•6 years ago
|
||
mozreview-review |
Comment on attachment 8974213 [details] Bug 1320052 - Fix remaining caller of histogram snapshot API. https://reviewboard.mozilla.org/r/242508/#review248428
Attachment #8974213 -
Flags: review?(alessio.placitelli) → review+
Comment 31•6 years ago
|
||
Pushed by alessio.placitelli@gmail.com: https://hg.mozilla.org/integration/autoland/rev/640fdf7b0189 Unify histogram storage into a single container. r=chutten https://hg.mozilla.org/integration/autoland/rev/2da7a07c369a Update internal snapshot callers to not deal with subsessions. r=chutten https://hg.mozilla.org/integration/autoland/rev/c84d5061ae3a Remove now obsolete full-session/subsession tests for histograms. r=chutten https://hg.mozilla.org/integration/autoland/rev/2901dd472949 Update external snapshot callers to not deal with subsessions. r=chutten https://hg.mozilla.org/integration/autoland/rev/ad8fc821ed3a Clamped count is not double-counted anymore. r=chutten https://hg.mozilla.org/integration/autoland/rev/a8dc47e76224 Test that clearing subsession data still works. r=chutten https://hg.mozilla.org/integration/autoland/rev/a2eccfbeb0ae Fix remaining caller of histogram snapshot API. r=Dexter
Comment 32•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/640fdf7b0189 https://hg.mozilla.org/mozilla-central/rev/2da7a07c369a https://hg.mozilla.org/mozilla-central/rev/c84d5061ae3a https://hg.mozilla.org/mozilla-central/rev/2901dd472949 https://hg.mozilla.org/mozilla-central/rev/ad8fc821ed3a https://hg.mozilla.org/mozilla-central/rev/a8dc47e76224 https://hg.mozilla.org/mozilla-central/rev/a2eccfbeb0ae
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•6 years ago
|
status-firefox53:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•