Closed Bug 1271160 Opened 4 years ago Closed 4 years ago

Add a telemetry for fullscreen transition

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(1 file)

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
Attachment #8750172 - Flags: feedback?(benjamin)
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.
Ok, so we're not trying to capture "2. whether the transition is ended by an after-paint event or timeout" data.
Attachment #8750172 - Flags: review?(bugs) → review+
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
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.
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)
Whiteboard: btpp-active
Duplicate of this bug: 1271767
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)
A note that TelemetryStopwatch.jsm could also be used instead of manually working with Dates.
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)
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)
Flags: needinfo?(bugzilla)
Attachment #8750172 - Flags: feedback?(benjamin)
(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 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+
Blocks: 1266799
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)
Flags: needinfo?(bugzilla)
Keywords: checkin-needed
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?
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/74637d664acf3964fa257c734176063fd59aafe4
Bug 1271160 - Add telemetry items for fullscreen transition. r=smaug data-review=bsmedberg
https://hg.mozilla.org/mozilla-central/rev/74637d664acf
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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)
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)
There is one day data comes in now. It seems to me the data makes sense.
Flags: needinfo?(bugzilla)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.