Histogram versions and extended error codes for SQLite databases.

Histogram versions and extended error codes for SQLite databases.

Track database versions across the fleet for purposes of making
decisions about whether old migration code can be removed.

As part of this, refactor histogram code to key off a
per-database tag rather than histogram name, and to log in a
form which can be fanned out via the field trial logic in
histograms.xml

[FYI michaeln from webkit/OWNERS, erikwright from {chrome,content}/net/OWNERS, sky for chrome/browser/history/OWNERS]

BUG=none
TBR=michaeln@chromium.org, erikwright@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200216

[Scott Hess - ex-Googler, 2013-05-07 23:19:53]

Jim, I figure I should toss this past you before seeking OWNERS stamps.

I considered also just adding a database "id" which could be used like
Sqlite.|id|.Error and so on.  Not sure what I'd ever use it for except for as a
prefix like this, so I'm not entirely sure how much value it would have.

[jar (doing other things), 2013-05-07 23:46:41]

https://codereview.chromium.org/14976003/diff/3001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/14976003/diff/3001/sql/connection.cc#newcode711
sql/connection.cc:711: void Connection::AddLinearHistogram(const std::string&
suffix,
It is also tempting to use the sparse histogram to record teh error code.  I'm
*guessing* they are far and few between.

https://codereview.chromium.org/14976003/diff/3001/sql/connection.cc#newcode712
sql/connection.cc:712: size_t sample, size_t sample_max) const {
nit: one arg per line

https://codereview.chromium.org/14976003/diff/3001/sql/connection.cc#newcode713
sql/connection.cc:713: if (!histogram_prefix_.empty()) {
nit: personal preference.  Early return on empty() would make this easier to
read... and you wouldn't need to indent the rest of this code.

https://codereview.chromium.org/14976003/diff/3001/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/14976003/diff/3001/sql/connection.h#newcode160
sql/connection.h:160: size_t sample, size_t sample_max) const;
nit: one arg per line when wrapping declarations and definitions.

https://codereview.chromium.org/14976003/diff/3001/sql/meta_table.cc
File sql/meta_table.cc (right):

https://codereview.chromium.org/14976003/diff/3001/sql/meta_table.cc#newcode60
sql/meta_table.cc:60: static size_t kSqliteVersionMax = 100;
This fits wonderfully into a UMA_HISTOGRAM_SPARSE_SLOWLY.  It will use less
memory, and never grow too old.

https://codereview.chromium.org/14976003/diff/3001/tools/metrics/histograms/h...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/14976003/diff/3001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:6767: <summary>Appcache db version at
startup.</summary>
For each of these histograms.... can you give me a sense of how many distinct
buckets we'd expect to see across our fleet of Chrome users?  Are these things
like "about 3-5 versions a year" or "each day a new version is invented??"

[Scott Hess - ex-Googler, 2013-05-08 19:56:18]

WRT using sparse histograms ... maybe?  Probably?

On the client side, I would expect each user to report one version value per
profile opened per browser session, and for most users all of those versions
should be the same.  So a sparse histogram would make sense versus a linear
histogram, if it is basically a {key, value} store versus an array of [N]
values.

Excepting Web (which seems to version faster for some reason, 50>28 already,
can't explain that), the version numbers usually stay stable across multiple
Chromium releases.  If we had had this code in there from the beginning, I would
expect to range reported to the center to hit all version numbers which have
been released - the vast majority (99% or better) would likely be the current
version, but there would be inevitable laggards.  My current primary project is
to fix those cases, but in the worst case we'll have things like auto-update
failures preventing us from getting everyone on the same version.  So if the
server-side handling of an open-ended series of {key, value} pairs is materially
worse than an array of [N] values, that might be a consideration.

For the error codes, I have no idea what the actual distribution is, but my
intuition is that it's the standard case where most users report none at all,
and few users report a large number.  The actual range is 1..26, plus there is
an extended range encoded into high bits (IOERR is an example case).  Using a
sparse histogram would allow having a histogram recording the actual code rather
than doing the IOERR dance.

My guess is that there is no way to shift from the current encoding to a new
encoding?  It would probably be reasonable to retain the current histogram with
linear encoding and add an ExtendedError histogram with sparse encoding.  WDYT?

https://codereview.chromium.org/14976003/diff/3001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/14976003/diff/3001/sql/connection.cc#newcode712
sql/connection.cc:712: size_t sample, size_t sample_max) const {
On 2013/05/07 23:46:41, jar wrote:
> nit: one arg per line

Done.

https://codereview.chromium.org/14976003/diff/3001/sql/connection.cc#newcode713
sql/connection.cc:713: if (!histogram_prefix_.empty()) {
On 2013/05/07 23:46:41, jar wrote:
> nit: personal preference.  Early return on empty() would make this easier to
> read... and you wouldn't need to indent the rest of this code.

Done.  I had pulled it out of the earlier context as-is and hadn't thought to
revisit such decisions.

https://codereview.chromium.org/14976003/diff/3001/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/14976003/diff/3001/sql/connection.h#newcode160
sql/connection.h:160: size_t sample, size_t sample_max) const;
On 2013/05/07 23:46:41, jar wrote:
> nit: one arg per line when wrapping declarations and definitions.

Done.

[Scott Hess - ex-Googler, 2013-05-09 22:41:10]

OK, so I tried out using a sparse histogram for .Version and .ExtendedError, and
it seems like a good fit.  So this version adds those two, maintains the .Error
code for continuity, starts dropping the recently-added .IOERR histogram, and
tightens up the summaries a little so that they don't vary format.

I didn't mark the .Error histograms obsolete, yet, because I don't want to send
mixed signals to anyone reviewing them.  I figure they can be marked obsolete
when I remove the logging code in M-31 or something like that.  I expect we'll
continue receiving records indefinitely from old versions which aren't
auto-updating or whatever, so I could see removing the histograms entirely to
reduce noise on the page.

[jar (doing other things), 2013-05-13 22:05:59]

We chatted about this... and if you're worried, you can go with the whole
current CL.... but the comment below should make it simpler.

https://codereview.chromium.org/14976003/diff/14005/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/14976003/diff/14005/sql/connection.cc#newcode737
sql/connection.cc:737: // sparse histograms.  Remove the .Error histograms once
the
I think you can nicely get away with using the existing names, and all the
histogram data *should* fit together with no problem.

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

https://codereview.chromium.org/14976003/diff/14005/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:6860: <histogram
name="Sqlite.AppCache.ExtendedError" enum="SqliteErrorCode">
As we discussed... I think you don't need to rename these to use the sparse
histogram.  Both enumerated, and sparse, and have buckets that are exactly
one-wide, so teh deltas (which list min/max for bucket, and count) be
indistinguishable by the time they reach our UMA servers.

[Scott Hess - ex-Googler, 2013-05-13 22:43:08]

https://codereview.chromium.org/14976003/diff/14005/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/14976003/diff/14005/sql/connection.cc#newcode737
sql/connection.cc:737: // sparse histograms.  Remove the .Error histograms once
the
On 2013/05/13 22:05:59, jar wrote:
> I think you can nicely get away with using the existing names, and all the
> histogram data *should* fit together with no problem.

Done.

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

https://codereview.chromium.org/14976003/diff/14005/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:6860: <histogram
name="Sqlite.AppCache.ExtendedError" enum="SqliteErrorCode">
On 2013/05/13 22:05:59, jar wrote:
> As we discussed... I think you don't need to rename these to use the sparse
> histogram.  Both enumerated, and sparse, and have buckets that are exactly
> one-wide, so teh deltas (which list min/max for bucket, and count) be
> indistinguishable by the time they reach our UMA servers.

Done.

[jar (doing other things), 2013-05-14 02:12:46]

LGTM... but please consider the nit below.

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

https://codereview.chromium.org/14976003/diff/24001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:6926: <histogram
name="Sqlite.AppCache.Version">
nit: A bunch of these histograms have a postfix of either ".Version" or
".Error".  You can really nicely use the FieldTrial construct in this file to
only have to list the prefix once, and then have the FieldTrial provide the two
alternative completion (and provide appropriate prose to add to the shared
prose).

Please take a look at it... It will make this change smaller... and easier to
maintain going forward.

[Scott Hess - ex-Googler, 2013-05-14 03:43:17]

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

https://codereview.chromium.org/14976003/diff/24001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:6926: <histogram
name="Sqlite.AppCache.Version">
On 2013/05/14 02:12:46, jar wrote:
> nit: A bunch of these histograms have a postfix of either ".Version" or
> ".Error".  You can really nicely use the FieldTrial construct in this file to
> only have to list the prefix once, and then have the FieldTrial provide the
two
> alternative completion (and provide appropriate prose to add to the shared
> prose).
> 
> Please take a look at it... It will make this change smaller... and easier to
> maintain going forward.

I am not sure I understand your suggestion.  Using a fake FieldTrial to vary
between Error and Version would be weird - they aren't the same units, so it
would seem misleading.  [I admit that it is all synthetic with labels so it can
be done.  It just seems odd to have a single histogram which records either
error codes or version number depending on field trial.]

I suppose you could turn everything over and have histograms Sqlite.Error and
Sqlite.Version, with FieldTrials "AppCache", "Cookie", etc.  Then the result
would be Sqlite.Error.AppCache, Sqlite.Error.Cookie, etc, and might give some
histograms.xml savings along the lines you're suggesting.  Is that what you're
thinking?

The above assumes that <fieldtrial> only has name= and optional separator=
fields.  I don't see any other cases which would imply that it could insert
things elsewhere than as a suffix to the histogram.

[Scott Hess - ex-Googler, 2013-05-14 18:19:42]

OK.  I made the change to have two histograms, Sqlite.Error and Sqlite.Version,
plus a field trial for each database.  AFAICT it works WRT about:histograms, so
I think that's +1.

It does mean that the Sqlite.X.Error histograms will have to stick around for a
bit for continuity, but later on those can be deleted.

[Scott Hess - ex-Googler, 2013-05-14 18:27:37]

On 2013/05/14 18:19:42, shess wrote:
> OK.  I made the change to have two histograms, Sqlite.Error and
Sqlite.Version,
> plus a field trial for each database.  AFAICT it works WRT about:histograms,
so
> I think that's +1.

OK, so also consider that a request for a new review.  Thanks!

[jar (doing other things), 2013-05-14 19:50:10]

LGTM (... and yeah... I think you should TBR the tiny changes in the other
files).

[commit-bot: I haz the power, 2013-05-14 19:59:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/14976003/38001

[michaeln, 2013-05-14 22:20:41]

webkit/ lgtm2

[commit-bot: I haz the power, 2013-05-15 00:15:04]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-15 00:57:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/14976003/38001

[commit-bot: I haz the power, 2013-05-15 09:10:41]

Change committed as 200216