1329160 - Custom tabs: Collect information on pre-warming sessions used by 3rd party apps

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Metrics, defect)

Description

[Joe Cheng [:jcheng] (please needinfo)]

As Mozilla, we want to understand how many 3rd party apps uses pre-warming and how many times pre-warming is used

Comment 1

[Nevin Chen(Not active on Bugzilla)]

I can't find a way to get the referrer when warm up. But I can track the how long it take to complete warm up.

Comment 2

[Nevin Chen(Not active on Bugzilla)]

I can't find a way to get the referrer when warm up. But I can track how long will it take to complete warm up.

Comment 3

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1329160 - Send an event when warmup() is called.

Review commit: https://reviewboard.mozilla.org/r/114860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114860/

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

How does this sound now Nevin?

As Mozilla, we want to understand how many times pre-warming is used and how long it takes to complete warm up

Comment 5

[Nevin Chen(Not active on Bugzilla)]

I agree with that. My patch is just for that purpose. I'll follow this document https://wiki.mozilla.org/Firefox/Data_Collection and seek for the reviewers' approval.

Hi Chenxia, Benjamin
Please help review my patch and give me some guidance. Thank you!

Comment 6

[Benjamin Smedberg]

A data review happens on the documentation changes you make. We cannot do a data review until you update the documentation. Then we review the doc changes and the code reviewer makes sure that your code matches the documentation.

Comment 7

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114860/diff/1-2/

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

https://reviewboard.mozilla.org/r/114860/#review118158

::: mobile/android/base/java/org/mozilla/gecko/customtabs/GeckoCustomTabsService.java:41
(Diff revision 2)
>      }
>  
>      @Override
>      protected boolean warmup(long flags) {
> +        try {
> +            Telemetry.startUISession(TelemetryContract.Session.CUSTOM_TAB, "warmup");

I don't think a session is what you want here. Sessions are more or less namespaces that are attached to events happening while the session is active.

You just want to send an event here if you are interested in raw counts (x users did y warmups).

More here:
http://gecko.readthedocs.io/en/latest/mobile/android/fennec/uitelemetry.html

If you are more interested in the time a warmup takes and you want to look at a graph showing the distribution (like this: http://bit.ly/2gy2Bid) then you might want to add a histogram.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #8)
> (like this: http://bit.ly/2gy2Bid) then

Wrong link. This should be: https://mzl.la/2lC629x

Comment 10

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114860/diff/2-3/

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

https://reviewboard.mozilla.org/r/114860/#review119154

I'll add Jim as additional reviewer for the GeckoService bits.

Note that adding the histogram will only give you a distribution, like this: https://mzl.la/2mLpW6p

If the product team has questions like:
* How many users are using the warmup feature how often?
* How does this affect other features (do they open more links? are they more engaged?)
.. then you want to at least add a telemetry event too!

Comment 12

[Benjamin Smedberg]

Why did you add these to the whitelists? New histograms should not be added to the whitelists.

This histogram should be fine for collecting per-user stats, since you can correlate it by clientid.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

https://reviewboard.mozilla.org/r/114860/#review119320

::: mobile/android/base/java/org/mozilla/gecko/GeckoService.java:225
(Diff revision 3)
>          stopSelf(startId);
> +        final long start = intent.getLongExtra(EXTRA_WARMUP, 0l);
> +        if (start != 0l) {
> +            final long end = SystemClock.uptimeMillis();
> +            final long took = end - start;
> +            Telemetry.addToHistogram(TELEMETRY_FENNEC_WARMUP_TIME_CUSTOM_TAB, (int) Math.min(took, Integer.MAX_VALUE));

1) I don't like this approach because it breaks GeckoService's encapsulation. Ideally you want to do everything inside the custom tabs class.

2) Because warming up happens asynchronously on the Gecko thread, this doesn't actually measure the complete warmup time.

3) Why do you need the complete warmup time? What does that tell us?

::: toolkit/components/telemetry/Histograms.json:6434
(Diff revision 3)
>      "expires_in_version": "never",
>      "kind": "flag",
>      "description": "Fennec is starting up but the Gecko thread was still running",
>      "cpp_guard": "ANDROID"
>    },
> +  "FENNEC_WARMUP_TIME_CUSTOM_TAB": {

Use "FENNEC_CUSTOM_TABS_WARMUP_TIME"

::: toolkit/components/telemetry/Histograms.json:6440
(Diff revision 3)
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "low": 10,
> +    "high": 20000,
> +    "n_buckets": 20,
> +    "description": "Time for a client app request gecko warm-up for custom tab",

Please indicate units in the description (ms?)

::: toolkit/components/telemetry/histogram-whitelists.json:237
(Diff revision 3)
>      "FENNEC_READING_LIST_COUNT",
>      "FENNEC_RESTORING_ACTIVITY",
>      "FENNEC_SEARCH_LOADER_TIME_MS",
>      "FENNEC_STARTUP_TIME_GECKOREADY",
>      "FENNEC_STARTUP_TIME_JAVAUI",
> +    "FENNEC_WARMUP_TIME_CUSTOM_TAB",

Don't need to change histogram-whitelists like bsmedberg said.

Comment 14

[Benjamin Smedberg]

I'm going to data-review this since liuche is busy. I'll wait for the next revision of the patch since it's not clear if you've decided what to collect.

Also be aware that histograms are really only collected on prerelease of Android: the only thing that's collected on release is the core ping which is much smaller.

Comment 15

[Benjamin Smedberg]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

Related note from reading the patch: I can't tell reading the histogram description what a warmup time is. A little more in the way of documentation would be good.

Comment 16

[Nevin Chen(Not active on Bugzilla)]

(In reply to Jim Chen [:jchen] [:darchons] from comment #13)

> 1) I don't like this approach because it breaks GeckoService's
> encapsulation. Ideally you want to do everything inside the custom tabs class.
>
> 2) Because warming up happens asynchronously on the Gecko thread, this
> doesn't actually measure the complete warmup time.
> 
> 3) Why do you need the complete warmup time? What does that tell us?

I agree it breaks the encapsulation and it's hard to comepare the wamrup time with custom tab 
I'll make this a simpler patch and just record the warm-up feature usage (count) and see if theres more useful information I can get from here.

Comment 17

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114860/diff/3-4/

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

https://reviewboard.mozilla.org/r/114860/#review119670

::: mobile/android/base/java/org/mozilla/gecko/customtabs/GeckoCustomTabsService.java:41
(Diff revision 4)
>      }
>  
>      @Override
>      protected boolean warmup(long flags) {
> +
> +        Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.INTENT,"customtab-warmup");

This is fine, although using a histogram is probably simpler.

Comment 19

[Nevin Chen(Not active on Bugzilla)]

(In reply to Jim Chen [:jchen] [:darchons] from comment #18)
> > +        Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.INTENT,"customtab-warmup");
> 
> This is fine, although using a histogram is probably simpler.

Yes.. Because I want to associate the warmup event with open link event when using custom tab. 
And using histogram seems to be impossible to do that.

Comment 20

[Nevin Chen(Not active on Bugzilla)]

I edited it because I thought that's required. But I can't find a document about it.

Comment 21

[Benjamin Smedberg]

The whitelist is for histograms that break "the rules", such as older ones that don't have bug numbers or people. That shouldn't be the case for new things you're adding.

Comment 22

[Nevin Chen(Not active on Bugzilla)]

Thanks a lot! Could you also give me some advice about the data review? Jim has r+ this patch, is there anything I need for data-review?
Thanks!

Comment 23

[Benjamin Smedberg]

This does need data-review. Data review happens against changes to in-tree documentation, and the patch here doesn't include any documentation changes so it is not ready for data review.

I am about 60% certain that uitelemetry.rst is the right place to document this (see http://gecko.readthedocs.io/en/latest/mobile/android/fennec/uitelemetry.html) but liuche might know that for certain.

Comment 24

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114860/diff/4-5/

Comment 25

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

Since 
1. I didn't add a new Event nor Method in UITelemetry.
2. I just added some information in the extras.
3. I can't find a place where I can update extras information.

So I added a "Extras" part in uitelemetry.rst. 

Please give me some advice. Thank you!

Comment 26

[Frank Bertsch [:frank]]

(In reply to Nevin Chen [:nechen] from comment #19)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #18)
> > > +        Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.INTENT,"customtab-warmup");
> > 
> > This is fine, although using a histogram is probably simpler.
> 
> Yes.. Because I want to associate the warmup event with open link event when
> using custom tab. 
> And using histogram seems to be impossible to do that.

So this is the most recent version of question that I see this probe is trying to answer: "How many warmup events happen for different types of open link events"? Please clarify more if that is not correct. I don't see "open link event" in the events list so it would be good to know which specific event to which you are referring.

In any case, if you want to associate with something else, you could make this a keyed histogram, with the key being the "open link event" type. This analysis would be far easier (in fact, done automatically in TMO).

Comment 27

[Nevin Chen(Not active on Bugzilla)]

(In reply to Frank Bertsch [:frank] from comment #26)
> So this is the most recent version of question that I see this probe is
> trying to answer: "How many warmup events happen for different types of open
> link events"? Please clarify more if that is not correct. I don't see "open
> link event" in the events list so it would be good to know which specific
> event to which you are referring.
> 
> In any case, if you want to associate with something else, you could make
> this a keyed histogram, with the key being the "open link event" type. This
> analysis would be far easier (in fact, done automatically in TMO).

Currently, I only want to know "How many warmup events happen". 
This information is already useful to me cause I can know if warmup() feature is really used.
This is the focus of this bug.

But in the future, I imagine I can associate the warmup event with other event by clientId and see what I can get from there. But that's another story.

The reason I didn't choose histogram is because I can't use it in stmo and produce more flexible reports.

Please help correct me if I'm wrong. Thank you!

Comment 28

[Frank Bertsch [:frank]]

(In reply to Nevin Chen [:nechen] from comment #27)
> The reason I didn't choose histogram is because I can't use it in stmo and
> produce more flexible reports.

You are correct that currently there is no way to access this data in STMO. I am working right now on a Dataset where the data will be available, which will hopefully be finished by end of quarter. If that time frame is acceptable to you, then I propose you add this instead as a histogram. In fact, if this is just a count, you can add this as a scalar [0].

[0] http://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/scalars.html

Comment 29

[Nevin Chen(Not active on Bugzilla)]

Can I join that data set seamlessly with android_events(Presto) table? If yes that'll be fine. Thank you!

Comment 30

[Frank Bertsch [:frank]]

(In reply to Nevin Chen [:nechen] from comment #29)
> Can I join that data set seamlessly with android_events(Presto) table? If
> yes that'll be fine. Thank you!

Not really, no. If you will need to associate this with events then it should also be an event.

Comment 31

[Nevin Chen(Not active on Bugzilla)]

Then I'd prefer to use event. Could you please also help review this patch? Or should I ask Benjamin again? Thank you!

Comment 32

[Frank Bertsch [:frank]]

As Benjamin mentioned earlier:

> This does need data-review. Data review happens against changes to in-tree documentation, and the patch here doesn't > include any documentation changes so it is not ready for data review.

You'll need a client-side who has implemented events before to do the review.

Comment 33

[Nevin Chen(Not active on Bugzilla)]

Sorry I don't understand.
jchen has reviewed+ my patch. And I've also edited the document.
What else do I need? 
Or do I need to use data-review flag instead of r flag?
Or should I ask Georg to review first?
Thank you!

Comment 34

[Frank Bertsch [:frank]]

Ah, you're right. Bsmedberg can do the data-r now.

Comment 35

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114860/diff/5-6/

Comment 36

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

updated document and requested data-r

Comment 37

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114860/diff/6-7/

Comment 38

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114860/diff/7-8/

Comment 39

[Chenxia Liu [:liuche]]

Comment on attachment 8840345 [details]
Bug 1329160 - Send an event when warmup() is called.

https://reviewboard.mozilla.org/r/114860/#review124612

r+ data-review. This looks good to me for Android mobile data review. Just collecting a count, and it seems reasonable to file this as an ACTION, SERVICE, and using customtab-warmup as an extra.

For future reference, if you do end up needing to add a new Event or Method, that should be documented in https://dxr.mozilla.org/mozilla-central/source/mobile/android/docs/uitelemetry.rst .

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea7a7aab4863
Send an event when warmup() is called. r=jchen,liuche,sebastian

Comment 41

[Nevin Chen(Not active on Bugzilla)]

Attachment: patch.txt

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6c1c2a3ff08
follow up fix for android-checkstyle failure. r=me

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea7a7aab4863
https://hg.mozilla.org/mozilla-central/rev/a6c1c2a3ff08