Closed Bug 1036764 Opened 10 years ago Closed 7 years ago

Add telemetry probes for mobile Flash content

Categories

(Firefox for Android Graveyard :: Plugins, defect, P1)

Product:
Firefox for Android Graveyard
Component:
Plugins
Platform:
All
Android
Type:
defect

Tracking

(fennec+, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
fennec + ---
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: cpeterson, Assigned: cnevinchen)

References

Details

(Whiteboard: [shumway:p1])

Attachments

(1 file)

Bug 1036764 - Add telemetry for flash. data-r?bsmedberg
59 bytes, text/x-review-board-request
snorp
: review+
benjamin
: review+
gchang
: approval-mozilla-aurora+
gchang
: approval-mozilla-beta+
Details 
For Fennec and Shumway product planning, we should collect telemetry about mobile Flash usage:

* users' plugin setting: "Tap to play" (default), "Enabled", "Disabled"
* mobile Flash ads vs real content (i.e. SWF URL has clickTag or other clues)
* mobile Flash click-to-played or auto-played

This information would help determine how much mobile Flash content exists and how (un)popular mobile Flash is with our users.
tracking-fennec: --- → ?
tracking-fennec: ? → +
Blocks: shumway-m4
Yury, would be great if you could take a look at this.
Flags: needinfo?(ydelendik)
filter on [mass-p5]
Priority: -- → P5
Jim can you look into this. We want to kill Flash support on Android if we have "low" usage (whatever that means).
Assignee: nobody → nchen
Flags: needinfo?(ydelendik)
Blocks: shumway-m5
No longer blocks: shumway-m4
> * users' plugin setting: "Tap to play" (default), "Enabled", "Disabled"
> * mobile Flash ads vs real content (i.e. SWF URL has clickTag or other clues)
> * mobile Flash click-to-played or auto-played

A histogram there would measure if the user installed Flash, but based on comment 0, we want to measure if the user is actually _using_ Flash.

But then we are probably getting rid of Flash support anyways, so this bug may not be worth our time. I think we already removed a large chunk of "Flash layers" support when we removed Java pan/zoom.
Assignee: nchen → nobody
Flags: needinfo?(nchen) → needinfo?(snorp)
How about do it here?
http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/mobile/android/chrome/content/PluginHelper.js#87

When the user click "Tap here to activate plguin" , objLoadingContent.actualType will show "application/x-shockwave-flash"
Flags: needinfo?(nchen)
It sounds like we actually need this telemetry in order to figure out if we're able to remove Flash support, so thanks Nevin for looking at this.
Flags: needinfo?(snorp)
(In reply to Nevin Chen [:nechen] from comment #6)
> How about do it here?
> http://searchfox.org/mozilla-central/rev/
> 31b6089ce26fa76459642765115605d50a6c67b4/mobile/android/chrome/content/
> PluginHelper.js#87
> 
> When the user click "Tap here to activate plguin" ,
> objLoadingContent.actualType will show "application/x-shockwave-flash"

That looks fine, but you want it to be outside of the conditional check. That way we will get telemetry if folks have it set to 'always allow' as well as 'click to play'.
Flags: needinfo?(nchen)
Attachment #8843817 - Flags: review?(nchen)
Priority: P5 → P1
Comment on attachment 8843817 [details]
Bug 1036764 - Add telemetry for flash.  data-r?bsmedberg

Hi Jim,Georg,Alessio
Could you please give me some advice?
1. Do I need to recompile backend code? Cause I got this error code [1] and I think the key name can't be found ...
2. There's already a PLUGIN_ACTIVATION_COUNT for desktop, do I need another FENNEC_PLUGIN_ACTIVATION_COUNT for Fennec? Or should I reuse PLUGIN_ACTIVATION_COUNT and add "cpp_guard": "ANDROID" to it?[2]

Thank you!

[1]"(NS_ERROR_FAILURE) [nsITelemetry.getKeyedHistogramById]"
[2]http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/toolkit/components/telemetry/Histograms.json#10293
Flags: needinfo?(nchen)
Flags: needinfo?(gfritzsche)
Flags: needinfo?(alessio.placitelli)
Attachment #8843817 - Flags: review?(nchen)
Comment on attachment 8843817 [details]
Bug 1036764 - Add telemetry for flash.  data-r?bsmedberg

https://reviewboard.mozilla.org/r/117414/#review119180

::: mobile/android/chrome/content/PluginHelper.js:86
(Diff revision 1)
>      plugins.forEach(this.playPlugin);
>    },
>  
>    playPlugin: function(plugin) {
>      let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +    let key = objLoadingContent.actualType;

Flyby: can key be empty here? Make sure it can't as we're disabling empty keys support in bug 1334469.
(In reply to Nevin Chen [:nechen] from comment #10)
> Comment on attachment 8843817 [details]
> Bug 1036764 - Add probe for Flash usage in Fennec.
> 
> Hi Jim,Georg,Alessio
> Could you please give me some advice?
> 1. Do I need to recompile backend code? Cause I got this error code [1] and
> I think the key name can't be found ...

Yes, when changing the Histogram.json files the cpp files must be compiled again.
Usually, "./mach build" is smart enough to update everything for you.

> 2. There's already a PLUGIN_ACTIVATION_COUNT for desktop, do I need another
> FENNEC_PLUGIN_ACTIVATION_COUNT for Fennec? Or should I reuse
> PLUGIN_ACTIVATION_COUNT and add "cpp_guard": "ANDROID" to it?[2]

That histogram expired in version 53, so it's not gathering any data since that version.
At the very least, you'd need to extend the expiration for that one. However, you would be
also changing the behaviour of the problem by restricting it to Android, so I think it would
be cleaner to just add a new probe as you did.

You would probably want to add test coverage too.

Also, and that's very important, keep in mind that you definitely need a data-review from a data collection peer (https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval).
Flags: needinfo?(alessio.placitelli)
Flags: needinfo?(gfritzsche)
Assignee: nobody → cnevinchen
Comment on attachment 8843817 [details]
Bug 1036764 - Add telemetry for flash.  data-r?bsmedberg

https://reviewboard.mozilla.org/r/117414/#review119662

Snorp should review this. I'm not sure if auto-play goes through PluginHelper.playPlugin, so we may need something else for when auto-play is enabled.

::: mobile/android/chrome/content/PluginHelper.js:87
(Diff revision 2)
>    },
>  
>    playPlugin: function(plugin) {
>      let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +    let key = objLoadingContent.actualType;
> +    Services.telemetry.getKeyedHistogramById("FENNEC_PLUGIN_ACTIVATION_COUNT").add(key);

Make sure `key` isn't empty like Alessio said.
Attachment #8843817 - Flags: review?(nchen)
Flags: needinfo?(nchen)
Attachment #8843817 - Flags: review?(snorp)
Comment on attachment 8843817 [details]
Bug 1036764 - Add telemetry for flash.  data-r?bsmedberg

https://reviewboard.mozilla.org/r/117414/#review119738
Attachment #8843817 - Flags: review?(snorp) → review+
Hi Benjamin.
Please help me with data-review and let me know if there's anything I need to add. Thanks!
Flags: needinfo?(benjamin)
Comment on attachment 8843817 [details]
Bug 1036764 - Add telemetry for flash.  data-r?bsmedberg

https://reviewboard.mozilla.org/r/117414/#review121054

::: toolkit/components/telemetry/Histograms.json:10261
(Diff revision 3)
>      "kind": "enumerated",
>      "n_values": 100,
>      "releaseChannelCollection": "opt-out",
>      "description": "Graphics Crash Reason (...)"
>    },
> +  "FENNEC_PLUGIN_ACTIVATION_COUNT": {

This documentation is incorrect. You say that this is keyed by plugin name, but the code has it keyed by MIME type.

Related, why do you need to key this at all? Fennec only allows the Flash plugin, so we already know what plugin this is.

::: toolkit/components/telemetry/Histograms.json:10266
(Diff revision 3)
> +  "FENNEC_PLUGIN_ACTIVATION_COUNT": {
> +    "alert_emails": ["mobile-frontend@mozilla.com"],
> +    "expires_in_version": "never",
> +    "kind": "count",
> +    "keyed": true,
> +    "releaseChannelCollection": "opt-out",

Do you know that opt-out collection on Android doesn't exist yet? Android builds only send saved-session pings for opt-in users.
Attachment #8843817 - Flags: review?(benjamin) → review-
Thanks a lot for pointing out that opt-out collection on Android doesn't exist yet.
I'm new to Telemetry in Fennec.. could you share with me where I can find related code?

btw, I'll change and use UI event here.
I'm going to redirect you to fbertsch who is the data engineer supporting Android.
Flags: needinfo?(benjamin) → needinfo?(fbertsch)
(In reply to Nevin Chen [:nechen] from comment #19)
> Thanks a lot for pointing out that opt-out collection on Android doesn't
> exist yet.
> I'm new to Telemetry in Fennec.. could you share with me where I can find
> related code?
> 
> btw, I'll change and use UI event here.

Why switch to event telemetry? If the question is "how many times is flash used", that is a simple enough query that histograms should easily answer. Event aren't sent for opt-out either, so there's no benefit there.
Flags: needinfo?(fbertsch) → needinfo?(cnevinchen)
If this probe can't be opt-out, I can't see the "real" Flash usage. I think opt-in users are kind heavy users. This probe helps us to decide if we going to kill Flash support or not. So I want to see the usage in a larger sample.
Flags: needinfo?(cnevinchen)
If this is needed to make a product decision, and the data needs to be collected from the release population, the *only* option is to add it to the core ping [0].

Since this is just one count, I don't think collection of it should be too much of an issue.

[0] https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/data/core-ping.html
1. What if I want to know the number of times the plugin is played? How can I get the count from stmo?

2. Please help me clarify this: "opt-out collection on Android doesn't exist yet" means "UITelemetry, Histogram, and Core ping are only sent for opt-in users in Fennec?

3. According to comment 18. "Android builds only send saved-session pings for opt-in users" . Where is the code in Fennec and what is it? I can only find this in readthedocs[1] but I still don't know how it works.


[1]http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/data/main-ping.html?highlight=saved-session%20ping
Flags: needinfo?(fbertsch)
(In reply to Nevin Chen [:nechen] from comment #25)
> 1. What if I want to know the number of times the plugin is played? How can
> I get the count from stmo?

If it's in the core ping, that data is currently available in mobile_clients on STMO.

> 2. Please help me clarify this: "opt-out collection on Android doesn't exist
> yet" means "UITelemetry, Histogram, and Core ping are only sent for opt-in
> users in Fennec?

Almost - opt-out doesn't exist for probes (histograms and scalars). The core ping is separate from those, and IS sent for all users. That's why I recommended putting this in the core ping.

> 3. According to comment 18. "Android builds only send saved-session pings
> for opt-in users" . Where is the code in Fennec and what is it? I can only
> find this in readthedocs[1] but I still don't know how it works.

This I'm not sure about. All of my work is done on server-side. Georg might have a better idea.

> [1]http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/
> telemetry/data/main-ping.html?highlight=saved-session%20ping
Flags: needinfo?(fbertsch) → needinfo?(gfritzsche)
Hi Georg
Please help me with below questions:
1. According to comment 18. "Android builds only send saved-session pings for opt-in users" . Where is the a)document or b)code for this in Fennec?

2. If I want to send the number of times a plugin is played, should I save the count in Android DB and send it when core ping is sent? Or should I send the play count with core ping every-time  when a plugin in is played?
Comment on attachment 8843817 [details]
Bug 1036764 - Add telemetry for flash.  data-r?bsmedberg

https://reviewboard.mozilla.org/r/117414/#review122078

This still doesn't change any documentation and is therefore not ready for data review.
Attachment #8843817 - Flags: review?(benjamin) → review-
Comment on attachment 8843817 [details]
Bug 1036764 - Add telemetry for flash.  data-r?bsmedberg

https://reviewboard.mozilla.org/r/117414/#review122524

data-r=me (I did not review the code)
Attachment #8843817 - Flags: review+
Frank, will this need any updates to the core ping schema/ingestion?
Flags: needinfo?(fbertsch)
Yes, if we are updating the core ping, we need to update the docs [0]. I'll also need to update the direct-to-parquet schema, but that's not yet available in any repo so I'll take care of it.

[0] http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/data/core-ping.html
Flags: needinfo?(fbertsch)
Looks like the docs were taken care of. This should be fine then.
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/36525c06f5a0
Add telemetry for flash. r=bsmedberg,snorp data-r?bsmedberg
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/36525c06f5a0
https://hg.mozilla.org/mozilla-central/rev/4bb7880d29e7
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: needinfo?(gfritzsche)
Comment on attachment 8843817 [details]
Bug 1036764 - Add telemetry for flash.  data-r?bsmedberg

Approval Request Comment
[Feature/Bug causing the regression]: NA
[User impact if declined]: This probe is to examine the Flash usage in Fennec. So if we can know this earlier, we will be able to decide whether we are gonna to support Flash in our next release.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: NA
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: It just an additional probe. No code flow will be impacted.
[String changes made/needed]: No
Attachment #8843817 - Flags: approval-mozilla-beta?
Attachment #8843817 - Flags: approval-mozilla-aurora?
Hi :bsmedberg,
Do we need to inform legal for the telemetry data gathering?
Flags: needinfo?(benjamin)
No. As data steward I will coordinate with legal if necessary, but it's not necessary in this case.
Flags: needinfo?(benjamin)
Comment on attachment 8843817 [details]
Bug 1036764 - Add telemetry for flash.  data-r?bsmedberg

This can help figure out if we're able to remove Flash support. Aurora54+ & Beta53+.
Attachment #8843817 - Flags: approval-mozilla-beta?
Attachment #8843817 - Flags: approval-mozilla-beta+
Attachment #8843817 - Flags: approval-mozilla-aurora?
Attachment #8843817 - Flags: approval-mozilla-aurora+
Hi Frank 
I can't see this value in mobule_clients table. Is it on stmo yet?
Flags: needinfo?(fbertsch)
Hi Nevin,

This is available in telemetry_core_parquet table. That is the direct-to-parquet table, mobile_clients is an ETL job.
Flags: needinfo?(fbertsch)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: