Closed Bug 892486 Opened 6 years ago Closed 6 years ago

Add telemetry for appcache

Categories

(Core :: Networking: Cache, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox23 --- fixed
firefox24 --- fixed
firefox25 --- fixed

People

(Reporter: ehsan, Assigned: mayhemer)

Details

(Whiteboard: [qa-])

Attachments

(1 file)

We need to know more about how many websites are using appcache.
Assignee: nobody → honzab.moz
We need to collect the ratio of appcached document loads versus non-appcached document loads.
Attached patch v1Splinter Review
- for top-level document loads it records boolean probe
- when appcache is found for the URL it comes from the appcache, otherwise not
Attachment #775862 - Flags: review?(ehsan)
Comment on attachment 775862 [details] [diff] [review]
v1

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

I'm not very well versed in these telemetry probes.  Patrick, could you please do the honors here?
Attachment #775862 - Flags: review?(ehsan) → review?(mcmanus)
Comment on attachment 775862 [details] [diff] [review]
v1

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2547,5 @@
>              NS_ENSURE_SUCCESS(rv, rv);
>          }
>      }
>  
> +    if (mLoadFlags & LOAD_INITIAL_DOCUMENT_URI)

one line: if (foo) {
Attachment #775862 - Flags: review?(mcmanus) → review+
(In reply to Honza Bambas (:mayhemer) from comment #5)
> Comment on attachment 775862 [details] [diff] [review]
> v1
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2d7cf35605

Technically it should've been r=mcmanus.  :-)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> (In reply to Honza Bambas (:mayhemer) from comment #5)
> > Comment on attachment 775862 [details] [diff] [review]
> > v1
> > 
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2d7cf35605
> 
> Technically it should've been r=mcmanus.  :-)

Looks like I've turned that upside down :))  Sorry!
As I'm thinking of it, we might want to add one more probe.

The probe in the patch only measures what top-level pages we load from appcache.  But we don't measure what pages are actually "using appcache" by means of defining the manifest attribute.

Since we have the prompt and there is an option "Never for This Site" some people could turn to never use appcache.  We don't find out this as well as users just close or ignore the prompt at all.

So, we may need another probe like "HAS_APPCACHE_MANIFEST" added to the parser.

Opinions?
Sounds like a good idea to me!
https://hg.mozilla.org/mozilla-central/rev/6c2d7cf35605
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
uplift to m-a, right?
Comment on attachment 775862 [details] [diff] [review]
v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: None
Testing completed (on m-c, etc.): Just landed
Risk to taking this patch (and alternatives if risky): Zero
String or IDL/UUID changes made by this patch: None
Attachment #775862 - Flags: approval-mozilla-aurora?
Any maybe beta?  I think we should try to start to get telemetry data ASAP.
Attachment #775862 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 775862 [details] [diff] [review]
v1

[Approval Request Comment]
same as for aurora
Attachment #775862 - Flags: approval-mozilla-beta?
Attachment #775862 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Do we know of particular websites or use cases where appcache is used so QA can verify the telemetry probes are working?
Anthony, don't worry about verification of this bug.  There are far more important things to test.
Thanks Doug.
Whiteboard: [qa-]
(In reply to Honza Bambas (:mayhemer) from comment #8)
> So, we may need another probe like "HAS_APPCACHE_MANIFEST" added to the
> parser.

If we decide to remove the prompt soon, I think there is no need for this additional probe.  I'll wait until that bug is solved or put to ice.
You need to log in before you can comment on or make changes to this bug.