Add histogram definitions for Chrome OS crouton statistics

Add histogram definitions for Chrome OS crouton statistics

This adds enum type definitions and histogram declaration for
crouton metric recorded by the Chrome OS metrics daemon. See also
https://chromium-review.googlesource.com/#/c/425529 .

BUG=chromium:678865
TEST=None

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

[drinkcat1, 2017-01-06 04:04:15]

Description was changed from

==========
Add histogram definitions for Chrome OS crouton statistics

This adds enum type definitions and histogram declaration for
crouton metric recorded by the Chrome OS metrics daemon. See also
https://chromium-review.googlesource.com/#/c/425529 .

BUG=chromium:678865
TEST=None
==========

to

==========
Add histogram definitions for Chrome OS crouton statistics

This adds enum type definitions and histogram declaration for
crouton metric recorded by the Chrome OS metrics daemon. See also
https://chromium-review.googlesource.com/#/c/425529 .

BUG=chromium:678865
TEST=None
==========

[drinkcat1, 2017-01-06 04:04:15]

drinkcat@chromium.org changed reviewers:
+ asvitkine@chromium.org, dnschneid@chromium.org, isherman@chromium.org

[drinkcat1, 2017-01-06 04:04:48]

drinkcat@chromium.org changed reviewers:
+ semenzato@chromium.org

[Alexei Svitkine (slow), 2017-01-06 16:18:19]

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

https://codereview.chromium.org/2616663005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:43939: +    since the previous boot.
Nit: Mention when this is logged. Once on startup? Periodically? Something else?

[Ilya Sherman, 2017-01-06 22:12:45]

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

https://codereview.chromium.org/2616663005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:96967: +  <int value="0" label="Total
count"/>
I think it might be easier to interpret the data if this bucket were
semantically "Not started" rather than a total count.  Is that more difficult to
implement?

[drinkcat1, 2017-01-07 05:13:20]

On 2017/01/06 16:18:19, Alexei Svitkine (slow) wrote:
>
https://codereview.chromium.org/2616663005/diff/1/tools/metrics/histograms/hi...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2616663005/diff/1/tools/metrics/histograms/hi...
> tools/metrics/histograms/histograms.xml:43939: +    since the previous boot.
> Nit: Mention when this is logged. Once on startup? Periodically? Something
else?

0 is always sent on startup, then file existence is checked periodically, and,
upon existence, a 1 is only sent once per boot.

I'll update the text depending on the outcome of the discussion below.

[drinkcat1, 2017-01-07 05:16:37]

On 2017/01/06 22:12:45, Ilya Sherman (Away De.29-Ja.4) wrote:
>
https://codereview.chromium.org/2616663005/diff/1/tools/metrics/histograms/hi...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2616663005/diff/1/tools/metrics/histograms/hi...
> tools/metrics/histograms/histograms.xml:96967: +  <int value="0" label="Total
> count"/>
> I think it might be easier to interpret the data if this bucket were
> semantically "Not started" rather than a total count.  Is that more difficult
to
> implement?

I was wondering about that, see bug and
https://chromium-review.googlesource.com/#/c/425529/ ...

"Not Started" is tricky as we probably want to count only once per boot, and
/run/crouton only appears when the user starts it. Then /run/crouton file never
goes away once it has been created, and only disappears on reboot (tmpfs), so we
never would report "0" again, anyway.

Actually, I was considering not sending the "0" value at all, and getting the
total number from some other metric (say Platform.BootTime.DevSwitch). Is that
something sensible? Or not recommended at all?

[Ilya Sherman, 2017-01-09 19:11:09]

On 2017/01/07 05:16:37, drinkcat1 wrote:
> On 2017/01/06 22:12:45, Ilya Sherman (Away De.29-Ja.4) wrote:
> >
>
https://codereview.chromium.org/2616663005/diff/1/tools/metrics/histograms/hi...
> > File tools/metrics/histograms/histograms.xml (right):
> > 
> >
>
https://codereview.chromium.org/2616663005/diff/1/tools/metrics/histograms/hi...
> > tools/metrics/histograms/histograms.xml:96967: +  <int value="0"
label="Total
> > count"/>
> > I think it might be easier to interpret the data if this bucket were
> > semantically "Not started" rather than a total count.  Is that more
difficult
> to
> > implement?
> 
> I was wondering about that, see bug and
> https://chromium-review.googlesource.com/#/c/425529/ ...
> 
> "Not Started" is tricky as we probably want to count only once per boot, and
> /run/crouton only appears when the user starts it. Then /run/crouton file
never
> goes away once it has been created, and only disappears on reboot (tmpfs), so
we
> never would report "0" again, anyway.
> 
> Actually, I was considering not sending the "0" value at all, and getting the
> total number from some other metric (say Platform.BootTime.DevSwitch). Is that
> something sensible? Or not recommended at all?

Okay, that explanation makes sense for why you've structured this histogram as
you have.

I do think that it's better to include the baseline within the histogram, rather
than getting the total number from some other metric.  Having everything in one
place makes it easier for someone viewing the histogram to quickly grok the
data, and it makes it less likely that semantics will drift without anyone
realizing that's happening.  So, the histogram setup looks good as you have it,
given the difficulty of calculating "Not started".

[drinkcat1, 2017-01-10 02:59:40]

On 2017/01/07 05:13:20, drinkcat1 wrote:
> On 2017/01/06 16:18:19, Alexei Svitkine (slow) wrote:
> >
>
https://codereview.chromium.org/2616663005/diff/1/tools/metrics/histograms/hi...
> > File tools/metrics/histograms/histograms.xml (right):
> > 
> >
>
https://codereview.chromium.org/2616663005/diff/1/tools/metrics/histograms/hi...
> > tools/metrics/histograms/histograms.xml:43939: +    since the previous boot.
> > Nit: Mention when this is logged. Once on startup? Periodically? Something
> else?
> 
> 0 is always sent on startup, then file existence is checked periodically, and,
> upon existence, a 1 is only sent once per boot.
> 
> I'll update the text depending on the outcome of the discussion below.

Done in patch set 2.

[Alexei Svitkine (slow), 2017-01-10 15:45:47]

asvitkine@chromium.org changed reviewers:
- asvitkine@chromium.org

[Ilya Sherman, 2017-01-18 02:16:30]

LGTM.  Sorry for the delay -- I thought Alexei was reviewing.

[drinkcat1, 2017-01-18 02:22:03]

The CQ bit was checked by drinkcat@chromium.org

[commit-bot: I haz the power, 2017-01-18 02:22:38]

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-01-18 02:30:45]

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

[commit-bot: I haz the power, 2017-01-18 02:30:46]

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

[drinkcat1, 2017-01-18 03:45:36]

On 2017/01/18 02:30:46, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

"histograms.xml is not formatted correctly; run
.../src/tools/metrics/histograms/pretty_print.py to fix"

I'll run that then (I thought the xml file was already badly formatted to start
with, but maybe I'm wrong).

[drinkcat1, 2017-01-18 03:50:55]

On 2017/01/18 03:45:36, drinkcat1 wrote:
> On 2017/01/18 02:30:46, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> "histograms.xml is not formatted correctly; run
> .../src/tools/metrics/histograms/pretty_print.py to fix"
> 
> I'll run that then (I thought the xml file was already badly formatted to
start
> with, but maybe I'm wrong).

Fixed, formatting problem was indeed in my added text.

[drinkcat1, 2017-01-18 03:51:01]

The CQ bit was checked by drinkcat@chromium.org

[drinkcat1, 2017-01-18 03:51:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2616663005/#ps40001
(title: "Add histogram definitions for Chrome OS crouton statistics")

[commit-bot: I haz the power, 2017-01-18 03:51:15]

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-01-18 04:55:01]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484711461498860,
"parent_rev": "93aaf8fc1ac87a2f774a0fdc523c9f04538dd481", "commit_rev":
"f19152f5dc5662089835b10bf7d98c7bcbc42efd"}

[commit-bot: I haz the power, 2017-01-18 04:55:33]

Description was changed from

==========
Add histogram definitions for Chrome OS crouton statistics

This adds enum type definitions and histogram declaration for
crouton metric recorded by the Chrome OS metrics daemon. See also
https://chromium-review.googlesource.com/#/c/425529 .

BUG=chromium:678865
TEST=None
==========

to

==========
Add histogram definitions for Chrome OS crouton statistics

This adds enum type definitions and histogram declaration for
crouton metric recorded by the Chrome OS metrics daemon. See also
https://chromium-review.googlesource.com/#/c/425529 .

BUG=chromium:678865
TEST=None

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

[commit-bot: I haz the power, 2017-01-18 04:55:34]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/f19152f5dc5662089835b10bf7d9...