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.

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?

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.

I am sorry but I don't know enough to help you :)

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

(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.

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.

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

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.

Nah, this can miss 43 and we'll be fine.

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.

Kyle, just wondering, has the telemetry data from Nightly confirmed that this is fixed?

Yup, it's working now.