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

RESOLVED FIXED in Firefox 49

Status

()

Toolkit
Telemetry
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: miketaylr, Assigned: miketaylr)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Ehsan, what else do you think we need (apart from window.applicationCache API usage?)
Flags: needinfo?(ehsan)
(Assignee)

Updated

2 years ago
See Also: → bug 899521
(Assignee)

Updated

2 years ago
Blocks: 1237782

Comment 2

2 years ago
(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.
Flags: needinfo?(ehsan)
(Assignee)

Comment 3

2 years ago
> 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?
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
(Assignee)

Comment 5

2 years ago
Thanks!
(Assignee)

Comment 6

2 years ago
Created attachment 8749714 [details] [diff] [review]
Add UseCounter telemetry for AppCache API. r=?
(Assignee)

Updated

2 years ago
Assignee: nobody → miket
(Assignee)

Comment 7

2 years ago
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...)
Attachment #8749714 - Flags: review?(odvarko)
(Assignee)

Comment 8

2 years ago
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.
Attachment #8749714 - Flags: feedback?(benjamin)
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
Attachment #8749714 - Flags: review?(odvarko)
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.
Attachment #8749714 - Flags: review+
(Assignee)

Comment 11

2 years ago
(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 ^_^.
(Assignee)

Comment 12

2 years ago
(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.
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
(Assignee)

Comment 14

2 years ago
OK, thanks Nathan and Honza. Let's land what we have right now, because it's better than nothing.
(Assignee)

Comment 15

2 years ago
Created attachment 8751350 [details] [diff] [review]
Add UseCounter telemetry for AppCache API.

Updating commit message to reflect r=mayhemer. Carrying forward r+.
Attachment #8751350 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8749714 - Attachment is obsolete: true
Attachment #8749714 - Flags: feedback?(benjamin)
(Assignee)

Comment 16

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

Still need data collection approval before landing. bsmedberg?
Attachment #8751350 - Flags: feedback?(benjamin)
If it's possible to add counters to the on* attributes, then I'd do it.  It's still some information.
(Assignee)

Comment 18

2 years ago
Created attachment 8751354 [details] [diff] [review]
Add UseCounter telemetry for AppCache event handlers. r?mayhemer
Attachment #8751354 - Flags: review?(honzab.moz)
Attachment #8751354 - Flags: feedback?(benjamin)
(Assignee)

Comment 19

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

Hang on, typo.
Attachment #8751354 - Flags: review?(honzab.moz)
(Assignee)

Comment 20

2 years ago
Created attachment 8751356 [details] [diff] [review]
Add UseCounter telemetry for AppCache event handlers. r?mayhemer
Attachment #8751356 - Flags: review?(honzab.moz)
Attachment #8751356 - Flags: feedback?(benjamin)
(Assignee)

Updated

2 years ago
Attachment #8751354 - Attachment is obsolete: true
Attachment #8751354 - Flags: feedback?(benjamin)
Attachment #8751356 - Flags: review?(honzab.moz) → review+

Updated

2 years ago
Attachment #8751350 - Flags: feedback?(benjamin) → feedback+

Updated

2 years ago
Attachment #8751356 - Flags: feedback?(benjamin) → feedback+
(Assignee)

Comment 21

2 years ago
Thanks for review and feedback, all.
(Assignee)

Comment 22

2 years ago
Created attachment 8751791 [details] [diff] [review]
Add UseCounter telemetry for AppCache event handlers.

Updating commit message, carrying over r+.
Attachment #8751791 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8751356 - Attachment is obsolete: true
(Assignee)

Comment 23

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

Ah, need review from a DOM peer. Blake, would you mind?
Attachment #8751350 - Flags: review?(mrbkap)
(Assignee)

Updated

2 years ago
Attachment #8751791 - Flags: review?(mrbkap)
(Assignee)

Comment 24

2 years ago
That was quick, thanks!
(Assignee)

Comment 26

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

Comment 27

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7dd02104f1ed
https://hg.mozilla.org/mozilla-central/rev/5737e7172325
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.