Closed Bug 1229971 Opened 4 years ago Closed 4 years ago

Change logic of and rename youtube embed tracking telemetry probe

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla45
Tracking Status
firefox43 --- ?
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: qdot, Assigned: qdot)

References

Details

Attachments

(1 file, 1 obsolete file)

Turns out the logic we have on YOUTUBE_EMBED_SEEN is backward, only counting URLs that DO have enablejsapi=1, where we wanted them without. Need to flip that logic to get a proper count to see if we want to proceed with bug 769117, as well as changing the name of the probe to YOUTUBE_REWRITABLE_EMBED_SEEN so we don't get things mixed up.
Attachment #8695029 - Flags: review?(vladan.bugzilla)
Attachment #8695029 - Flags: review?(cpeterson)
I know :sylvestre was hoping for tests on this over in bug 1218952. I'm not really sure how to write a mochi for this though, since it requires accessing remote content. Is there a way to fake that content in mochitests?
Flags: needinfo?(sledru)
Comment on attachment 8695029 [details] [diff] [review]
Patch 1 (v1) - Change name and logic of youtube embed telemetry probe

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

::: toolkit/components/telemetry/Histograms.json
@@ -5177,5 @@
> -    "high": "100",
> -    "n_buckets": 15,
> -    "extended_statistics_ok": true,
> -    "description": "Median of tabs in groups in Panorama"
> -  },

You probably don't want these changes. Wrong diff base? :)

@@ +10160,5 @@
>      "kind": "boolean",
>      "bug_numbers": [1188391],
>      "description": "The number of ICE connections which immediately failed (0) vs. reached at least checking state (1)."
> +  },
> +  "YOUTUBE_REWRITABLE_EMBED_SEEN": {

You can remove the old YOUTUBE_EMBED_SEEN entry from Histograms.json if we're no longer reporting YOUTUBE_EMBED_SEEN.
Attachment #8695029 - Flags: review?(cpeterson) → feedback+
I am sorry but I don't know enough to help you :)
Flags: needinfo?(sledru)
Comment on attachment 8695029 [details] [diff] [review]
Patch 1 (v1) - Change name and logic of youtube embed telemetry probe

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

::: toolkit/components/telemetry/Histograms.json
@@ +10169,1 @@
>    }

* add a bug_numbers field as well
* flag histograms give you once-per session semantics, but maybe that's what you want? i.e. % of sessions which saw at least one rewriteable youtube flash embed
* you only need this data from pre-release, right? because on Release-channel, only users who opted into telemetry would report any data to this probe
Attachment #8695029 - Flags: review?(vladan.bugzilla) → review+
Priority: -- → P2
(In reply to Vladan Djeric (:vladan) -- please needinfo | PTO Friday Dec 4th from comment #5)
> * flag histograms give you once-per session semantics, but maybe that's what
> you want? i.e. % of sessions which saw at least one rewriteable youtube
> flash embed

Yup, only want to know if a session sees at least one. Decided that any more info might be iffy for privacy.

> * you only need this data from pre-release, right? because on
> Release-channel, only users who opted into telemetry would report any data
> to this probe

Yeah, only need it on pre-release, choice to rewrite will be made (and therefore this probe will be removed) before this would hit release-channel, I think
Fixing comments from first patch, making diff off right branch this time.
Attachment #8695029 - Attachment is obsolete: true
Attachment #8696567 - Flags: review?(cpeterson)
Comment on attachment 8696567 [details] [diff] [review]
Patch 1 (v2) - Change name and logic of youtube embed telemetry probe; r=vladan

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

LGTM! After your fix lands on m-c, we should ask to uplift this to Aurora and Beta.
Attachment #8696567 - Flags: review?(cpeterson) → review+
Comment on attachment 8696567 [details] [diff] [review]
Patch 1 (v2) - Change name and logic of youtube embed telemetry probe; r=vladan

Approval Request Comment
[Feature/regressing bug #]: Bug 1229971
[User impact if declined]: We count the wrong type of youtube embeds
[Describe test coverage new/current, TreeHerder]: Already been a probe in pre-release builds for a while, just flipping logic
[Risks and why]: None
[String/UUID change made/needed]: None
Attachment #8696567 - Flags: approval-mozilla-beta?
Attachment #8696567 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/266672b18676
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Do you mean this for 43? I looked briefly yesterday, saw "pre-release data" and thought you were aiming for 44.  This missed the 43.0 RC build.
Flags: needinfo?(kyle)
Nah, this can miss 43 and we'll be fine.
Flags: needinfo?(kyle)
Comment on attachment 8696567 [details] [diff] [review]
Patch 1 (v2) - Change name and logic of youtube embed telemetry probe; r=vladan

Since this landed on Nightly before merge, this is already on Aurora45. Given that it has been on Nightly for a few days, it seems safe to uplift to Beta44.
Attachment #8696567 - Flags: approval-mozilla-beta?
Attachment #8696567 - Flags: approval-mozilla-beta+
Attachment #8696567 - Flags: approval-mozilla-aurora?
Kyle, just wondering, has the telemetry data from Nightly confirmed that this is fixed?
Flags: needinfo?(kyle)
Yup, it's working now.
Flags: needinfo?(kyle)
Status: RESOLVED → VERIFIED
Depends on: 1257277
You need to log in before you can comment on or make changes to this bug.