Closed Bug 1237401 Opened 4 years ago Closed 4 years ago

Update youtube rewrite telemetry probes to cover all cases (YOUTUBE_NONREWRITABLE_EMBED_SEEN)

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: qdot, Assigned: qdot)

References

Details

Attachments

(1 file)

Once bug 769117 lands, it'd be nice to know how many flash youtube embeds we're rewriting, and how many we're leaving alone. We should expand our telemetry probes to cover all of these cases.
Will put up for review once bug 769117 lands, as this patch could change based on those reviews.
Priority: -- → P2
hi Kyle, is your YOUTUBE_NONREWRITABLE_EMBED_SEEN patch ready for review? Our YouTube contact estimated that only 5% of Flash embeds are scripted (enablejsapi=1), so it will be interesting to compare with our telemetry probe.
Flags: needinfo?(kyle)
Summary: Update youtube rewrite telemetry probes to cover all cases → Update youtube rewrite telemetry probes to cover all cases (YOUTUBE_NONREWRITABLE_EMBED_SEEN)
Attachment #8704913 - Flags: review?(cpeterson)
I think it should be ok now.
Flags: needinfo?(kyle)
Comment on attachment 8704913 [details] [diff] [review]
Patch 1 (v1) - Add telemetry probe for non-rewritable youtube flash embeds; r=cpeterson

Review of attachment 8704913 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!
Attachment #8704913 - Flags: review?(cpeterson) → review+
Comment on attachment 8704913 [details] [diff] [review]
Patch 1 (v1) - Add telemetry probe for non-rewritable youtube flash embeds; r=cpeterson

Forgot to have a metrics peer review.
Attachment #8704913 - Flags: review+ → review?(vladan.bugzilla)
Attachment #8704913 - Attachment description: Patch 1 (v1) - Add telemetry probe for non-rewritable youtube flash embeds → Patch 1 (v1) - Add telemetry probe for non-rewritable youtube flash embeds; r=cpeterson
Comment on attachment 8704913 [details] [diff] [review]
Patch 1 (v1) - Add telemetry probe for non-rewritable youtube flash embeds; r=cpeterson

Review of attachment 8704913 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Histograms.json
@@ +10033,5 @@
>    },
> +  "YOUTUBE_NONREWRITABLE_EMBED_SEEN": {
> +    "alert_emails": ["cpeterson@mozilla.com"],
> +    "expires_in_version": "48",
> +    "kind": "flag",

This probe will log False in all sessions where a non-rewriteable YouTube embed was NOT seen, even sessions that didn't visit YouTube. Is that the # you're after?

Also note that this probe will collect data from Release channel on an opt-in basis.
Attachment #8704913 - Flags: review?(vladan.bugzilla) → review+
(In reply to Vladan Djeric (:vladan) -- please needinfo from comment #6)
> Comment on attachment 8704913 [details] [diff] [review]
> Patch 1 (v1) - Add telemetry probe for non-rewritable youtube flash embeds;
> r=cpeterson
> 
> Review of attachment 8704913 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ +10033,5 @@
> >    },
> > +  "YOUTUBE_NONREWRITABLE_EMBED_SEEN": {
> > +    "alert_emails": ["cpeterson@mozilla.com"],
> > +    "expires_in_version": "48",
> > +    "kind": "flag",
> 
> This probe will log False in all sessions where a non-rewriteable YouTube
> embed was NOT seen, even sessions that didn't visit YouTube. Is that the #
> you're after?

Yeah, we're after the total percentage of sessions that see a youtube embed with enablejsapi=1. We expect this to be fairly tiny.

> 
> Also note that this probe will collect data from Release channel on an
> opt-in basis.

ni?'ing :cpeterson. Not sure how widespread we want this.
Flags: needinfo?(cpeterson)
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #7)
> (In reply to Vladan Djeric (:vladan) -- please needinfo from comment #6)
> > Also note that this probe will collect data from Release channel on an
> > opt-in basis.
> 
> ni?'ing :cpeterson. Not sure how widespread we want this.

I think that is OK. We don't care about exact count of Flash embeds, just the scale of the problem.
Flags: needinfo?(cpeterson)
https://hg.mozilla.org/mozilla-central/rev/c525c5164cdd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Target Milestone: mozilla46 → mozilla47
Depends on: 1257277
You need to log in before you can comment on or make changes to this bug.