Closed Bug 1388274 Opened 4 years ago Closed 4 years ago

Adjust WebVR telemetry histogram's high bound for user time spend

Categories

(Core :: WebVR, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: daoshengmu, Assigned: daoshengmu)

References

Details

Attachments

(1 file)

Because we only define 50 sec for the high bound, in the use case of WebVR, it is very easy to over the upper bound.
Assignee: nobody → dmu
Blocks: 1306156
Attachment #8894834 - Flags: review?(kgilbert)
Attachment #8894834 - Flags: review?(francois)
:Francois,

I just notice I need a data collection peer to review the telemetry patch at Bug 1306156. Please refer https://bugzilla.mozilla.org/show_bug.cgi?id=1306156#c5, give me some feedback. I am willing to do any adjustment here if I need to modify. Thanks.
Comment on attachment 8894834 [details]
Bug 1388274 - Adjust WebVR telemetry histogram's high bound for user time spent;

https://reviewboard.mozilla.org/r/165988/#review171300

You likely need to create new probes for these changes (e.g. by renaming from `WEBVR_TIME_SPEND_FOR_VIEWING_IN_OCCULUS` to `WEBVR_TIME_SPEND_FOR_VIEWING_IN_OCCULUS_2`): https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/histograms.html#changing-a-histogram

In this case however, it should probably have been "TIME_SPENT_VIEWING_IN" instead of "TIME_SPEND_FOR_VIEWING_IN". So that would also work as a new name.
Attachment #8894834 - Flags: review?(francois) → review-
Comment on attachment 8894834 [details]
Bug 1388274 - Adjust WebVR telemetry histogram's high bound for user time spent;

https://reviewboard.mozilla.org/r/165988/#review171392

r=me with Francois' fixes addressed.

Need to change "TIME_SPEND_VIEWING_IN" to "TIME_SPENT_VIEWING_IN" to create a new set of probes.
Attachment #8894834 - Flags: review?(kgilbert) → review+
(In reply to Daosheng Mu[:daoshengmu] from comment #7)
> Comment on attachment 8894834 [details]
> Bug 1388274 - Adjust WebVR telemetry histogram's high bound for user time
> spent;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/165988/diff/2-3/

After some internal discussion, we decide to change the high bound as 20 mins and don't set the low bound in order to collect the crash results when loading the content.
Comment on attachment 8894834 [details]
Bug 1388274 - Adjust WebVR telemetry histogram's high bound for user time spent;

https://reviewboard.mozilla.org/r/165988/#review171916

datareview+
Attachment #8894834 - Flags: review?(francois) → review+
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67ade393270e
Adjust WebVR telemetry histogram's high bound for user time spent; r=francois,kip
It needs to be uplift to FF 56 after confirming the functionality.
https://hg.mozilla.org/mozilla-central/rev/67ade393270e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8894834 [details]
Bug 1388274 - Adjust WebVR telemetry histogram's high bound for user time spent;

Approval Request Comment
[Feature/Bug causing the regression]: Adjust WebVR telemetry histogram's high bound
[User impact if declined]: The histogram would be too low to get the correct data.
[Is this code covered by automated tests?]: nope. This is a telemetry function. I have tested in at about:telemetry.
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  nope.
[List of other uplifts needed for the feature/fix]: nope.
[Is the change risky?]: nope.
[Why is the change risky/not risky?]: Just a telemetry, would not break anything.
[String changes made/needed]:
Attachment #8894834 - Flags: approval-mozilla-beta?
Comment on attachment 8894834 [details]
Bug 1388274 - Adjust WebVR telemetry histogram's high bound for user time spent;

Fix WebVR telemetry histogram's high bound issue. Beta56+. Should be in 56.0b3.
Attachment #8894834 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Daosheng Mu[:daoshengmu] from comment #13)
> [Is this code covered by automated tests?]: nope. This is a telemetry
> function. I have tested in at about:telemetry.
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]:  nope.

Setting qe-verify- based on Daosheng Mu's assessment on manual testing needs.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.