Closed Bug 1147973 Opened 5 years ago Closed 5 years ago

Detailed disk cache hit rate telemetry is wrong

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: michal, Assigned: michal)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch fix (obsolete) — Splinter Review
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?
Attached patch patch v2Splinter Review
(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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.