Closed
Bug 1280644
Opened 8 years ago
Closed 8 years ago
[webvtt] Add the Telemetry for webvtt
Categories
(Core :: Audio/Video: Playback, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: bwu, Assigned: alwu)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
58 bytes,
text/x-review-board-request
|
rillian
:
review+
benjamin
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
rillian
:
review+
|
Details |
6.72 KB,
patch
|
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
It would be better to add some telemetry for WebVTT to know how people use it. With these info, we can make Firefox better!
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → alwu
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61504/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61504/
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61506/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61506/
Assignee | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c640525a4438
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ad9f8604f7b
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8766702 [details] Bug 1280644 - part1 : add Telemetry for webvtt. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61504/diff/1-2/
Attachment #8766702 -
Flags: review?(giles)
Attachment #8766703 -
Flags: review?(giles)
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8766703 [details] Bug 1280644 - part2 : modify naming and add scope indentifier. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61506/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Attachment #8766702 -
Flags: feedback?(benjamin)
Assignee | ||
Updated•8 years ago
|
Summary: Add WebVTT telemetry → [webvtt] Add the Telemetry for webvtt
Comment 7•8 years ago
|
||
Comment on attachment 8766703 [details] Bug 1280644 - part2 : modify naming and add scope indentifier. https://reviewboard.mozilla.org/r/61506/#review58904 ::: dom/html/TextTrackManager.cpp:782 (Diff revision 2) > Telemetry::Accumulate(Telemetry::WEBVTT_KIND_METADATA, 1); > break; > default: > + MOZ_LOG(gTextTrackManagerLog, LogLevel::Debug, > + ("ReportTelemetryForTrack, unknown text track kind %d.", kind)); > break; You could also report this as a telemetry type. Up to you.
Attachment #8766703 -
Flags: review?(giles) → review+
Comment 8•8 years ago
|
||
Comment on attachment 8766702 [details] Bug 1280644 - part1 : add Telemetry for webvtt. https://reviewboard.mozilla.org/r/61504/#review58908 Please respond to comments and ask for review again. Thanks! ::: dom/html/TextTrackManager.h:155 (Diff revision 2) > + void ReportTelemetryForCue(); > + > + // If there is at least one cue has been added to the cue list once, we would > + // report the usage of cue to Telemetry. > + bool mAtLeastAddedOneCue; > + Better to call this `mCueTelemetryReported`. ::: dom/html/TextTrackManager.cpp:149 (Diff revision 2) > RefPtr<TextTrack> ttrack = > mTextTracks->AddTextTrack(aKind, aLabel, aLanguage, aMode, aReadyState, > aTextTrackSource, CompareTextTracks(mMediaElement)); > AddCues(ttrack); > + ReportTelemetryForTrack(ttrack); > Do all these calls happen on the main thread? Telemetry::Accumulate has a global lock now, so calling it off the main thread is safe, but can still be slow since it has to wait for the lock. Better to dispatch a task if these can be called from another thread. If it's always on the main thread, this is fine. ::: toolkit/components/telemetry/Histograms.json:10700 (Diff revision 2) > "releaseChannelCollection": "opt-out" > }, > + "WEBVTT_KIND_SUBTITLES": { > + "alert_emails": ["alwu@mozilla.com"], > + "expires_in_version": "never", > + "kind": "count", Please select an expiry some foreseeable distance in the future, like 55 or 60. That ensures we'll re-evalute periodically whether we want to continue collecting this data. ::: toolkit/components/telemetry/Histograms.json:10706 (Diff revision 2) > + "bug_numbers": [1280644], > + "description": "Number of the use of the subtitles kind track.", > + "releaseChannelCollection": "opt-out" > + }, > + "WEBVTT_KIND_CAPTIONS": { > + "alert_emails": ["alwu@mozilla.com"], Do you really need *per session* counts of each of the TextTrack kinds? Having each kind be an enum value in a shared histogram seems like it would be harder to de-anonymise, result in less traffic to the server, and give us much the same data. Just put the number-to-kind mapping in the description.
Attachment #8766702 -
Flags: review?(giles)
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8766702 [details] Bug 1280644 - part1 : add Telemetry for webvtt. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61504/diff/2-3/
Attachment #8766702 -
Attachment description: Bug 1280644 - add Telemetry for webvtt. → Bug 1280644 - part1 : add Telemetry for webvtt.
Attachment #8766703 -
Attachment description: Bug 1280644 - add log. → Bug 1280644 - part2 : modify naming and add scope indentifier.
Attachment #8766702 -
Flags: feedback?(benjamin) → review?(giles)
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8766703 [details] Bug 1280644 - part2 : modify naming and add scope indentifier. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61506/diff/2-3/
Assignee | ||
Updated•8 years ago
|
Attachment #8766703 -
Flags: review+ → review?(giles)
Assignee | ||
Updated•8 years ago
|
Attachment #8766702 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Ralph Giles (:rillian) needinfo me from comment #8) > ::: dom/html/TextTrackManager.cpp:149 > > Do all these calls happen on the main thread? > > Telemetry::Accumulate has a global lock now, so calling it off the main > thread is safe, but can still be slow since it has to wait for the lock. > Better to dispatch a task if these can be called from another thread. If > it's always on the main thread, this is fine. > Yes, I think that is only called on the main thread. In case it changes to other thread, I add a assertion to check that.
Updated•8 years ago
|
Attachment #8766702 -
Flags: review?(giles) → review+
Comment 12•8 years ago
|
||
Comment on attachment 8766702 [details] Bug 1280644 - part1 : add Telemetry for webvtt. https://reviewboard.mozilla.org/r/61504/#review59186 ::: dom/html/TextTrackManager.cpp:758 (Diff revision 3) > > +void > +TextTrackManager::ReportTelemetryForTrack(TextTrack* aTextTrack) const > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(aTextTrack); Thanks. ::: toolkit/components/telemetry/Histograms.json:9994 (Diff revision 3) > + "expires_in_version": "60", > + "kind": "enumerated", > + "n_values": 10, > + "bug_numbers": [1280644], > + "description": "Number of the use of the subtitles kind track.", > + "releaseChannelCollection": "opt-out" Please do add the mapping from values to TextTrackKind to the description. It is very helpful when using the generic telemetry dashboard. e.g. "description": "Number of the use of the subtitles kind track. 0=Subtitles, 1=Captions, ..."
Comment 13•8 years ago
|
||
Comment on attachment 8766703 [details] Bug 1280644 - part2 : modify naming and add scope indentifier. https://reviewboard.mozilla.org/r/61506/#review59196
Attachment #8766703 -
Flags: review?(giles) → review+
Comment 14•8 years ago
|
||
Current nightly is Firefox 50. The typical expiration period for an exploratory measurement is 6 months/5 releases. Is this an exploratory measurement, or are there specific questions you're asking where you know you need to collect this over a full year? Can you briefly describe the value that users will get by collecting this data? Are there specific decisions that you plan to make? Who is responsible for generating the reports from this data? Who is responsible for monitoring those reports, and how often will they do so?
Flags: needinfo?(alwu)
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #14) > Current nightly is Firefox 50. The typical expiration period for an > exploratory measurement is 6 months/5 releases. Is this an exploratory > measurement, or are there specific questions you're asking where you know > you need to collect this over a full year? OK, I can change it to FF55. > Can you briefly describe the value that users will get by collecting this > data? Are there specific decisions that you plan to make? WebVTT is a web standard for the subtitle in media, and we want to know how ofter people use it in order to evaluate our implementation plan for it because our implementation is out-of-date. > Who is responsible for generating the reports from this data? Who is > responsible for monitoring those reports, and how often will they do so? We don't decide the specific plan and person who will monitor these data, but I guess that will be me and I maybe report it every quarter. In addition, I would like to ask how to get the total telemetry data from FF? Thanks!
Flags: needinfo?(alwu) → needinfo?(benjamin)
Mass change P2 -> P3
Priority: P2 → P3
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8766702 [details] Bug 1280644 - part1 : add Telemetry for webvtt. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61504/diff/3-4/
Attachment #8766702 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8766703 [details] Bug 1280644 - part2 : modify naming and add scope indentifier. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61506/diff/3-4/
Assignee | ||
Updated•8 years ago
|
Attachment #8766702 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8766702 [details] Bug 1280644 - part1 : add Telemetry for webvtt. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61504/diff/4-5/
Attachment #8766702 -
Flags: feedback?(benjamin) → review?(benjamin)
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8766703 [details] Bug 1280644 - part2 : modify naming and add scope indentifier. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61506/diff/4-5/
Comment 21•8 years ago
|
||
Comment on attachment 8766702 [details] Bug 1280644 - part1 : add Telemetry for webvtt. https://reviewboard.mozilla.org/r/61504/#review59432 ok. You're committing to looking at this data to make a product decision, which is good. I will introduce you to some of our data engineering team who can help you query and create reports.
Attachment #8766702 -
Flags: review?(benjamin) → review+
Updated•8 years ago
|
Flags: needinfo?(benjamin)
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8766702 [details] Bug 1280644 - part1 : add Telemetry for webvtt. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61504/diff/5-6/
Assignee | ||
Comment 23•8 years ago
|
||
Comment on attachment 8766703 [details] Bug 1280644 - part2 : modify naming and add scope indentifier. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61506/diff/5-6/
Comment 24•8 years ago
|
||
Pushed by alwu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ea7956bcdfeb part1 : add Telemetry for webvtt. r=bsmedberg,rillian https://hg.mozilla.org/integration/autoland/rev/bd87e21dde82 part2 : modify naming and add scope indentifier. r=rillian
Comment 25•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ea7956bcdfeb https://hg.mozilla.org/mozilla-central/rev/bd87e21dde82
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Reporter | ||
Comment 26•8 years ago
|
||
It should be worth uplifting to FF 49 to get more user data and this patch is simple. Alastor, Could you request a uplift?
Flags: needinfo?(alwu)
Assignee | ||
Comment 27•8 years ago
|
||
Approval Request Comment [Feature/regressing bug #]: Add telemetry support [User impact if declined]: we can't collect webvtt related information [Describe test coverage new/current, TreeHerder]: no [Risks and why]: low, it only collects the telemetry information [String/UUID change made/needed]: no
Flags: needinfo?(alwu)
Attachment #8777280 -
Flags: approval-mozilla-beta?
Updated•8 years ago
|
status-firefox49:
--- → affected
Comment 28•8 years ago
|
||
Hi Alastor, Do all patches need to be uplift or just the first patch?
Flags: needinfo?(alwu)
Assignee | ||
Comment 29•8 years ago
|
||
Just need to uplift the first patch, thanks.
Flags: needinfo?(alwu)
Comment 30•8 years ago
|
||
Comment on attachment 8777280 [details] [diff] [review] bug1280644_uplift_49 Review of attachment 8777280 [details] [diff] [review]: ----------------------------------------------------------------- This patch can collect data for webvtt. Take it in 49 beta. Should be in 49 beta 2.
Attachment #8777280 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in
before you can comment on or make changes to this bug.
Description
•