781200 - fixup several cache histograms

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Nathan Froyd [:froydnj]]

Several cache histograms aren't properly using the LINEAR format; this is blocking bug 764585 from making LINEAR histogram checking more stringent.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: patch

This patch changes several LINEAR histograms that didn't bucket properly into EXPONENTIAL histograms with bigger ranges.  The ranges are more in line with other cache histograms now; 100ms at the top end seemed rather restrictive anyway.

I also took the liberty of:

- Making DISK_CACHE_CORRUPT_DETAILS use HISTOGRAM_ENUMERATED_VALUES;
- Making CACHE_LM_INCONSISTENT use HISTOGRAM_BOOLEAN.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 650157 [details] [diff] [review]
patch

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

LGTM, but this will mean that it is difficult to compare this telemetry across this change, especially the lock wait telemetry. Please check with Nick to make sure that won't cause problems with the cache lock work he's working on.

Comment 3

[u408661]

Comment on attachment 650157 [details] [diff] [review]
patch

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

Looks fine to me, with one (possible) issue below. r+ with that addressed (or ignored, as appropriate)

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +194,5 @@
>  
>  #undef _HTTP_HIST
>  #undef HTTP_HISTOGRAMS
>  
> +HISTOGRAM_ENUMERATED_VALUES(DISK_CACHE_CORRUPT_DETAILS, 50, "Why the HTTP disk cache was corrupted at startup")

Do we need to rename this DISK_CACHE_CORRUPT_DETAILS_2 like we did for the other ones that have already been converted to an enumerated values histogram? I'm not versed on the details of the translation between the two types.

Comment 4

[Nathan Froyd [:froydnj]]

Brian, Nick, thanks for the reviews.

(In reply to Nick Hurley [:hurley] from comment #3)
> > +HISTOGRAM_ENUMERATED_VALUES(DISK_CACHE_CORRUPT_DETAILS, 50, "Why the HTTP disk cache was corrupted at startup")
> 
> Do we need to rename this DISK_CACHE_CORRUPT_DETAILS_2 like we did for the
> other ones that have already been converted to an enumerated values
> histogram?

We do not need to rename this one, since the current definition is:

HISTOGRAM(DISK_CACHE_CORRUPT_DETAILS, 1, 50, 51, LINEAR, "Why the HTTP disk cache was corrupted at startup")

and HISTOGRAM_ENUMERATED_VALUES is defined as:

#define HISTOGRAM_ENUMERATED_VALUES(id, n, message) \
  HISTOGRAM(id, 1, n, n+1, LINEAR, message)

and so the new definition of DISK_CACHE_CORRUPT_DETAILS expands to the same thing as the old definition.  It simply makes intent clearer.

(In reply to Brian Smith (:bsmith) from comment #2)
> LGTM, but this will mean that it is difficult to compare this telemetry
> across this change, especially the lock wait telemetry. Please check with
> Nick to make sure that won't cause problems with the cache lock work he's
> working on.

For avoidance of doubt, Nick, are you OK with the cache lock telemetry measurements changing out from underneath you?  (I don't know anything about your cache lock work or when it's projected to land; if it's not for another cycle or two, these changes may be beneficial because they'll give you better data than the current histograms.)

Comment 5

[u408661]

Yes, I'm fine with the measurements changing out from underneath me. Better data is... better :)

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Nick Hurley [:hurley] from comment #5)
> Yes, I'm fine with the measurements changing out from underneath me. Better
> data is... better :)

:)  Thanks for the confirmation.  I took the additional liberty of using HISTOGRAM_BOOLEAN for DISK_CACHE_SMART_SIZE_USING_OLD_MAX as well.

https://hg.mozilla.org/integration/mozilla-inbound/rev/042463c88673

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/042463c88673