[Manifest] Add metrics recording.

[Manifest] Add metrics recording.

This is recording whether the manifest fetching was successful or failed.
It is also recording which properties are present in the parsed Manifest
and whether the output of the parsing is empty.
Finally, it is recording whether a Manifest was used when creating a
shortcut.

BUG=366145
Committed: https://crrev.com/269f776a43e8d4534d8040f953a427c91ec4f44e
Cr-Commit-Position: refs/heads/master@{#298575}

[mlamouri (slow - plz ping), 2014-10-02 14:15:43]

mlamouri@chromium.org changed reviewers:
+ miguelg@google.com

[mlamouri (slow - plz ping), 2014-10-02 14:16:19]

mlamouri@chromium.org changed reviewers:
+ miguelg@chromium.org
- miguelg@google.com

[mlamouri (slow - plz ping), 2014-10-02 14:17:29]

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

[mlamouri (slow - plz ping), 2014-10-02 14:17:29]

mpearson@, please review changes in 
 tools/metrics/

miguelg@, please review changes in:
 content/
 chrome/

[Miguel Garcia, 2014-10-02 14:26:47]

Good stuff, happy to see we are going to be able to know if manifest usage
grows!

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
File content/renderer/manifest/manifest_manager.cc (right):

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
content/renderer/manifest/manifest_manager.cc:107:
ManifestUmaUtil::FetchFailed();
Don't you want to distinguish this case and the one below? It's very different
if the URL does not exist than if we were not able to fetch.

Chance are that we will find more reasons for failure so perhaps it's worth
creating a UMA enum for each error kind and fail with a specific reason.

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
content/renderer/manifest/manifest_manager.cc:133: manifest_ =
ManifestParser::Parse(data, response.url(), document_url);
don't you want to track when manifest is not parsable?

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
content/renderer/manifest/manifest_parser.cc:320:
ManifestUmaUtil::ParsedManifest(manifest);
since you only parse from one place I would do this at the caller site. and
depending on wether you got a a full manifest or an empty one log a manifest
valid or manifest error code.
RIght now all you have is wether the manifest is empty but you don't log parsing
errors (since you are returning early above)

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
File content/renderer/manifest/manifest_uma_util.cc (right):

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
content/renderer/manifest/manifest_uma_util.cc:13:
UMA_HISTOGRAM_BOOLEAN("Manifest.IsEmpty", manifest.IsEmpty());
you could early return here if it's empty.

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
content/renderer/manifest/manifest_uma_util.cc:23:
!manifest.gcm_sender_id.is_null());
what if it's empty?

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
File content/renderer/manifest/manifest_uma_util.h (right):

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
content/renderer/manifest/manifest_uma_util.h:14: static void
ParsedManifest(const Manifest& manifest);
You probably want to add documentation to what each of the methods does so they
can be understood without having to look at the code that calls them

[mlamouri (slow - plz ping), 2014-10-02 15:35:06]

PTAL

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
File content/renderer/manifest/manifest_manager.cc (right):

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
content/renderer/manifest/manifest_manager.cc:107:
ManifestUmaUtil::FetchFailed();
On 2014/10/02 14:26:46, Miguel Garcia wrote:
> Don't you want to distinguish this case and the one below? It's very different
> if the URL does not exist than if we were not able to fetch.
> 
> Chance are that we will find more reasons for failure so perhaps it's worth
> creating a UMA enum for each error kind and fail with a specific reason.

I've added "empty url" and "unspecified" for the fetch erorr types. We should
improve the granularity if we happen to need it.

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
content/renderer/manifest/manifest_manager.cc:133: manifest_ =
ManifestParser::Parse(data, response.url(), document_url);
On 2014/10/02 14:26:46, Miguel Garcia wrote:
> don't you want to track when manifest is not parsable?

I've added something. It will be tracked in the parser.

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
content/renderer/manifest/manifest_parser.cc:320:
ManifestUmaUtil::ParsedManifest(manifest);
On 2014/10/02 14:26:46, Miguel Garcia wrote:
> since you only parse from one place I would do this at the caller site. and
> depending on wether you got a a full manifest or an empty one log a manifest
> valid or manifest error code.
> RIght now all you have is wether the manifest is empty but you don't log
parsing
> errors (since you are returning early above)

Done.

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
File content/renderer/manifest/manifest_uma_util.cc (right):

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
content/renderer/manifest/manifest_uma_util.cc:13:
UMA_HISTOGRAM_BOOLEAN("Manifest.IsEmpty", manifest.IsEmpty());
On 2014/10/02 14:26:46, Miguel Garcia wrote:
> you could early return here if it's empty.

Done.

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
content/renderer/manifest/manifest_uma_util.cc:23:
!manifest.gcm_sender_id.is_null());
On 2014/10/02 14:26:46, Miguel Garcia wrote:
> what if it's empty?

It will be null if the value was not present of failed to be parsed. If the
value was set to the empty string, it is considered present. I don't think we
should try to special case that.

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
File content/renderer/manifest/manifest_uma_util.h (right):

https://codereview.chromium.org/622813002/diff/1/content/renderer/manifest/ma...
content/renderer/manifest/manifest_uma_util.h:14: static void
ParsedManifest(const Manifest& manifest);
On 2014/10/02 14:26:47, Miguel Garcia wrote:
> You probably want to add documentation to what each of the methods does so
they
> can be understood without having to look at the code that calls them

Done.

[Miguel Garcia, 2014-10-02 15:56:43]

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_manager.cc (right):

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_manager.cc:127:
ManifestUmaUtil::FetchFailed(ManifestUmaUtil::FETCH_UNSPECIFIED_REASON);
This can happen if fetching the url throws a 500 I assume. So we do know
something about the reason?

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_uma_util.cc (right):

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_uma_util.cc:55: fetch_result_type =
MANIFEST_FETCH_ERROR_EMPTY_URL;
why do you want two enums? one for uma and one externally? can't you simply move
this one to the .h file and use it instead? that way you avoid the switch here

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_uma_util.h (right):

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_uma_util.h:14: enum FetchFailureReason {
can't we forward declare enums now?

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_uma_util.h:16: FETCH_UNSPECIFIED_REASON
Oh you need all kind of parafernalia for UMA enums :)

See for example
https://cs.corp.google.com/#clankium/src/chrome/browser/content_settings/perm...

Namely, a comment that says to never remove elements and add elements in the
right order

a final FETCH_NUM element.

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_uma_util.h:27: // Record that the Manifest
fetching succeeded.
Record whether it succeeded (since you track failures too)

[mlamouri (slow - plz ping), 2014-10-02 16:10:55]

PTAL

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_manager.cc (right):

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_manager.cc:127:
ManifestUmaUtil::FetchFailed(ManifestUmaUtil::FETCH_UNSPECIFIED_REASON);
On 2014/10/02 15:56:42, Miguel Garcia wrote:
> This can happen if fetching the url throws a 500 I assume. So we do know
> something about the reason?

We knows the http error code but I don't think we should worry about that for
the moment.

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_uma_util.cc (right):

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_uma_util.cc:55: fetch_result_type =
MANIFEST_FETCH_ERROR_EMPTY_URL;
On 2014/10/02 15:56:42, Miguel Garcia wrote:
> why do you want two enums? one for uma and one externally? can't you simply
move
> this one to the .h file and use it instead? that way you avoid the switch here

One enum is the UMA enum, it's an implementation detail. Consumers of that
helper class shouldn't have to worry about it. The UMA enums has SUCCESS/FAILURE
while the exposed enum only has the failures information.

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_uma_util.h (right):

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_uma_util.h:14: enum FetchFailureReason {
On 2014/10/02 15:56:42, Miguel Garcia wrote:
> can't we forward declare enums now?

Not yet, sadly.

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_uma_util.h:16: FETCH_UNSPECIFIED_REASON
On 2014/10/02 15:56:42, Miguel Garcia wrote:
> Oh you need all kind of parafernalia for UMA enums :)
> 
> See for example
>
https://cs.corp.google.com/#clankium/src/chrome/browser/content_settings/perm...
> 
> Namely, a comment that says to never remove elements and add elements in the
> right order
> 
> a final FETCH_NUM element.

That's what I do for the internal enum. Not the exposed one.

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_uma_util.h:27: // Record that the Manifest
fetching succeeded.
On 2014/10/02 15:56:42, Miguel Garcia wrote:
> Record whether it succeeded (since you track failures too)

Did not understood.

[Mark P, 2014-10-03 19:33:19]

You seem to be using histograms.xml incorrectly.  You should have an entry in
there for each histogram you emit to.  Also, you do not need an entry for the
generic group "Manifest" (without a dot).  This is not a histogram; you do not
emit to it.

--mark

https://codereview.chromium.org/622813002/diff/20001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/622813002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:11605: Shortcut added to the homescreen with a
valid Manifest. This action will be
for my information: can this overlap with the Bookmark action?

https://codereview.chromium.org/622813002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:11606: recorded in addition of the three
actions below to keep a trend of whether
below?

alternate suggestion:
three actions below to
->
actions such as X, Y, and Z that

[Mark P, 2014-10-03 19:34:04]

Oops, ignore my histograms.xml comment.  I didn't see the suffix list stuff at
the end of the file.

I'll re-review that file.

--mark

[Mark P, 2014-10-03 19:57:50]

I only reviewed histograms.xml and the UMA_ calls themselves, not anything else.
 I'll leave that to your primary reviewer.

--mark

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_uma_util.cc (right):

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_uma_util.cc:14: enum ManifestFetchResultType
{
Please put the standard warning here that this enum is recorded in UMA logs and
hence you cannot delete or reorder or renumber any elements; new elements should
only be placed at the end.

Also please explicitly do = whatever for each element.

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_uma_util.cc:42:
UMA_HISTOGRAM_BOOLEAN("Manifest.ParseSuccess", false);
You use this name in two places.  Consider making it a local-file const char [].

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_uma_util.cc:46:
UMA_HISTOGRAM_ENUMERATION("Manifest.FetchResult",
ditto for this histogram name.

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

https://codereview.chromium.org/622813002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:11311: +<histogram name="Manifest"
enum="Boolean">
Can you move this histogram into a subgroup such as
"Manifest.PropertiesPresent".  It's confusing that have this group that has
suffixes and then you have other histograms that also come after this prefix.

[mlamouri (slow - plz ping), 2014-10-07 11:11:39]

Review comments applied. PTAL.

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
File content/renderer/manifest/manifest_uma_util.cc (right):

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_uma_util.cc:14: enum ManifestFetchResultType
{
On 2014/10/03 19:57:49, Mark P wrote:
> Please put the standard warning here that this enum is recorded in UMA logs
and
> hence you cannot delete or reorder or renumber any elements; new elements
should
> only be placed at the end.
> 
> Also please explicitly do = whatever for each element.

Done.

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_uma_util.cc:42:
UMA_HISTOGRAM_BOOLEAN("Manifest.ParseSuccess", false);
On 2014/10/03 19:57:49, Mark P wrote:
> You use this name in two places.  Consider making it a local-file const char
[].

Done.

https://codereview.chromium.org/622813002/diff/20001/content/renderer/manifes...
content/renderer/manifest/manifest_uma_util.cc:46:
UMA_HISTOGRAM_ENUMERATION("Manifest.FetchResult",
On 2014/10/03 19:57:49, Mark P wrote:
> ditto for this histogram name.

Done.

https://codereview.chromium.org/622813002/diff/20001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/622813002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:11605: Shortcut added to the homescreen with a
valid Manifest. This action will be
On 2014/10/03 19:33:18, Mark P wrote:
> for my information: can this overlap with the Bookmark action?

Yes, as mentioned below.

https://codereview.chromium.org/622813002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:11606: recorded in addition of the three
actions below to keep a trend of whether
On 2014/10/03 19:33:19, Mark P wrote:
> below?
> 
> alternate suggestion:
> three actions below to
> ->
> actions such as X, Y, and Z that

Done.

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

https://codereview.chromium.org/622813002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:11311: +<histogram name="Manifest"
enum="Boolean">
On 2014/10/03 19:57:49, Mark P wrote:
> Can you move this histogram into a subgroup such as
> "Manifest.PropertiesPresent".  It's confusing that have this group that has
> suffixes and then you have other histograms that also come after this prefix.
>  

Done.

[Miguel Garcia, 2014-10-07 14:45:10]

lgtm

[Mark P, 2014-10-07 16:43:53]

https://codereview.chromium.org/622813002/diff/40001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/622813002/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:11606: recorded in addition of AppShortcut,
AppShortcutApple and Bookmark actions
nit: of -> to

https://codereview.chromium.org/622813002/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:11607: above to keep a trend of whether the
webapp also add meta tags.
nit: omit "above"

These actions can appear in arbitrary orders in the dashboard.  Above doesn't
make sense.

https://codereview.chromium.org/622813002/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:11607: above to keep a trend of whether the
webapp also add meta tags.
meta tags?
I understand this comment in the context of AppShortcut and AppShortcutApple. 
Both of those actions mention meta tags.  But the Bookmark action doesn't
mention meta tags at all.  So either this comment is slightly wrong, or perhaps
you can revise the description of the Bookmark action to mention something about
meta tags?

[mlamouri (slow - plz ping), 2014-10-07 17:30:42]

PTAL

https://codereview.chromium.org/622813002/diff/40001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/622813002/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:11606: recorded in addition of AppShortcut,
AppShortcutApple and Bookmark actions
On 2014/10/07 16:43:53, Mark P wrote:
> nit: of -> to

Done.

https://codereview.chromium.org/622813002/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:11607: above to keep a trend of whether the
webapp also add meta tags.
On 2014/10/07 16:43:53, Mark P wrote:
> nit: omit "above"
> 
> These actions can appear in arbitrary orders in the dashboard.  Above doesn't
> make sense.

Done.

https://codereview.chromium.org/622813002/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:11607: above to keep a trend of whether the
webapp also add meta tags.
On 2014/10/07 16:43:53, Mark P wrote:
> meta tags?
> I understand this comment in the context of AppShortcut and AppShortcutApple. 
> Both of those actions mention meta tags.  But the Bookmark action doesn't
> mention meta tags at all.  So either this comment is slightly wrong, or
perhaps
> you can revise the description of the Bookmark action to mention something
about
> meta tags?

Done.

[Mark P, 2014-10-07 17:53:21]

lgtm

[mlamouri (slow - plz ping), 2014-10-07 19:43:55]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-10-07 19:46:15]

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

[commit-bot: I haz the power, 2014-10-07 21:24:48]

Committed patchset #5 (id:80001) as 79317ae75f627a57396187e20e95daea397bc94c

[commit-bot: I haz the power, 2014-10-07 21:25:24]

Patchset 5 (id:??) landed as
https://crrev.com/269f776a43e8d4534d8040f953a427c91ec4f44e
Cr-Commit-Position: refs/heads/master@{#298575}