Closed Bug 1280644 Opened 8 years ago Closed 8 years ago

[webvtt] Add the Telemetry for webvtt

Categories

(Core :: Audio/Video: Playback, defect, P3)

37 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: bwu, Assigned: alwu)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

It would be better to add some telemetry for WebVTT to know how people use it. 
With these info, we can make Firefox better!
Assignee: nobody → alwu
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)
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/
Attachment #8766702 - Flags: feedback?(benjamin)
Summary: Add WebVTT telemetry → [webvtt] Add the Telemetry for webvtt
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 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)
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)
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/
Attachment #8766703 - Flags: review+ → review?(giles)
Attachment #8766702 - Flags: feedback?(benjamin)
(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.
Attachment #8766702 - Flags: review?(giles) → review+
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 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+
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)
(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
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)
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/
Attachment #8766702 - Flags: feedback?(benjamin)
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)
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 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+
Flags: needinfo?(benjamin)
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/
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/
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
https://hg.mozilla.org/mozilla-central/rev/ea7956bcdfeb
https://hg.mozilla.org/mozilla-central/rev/bd87e21dde82
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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)
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?
Hi Alastor,
Do all patches need to be uplift or just the first patch?
Flags: needinfo?(alwu)
Just need to uplift the first patch, thanks.
Flags: needinfo?(alwu)
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.

Attachment

General

Created:
Updated:
Size: