IndexedDB: Add histograms for key type

IndexedDB: Add histograms for key type

Gather the type of key computed when adding records to object stores,
primarily to gauge usage of array keys (which are not implemented in
all browsers).

BUG=689211
R=mpearson@chromium.org,cmumford@chromium.org

Review-Url: https://codereview.chromium.org/2676403003
Cr-Commit-Position: refs/heads/master@{#449427}
Committed: https://chromium.googlesource.com/chromium/src/+/b5af6809340d677b2121a294321cb6e32d94a0b4

[jsbell, 2017-02-06 22:14:23]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 22:15:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 23:53:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 23:53:44]

Dry run: This issue passed the CQ dry run.

[jsbell, 2017-02-07 00:34:39]

mpearson@ - can you review histograms?
cmumford@ - can you review the Indexed DB changes?

Note that IDBKey::MinType was unused. It's a remnant from the back end logic
(which was in webkit in ages past) that encodes for a key before any other keys,
but is not exposed to script. It doesn't need to be in this enum.

[Mark P, 2017-02-07 19:12:22]

https://codereview.chromium.org/2676403003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBKey.h (right):

https://codereview.chromium.org/2676403003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBKey.h:66: // Also used for UMA.
Append only.
Please follow the comments and = guidelines here.
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...
Also, generally it's better practice to make the max bigger than the last
element so you can expand this histogram correctly later.

https://codereview.chromium.org/2676403003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2676403003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:74502: +    being stored/updated in an
Indexed DB object store.
Recorded upon any time an item is stored or updated, I assume?  (If yes, no
changes are necessary.)

Would it be useful to distinguish between "newly stored" and "updated"?

Also, if the key is an "array of keys", is each separate item (which you imply
are "sub keys") also emitted to?  (I assume the array value is always emitted
regardless.)

all the same questions apply below

https://codereview.chromium.org/2676403003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:93723: +  <int value="1"
label="Array">Key is an array of other keys.</int>
nit: does "of other keys" add anything here?

[jsbell, 2017-02-07 19:54:45]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[jsbell, 2017-02-07 19:54:57]

Thanks mpearson@ !

https://codereview.chromium.org/2676403003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBKey.h (right):

https://codereview.chromium.org/2676403003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBKey.h:66: // Also used for UMA.
Append only.
On 2017/02/07 19:12:22, Mark P (away thru Feb 5 2017) wrote:
> Please follow the comments and = guidelines here.
>
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...
> Also, generally it's better practice to make the max bigger than the last
> element so you can expand this histogram correctly later.

Done.

https://codereview.chromium.org/2676403003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2676403003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:74502: +    being stored/updated in an
Indexed DB object store.
On 2017/02/07 19:12:22, Mark P (away thru Feb 5 2017) wrote:
> Recorded upon any time an item is stored or updated, I assume?  (If yes, no
> changes are necessary.)

Yes. Made it "newly stored or updated"

> Would it be useful to distinguish between "newly stored" and "updated"?

Not really (but I stole your terminology). Also, it would be hard to implement
(for updates we can store without checking to see if a previous value exists.)

> Also, if the key is an "array of keys", is each separate item (which you imply
> are "sub keys") also emitted to? 

No - added further note to clarify. While I can imagine circumstances where that
would be useful (maybe no-one uses binary keys except inside arrays...)
gathering it now is not needed for anything - we're really just trying to
measure usage of (1) array keys to encourage Edge to implement, and (2) measure
uptake of binary keys when we finally ship that support.

>  (I assume the array value is always emitted
> regardless.)

Yes.

https://codereview.chromium.org/2676403003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:93723: +  <int value="1"
label="Array">Key is an array of other keys.</int>
On 2017/02/07 19:12:22, Mark P (away thru Feb 5 2017) wrote:
> nit: does "of other keys" add anything here?

Not really. Dropped it.

[commit-bot: I haz the power, 2017-02-07 19:55:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cmumford, 2017-02-07 20:10:38]

lgtm % nits.

https://codereview.chromium.org/2676403003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/2676403003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:28: #include
<v8.h>
I think it was correct to move <memory> up here, but v8.h should be down below
where it was.

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

https://codereview.chromium.org/2676403003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:93712: +  <int value="0"
label="Invalid">Invalid key.</int>
This will never be logged right? If so how about deleting this enum value, if it
does get logged then UMA will display "0" right?

https://codereview.chromium.org/2676403003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/2676403003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:28: #include
<v8.h>
<v8.h> below I think.

[jsbell, 2017-02-07 21:58:24]

https://codereview.chromium.org/2676403003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/2676403003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:28: #include
<v8.h>
On 2017/02/07 20:10:37, cmumford wrote:
> I think it was correct to move <memory> up here, but v8.h should be down below
> where it was.

D'oh - it should be further down at any rate.

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
says: Related header, C library, C++ library, other libraries' .h, your
project's .h.

So as a "other libraries' .h" v8.h would be below C++ library and above "your
project's .h" ? But per the blink-dev PSA thread on blink-dev, maybe we'll
change this to "v8/include/v8.h"

`git cl format` doesn't let `#include <x.h>` follow `#include "y.h"` in the same
block, so it can't stay where it is.

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

https://codereview.chromium.org/2676403003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:93712: +  <int value="0"
label="Invalid">Invalid key.</int>
On 2017/02/07 20:10:37, cmumford wrote:
> This will never be logged right? If so how about deleting this enum value, if
it
> does get logged then UMA will display "0" right?

Correct, it should never be logged.

mpearson@ - what is preferred here?

[Mark P, 2017-02-07 22:59:46]

histograms.xml lgtm

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

https://codereview.chromium.org/2676403003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:93712: +  <int value="0"
label="Invalid">Invalid key.</int>
On 2017/02/07 21:58:24, jsbell wrote:
> On 2017/02/07 20:10:37, cmumford wrote:
> > This will never be logged right? If so how about deleting this enum value,
if
> it
> > does get logged then UMA will display "0" right?
> 
> Correct, it should never be logged.
> 
> mpearson@ - what is preferred here?

It could go either way in this case.  It depends in which case you think someone
is more likely to contact you: if someone sees a "0" on the dashboard or if
someone sees an "Invalid key."?

https://codereview.chromium.org/2676403003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBKey.h (right):

https://codereview.chromium.org/2676403003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBKey.h:75: TypeEnumMax = 6,
nit: leave max unassigned; it will default to 6.  Then it becomes easier that
it's okay to change the max value.

[haraken, 2017-02-07 23:53:37]

bindings LGTM

[jsbell, 2017-02-09 20:03:00]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2017-02-09 20:03:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
cmumford@chromium.org, haraken@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2676403003/#ps80001
(title: "Leave number off enum max")

[commit-bot: I haz the power, 2017-02-09 20:04:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 21:52:21]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486670580772870,
"parent_rev": "5cd331dec1c51a6040c15c1afb4eb03d77187dbe", "commit_rev":
"b5af6809340d677b2121a294321cb6e32d94a0b4"}

[commit-bot: I haz the power, 2017-02-09 21:53:02]

Description was changed from

==========
IndexedDB: Add histograms for key type

Gather the type of key computed when adding records to object stores,
primarily to gauge usage of array keys (which are not implemented in
all browsers).

BUG=689211
R=mpearson@chromium.org,cmumford@chromium.org
==========

to

==========
IndexedDB: Add histograms for key type

Gather the type of key computed when adding records to object stores,
primarily to gauge usage of array keys (which are not implemented in
all browsers).

BUG=689211
R=mpearson@chromium.org,cmumford@chromium.org

Review-Url: https://codereview.chromium.org/2676403003
Cr-Commit-Position: refs/heads/master@{#449427}
Committed:
https://chromium.googlesource.com/chromium/src/+/b5af6809340d677b2121a294321c...
==========

[commit-bot: I haz the power, 2017-02-09 21:53:03]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b5af6809340d677b2121a294321c...