Closed
Bug 1271160
Opened 8 years ago
Closed 8 years ago
Add a telemetry for fullscreen transition
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: xidorn, Assigned: xidorn)
References
(Blocks 1 open bug)
Details
(Whiteboard: btpp-active)
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
smaug
:
review+
benjamin
:
feedback+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
I think we want to collect two items: 1. the time in black for fullscreen 2. whether the transition is ended by an after-paint event or timeout
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51315/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51315/
Attachment #8750172 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8750172 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 2•8 years ago
|
||
Two telemetry items are added in this patch: 1. FULLSCREEN_TRANSITION_BLACK_MS to measure how long users experience in completely black screen, and it could also be used to measure whether our current timeout setting works well. 2. FULLSCREEN_CHANGE_MS to measure how long do we actually take for the content to do fullscreen change. This is something we should improve. And we may use this as a guide for a probably better default timeout value. FULLSCREEN_TRANSITION_BLACK_MS is collected only if fullscreen transition is active (neither disabled by user nor by system capacity), while FULLSCREEN_CHANGE_MS is recorded regardless of that.
Comment 3•8 years ago
|
||
Ok, so we're not trying to capture "2. whether the transition is ended by an after-paint event or timeout" data.
Updated•8 years ago
|
Attachment #8750172 -
Flags: review?(bugs) → review+
Comment 4•8 years ago
|
||
Comment on attachment 8750172 [details] MozReview Request: Bug 1271160 - Add telemetry items for fullscreen transition. r=smaug https://reviewboard.mozilla.org/r/51315/#review48133 Those fixed or explained, r+. I'm pretty sure you want exponential, linear would be better for percentiles and so. ::: toolkit/components/telemetry/Histograms.json:342 (Diff revision 1) > "description": "Max time spent on one forget skippable (ms)" > }, > + "FULLSCREEN_TRANSITION_BLACK_MS": { > + "alert_emails": ["xquan@mozilla.com"], > + "expires_in_version": "never", > + "kind": "linear", Curious, why do we want linear here and not exponential? Looks like most (or all other?) users of AccumulateTimeDelta use exponential. ::: toolkit/components/telemetry/Histograms.json:351 (Diff revision 1) > + "description": "The time spent in the fully-black screen in fullscreen transition" > + }, > + "FULLSCREEN_CHANGE_MS": { > + "alert_emails": ["xquan@mozilla.com"], > + "expires_in_version": "never", > + "kind": "linear", ditto
Assignee | ||
Comment 5•8 years ago
|
||
https://reviewboard.mozilla.org/r/51315/#review48133 > Curious, why do we want linear here and not exponential? Looks like most (or all other?) users of AccumulateTimeDelta use exponential. OK, after investigating a bit how the bucket range is calculated, I guess you are right. We'll need a "low" as well if we use exponential.
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8750172 [details] MozReview Request: Bug 1271160 - Add telemetry items for fullscreen transition. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51315/diff/1-2/
Attachment #8750172 -
Attachment description: MozReview Request: Bug 1271160 - Add telemetry items for fullscreen transition. r?smaug → MozReview Request: Bug 1271160 - Add telemetry items for fullscreen transition. r=smaug
Attachment #8750172 -
Flags: feedback?(benjamin)
Updated•8 years ago
|
Whiteboard: btpp-active
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8750172 [details] MozReview Request: Bug 1271160 - Add telemetry items for fullscreen transition. r=smaug I didn't know MozReview cancels r? automatically... f? for data collection review.
Attachment #8750172 -
Flags: feedback?(benjamin)
Comment 9•8 years ago
|
||
A note that TelemetryStopwatch.jsm could also be used instead of manually working with Dates.
Assignee | ||
Comment 10•8 years ago
|
||
Ni? myself for the improvement in comment 9. I wasn't aware of that. I think this should be mentioned in the MDN page. https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe
Flags: needinfo?(bugzilla)
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8750172 [details] MozReview Request: Bug 1271160 - Add telemetry items for fullscreen transition. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51315/diff/2-3/
Attachment #8750172 -
Flags: feedback?(benjamin)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(bugzilla)
Attachment #8750172 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #10) > I think this should be mentioned in the MDN page. > https://developer.mozilla.org/en-US/docs/Mozilla/Performance/ > Adding_a_new_Telemetry_probe Added: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe$compare?locale=en-US&to=1058480&from=1050438
Comment 13•8 years ago
|
||
Comment on attachment 8750172 [details] MozReview Request: Bug 1271160 - Add telemetry items for fullscreen transition. r=smaug I'm making some assumptions here: 1) This never expires because it's an important ongoing performance measurement that your team is responsible for 2) The telemetry alerting mechanism is good enough to warn you about regressions If I'm right about this, then data-review=me If this is more of an exploratory measurement, you should make it expire in 6 months rather than expires="never"
Attachment #8750172 -
Flags: feedback?(benjamin) → feedback+
Comment 14•8 years ago
|
||
Hey xidorn, this is blocking an e10s related bug that's in the e10s triage queue and might block rollout. we would appreciate it if you could push this along and get it landed. (potentially land this on aurora and beta too?)
Flags: needinfo?(bugzilla)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(bugzilla)
Keywords: checkin-needed
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8750172 [details] MozReview Request: Bug 1271160 - Add telemetry items for fullscreen transition. r=smaug Approval Request Comment [Feature/regressing bug #]: n/a [User impact if declined]: n/a [Describe test coverage new/current, TreeHerder]: n/a [Risks and why]: low risk, just add a pair of new probes [String/UUID change made/needed]: n/a
Attachment #8750172 -
Flags: approval-mozilla-beta?
Attachment #8750172 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 16•8 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #13) > Comment on attachment 8750172 [details] > MozReview Request: Bug 1271160 - Add telemetry items for fullscreen > transition. r=smaug > > I'm making some assumptions here: > 1) This never expires because it's an important ongoing performance > measurement that your team is responsible for > 2) The telemetry alerting mechanism is good enough to warn you about > regressions > > If I'm right about this, then data-review=me Yes, this is an ongoing performance measurement we want to monitor. For short term, it also serves as an exploratory measurement, though.
Will wait for this to land on Nightly before uplifting to Beta/Aurora.
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/74637d664acf
Keywords: checkin-needed
Assignee | ||
Comment 19•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/74637d664acf3964fa257c734176063fd59aafe4 Bug 1271160 - Add telemetry items for fullscreen transition. r=smaug data-review=bsmedberg
status-firefox47:
--- → affected
status-firefox48:
--- → affected
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/74637d664acf
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 21•8 years ago
|
||
The e10s team would like these probes in beta 47 to diagnose how bad bug 1266799 is.
Comment on attachment 8750172 [details] MozReview Request: Bug 1271160 - Add telemetry items for fullscreen transition. r=smaug Additional telemetry data on full screen transition, Aurora48+, Beta47+
Attachment #8750172 -
Flags: approval-mozilla-beta?
Attachment #8750172 -
Flags: approval-mozilla-beta+
Attachment #8750172 -
Flags: approval-mozilla-aurora?
Attachment #8750172 -
Flags: approval-mozilla-aurora+
Hello Xidorn, I hope you have verified the telemetry data coming from Nightly is as expected. This helps minimize code churn on Aurora and Beta channels. Thanks!
Flags: needinfo?(bugzilla)
Assignee | ||
Comment 24•8 years ago
|
||
I haven't seen any telemetry data from Nightly coming in. The entries are not even listed in the dashboard yet. I'll check it in the following days, and see whether they look fine.
Hitting conflicts uplifting this to aurora: $ hg graft -er 74637d664acf grafting 343280:74637d664acf "Bug 1271160 - Add telemetry items for fullscreen transition. r=smaug data-review=bsmedberg" merging browser/base/content/browser-fullScreen.js merging dom/base/nsGlobalWindow.cpp merging toolkit/components/telemetry/Histograms.json warning: conflicts while merging browser/base/content/browser-fullScreen.js! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/base/nsGlobalWindow.cpp! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use hg resolve and hg graft --continue)
Assignee | ||
Comment 26•8 years ago
|
||
There is one day data comes in now. It seems to me the data makes sense.
Flags: needinfo?(bugzilla)
Assignee | ||
Comment 27•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/c41ee7132426
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•