Set up the infrastructure for Extension event metrics.

Set up the infrastructure for Extension event metrics.

This patch adds extension_event_histogram_value.h with a value for every
Extension event (scraped using the Docserver), changes the Event struct
constructor to require one, then makes every caller pass in UNKNOWN.
These are steps 1 and 2 in the bug.

Next I will be submitting bite-sized changes to use the real histogram values.

BUG=503402
R=rdevlin.cronin@chromium.org, isherman@chromium.org
TBR=achuith@chromium.org, rkc@chromium.org, dewittj@chromium.org, tommi@chromium.org, msw@chromium.org

Committed: https://crrev.com/f1b4d784ec2cf933cd261b3f3371b131ead9e567
Cr-Commit-Position: refs/heads/master@{#336003}

[not at google - send to devlin, 2015-06-22 23:42:11]

I've marked the interesting places.

Everything else is a very dumb script to make every Event construction use
UNKNOWN. You can trust me on that, or you can go through a lot of files - up to
you.

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.h:339: // The identifier for the event, for
histograms. In most cases this
Look here.

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_event_histogram_value.h (right):

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/extensio...
extensions/browser/extension_event_histogram_value.h:11: // TODO(kalman): I am
still in the process of migrating Event construction away
And here.

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/extensio...
extensions/browser/extension_event_histogram_value.h:22:
ACTIVITY_LOG_PRIVATE_ON_EXTENSION_ACTIVITY,
I scraped this using the Docserver, and then realised I left out internal APIs.
People are adding APIs even now, so either way it's quickly going to get out of
sync.

For now that's ok because these aren't actually recorded anywhere, but before
submitting I'll regenerate this list. Then later on, when I turn on metrics,
I'll generate it again.

https://codereview.chromium.org/1201063002/diff/1/tools/metrics/histograms/up...
File tools/metrics/histograms/update_extension_histograms.py (right):

https://codereview.chromium.org/1201063002/diff/1/tools/metrics/histograms/up...
tools/metrics/histograms/update_extension_histograms.py:24: ('ExtensionEvents',
This is just update_extension_functions.py moved, then updated to also modify
the events.

[Devlin, 2015-06-23 17:20:43]

Wow, that's a lot of files.

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.h:339: // The identifier for the event, for
histograms. In most cases this
On 2015/06/22 23:42:11, kalman wrote:
> Look here.

When will an event generate its own name?  If it weren't for that, this would be
a lot cleaner and more stable by passing in only one of these.

[not at google - send to devlin, 2015-06-23 17:49:10]

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.h:339: // The identifier for the event, for
histograms. In most cases this
On 2015/06/23 17:20:43, Devlin wrote:
> On 2015/06/22 23:42:11, kalman wrote:
> > Look here.
> 
> When will an event generate its own name?  If it weren't for that, this would
be
> a lot cleaner and more stable by passing in only one of these.

What do you mean "generate its own name"? I agree it would be more stable to
only pass in one of these, and I looked into that initially (I liked the idea of
enumerating the possible events then mapping that onto strings) but there are
some problems with this.

Mainly, dependency management. Idiomatically, even this histogram thing is
borderline breaking the extensions-module-is-separate thing, and pulling all
event *names* into the extension module is further down that path. Histogram
names are a weak dependency. Event names are not.

There are a couple of other problems that aren't worth going into so much -
they're solvable, but do increase the yak size. Generally I think this would be
a pretty dangerous path to take. I can predict exactly how complex this is going
to be: this patch, and subsequent patches to use the right event histogram
enums. More fundamentally changing how we dispatch events... I can't so well
predict what might happen there, but much more could go wrong.

[Devlin, 2015-06-23 17:55:56]

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.h:339: // The identifier for the event, for
histograms. In most cases this
On 2015/06/23 17:49:09, kalman wrote:
> On 2015/06/23 17:20:43, Devlin wrote:
> > On 2015/06/22 23:42:11, kalman wrote:
> > > Look here.
> > 
> > When will an event generate its own name?  If it weren't for that, this
would
> be
> > a lot cleaner and more stable by passing in only one of these.
> 
> What do you mean "generate its own name"? I agree it would be more stable to
> only pass in one of these, and I looked into that initially (I liked the idea
of
> enumerating the possible events then mapping that onto strings) but there are
> some problems with this.
> 
> Mainly, dependency management. Idiomatically, even this histogram thing is
> borderline breaking the extensions-module-is-separate thing, and pulling all
> event *names* into the extension module is further down that path. Histogram
> names are a weak dependency. Event names are not.
> 
> There are a couple of other problems that aren't worth going into so much -
> they're solvable, but do increase the yak size. Generally I think this would
be
> a pretty dangerous path to take. I can predict exactly how complex this is
going
> to be: this patch, and subsequent patches to use the right event histogram
> enums. More fundamentally changing how we dispatch events... I can't so well
> predict what might happen there, but much more could go wrong.

By "generate its own name", I mean whatever you meant on line 340-341 with "in
some cases events will generate their own names". ;)  And yeah, it would be kind
of a pain, but relatively solvable - I could see something like having a mapping
on the extensions/ side and the chrome/ side, and we check in the former first,
followed by the latter if it's not present.  But I do see what you mean about it
increasing size.

All that said, original question stands - when will events generate their own
names?

[not at google - send to devlin, 2015-06-23 17:59:02]

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.h:339: // The identifier for the event, for
histograms. In most cases this
On 2015/06/23 17:55:56, Devlin wrote:
> On 2015/06/23 17:49:09, kalman wrote:
> > On 2015/06/23 17:20:43, Devlin wrote:
> > > On 2015/06/22 23:42:11, kalman wrote:
> > > > Look here.
> > > 
> > > When will an event generate its own name?  If it weren't for that, this
> would
> > be
> > > a lot cleaner and more stable by passing in only one of these.
> > 
> > What do you mean "generate its own name"? I agree it would be more stable to
> > only pass in one of these, and I looked into that initially (I liked the
idea
> of
> > enumerating the possible events then mapping that onto strings) but there
are
> > some problems with this.
> > 
> > Mainly, dependency management. Idiomatically, even this histogram thing is
> > borderline breaking the extensions-module-is-separate thing, and pulling all
> > event *names* into the extension module is further down that path. Histogram
> > names are a weak dependency. Event names are not.
> > 
> > There are a couple of other problems that aren't worth going into so much -
> > they're solvable, but do increase the yak size. Generally I think this would
> be
> > a pretty dangerous path to take. I can predict exactly how complex this is
> going
> > to be: this patch, and subsequent patches to use the right event histogram
> > enums. More fundamentally changing how we dispatch events... I can't so well
> > predict what might happen there, but much more could go wrong.
> 
> By "generate its own name", I mean whatever you meant on line 340-341 with "in
> some cases events will generate their own names". ;)  And yeah, it would be
kind
> of a pain, but relatively solvable - I could see something like having a
mapping
> on the extensions/ side and the chrome/ side, and we check in the former
first,
> followed by the latter if it's not present.  But I do see what you mean about
it
> increasing size.
> 
> All that said, original question stands - when will events generate their own
> names?

APIs which have their schemas compiled will get event names generated, that is
what the comment is referred to. Not all schemas are compiled though.

OTOH there is no way for the schema to generate a histogram identifier because
the order is not predictable and would mess up the histogram.

[Devlin, 2015-06-23 18:07:38]

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.h (right):

https://codereview.chromium.org/1201063002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.h:339: // The identifier for the event, for
histograms. In most cases this
On 2015/06/23 17:59:01, kalman wrote:
> On 2015/06/23 17:55:56, Devlin wrote:
> > On 2015/06/23 17:49:09, kalman wrote:
> > > On 2015/06/23 17:20:43, Devlin wrote:
> > > > On 2015/06/22 23:42:11, kalman wrote:
> > > > > Look here.
> > > > 
> > > > When will an event generate its own name?  If it weren't for that, this
> > would
> > > be
> > > > a lot cleaner and more stable by passing in only one of these.
> > > 
> > > What do you mean "generate its own name"? I agree it would be more stable
to
> > > only pass in one of these, and I looked into that initially (I liked the
> idea
> > of
> > > enumerating the possible events then mapping that onto strings) but there
> are
> > > some problems with this.
> > > 
> > > Mainly, dependency management. Idiomatically, even this histogram thing is
> > > borderline breaking the extensions-module-is-separate thing, and pulling
all
> > > event *names* into the extension module is further down that path.
Histogram
> > > names are a weak dependency. Event names are not.
> > > 
> > > There are a couple of other problems that aren't worth going into so much
-
> > > they're solvable, but do increase the yak size. Generally I think this
would
> > be
> > > a pretty dangerous path to take. I can predict exactly how complex this is
> > going
> > > to be: this patch, and subsequent patches to use the right event histogram
> > > enums. More fundamentally changing how we dispatch events... I can't so
well
> > > predict what might happen there, but much more could go wrong.
> > 
> > By "generate its own name", I mean whatever you meant on line 340-341 with
"in
> > some cases events will generate their own names". ;)  And yeah, it would be
> kind
> > of a pain, but relatively solvable - I could see something like having a
> mapping
> > on the extensions/ side and the chrome/ side, and we check in the former
> first,
> > followed by the latter if it's not present.  But I do see what you mean
about
> it
> > increasing size.
> > 
> > All that said, original question stands - when will events generate their
own
> > names?
> 
> APIs which have their schemas compiled will get event names generated, that is
> what the comment is referred to. Not all schemas are compiled though.
> 
> OTOH there is no way for the schema to generate a histogram identifier because
> the order is not predictable and would mess up the histogram.

Ah, gotcha.  So in that case, it would be fairly easy to have a chrome/ and
extensions/ mapping of histogram values to event names, and it would also ensure
that we never create an event with the wrong name/value (and prevent callers
from needing to pass in duplicate values).  Apart from the obvious overhead of
creating the maps, is there a big downside?  (I'm not sure I follow how this
would be "fundamentally changing how we dispatch events...)

[not at google - send to devlin, 2015-06-23 18:12:10]

On 2015/06/23 18:07:38, Devlin wrote:
>
https://codereview.chromium.org/1201063002/diff/1/extensions/browser/event_ro...
> File extensions/browser/event_router.h (right):
> 
>
https://codereview.chromium.org/1201063002/diff/1/extensions/browser/event_ro...
> extensions/browser/event_router.h:339: // The identifier for the event, for
> histograms. In most cases this
> On 2015/06/23 17:59:01, kalman wrote:
> > On 2015/06/23 17:55:56, Devlin wrote:
> > > On 2015/06/23 17:49:09, kalman wrote:
> > > > On 2015/06/23 17:20:43, Devlin wrote:
> > > > > On 2015/06/22 23:42:11, kalman wrote:
> > > > > > Look here.
> > > > > 
> > > > > When will an event generate its own name?  If it weren't for that,
this
> > > would
> > > > be
> > > > > a lot cleaner and more stable by passing in only one of these.
> > > > 
> > > > What do you mean "generate its own name"? I agree it would be more
stable
> to
> > > > only pass in one of these, and I looked into that initially (I liked the
> > idea
> > > of
> > > > enumerating the possible events then mapping that onto strings) but
there
> > are
> > > > some problems with this.
> > > > 
> > > > Mainly, dependency management. Idiomatically, even this histogram thing
is
> > > > borderline breaking the extensions-module-is-separate thing, and pulling
> all
> > > > event *names* into the extension module is further down that path.
> Histogram
> > > > names are a weak dependency. Event names are not.
> > > > 
> > > > There are a couple of other problems that aren't worth going into so
much
> -
> > > > they're solvable, but do increase the yak size. Generally I think this
> would
> > > be
> > > > a pretty dangerous path to take. I can predict exactly how complex this
is
> > > going
> > > > to be: this patch, and subsequent patches to use the right event
histogram
> > > > enums. More fundamentally changing how we dispatch events... I can't so
> well
> > > > predict what might happen there, but much more could go wrong.
> > > 
> > > By "generate its own name", I mean whatever you meant on line 340-341 with
> "in
> > > some cases events will generate their own names". ;)  And yeah, it would
be
> > kind
> > > of a pain, but relatively solvable - I could see something like having a
> > mapping
> > > on the extensions/ side and the chrome/ side, and we check in the former
> > first,
> > > followed by the latter if it's not present.  But I do see what you mean
> about
> > it
> > > increasing size.
> > > 
> > > All that said, original question stands - when will events generate their
> own
> > > names?
> > 
> > APIs which have their schemas compiled will get event names generated, that
is
> > what the comment is referred to. Not all schemas are compiled though.
> > 
> > OTOH there is no way for the schema to generate a histogram identifier
because
> > the order is not predictable and would mess up the histogram.
> 
> Ah, gotcha.  So in that case, it would be fairly easy to have a chrome/ and
> extensions/ mapping of histogram values to event names, and it would also
ensure
> that we never create an event with the wrong name/value (and prevent callers
> from needing to pass in duplicate values).  Apart from the obvious overhead of
> creating the maps, is there a big downside?

Yes you could have an enum of every event name, then delegate out mapping those
to strings. Complex - and also, not runtime typesafe which bothers me.

> (I'm not sure I follow how this
> would be "fundamentally changing how we dispatch events...)

The patch I've written is purely additive and doesn't introduce behavioral
changes. If I *modify* things then it does involve behavioral changes and I'd
rather not reason about that over 170+ files.

[Devlin, 2015-06-23 19:41:58]

On 2015/06/23 18:12:10, kalman wrote:
> Yes you could have an enum of every event name, then delegate out mapping
those
> to strings. Complex - and also, not runtime typesafe which bothers me.
> 
> > (I'm not sure I follow how this
> > would be "fundamentally changing how we dispatch events...)
> 
> The patch I've written is purely additive and doesn't introduce behavioral
> changes. If I *modify* things then it does involve behavioral changes and I'd
> rather not reason about that over 170+ files.

Talked about this more offline, and decided that while my proposed approach
would probably be better (it also has some nice advantages for IPC), it might be
prohibitively cumbersome.  LGTM for this patch.

[not at google - send to devlin, 2015-06-23 19:43:49]

kalman@chromium.org changed reviewers:
+ isherman@chromium.org

[not at google - send to devlin, 2015-06-23 19:44:54]

+isherman for histograms. This is not trivial, but the relevant files for you
are the same I pointed out to Devlin in the first CL, plus note I added you +
other metrics owners to extensions/browser/OWNERS.

[Ilya Sherman, 2015-06-23 21:34:21]

LGTM % nits, assuming that you're not yet recording this metric.  If that's not
the case, could you please point me to the code that does record the metric? 
(FWIW, when you do add that code, please make sure to add me or another
histograms code reviewer to that CL, and call out the new C++ histogram code.)

https://codereview.chromium.org/1201063002/diff/40001/extensions/browser/exte...
File extensions/browser/extension_event_histogram_value.h (right):

https://codereview.chromium.org/1201063002/diff/40001/extensions/browser/exte...
extensions/browser/extension_event_histogram_value.h:346: //
tools/metrics/histograms/histograms.xml.
Please mention the script that automates this update.  Also, people seem to
frequently miss this comment in the other file.  Perhaps we could accentuate it
somehow?

https://codereview.chromium.org/1201063002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/update_extension_histograms.py (right):

https://codereview.chromium.org/1201063002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/update_extension_histograms.py:5: """Updates
ExtensionEvents and ExtensionFunctions enum in histograms.xml file
nit: "Updates the ExtensionEvents and ExtensionFunctions enums in histograms.xml
with..." (i.e. add articles, drop "file")

[not at google - send to devlin, 2015-06-23 23:27:09]

Thanks. I'm not recording the metric yet, I need to make all call sites not use
UNKNOWN, then I'm going to add it. I'll need a histogram owner for that anyway.

https://codereview.chromium.org/1201063002/diff/40001/extensions/browser/exte...
File extensions/browser/extension_event_histogram_value.h (right):

https://codereview.chromium.org/1201063002/diff/40001/extensions/browser/exte...
extensions/browser/extension_event_histogram_value.h:346: //
tools/metrics/histograms/histograms.xml.
On 2015/06/23 21:34:20, Ilya Sherman wrote:
> Please mention the script that automates this update.  Also, people seem to
> frequently miss this comment in the other file.  Perhaps we could accentuate
it
> somehow?

Good point there should be a comment. Updated other file as well.

https://codereview.chromium.org/1201063002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/update_extension_histograms.py (right):

https://codereview.chromium.org/1201063002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/update_extension_histograms.py:5: """Updates
ExtensionEvents and ExtensionFunctions enum in histograms.xml file
On 2015/06/23 21:34:20, Ilya Sherman wrote:
> nit: "Updates the ExtensionEvents and ExtensionFunctions enums in
histograms.xml
> with..." (i.e. add articles, drop "file")

Done.

[not at google - send to devlin, 2015-06-23 23:52:50]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-06-23 23:52:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1201063002/#ps60001
(title: "comments")

[not at google - send to devlin, 2015-06-23 23:53:32]

The CQ bit was unchecked by kalman@chromium.org

[not at google - send to devlin, 2015-06-23 23:55:01]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-06-23 23:55:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1201063002/#ps80001
(title: "update histograms")

[commit-bot: I haz the power, 2015-06-23 23:57:19]

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

[commit-bot: I haz the power, 2015-06-24 00:16:01]

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

[commit-bot: I haz the power, 2015-06-24 00:16:04]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[not at google - send to devlin, 2015-06-24 00:29:36]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-06-24 00:29:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1201063002/#ps100001
(title: "add back magic markers")

[commit-bot: I haz the power, 2015-06-24 00:31:58]

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

[commit-bot: I haz the power, 2015-06-24 00:46:05]

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

[commit-bot: I haz the power, 2015-06-24 00:46:08]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[not at google - send to devlin, 2015-06-24 18:34:51]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-06-24 18:34:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1201063002/#ps120001
(title: "disable presubmit for now")

[commit-bot: I haz the power, 2015-06-24 18:38:20]

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

[commit-bot: I haz the power, 2015-06-24 18:43:40]

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

[commit-bot: I haz the power, 2015-06-24 18:43:42]

Exceeded global retry quota

[not at google - send to devlin, 2015-06-24 19:44:39]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-06-24 19:44:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1201063002/#ps140001
(title: "rebaaaaase")

[commit-bot: I haz the power, 2015-06-24 19:46:22]

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

[commit-bot: I haz the power, 2015-06-24 21:14:20]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-06-24 21:15:18]

Patchset 8 (id:??) landed as
https://crrev.com/f1b4d784ec2cf933cd261b3f3371b131ead9e567
Cr-Commit-Position: refs/heads/master@{#336003}