Detailed disk cache hit rate telemetry is wrong

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: michal.novotny, Assigned: michal.novotny)

Tracking

Trunk
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 8583918 [details] [diff] [review]
fix

Entries that are created as new are not reported.
Attachment #8583918 - Flags: review?(honzab.moz)
Comment on attachment 8583918 [details] [diff] [review]
fix

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

::: netwerk/cache2/CacheEntry.cpp
@@ +386,5 @@
>      mozilla::MutexAutoUnlock unlock(mLock);
>  
> +    if (reportMiss) {
> +      CacheFileUtils::DetailedCacheHitTelemetry::AddRecord(
> +        CacheFileUtils::DetailedCacheHitTelemetry::MISS, TimeStamp());

You are actually filling the gap when we open the file directly (not getting OnFileReady call), right?

Why exactly you don't want to report NETWORK_CACHE_V2_MISS_TIME_MS here?  I think to report 0ms is pretty much OK, we want to see how fast we detect a cache miss, no?
(Assignee)

Comment 2

4 years ago
Created attachment 8584021 [details] [diff] [review]
patch v2

(In reply to Honza Bambas (:mayhemer) from comment #1)
> You are actually filling the gap when we open the file directly (not getting
> OnFileReady call), right?

Right.

 
> Why exactly you don't want to report NETWORK_CACHE_V2_MISS_TIME_MS here?  I
> think to report 0ms is pretty much OK, we want to see how fast we detect a
> cache miss, no?

I wanted to fix hit rate stats without affecting NETWORK_CACHE_V2_MISS_TIME_MS but I'm not against.
Attachment #8583918 - Attachment is obsolete: true
Attachment #8583918 - Flags: review?(honzab.moz)
Attachment #8584021 - Flags: review?(honzab.moz)
Attachment #8584021 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/9bf6a6c84045
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.