Let MetricsService know about some Android Activities

Let MetricsService know about some Android Activities

To tune our crash states more finely, we need to track which Activity is in the
foreground when Chrome is killed. Add piping to let MetricsService know when
Activities are swapped in and out by the user.

The CL makes MetricsService track how many times particular Activities have been
launched, as well as how often Chrome was improperly closed while one of these
Activities was in the foreground (i.e. crashed). These numbers are concatenated
into a histogram, which is then sent to the server.

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

[Kibeom Kim (inactive), 2014-01-16 00:20:23]

does this look right approach?

[Kibeom Kim (inactive), 2014-01-21 22:52:15]

asvitkine@ : ping (for just in case you missed)

[Alexei Svitkine (slow), 2014-01-22 15:31:25]

Sorry, I did indeed miss your mail - will take a look later today.

[Alexei Svitkine (slow), 2014-01-22 21:14:18]

Thanks for working on this!

I think this is definitely going in the right direction, though I've left some
comments for you.

https://codereview.chromium.org/137623002/diff/440001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/137623002/diff/440001/base/metrics/histogram....
base/metrics/histogram.h:356: #define UMA_STABILITY_HISTOGRAM_ENUMERATION(name,
sample, boundary_value) \
Please add a comment that explains this macro (and when it should be used).

https://codereview.chromium.org/137623002/diff/440001/base/metrics/histogram....
base/metrics/histogram.h:357: STATIC_HISTOGRAM_POINTER_BLOCK(name, Add(sample),
\
I would maybe make an UMA_HISTOGRAM_ENUMERATION_WITH_FLAGS() macro that both
this and UMA_HISTOGRAM_ENUMERATION() use and specify different flags, to avoid
logic duplication between the two.

https://codereview.chromium.org/137623002/diff/440001/base/metrics/histogram_...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/137623002/diff/440001/base/metrics/histogram_...
base/metrics/histogram_base.h:61: kUmaTargetedStabilityHistogramFlag =0x2,
I think the flag should mention it's an Stability histogram, but should still
set kUmaTargetedHistogramFlag flag. (So stability histograms would set both that
flag and the new one.)

Also, I think these should be included in regular UMA logs as well (not just
initial stability). In some cases, the initial stability log won't be created,
so we don't want to lose these histograms for that.

Also, nit: space after =

https://codereview.chromium.org/137623002/diff/440001/base/metrics/histogram_...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/137623002/diff/440001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager.h:33: // recording, or to set a flag in
each recorded histogram.
Please expand comment to explain |flag_filter_to_record|. Specifically, if you
pass multiple flags in that param, does "any" of them have to be set or do they
"all" have to be set. (In your implementation, it looks like "all").

Also, please add unit tests for this.

https://codereview.chromium.org/137623002/diff/440001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/137623002/diff/440001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1553:
RecordCurrentStabilityHistograms();
The naming of these two functions is confusing. One is logging histograms while
the other is exporting them to the log. Can you pick different names for one or
the other or both, so that it's less confusing?

[Kibeom Kim (inactive), 2014-01-24 19:45:03]

https://codereview.chromium.org/137623002/diff/440001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/137623002/diff/440001/base/metrics/histogram....
base/metrics/histogram.h:356: #define UMA_STABILITY_HISTOGRAM_ENUMERATION(name,
sample, boundary_value) \
On 2014/01/22 21:14:19, Alexei Svitkine wrote:
> Please add a comment that explains this macro (and when it should be used).

Done.

https://codereview.chromium.org/137623002/diff/440001/base/metrics/histogram....
base/metrics/histogram.h:357: STATIC_HISTOGRAM_POINTER_BLOCK(name, Add(sample),
\
On 2014/01/22 21:14:19, Alexei Svitkine wrote:
> I would maybe make an UMA_HISTOGRAM_ENUMERATION_WITH_FLAGS() macro that both
> this and UMA_HISTOGRAM_ENUMERATION() use and specify different flags, to avoid
> logic duplication between the two.

Done.

https://codereview.chromium.org/137623002/diff/440001/base/metrics/histogram_...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/137623002/diff/440001/base/metrics/histogram_...
base/metrics/histogram_base.h:61: kUmaTargetedStabilityHistogramFlag =0x2,
On 2014/01/22 21:14:19, Alexei Svitkine wrote:
> I think the flag should mention it's an Stability histogram, but should still
> set kUmaTargetedHistogramFlag flag. (So stability histograms would set both
that
> flag and the new one.)
> 
> Also, I think these should be included in regular UMA logs as well (not just
> initial stability). In some cases, the initial stability log won't be created,
> so we don't want to lose these histograms for that.
> 
> Also, nit: space after =

Done.

https://codereview.chromium.org/137623002/diff/440001/base/metrics/histogram_...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/137623002/diff/440001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager.h:33: // recording, or to set a flag in
each recorded histogram.
On 2014/01/22 21:14:19, Alexei Svitkine wrote:
> Please expand comment to explain |flag_filter_to_record|. Specifically, if you
> pass multiple flags in that param, does "any" of them have to be set or do
they
> "all" have to be set. (In your implementation, it looks like "all").
> 
> Also, please add unit tests for this.

Done.

https://codereview.chromium.org/137623002/diff/440001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/137623002/diff/440001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1553:
RecordCurrentStabilityHistograms();
On 2014/01/22 21:14:19, Alexei Svitkine wrote:
> The naming of these two functions is confusing. One is logging histograms
while
> the other is exporting them to the log. Can you pick different names for one
or
> the other or both, so that it's less confusing?

Done.

https://codereview.chromium.org/137623002/diff/440001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1782: service &&
service->process_map()->Contains(host->GetID());
these lines changes are just rebase.

https://codereview.chromium.org/137623002/diff/640001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/137623002/diff/640001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1290:
ConvertAndroidStabilityPrefsToHistograms(pref);
dfalcantara@ -- Q : This was in your last patch set, but we're only updating
prefs in MetricsService::InitializeMetricsState(...), so I think this line can
be removed. Could you confirm it? Just to make sure.

[gone, 2014-01-24 20:03:02]

https://codereview.chromium.org/137623002/diff/640001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/137623002/diff/640001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1290:
ConvertAndroidStabilityPrefsToHistograms(pref);
On 2014/01/24 19:45:09, Kibeom Kim wrote:
> dfalcantara@ -- Q : This was in your last patch set, but we're only updating
> prefs in MetricsService::InitializeMetricsState(...), so I think this line can
> be removed. Could you confirm it? Just to make sure.

Yeah, I think it should be fine.  I think you moved it down to where it actually
needs to happen...

[Kibeom Kim (inactive), 2014-01-24 23:21:34]

https://codereview.chromium.org/137623002/diff/640001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/137623002/diff/640001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1290:
ConvertAndroidStabilityPrefsToHistograms(pref);
On 2014/01/24 20:03:07, dfalcantara wrote:
> On 2014/01/24 19:45:09, Kibeom Kim wrote:
> > dfalcantara@ -- Q : This was in your last patch set, but we're only updating
> > prefs in MetricsService::InitializeMetricsState(...), so I think this line
can
> > be removed. Could you confirm it? Just to make sure.
> 
> Yeah, I think it should be fine.  I think you moved it down to where it
actually
> needs to happen...

Done.

[Alexei Svitkine (slow), 2014-01-27 19:12:53]

Looks really good. I still have some comments below, though.

By the way, have you done any sort of manual testing with this to make sure e.g.
the right histograms are included in the initial stability log? If not, ping me
and I can help you do this testing.

https://codereview.chromium.org/137623002/diff/800002/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/137623002/diff/800002/base/metrics/histogram....
base/metrics/histogram.h:350: // details, see the comment for the
|HISTOGRAM_ENUMERATION| macro, above.
This comment should be above UMA_HISTOGRAM_ENUMERATION().

For UMA_HISTOGRAM_ENUMERATION_WITH_FLAGS(), add a comment that it's a helper
used by other macros and shouldn't be used directly.

https://codereview.chromium.org/137623002/diff/800002/base/metrics/histogram_...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/137623002/diff/800002/base/metrics/histogram_...
base/metrics/histogram_base.h:59: kUmaTargetedHistogramFlag = 0x1,  // Histogram
should be UMA uploaded.
Nit: Can you move the comment above this line to be consistent with the other
comments?

https://codereview.chromium.org/137623002/diff/800002/base/metrics/histogram_...
base/metrics/histogram_base.h:61: // Indicate that this is a stability
histogram.
Nit: "Indicate" -> "Indicates". Can you fix the comment below too?

Also, I'd expand this comment to mention some information about why this flag
exists.

https://codereview.chromium.org/137623002/diff/800002/base/metrics/histogram_...
File base/metrics/histogram_snapshot_manager_unittest.cc (right):

https://codereview.chromium.org/137623002/diff/800002/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager_unittest.cc:65:
histogram_flattener_delta_recorder.ClearRecordedData();
Instead of calling ClearRecordedData() after each test case, use separate TEST_F
blocks.

https://codereview.chromium.org/137623002/diff/800002/chrome/browser/android/...
File chrome/browser/android/activity_type_ids.h (right):

https://codereview.chromium.org/137623002/diff/800002/chrome/browser/android/...
chrome/browser/android/activity_type_ids.h:20: }  // namespace ActivityTypeIds
Nit: Add a blank line before this.

https://codereview.chromium.org/137623002/diff/800002/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service_android.cc (right):

https://codereview.chromium.org/137623002/diff/800002/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_android.cc:75:
MetricsService::ConvertAndroidStabilityPrefsToHistograms(PrefService* pref) {
Nit: Wrap after the (, instead of after the void.

https://codereview.chromium.org/137623002/diff/800002/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_android.cc:109: void
MetricsService::OnForegroundActivityChanged(PrefService* pref,
I thought I mentioned this before (on the previous review), but maybe I didn't.

I think this code should have some units tests that simulate
OnForegroundActivityChanged and other events and verify local state changes.

https://codereview.chromium.org/137623002/diff/800002/chrome/common/metrics/m...
File chrome/common/metrics/metrics_service_base.cc (right):

https://codereview.chromium.org/137623002/diff/800002/chrome/common/metrics/m...
chrome/common/metrics/metrics_service_base.cc:37:
static_cast<base::Histogram::Flags>(
This cast is kind of ugly, but I understand why it's there.

I know I'm changing my mind here, but now I think it would be better to have the
stability histogram mean both UMA and Stability, so that this isn't needed.

However I think it can be defined thus:

  kUmaStabilityHistogramFlag = kUmaTargetedHistogramFlag | 0x2,

That way, code checking for just kUmaTargetedHistogramFlag will still include
these (without needing special handling), but this code can just pass in
kUmaTargetedHistogramFlag without ugly casts.

https://codereview.chromium.org/137623002/diff/800002/chrome/common/metrics/m...
File chrome/common/metrics/metrics_service_base.h (right):

https://codereview.chromium.org/137623002/diff/800002/chrome/common/metrics/m...
chrome/common/metrics/metrics_service_base.h:46: // Record complete list of
stability histograms into the current log.
I would expand comment to mention what "stability histgrams" means (i.e. mention
the flag).

[Kibeom Kim (inactive), 2014-01-29 01:52:31]

I manually tried it by force calling
|MetricsService::PrepareInitialStabilityLog| inside
|MetricsService::InitializeMetricsState| by setting the two conditional
statements to true, and printing what histograms are passed to
|HistogramSnapshotManager::PrepareDelta|. And yes, this is too arbitrary :(.
Could you suggest a more correct way of testing? (over chat?)

https://codereview.chromium.org/137623002/diff/800002/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/137623002/diff/800002/base/metrics/histogram....
base/metrics/histogram.h:350: // details, see the comment for the
|HISTOGRAM_ENUMERATION| macro, above.
On 2014/01/27 19:12:54, Alexei Svitkine wrote:
> This comment should be above UMA_HISTOGRAM_ENUMERATION().
> 
> For UMA_HISTOGRAM_ENUMERATION_WITH_FLAGS(), add a comment that it's a helper
> used by other macros and shouldn't be used directly.

Done. (maybe #undef?)

https://codereview.chromium.org/137623002/diff/800002/base/metrics/histogram_...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/137623002/diff/800002/base/metrics/histogram_...
base/metrics/histogram_base.h:59: kUmaTargetedHistogramFlag = 0x1,  // Histogram
should be UMA uploaded.
On 2014/01/27 19:12:54, Alexei Svitkine wrote:
> Nit: Can you move the comment above this line to be consistent with the other
> comments?

Done.

https://codereview.chromium.org/137623002/diff/800002/base/metrics/histogram_...
base/metrics/histogram_base.h:61: // Indicate that this is a stability
histogram.
On 2014/01/27 19:12:54, Alexei Svitkine wrote:
> Nit: "Indicate" -> "Indicates". Can you fix the comment below too?
> 
> Also, I'd expand this comment to mention some information about why this flag
> exists.

Done.

https://codereview.chromium.org/137623002/diff/800002/base/metrics/histogram_...
File base/metrics/histogram_snapshot_manager_unittest.cc (right):

https://codereview.chromium.org/137623002/diff/800002/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager_unittest.cc:65:
histogram_flattener_delta_recorder.ClearRecordedData();
On 2014/01/27 19:12:54, Alexei Svitkine wrote:
> Instead of calling ClearRecordedData() after each test case, use separate
TEST_F
> blocks.

Done.

https://codereview.chromium.org/137623002/diff/800002/chrome/browser/android/...
File chrome/browser/android/activity_type_ids.h (right):

https://codereview.chromium.org/137623002/diff/800002/chrome/browser/android/...
chrome/browser/android/activity_type_ids.h:20: }  // namespace ActivityTypeIds
On 2014/01/27 19:12:54, Alexei Svitkine wrote:
> Nit: Add a blank line before this.

Done.

https://codereview.chromium.org/137623002/diff/800002/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service_android.cc (right):

https://codereview.chromium.org/137623002/diff/800002/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_android.cc:75:
MetricsService::ConvertAndroidStabilityPrefsToHistograms(PrefService* pref) {
On 2014/01/27 19:12:54, Alexei Svitkine wrote:
> Nit: Wrap after the (, instead of after the void.

Done.

https://codereview.chromium.org/137623002/diff/800002/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_android.cc:109: void
MetricsService::OnForegroundActivityChanged(PrefService* pref,
On 2014/01/27 19:12:54, Alexei Svitkine wrote:
> I thought I mentioned this before (on the previous review), but maybe I
didn't.
> 
> I think this code should have some units tests that simulate
> OnForegroundActivityChanged and other events and verify local state changes.

what do you mean by other events?? (btw,
https://chrome-internal-review.googlesource.com/#/c/149216/ will call this
function )

https://codereview.chromium.org/137623002/diff/800002/chrome/common/metrics/m...
File chrome/common/metrics/metrics_service_base.cc (right):

https://codereview.chromium.org/137623002/diff/800002/chrome/common/metrics/m...
chrome/common/metrics/metrics_service_base.cc:37:
static_cast<base::Histogram::Flags>(
On 2014/01/27 19:12:54, Alexei Svitkine wrote:
> This cast is kind of ugly, but I understand why it's there.
> 
> I know I'm changing my mind here, but now I think it would be better to have
the
> stability histogram mean both UMA and Stability, so that this isn't needed.
> 
> However I think it can be defined thus:
> 
>   kUmaStabilityHistogramFlag = kUmaTargetedHistogramFlag | 0x2,
> 
> That way, code checking for just kUmaTargetedHistogramFlag will still include
> these (without needing special handling), but this code can just pass in
> kUmaTargetedHistogramFlag without ugly casts.

Done. (nice suggestion!)

https://codereview.chromium.org/137623002/diff/800002/chrome/common/metrics/m...
File chrome/common/metrics/metrics_service_base.h (right):

https://codereview.chromium.org/137623002/diff/800002/chrome/common/metrics/m...
chrome/common/metrics/metrics_service_base.h:46: // Record complete list of
stability histograms into the current log.
On 2014/01/27 19:12:54, Alexei Svitkine wrote:
> I would expand comment to mention what "stability histgrams" means (i.e.
mention
> the flag).

Done.

[Alexei Svitkine (slow), 2014-01-29 16:54:09]

Looks really good, almost ready to land. Just a few more style comments below.

https://codereview.chromium.org/137623002/diff/990001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/137623002/diff/990001/base/metrics/histogram....
base/metrics/histogram.h:350: #define UMA_HISTOGRAM_ENUMERATION_WITH_FLAG(name,
sample, boundary_value, flag) \
Nit: Seems this goes over the 80 char limit. Maybe rename boundary_value to
boundary to make it fit?

https://codereview.chromium.org/137623002/diff/990001/base/metrics/histogram_...
File base/metrics/histogram_snapshot_manager_unittest.cc (right):

https://codereview.chromium.org/137623002/diff/990001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager_unittest.cc:61:
std::vector<std::string>& histograms =
Non-const refs are generally not allowed. Either make it a const ref or a
pointer. Same in the tests below.

https://codereview.chromium.org/137623002/diff/990001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager_unittest.cc:63: EXPECT_EQ((unsigned)2,
histograms.size());
(unsigned)2 -> 2U

Same below.

https://codereview.chromium.org/137623002/diff/990001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service_android.cc (right):

https://codereview.chromium.org/137623002/diff/990001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_android.cc:28: CHECK(type <
ActivityTypeIds::ACTIVITY_MAX_VALUE);
Can this be a CHECK_LT?

[Kibeom Kim (inactive), 2014-01-29 20:16:07]

Adding other OWNERS (This CL is from https://codereview.chromium.org/103943006/
)

asvitkine:
  chrome/common/metrics/*
  tools/metrics/histograms/histograms.xml
isherman:
  base/metrics/*
  chrome/browser/metrics/*
yfriedman:
  chrome/android/java/ActivityTypeIds.template
  chrome/browser/android/*
thakis:
  base/base.gyp
  chrome/chrome.gyp

https://codereview.chromium.org/137623002/diff/990001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/137623002/diff/990001/base/metrics/histogram....
base/metrics/histogram.h:350: #define UMA_HISTOGRAM_ENUMERATION_WITH_FLAG(name,
sample, boundary_value, flag) \
On 2014/01/29 16:54:09, Alexei Svitkine wrote:
> Nit: Seems this goes over the 80 char limit. Maybe rename boundary_value to
> boundary to make it fit?

Done.

https://codereview.chromium.org/137623002/diff/990001/base/metrics/histogram_...
File base/metrics/histogram_snapshot_manager_unittest.cc (right):

https://codereview.chromium.org/137623002/diff/990001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager_unittest.cc:61:
std::vector<std::string>& histograms =
On 2014/01/29 16:54:09, Alexei Svitkine wrote:
> Non-const refs are generally not allowed. Either make it a const ref or a
> pointer. Same in the tests below.

Done.

https://codereview.chromium.org/137623002/diff/990001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager_unittest.cc:63: EXPECT_EQ((unsigned)2,
histograms.size());
On 2014/01/29 16:54:09, Alexei Svitkine wrote:
> (unsigned)2 -> 2U
> 
> Same below.

Done.

https://codereview.chromium.org/137623002/diff/990001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service_android.cc (right):

https://codereview.chromium.org/137623002/diff/990001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service_android.cc:28: CHECK(type <
ActivityTypeIds::ACTIVITY_MAX_VALUE);
On 2014/01/29 16:54:09, Alexei Svitkine wrote:
> Can this be a CHECK_LT?

Done.

[Alexei Svitkine (slow), 2014-01-29 20:20:21]

LGTM, thanks!

https://codereview.chromium.org/137623002/diff/1010001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/137623002/diff/1010001/base/metrics/histogram...
base/metrics/histogram.h:353: boundary + 1, flag))
Nit: boundary + 1 can fit on the previous line. Align flag.

https://codereview.chromium.org/137623002/diff/1010001/chrome/browser/android...
File chrome/browser/android/activity_type_ids.cc (right):

https://codereview.chromium.org/137623002/diff/1010001/chrome/browser/android...
chrome/browser/android/activity_type_ids.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
Nit: It's 2014 (throughout).

[Kibeom Kim (inactive), 2014-01-29 20:25:15]

https://codereview.chromium.org/137623002/diff/1010001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/137623002/diff/1010001/base/metrics/histogram...
base/metrics/histogram.h:353: boundary + 1, flag))
On 2014/01/29 20:20:21, Alexei Svitkine wrote:
> Nit: boundary + 1 can fit on the previous line. Align flag.

Done.

https://codereview.chromium.org/137623002/diff/1010001/chrome/browser/android...
File chrome/browser/android/activity_type_ids.cc (right):

https://codereview.chromium.org/137623002/diff/1010001/chrome/browser/android...
chrome/browser/android/activity_type_ids.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2014/01/29 20:20:21, Alexei Svitkine wrote:
> Nit: It's 2014 (throughout).

Done.

[Yaron, 2014-01-29 20:43:25]

android bits lgtm

[Nico, 2014-01-29 22:30:24]

gyp changes lgtm

[Kibeom Kim (inactive), 2014-02-05 18:49:04]

isherman@ : ping?

[Ilya Sherman, 2014-02-07 02:55:08]

On 2014/02/05 18:49:04, Kibeom Kim wrote:
> isherman@ : ping?

Sorry, still catching up from vacation backlog.  Will try to get to this by EOD
tomorrow.

[Ilya Sherman, 2014-02-08 00:35:23]

LGTM, thanks :)

[Ilya Sherman, 2014-02-08 00:36:03]

On 2014/02/08 00:35:23, Ilya Sherman (catching up...) wrote:
> LGTM, thanks :)

Wait, sorry, that got sent on the wrong CL.  Not LGTM yet, but I'm in the midst
of reviewing right now.  Hopefully will re-approve momentarily :)

[Ilya Sherman, 2014-02-08 00:59:48]

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram.h:350: #define UMA_HISTOGRAM_ENUMERATION_WITH_FLAG(name,
sample, boundary, flag) \
Please drop the UMA_ prefix from this name.  UMA_ macros are for histograms that
upload to the UMA servers.

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram.h:363: // should be a part of the initial stability log.
nit: Minor suggested phrasing tweaks:

"Similar to UMA_HISTOGRAM_ENUMERATION, but used for recording stability
histograms.  Use this if recording a histogram that should be part of the
initial stability log."

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_base.h:66: kUmaStabilityHistogramFlag =
kUmaTargetedHistogramFlag | 0x2,
It's unfortunate to need to leak this detail into the base layer.  Can we avoid
doing that, perhaps by having a whitelist of which histograms are
stability-targeted?

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:35: // chosen. If all histograms
should to be recorded, set it to
nit: "should to be" -> "should be"

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:36: // Histogram::kNoFlags .
nit: Spurious space before the period.

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:38: HistogramBase::Flags
flag_filter_to_record);
nit: "flag_filter_to_record" is pretty hard to make sense of.  Perhaps something
more like "flags_required_for_recording", or maybe even something shorter like
"required_flags"?

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager_unittest.cc (right):

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager_unittest.cc:38: std::vector<std::string>
recorded_delta_histogram_names_;
nit: Please set this to have private visibility, and provide a (read-only?)
accessor.

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager_unittest.cc:39: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager_unittest.cc:97: }  // namespace base
Thanks for adding test coverage :)

https://codereview.chromium.org/137623002/diff/1040001/chrome/browser/metrics...
File chrome/browser/metrics/metrics_service_android.cc (right):

https://codereview.chromium.org/137623002/diff/1040001/chrome/browser/metrics...
chrome/browser/metrics/metrics_service_android.cc:28: CHECK_LT(type,
ActivityTypeIds::ACTIVITY_MAX_VALUE);
nit: Why is this a CHECK rather than a DCHECK?

https://codereview.chromium.org/137623002/diff/1040001/chrome/common/metrics/...
File chrome/common/metrics/metrics_service_base.h (right):

https://codereview.chromium.org/137623002/diff/1040001/chrome/common/metrics/...
chrome/common/metrics/metrics_service_base.h:47: // i.e., histograms with
|kUmaStabilityHistogramFlag| flag,
nit: "histograms with |kUmaStabilityHistogramFlag| flag," -> "histograms with
the |kUmaStabilityHistogramFlag| flag set."

https://codereview.chromium.org/137623002/diff/1040001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/137623002/diff/1040001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:1438: +    foreground when a UMA session
was terminated abnormally.
What is an "UMA session"?  Do you mean an instance of Chrome for which UMA is
enabled?  A 5-minute window during which UMA stats are gathered?  Something
else?

https://codereview.chromium.org/137623002/diff/1040001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:21994: +  <int value="1"
label="Unknown"/>
Are you intentionally omitting bucket 0?  If so, why?

[gone, 2014-02-08 01:16:32]

https://codereview.chromium.org/137623002/diff/1040001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/137623002/diff/1040001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:1438: +    foreground when a UMA session
was terminated abnormally.
On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> What is an "UMA session"?  Do you mean an instance of Chrome for which UMA is
> enabled?  A 5-minute window during which UMA stats are gathered?  Something
> else?

Goes with https://chrome-internal-review.googlesource.com/#/c/149216/

"Moves UmaSessionStats back to ChromeMobileApplication, redefining UMA sessions
to last as long as Chrome remains in the foreground."

Is there a more logical place to put the definition of a UMA session in Chromium
code, given that they're started and stopped from Clank code?

https://codereview.chromium.org/137623002/diff/1040001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:21994: +  <int value="1"
label="Unknown"/>
On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> Are you intentionally omitting bucket 0?  If so, why?

Goes with the list at
https://codereview.chromium.org/137623002/diff/1040001/chrome/browser/android...

0 corresponds to a lack of Activity and is never expected to be sent back.  IIRC
we were asked to omit it.

[Ilya Sherman, 2014-02-08 01:18:44]

https://codereview.chromium.org/137623002/diff/1040001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/137623002/diff/1040001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:1438: +    foreground when a UMA session
was terminated abnormally.
On 2014/02/08 01:16:32, dfalcantara wrote:
> On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> > What is an "UMA session"?  Do you mean an instance of Chrome for which UMA
is
> > enabled?  A 5-minute window during which UMA stats are gathered?  Something
> > else?
> 
> Goes with https://chrome-internal-review.googlesource.com/#/c/149216/
> 
> "Moves UmaSessionStats back to ChromeMobileApplication, redefining UMA
sessions
> to last as long as Chrome remains in the foreground."
> 
> Is there a more logical place to put the definition of a UMA session in
Chromium
> code, given that they're started and stopped from Clank code?

I think it would be good to either document inline, or link to somewhere where
this is documented.  Stability stats are important, so it's important that the
documentation be especially clear and easy to understand for folks who might not
be very familiar with other UMA stats.

https://codereview.chromium.org/137623002/diff/1040001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:21994: +  <int value="1"
label="Unknown"/>
On 2014/02/08 01:16:32, dfalcantara wrote:
> On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> > Are you intentionally omitting bucket 0?  If so, why?
> 
> Goes with the list at
>
https://codereview.chromium.org/137623002/diff/1040001/chrome/browser/android...
> 
> 0 corresponds to a lack of Activity and is never expected to be sent back. 
IIRC
> we were asked to omit it.

Makes sense.  Thanks :)

[Kibeom Kim (inactive), 2014-02-10 22:24:44]

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram.h:350: #define UMA_HISTOGRAM_ENUMERATION_WITH_FLAG(name,
sample, boundary, flag) \
On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> Please drop the UMA_ prefix from this name.  UMA_ macros are for histograms
that
> upload to the UMA servers.

Done.

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram.h:363: // should be a part of the initial stability log.
On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> nit: Minor suggested phrasing tweaks:
> 
> "Similar to UMA_HISTOGRAM_ENUMERATION, but used for recording stability
> histograms.  Use this if recording a histogram that should be part of the
> initial stability log."

Done.

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_base.h:66: kUmaStabilityHistogramFlag =
kUmaTargetedHistogramFlag | 0x2,
On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> It's unfortunate to need to leak this detail into the base layer.  Can we
avoid
> doing that, perhaps by having a whitelist of which histograms are
> stability-targeted?

asvitkine@ : Do you have any suggestion? Maybe we can have another
HistogramSnapshotManager::PrepareDeltas(...) function that takes the histogram
names instead of |required_flags|?

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:35: // chosen. If all histograms
should to be recorded, set it to
On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> nit: "should to be" -> "should be"

Done.

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:36: // Histogram::kNoFlags .
On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> nit: Spurious space before the period.

Done.

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:38: HistogramBase::Flags
flag_filter_to_record);
On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> nit: "flag_filter_to_record" is pretty hard to make sense of.  Perhaps
something
> more like "flags_required_for_recording", or maybe even something shorter like
> "required_flags"?

Done. (thanks for the better naming!)

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager_unittest.cc (right):

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager_unittest.cc:38: std::vector<std::string>
recorded_delta_histogram_names_;
On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> nit: Please set this to have private visibility, and provide a (read-only?)
> accessor.

Done.

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager_unittest.cc:39: };
On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/137623002/diff/1040001/chrome/browser/metrics...
File chrome/browser/metrics/metrics_service_android.cc (right):

https://codereview.chromium.org/137623002/diff/1040001/chrome/browser/metrics...
chrome/browser/metrics/metrics_service_android.cc:28: CHECK_LT(type,
ActivityTypeIds::ACTIVITY_MAX_VALUE);
On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> nit: Why is this a CHECK rather than a DCHECK?

Done.

https://codereview.chromium.org/137623002/diff/1040001/chrome/common/metrics/...
File chrome/common/metrics/metrics_service_base.h (right):

https://codereview.chromium.org/137623002/diff/1040001/chrome/common/metrics/...
chrome/common/metrics/metrics_service_base.h:47: // i.e., histograms with
|kUmaStabilityHistogramFlag| flag,
On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> nit: "histograms with |kUmaStabilityHistogramFlag| flag," -> "histograms with
> the |kUmaStabilityHistogramFlag| flag set."

Done.

https://codereview.chromium.org/137623002/diff/1040001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/137623002/diff/1040001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:1438: +    foreground when a UMA session
was terminated abnormally.
On 2014/02/08 01:18:44, Ilya Sherman (catching up...) wrote:
> On 2014/02/08 01:16:32, dfalcantara wrote:
> > On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> > > What is an "UMA session"?  Do you mean an instance of Chrome for which UMA
> is
> > > enabled?  A 5-minute window during which UMA stats are gathered? 
Something
> > > else?
> > 
> > Goes with https://chrome-internal-review.googlesource.com/#/c/149216/
> > 
> > "Moves UmaSessionStats back to ChromeMobileApplication, redefining UMA
> sessions
> > to last as long as Chrome remains in the foreground."
> > 
> > Is there a more logical place to put the definition of a UMA session in
> Chromium
> > code, given that they're started and stopped from Clank code?
> 
> I think it would be good to either document inline, or link to somewhere where
> this is documented.  Stability stats are important, so it's important that the
> documentation be especially clear and easy to understand for folks who might
not
> be very familiar with other UMA stats.

Done.

[Alexei Svitkine (slow), 2014-02-10 22:48:22]

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_base.h:66: kUmaStabilityHistogramFlag =
kUmaTargetedHistogramFlag | 0x2,
On 2014/02/10 22:24:45, Kibeom Kim wrote:
> On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> > It's unfortunate to need to leak this detail into the base layer.  Can we
> avoid
> > doing that, perhaps by having a whitelist of which histograms are
> > stability-targeted?
> 
> asvitkine@ : Do you have any suggestion? Maybe we can have another
> HistogramSnapshotManager::PrepareDeltas(...) function that takes the histogram
> names instead of |required_flags|?

Adding another function that takes an array of histogram names makes sense to me
(though it should have a different name since overloading is discouraged).

(But I was fine with the previous approach too since the current Flags are
already pretty bad in terms of having base know about things outside of base...)

[Ilya Sherman, 2014-02-11 07:47:26]

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/137623002/diff/1040001/base/metrics/histogram...
base/metrics/histogram_base.h:66: kUmaStabilityHistogramFlag =
kUmaTargetedHistogramFlag | 0x2,
On 2014/02/10 22:48:23, Alexei Svitkine wrote:
> On 2014/02/10 22:24:45, Kibeom Kim wrote:
> > On 2014/02/08 00:59:49, Ilya Sherman (catching up...) wrote:
> > > It's unfortunate to need to leak this detail into the base layer.  Can we
> > avoid
> > > doing that, perhaps by having a whitelist of which histograms are
> > > stability-targeted?
> > 
> > asvitkine@ : Do you have any suggestion? Maybe we can have another
> > HistogramSnapshotManager::PrepareDeltas(...) function that takes the
histogram
> > names instead of |required_flags|?
> 
> Adding another function that takes an array of histogram names makes sense to
me
> (though it should have a different name since overloading is discouraged).
> 
> (But I was fine with the previous approach too since the current Flags are
> already pretty bad in terms of having base know about things outside of
base...)

Yeah, the flags are already pretty leaky, and are certainly more efficient than
comparing names.  It might be that implementing an efficient solution that
avoids leaking this information is too much work for the payoff that it gives. 
I'm not sure -- I don't have any great ideas right now on how to smoothly
improve on this design, which means that maybe we should keep this approach for
now.

That said, I would probably at least drop the last sentence, which references
the MetricsService -- that's likely to grow stale.

https://codereview.chromium.org/137623002/diff/1250001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/137623002/diff/1250001/base/metrics/histogram...
base/metrics/histogram.h:353: flag))
nit: Please sort this with the other HISTOGRAM (as opposed to UMA_HISTOGRAM)
macros.

[Ilya Sherman, 2014-02-11 07:47:50]

(Also, LGTM with those remaining comments addressed.)

[Kibeom Kim (inactive), 2014-02-11 08:05:34]

https://codereview.chromium.org/137623002/diff/1250001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/137623002/diff/1250001/base/metrics/histogram...
base/metrics/histogram.h:353: flag))
On 2014/02/11 07:47:27, Ilya Sherman (catching up...) wrote:
> nit: Please sort this with the other HISTOGRAM (as opposed to UMA_HISTOGRAM)
> macros.

Done.

[Kibeom Kim (inactive), 2014-02-11 09:11:21]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2014-02-11 09:11:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/137623002/1590001

[commit-bot: I haz the power, 2014-02-11 10:14:21]

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

[commit-bot: I haz the power, 2014-02-11 10:14:23]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
compositor_unittests, content_browsertests, content_unittests, events_unittests,
interactive_ui_tests, ipc_tests, mini_installer_test, nacl_integration,
net_unittests, sql_unittests, sync_integration_tests, sync_unit_tests,
telemetry_perf_unittests, telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Kibeom Kim (inactive), 2014-02-11 10:20:41]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2014-02-11 10:20:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/137623002/1590001

[commit-bot: I haz the power, 2014-02-11 14:46:48]

Change committed as 250410

[Alexei Svitkine (slow), 2014-05-21 08:21:13]

https://codereview.chromium.org/137623002/diff/1590001/chrome/browser/metrics...
File chrome/browser/metrics/metrics_service_android.cc (right):

https://codereview.chromium.org/137623002/diff/1590001/chrome/browser/metrics...
chrome/browser/metrics/metrics_service_android.cc:110: ActivityTypeIds::Type
type) {
Can you point me at the code from the Android side that actually calls this?

I couldn't find any callers of this - for example it doesn't seem to be called
from uma_session_stats.cc.

(We're in the middle of refactoring MetricsService now to make it a component
and I'm hoping to not break this code in the process.)

[gone, 2014-05-21 16:45:55]

On 2014/05/21 08:21:13, Alexei Svitkine wrote:
>
https://codereview.chromium.org/137623002/diff/1590001/chrome/browser/metrics...
> File chrome/browser/metrics/metrics_service_android.cc (right):
> 
>
https://codereview.chromium.org/137623002/diff/1590001/chrome/browser/metrics...
> chrome/browser/metrics/metrics_service_android.cc:110: ActivityTypeIds::Type
> type) {
> Can you point me at the code from the Android side that actually calls this?
> 
> I couldn't find any callers of this - for example it doesn't seem to be called
> from uma_session_stats.cc.
> 
> (We're in the middle of refactoring MetricsService now to make it a component
> and I'm hoping to not break this code in the process.)

Got pulled away from landing the Android-side because the Chromium-side took so
long to land.  The CL that would have gone in is
https://chrome-internal-review.googlesource.com/#/c/149216/, but with the
Samsung multi-window mode changes it has to be rewritten. 

If you break this code, I'll fix it when I find the opportunity to work on the
Android side of things again.