Suffixed histograms for SB2.DatabaseKilobytes and SB2.PrefixSetKilobytes

Suffixed histograms for SB2.DatabaseKilobytes and SB2.PrefixSetKilobytes

This is a follow-up to https://codereview.chromium.org/790703003/ to
recover the UMA stats recording granularity it removed.
It uses store IDs introduced in https://codereview.chromium.org/794273002/
to pass configuration information into histogram helper methods.

This also introduces SB2.DatabaseKilobytes stats for a few more stores
which weren't tracked (IPBlacklist, CSD whitelist, and download whitelist).

It also brings ExtensionBlacklist back on the map as it was previously
recorded as SB2.ExtensionBlacklistDatabaseKilobytes but there was no
matching entry for this in histograms.xml...

BUG=440517
Committed: https://crrev.com/696427220a29f9640f5eac7df72d4e657a457f0b
Cr-Commit-Position: refs/heads/master@{#309995}

[gab, 2014-12-18 21:21:16]

gab@chromium.org changed reviewers:
+ shess@chromium.org

[gab, 2014-12-18 21:21:16]

Hey Scott, PTAL, as mentioned on https://codereview.chromium.org/790703003/,
this is a proposal to regain the histograms granularity lost on that CL (and
more).

Thanks,
Gab

[Scott Hess - ex-Googler, 2014-12-18 23:57:55]

I get the impression you're waiting for me to review things :-).  Sorry, I've
been trying to focus on bootstrapping my own work this week so that I manage
some momentum.

I tend to get techy when faced with networks of dependent reviews.  If you were
to strip this down to what makes sense on origin/master, then push the other
bits to their respective CLs, it's plausible that I might give a thumbs-up more
quickly.  I'm not suggesting this as a hard response of any sort, just saying
that if an uncontroversial CL came my way which _didn't_ require looking at and
understanding other CLs, I might give my approval more quickly, so you aren't
waiting around on me.

https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_database.cc:1511:
static_cast<int>(file_size / 1024));
I find the prefix-set references misleading, here.  Really this seems like it
should be a store-id, some of which happen to have prefix sets associated with
them.  If that were the case, then it would be easy to pull the entire 3 lines
into a histogram helper for each of the points where it's happening.

It would also unify this helper with the whitelist helper.

https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_database.cc:1857: }
I think these might usefully use a helper on the order of:
   void HistogramHelper(name_base, name_suffix, sample);

https://codereview.chromium.org/815863002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/815863002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:29663: +    Deprecated 12/2014. Moved to
SB2.DatabaseKilobytes.Browse.
Doesn't this imply some changes to SB2.DatabaseKilobyes?

[gab, 2014-12-19 15:25:54]

Hey Scott,

what I want really is an lgtm on https://codereview.chromium.org/790703003/
whose only push back is that I'm unifying previously split histograms (and this
current CL is my promise to fix this as a follow-up).

Landing that will then allow me to finish my work with Matt on
https://codereview.chromium.org/794273002/.

Which will in turn allow me to land this current CL (and I can't do this current
CL on trunk as it's made much simpler by the PrefixSetId/SBWhitelistId's
introduced in CL 794273002)...

So I'm more than happy to put this review on hold until trunk catches up, but I
need an lgtm on CL 790703003 for that or we're in a code review deadlock..!

(or if we want to play conservative, we can finish this current CL (i.e. wait
for lgtm with it based on the 2 other CLs), then approve/land 790703003, and
wait until trunk catches up post-794273002 to actually land this current CL...)

Thanks for your time,
Gab



PS: Dependency chain simplified:

790703003 (removes duplicate code)

-> 794273002 (simpler thanks to removed duplicate code + introduces
PrefixSetId/SBWhitelistId among other things) 

-> 815863002 (this CL: brings back histogram granularity lost in 790703003 by
making use of IDs introduced in 794273002).

https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_database.cc:1511:
static_cast<int>(file_size / 1024));
On 2014/12/18 23:57:55, Scott Hess wrote:
> I find the prefix-set references misleading, here.  Really this seems like it
> should be a store-id, some of which happen to have prefix sets associated with
> them.  If that were the case, then it would be easy to pull the entire 3 lines
> into a histogram helper for each of the points where it's happening.
> 
> It would also unify this helper with the whitelist helper.

Right but I was trying to avoid introducing yet another enum which would have an
ID for every store (we already have an enum with an ID for every list and the
only difference would be that lists MALWARE/PHISHING are in the same BROWSE
store...).

And it felt that re-using the upcoming (and already piped through the relevant
calls) PrefixSetId/SBWhitelistId was natural (sure we could add a
PrefixSetIdToStoreId() helper to alleviate more ID piping, but I felt the
complexity of two different histogram helper was less than the complexity of yet
another store ID as it is centralized to a single use-case whereas a StoreId
enum is more global and invasive).

https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_database.cc:1857: }
On 2014/12/18 23:57:55, Scott Hess wrote:
> I think these might usefully use a helper on the order of:
>    void HistogramHelper(name_base, name_suffix, sample);

It felt to me like that avoiding the readability overhead of one more
indirection level was worth just inlining the Get()->Add(sample) without much
harm.

https://codereview.chromium.org/815863002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/815863002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:29663: +    Deprecated 12/2014. Moved to
SB2.DatabaseKilobytes.Browse.
On 2014/12/18 23:57:55, Scott Hess wrote:
> Doesn't this imply some changes to SB2.DatabaseKilobyes?

Yes, it is being suffixed in this CL, see <histogram_suffixes> additions at the
end of this file.

[gab, 2014-12-28 16:11:00]

Alright, this is now based on trunk with very little changes, so my replies
below still hold, PTAL.

Thanks and happy holidays!
Gab


>
https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
> File chrome/browser/safe_browsing/safe_browsing_database.cc (right):
> 
>
https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
> chrome/browser/safe_browsing/safe_browsing_database.cc:1511:
> static_cast<int>(file_size / 1024));
> On 2014/12/18 23:57:55, Scott Hess wrote:
> > I find the prefix-set references misleading, here.  Really this seems like
it
> > should be a store-id, some of which happen to have prefix sets associated
with
> > them.  If that were the case, then it would be easy to pull the entire 3
lines
> > into a histogram helper for each of the points where it's happening.
> > 
> > It would also unify this helper with the whitelist helper.
> 
> Right but I was trying to avoid introducing yet another enum which would have
an
> ID for every store (we already have an enum with an ID for every list and the
> only difference would be that lists MALWARE/PHISHING are in the same BROWSE
> store...).
> 
> And it felt that re-using the upcoming (and already piped through the relevant
> calls) PrefixSetId/SBWhitelistId was natural (sure we could add a
> PrefixSetIdToStoreId() helper to alleviate more ID piping, but I felt the
> complexity of two different histogram helper was less than the complexity of
yet
> another store ID as it is centralized to a single use-case whereas a StoreId
> enum is more global and invasive).
> 
>
https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
> chrome/browser/safe_browsing/safe_browsing_database.cc:1857: }
> On 2014/12/18 23:57:55, Scott Hess wrote:
> > I think these might usefully use a helper on the order of:
> >    void HistogramHelper(name_base, name_suffix, sample);
> 
> It felt to me like that avoiding the readability overhead of one more
> indirection level was worth just inlining the Get()->Add(sample) without much
> harm.
> 
>
https://codereview.chromium.org/815863002/diff/1/tools/metrics/histograms/his...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/815863002/diff/1/tools/metrics/histograms/his...
> tools/metrics/histograms/histograms.xml:29663: +    Deprecated 12/2014. Moved
to
> SB2.DatabaseKilobytes.Browse.
> On 2014/12/18 23:57:55, Scott Hess wrote:
> > Doesn't this imply some changes to SB2.DatabaseKilobyes?
> 
> Yes, it is being suffixed in this CL, see <histogram_suffixes> additions at
the
> end of this file.

[Scott Hess - ex-Googler, 2014-12-29 19:16:30]

On 2014/12/28 16:11:00, gab (mostly OOO until Jan 5th) wrote:
> Alright, this is now based on trunk with very little changes, so my replies
> below still hold, PTAL.
> 
> Thanks and happy holidays!
> Gab
> 
> 
> >
>
https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
> > File chrome/browser/safe_browsing/safe_browsing_database.cc (right):
> > 
> >
>
https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
> > chrome/browser/safe_browsing/safe_browsing_database.cc:1511:
> > static_cast<int>(file_size / 1024));
> > On 2014/12/18 23:57:55, Scott Hess wrote:
> > > I find the prefix-set references misleading, here.  Really this seems like
> it
> > > should be a store-id, some of which happen to have prefix sets associated
> with
> > > them.  If that were the case, then it would be easy to pull the entire 3
> lines
> > > into a histogram helper for each of the points where it's happening.
> > > 
> > > It would also unify this helper with the whitelist helper.
> > 
> > Right but I was trying to avoid introducing yet another enum which would
have
> an
> > ID for every store (we already have an enum with an ID for every list and
the
> > only difference would be that lists MALWARE/PHISHING are in the same BROWSE
> > store...).
> > 
> > And it felt that re-using the upcoming (and already piped through the
relevant
> > calls) PrefixSetId/SBWhitelistId was natural (sure we could add a
> > PrefixSetIdToStoreId() helper to alleviate more ID piping, but I felt the
> > complexity of two different histogram helper was less than the complexity of
> yet
> > another store ID as it is centralized to a single use-case whereas a StoreId
> > enum is more global and invasive).
> > 
> >
>
https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
> > chrome/browser/safe_browsing/safe_browsing_database.cc:1857: }
> > On 2014/12/18 23:57:55, Scott Hess wrote:
> > > I think these might usefully use a helper on the order of:
> > >    void HistogramHelper(name_base, name_suffix, sample);
> > 
> > It felt to me like that avoiding the readability overhead of one more
> > indirection level was worth just inlining the Get()->Add(sample) without
much
> > harm.
> > 
> >
>
https://codereview.chromium.org/815863002/diff/1/tools/metrics/histograms/his...
> > File tools/metrics/histograms/histograms.xml (right):
> > 
> >
>
https://codereview.chromium.org/815863002/diff/1/tools/metrics/histograms/his...
> > tools/metrics/histograms/histograms.xml:29663: +    Deprecated 12/2014.
Moved
> to
> > SB2.DatabaseKilobytes.Browse.
> > On 2014/12/18 23:57:55, Scott Hess wrote:
> > > Doesn't this imply some changes to SB2.DatabaseKilobyes?
> > 
> > Yes, it is being suffixed in this CL, see <histogram_suffixes> additions at
> the
> > end of this file.

I mean that SB2.DatabaseKilobytes current has:
> <obsolete>
>     Replaced by SB2.BrowseDatabaseKilobytes.
> </obsolete>
and I don't see that change in here.

I can see what you mean WRT having separate enums, I'm just worried that all of
these points are doing basically-identical operations in parallel (copy/paste
inheritance), and now it's sometimes this way, sometimes this slightly different
way, and sometimes this other slightly different way, and the filenames have to
match up with the enums, with nothing to catch errors.  In terms of
histogramming, I wonder if it wouldn't be cleaner to have a helper for
"Histogram database file size for <filename>" and have the helper intuit which
histogram suffix to use from the filename.  That's gross, but at least it allows
putting the boilerplate all in one place.

I'd also be find with using the list id for this.  It's all a horrible hack, but
I think we have to live with the list id enum regardless.

[Scott Hess - ex-Googler, 2014-12-29 19:18:15]

LGTM, on the basis that this is mostly just a sideways step (the existing code
is nasty).  Other comment just in the way of encouragement to close some loops
if you can, before you move on.

[gab, 2014-12-30 23:49:35]

On 2014/12/29 19:16:30, Scott Hess wrote:
> On 2014/12/28 16:11:00, gab (mostly OOO until Jan 5th) wrote:
> > Alright, this is now based on trunk with very little changes, so my replies
> > below still hold, PTAL.
> > 
> > Thanks and happy holidays!
> > Gab
> > 
> > 
> > >
> >
>
https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
> > > File chrome/browser/safe_browsing/safe_browsing_database.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
> > > chrome/browser/safe_browsing/safe_browsing_database.cc:1511:
> > > static_cast<int>(file_size / 1024));
> > > On 2014/12/18 23:57:55, Scott Hess wrote:
> > > > I find the prefix-set references misleading, here.  Really this seems
like
> > it
> > > > should be a store-id, some of which happen to have prefix sets
associated
> > with
> > > > them.  If that were the case, then it would be easy to pull the entire 3
> > lines
> > > > into a histogram helper for each of the points where it's happening.
> > > > 
> > > > It would also unify this helper with the whitelist helper.
> > > 
> > > Right but I was trying to avoid introducing yet another enum which would
> have
> > an
> > > ID for every store (we already have an enum with an ID for every list and
> the
> > > only difference would be that lists MALWARE/PHISHING are in the same
BROWSE
> > > store...).
> > > 
> > > And it felt that re-using the upcoming (and already piped through the
> relevant
> > > calls) PrefixSetId/SBWhitelistId was natural (sure we could add a
> > > PrefixSetIdToStoreId() helper to alleviate more ID piping, but I felt the
> > > complexity of two different histogram helper was less than the complexity
of
> > yet
> > > another store ID as it is centralized to a single use-case whereas a
StoreId
> > > enum is more global and invasive).
> > > 
> > >
> >
>
https://codereview.chromium.org/815863002/diff/1/chrome/browser/safe_browsing...
> > > chrome/browser/safe_browsing/safe_browsing_database.cc:1857: }
> > > On 2014/12/18 23:57:55, Scott Hess wrote:
> > > > I think these might usefully use a helper on the order of:
> > > >    void HistogramHelper(name_base, name_suffix, sample);
> > > 
> > > It felt to me like that avoiding the readability overhead of one more
> > > indirection level was worth just inlining the Get()->Add(sample) without
> much
> > > harm.
> > > 
> > >
> >
>
https://codereview.chromium.org/815863002/diff/1/tools/metrics/histograms/his...
> > > File tools/metrics/histograms/histograms.xml (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/815863002/diff/1/tools/metrics/histograms/his...
> > > tools/metrics/histograms/histograms.xml:29663: +    Deprecated 12/2014.
> Moved
> > to
> > > SB2.DatabaseKilobytes.Browse.
> > > On 2014/12/18 23:57:55, Scott Hess wrote:
> > > > Doesn't this imply some changes to SB2.DatabaseKilobyes?
> > > 
> > > Yes, it is being suffixed in this CL, see <histogram_suffixes> additions
at
> > the
> > > end of this file.
> 
> I mean that SB2.DatabaseKilobytes current has:
> > <obsolete>
> >     Replaced by SB2.BrowseDatabaseKilobytes.
> > </obsolete>
> and I don't see that change in here.

Oh I see what the confusion was, you were right in your initial review w.r.t
trunk, but this CL was based on https://codereview.chromium.org/790703003/ which
has now landed.

So SB2.BrowseDatabaseKilobytes is no longer obsolete on trunk as of that CL.

> 
> I can see what you mean WRT having separate enums, I'm just worried that all
of
> these points are doing basically-identical operations in parallel (copy/paste
> inheritance), and now it's sometimes this way, sometimes this slightly
different
> way, and sometimes this other slightly different way, and the filenames have
to
> match up with the enums, with nothing to catch errors.  In terms of
> histogramming, I wonder if it wouldn't be cleaner to have a helper for
> "Histogram database file size for <filename>" and have the helper intuit which
> histogram suffix to use from the filename.  That's gross, but at least it
allows
> putting the boilerplate all in one place.
> 
> I'd also be find with using the list id for this.  It's all a horrible hack,
but
> I think we have to live with the list id enum regardless.

Right, PTAL at patch set 3 which I think is what your proposal would look like
in practice (i.e. a histogram method taking the file's path).

This does provide nicer (less -- +72,-99 LOCs vs patch set 2) code, but the
histogram recording is less efficient in practice (more string operations --
copying/comparing) and also introduces yet another piece of code to update when
adding a new list (which I guess was already the case but switches with missing
cases break clang compile so are harder to miss than a NOTREACHED() which
typically requires making it to running the tests). 

Let me know whether you prefer patch set 2 or 3.

Cheers!
Gab

[Mark P, 2015-01-02 19:51:06]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2015-01-02 19:56:59]

histograms.xml not lgtm yet, though I'll be happy to approve once you deal with
my comments on the earlier changelist.  (The new changes here look fine.)

--mark

[gab, 2015-01-05 18:25:07]

On 2015/01/02 19:56:59, Mark P wrote:
> histograms.xml not lgtm yet, though I'll be happy to approve once you deal
with
> my comments on the earlier changelist.  (The new changes here look fine.)
> 
> --mark

Thanks, comments from https://codereview.chromium.org/790703003/ addressed in
patch set 5.

[Mark P, 2015-01-05 18:33:27]

histograms.xml lgtm baring one important comment below

https://codereview.chromium.org/815863002/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/815863002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:29921: +<histogram
name="SB2.DatabaseSizeKilobytes" units="KB">
You forgot to change the old histogram to SB2.DatabaseSizeKilobytes in the code.

[gab, 2015-01-05 19:11:31]

Thanks, done.

@shess: please let me know if you prefer patch set 2 or patch set 3's general
approach and we'll go from there.

Cheers,
Gab

https://codereview.chromium.org/815863002/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/815863002/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:29921: +<histogram
name="SB2.DatabaseSizeKilobytes" units="KB">
On 2015/01/05 18:33:27, Mark P wrote:
> You forgot to change the old histogram to SB2.DatabaseSizeKilobytes in the
code.

Oops, thanks, done.

[Scott Hess - ex-Googler, 2015-01-05 20:25:09]

On 2015/01/05 19:11:31, gab wrote:
> @shess: please let me know if you prefer patch set 2 or patch set 3's general
> approach and we'll go from there.

I dislike both!  I prefer 3 in terms of the call sites not having duplicate
boilerplate, but the decoder function does look nasty.  I don't think it
degrades the overall implementation, though, given the amount of manual mapping
going on in there.  Maybe someone can step up and clear up the id system so that
various things can become more table-driven.  I think having things
compile-break when a new list is added would be a good goal, but I'm not upset
about it at a micro level, that's the kind of thing that would be nice to
improve comprehensively in this code.

LGTM

[Scott Hess - ex-Googler, 2015-01-05 21:16:51]

On 2015/01/05 20:25:09, Scott Hess wrote:
> On 2015/01/05 19:11:31, gab wrote:
> > @shess: please let me know if you prefer patch set 2 or patch set 3's
general
> > approach and we'll go from there.
> 
> I dislike both!  I prefer 3 in terms of the call sites not having duplicate
> boilerplate, but the decoder function does look nasty.

Just to clarify - not jumping on you, I asked for that change, I mostly hate all
code.  So ignore my editorializing.

[gab, 2015-01-05 21:31:21]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2015-01-05 21:32:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/815863002/100001

[commit-bot: I haz the power, 2015-01-05 23:37:27]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-05 23:39:35]

Patchset 6 (id:??) landed as
https://crrev.com/696427220a29f9640f5eac7df72d4e657a457f0b
Cr-Commit-Position: refs/heads/master@{#309995}