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

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.

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.

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

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!

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

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

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

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

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

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

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

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.

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!

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 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!

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

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

(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

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

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

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

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.

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!

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

Comment on attachment 8840345 [details] Bug 1329160 - Send an event when warmup() is called. updated document and requested data-r

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 .

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ea7a7aab4863 Send an event when warmup() is called. r=jchen,liuche,sebastian

Pushed by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a6c1c2a3ff08 follow up fix for android-checkstyle failure. r=me

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