1262921 - Round out AppCache telemetry so we can make decisions around timing of its removal

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Mike Taylor [:miketaylr]]

We have a few Telemetry probes related to AppCache:

HTTP_OFFLINE_CACHE_DOCUMENT_LOAD:

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#3710

We also have "USE_COUNTER2_DEPRECATED_AppCache_DOCUMENT" and  "USE_COUNTER2_DEPRECATED_AppCache_PAGE":

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/histogram-whitelists.json#1013


There's also Bug 899521.

We also want to measure AppCache API usage.

Comment 1

[Mike Taylor [:miketaylr]]

Ehsan, what else do you think we need (apart from window.applicationCache API usage?)

Comment 2

[(no longer active)]

(In reply to Mike Taylor [:miketaylr] from comment #1)
> Ehsan, what else do you think we need (apart from window.applicationCache
> API usage?)

Probing the individual API calls and also checks for the existence of applicationCache would be nice to have.

Comment 3

[Mike Taylor [:miketaylr]]

> checks for the existence of applicationCache would be nice to have.

No idea how to do that part.

Docs for adding use counter info: https://dxr.mozilla.org/mozilla-central/source/dom/base/UseCounters.conf

OK, looking into adding use counters for the API bits. I see we have:

https://dxr.mozilla.org/mozilla-central/source/dom/webidl/OfflineResourceList.webidl
https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/offline/nsIDOMOfflineResourceList.idl

Just to verify (since I've never done UseCounter telemetry before), I want to be adding [UseCounter] annotations to  OfflineResourceList.webidl and not the nsIDOMOfflineResourceList xpidl. Is this correct Nathan?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Mike Taylor [:miketaylr] from comment #3)
> Docs for adding use counter info:
> https://dxr.mozilla.org/mozilla-central/source/dom/base/UseCounters.conf
> 
> OK, looking into adding use counters for the API bits. I see we have:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/webidl/
> OfflineResourceList.webidl
> https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/offline/
> nsIDOMOfflineResourceList.idl
> 
> Just to verify (since I've never done UseCounter telemetry before), I want
> to be adding [UseCounter] annotations to  OfflineResourceList.webidl and not
> the nsIDOMOfflineResourceList xpidl. Is this correct Nathan?

That's correct.  You also need to add the appropriate lines to UseCounters.conf.  I see that the large block comment doesn't make it clear that you need the [UseCounter] annotation and the UseCounters.conf modification; I'll file a bug on fixing that.

Comment 5

[Mike Taylor [:miketaylr]]

Thanks!

Comment 6

[Mike Taylor [:miketaylr]]

Attachment: Add UseCounter telemetry for AppCache API. r=?

Comment 7

[Mike Taylor [:miketaylr]]

Comment on attachment 8749714 [details] [diff] [review]
Add UseCounter telemetry for AppCache API. r=?

Honza, would you mind reviewing? 

(to make sure I'm in the right place...)

Comment 8

[Mike Taylor [:miketaylr]]

Comment on attachment 8749714 [details] [diff] [review]
Add UseCounter telemetry for AppCache API. r=?

Requesting approval to collect this telemetry to help us make decisions around removing AppCache.

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8749714 [details] [diff] [review]
Add UseCounter telemetry for AppCache API. r=?

Sorry, I am not qualified to do this review. I am member of the DevTools team and this is out of my scope.

Honza

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8749714 [details] [diff] [review]
Add UseCounter telemetry for AppCache API. r=?

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

Probably meant for me.

I assume we can't do this also for the event handlers?  Those are probably going to be used a bit more than these methods.

Comment 11

[Mike Taylor [:miketaylr]]

(welp, wrong Honza, my bad!)(In reply to Jan Honza Odvarko [:Honza] from comment #9)
> Sorry, I am not qualified to do this review. I am member of the DevTools
> team and this is out of my scope.

Ooops. Sorry about that ^_^.

Comment 12

[Mike Taylor [:miketaylr]]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> I assume we can't do this also for the event handlers?  Those are probably
> going to be used a bit more than these methods.

I think we can? They're attributes in the IDL, so we should be able to UseCounter them.

ni? Nathan to double check before I update the patch.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Mike Taylor [:miketaylr] from comment #12)
> (In reply to Honza Bambas (:mayhemer) from comment #10)
> > I assume we can't do this also for the event handlers?  Those are probably
> > going to be used a bit more than these methods.
> 
> I think we can? They're attributes in the IDL, so we should be able to
> UseCounter them.
> 
> ni? Nathan to double check before I update the patch.

Hm, that's a good question.  Attributes are indeed UseCounter-able, but things like addEventListener aren't going to go through the bindings layer where the use counters are incremented.  So you could UseCounter the attributes, but I don't know that they're going to give you the data you want.

Comment 14

[Mike Taylor [:miketaylr]]

OK, thanks Nathan and Honza. Let's land what we have right now, because it's better than nothing.

Comment 15

[Mike Taylor [:miketaylr]]

Attachment: Add UseCounter telemetry for AppCache API.

Updating commit message to reflect r=mayhemer. Carrying forward r+.

Comment 16

[Mike Taylor [:miketaylr]]

Comment on attachment 8751350 [details] [diff] [review]
Add UseCounter telemetry for AppCache API.

Still need data collection approval before landing. bsmedberg?

Comment 17

[Honza Bambas (:mayhemer)]

If it's possible to add counters to the on* attributes, then I'd do it.  It's still some information.

Comment 18

[Mike Taylor [:miketaylr]]

Attachment: Add UseCounter telemetry for AppCache event handlers. r?mayhemer

Comment 19

[Mike Taylor [:miketaylr]]

Comment on attachment 8751354 [details] [diff] [review]
Add UseCounter telemetry for AppCache event handlers. r?mayhemer

Hang on, typo.

Comment 20

[Mike Taylor [:miketaylr]]

Attachment: Add UseCounter telemetry for AppCache event handlers. r?mayhemer

Comment 21

[Mike Taylor [:miketaylr]]

Thanks for review and feedback, all.

Comment 22

[Mike Taylor [:miketaylr]]

Attachment: Add UseCounter telemetry for AppCache event handlers.

Updating commit message, carrying over r+.

Comment 23

[Mike Taylor [:miketaylr]]

Comment on attachment 8751350 [details] [diff] [review]
Add UseCounter telemetry for AppCache API.

Ah, need review from a DOM peer. Blake, would you mind?

Comment 24

[Mike Taylor [:miketaylr]]

That was quick, thanks!

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd02104f1ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/5737e7172325

Comment 26

[Mike Taylor [:miketaylr]]

Green try run (w/o tests) just to prove this actually builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab191af9cd76

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7dd02104f1ed
https://hg.mozilla.org/mozilla-central/rev/5737e7172325