Add a mechanism for collecting Rappor samples on a daily interval.

Add a mechanism for collecting Rappor samples on a daily interval.

BUG=400927
Committed: https://crrev.com/07637b015c74b180c1163f38c1458ec41647644e
Cr-Commit-Position: refs/heads/master@{#296025}

[Steven Holte, 2014-08-26 22:18:27]

holte@chromium.org changed reviewers:
+ jwd@chromium.org

[jwd, 2014-08-29 15:24:16]

https://codereview.chromium.org/511623002/diff/1/components/metrics/daily_int...
File components/metrics/daily_interval.cc (right):

https://codereview.chromium.org/511623002/diff/1/components/metrics/daily_int...
components/metrics/daily_interval.cc:53: DVLOG(1) << "DailyInterval clock change
detected.";
It might be nice to log an uma metric here, to see how often this happens. I'm
not sure if I like the idea of one metric for all DailyInterval instances, or
going through the trouble of suffixing it by pref_name. 

It's an optional change.

https://codereview.chromium.org/511623002/diff/1/components/metrics/daily_int...
File components/metrics/daily_interval.h (right):

https://codereview.chromium.org/511623002/diff/1/components/metrics/daily_int...
components/metrics/daily_interval.h:18: // DailyInterval is used for firing an
event about once per day, even if chrome
Hmm, this implies to me that DailyInterval is causing the event to fire, whereas
it's preventing the event when the interval hasn't elapsed.

https://codereview.chromium.org/511623002/diff/1/components/metrics/daily_int...
components/metrics/daily_interval.h:19: // is restarted more or less frequently.
Maybe mention it's based on local machine time, and is subject to skew and
changing.

https://codereview.chromium.org/511623002/diff/1/components/metrics/daily_int...
components/metrics/daily_interval.h:25: // DailyInterval.
Mention that RegisterPref had to be called with the same pref_name.

https://codereview.chromium.org/511623002/diff/1/components/metrics/daily_int...
components/metrics/daily_interval.h:36: // Check if a day has elapsed.
Mention the event is called when the day has elapsed.

[Steven Holte, 2014-09-03 19:33:45]

https://codereview.chromium.org/511623002/diff/1/components/metrics/daily_int...
File components/metrics/daily_interval.cc (right):

https://codereview.chromium.org/511623002/diff/1/components/metrics/daily_int...
components/metrics/daily_interval.cc:53: DVLOG(1) << "DailyInterval clock change
detected.";
On 2014/08/29 15:24:15, Jesse Doherty wrote:
> It might be nice to log an uma metric here, to see how often this happens. I'm
> not sure if I like the idea of one metric for all DailyInterval instances, or
> going through the trouble of suffixing it by pref_name. 
> 
> It's an optional change.

Done.

https://codereview.chromium.org/511623002/diff/1/components/metrics/daily_int...
File components/metrics/daily_interval.h (right):

https://codereview.chromium.org/511623002/diff/1/components/metrics/daily_int...
components/metrics/daily_interval.h:18: // DailyInterval is used for firing an
event about once per day, even if chrome
On 2014/08/29 15:24:16, Jesse Doherty wrote:
> Hmm, this implies to me that DailyInterval is causing the event to fire,
whereas
> it's preventing the event when the interval hasn't elapsed.

Done.

https://codereview.chromium.org/511623002/diff/1/components/metrics/daily_int...
components/metrics/daily_interval.h:19: // is restarted more or less frequently.
On 2014/08/29 15:24:16, Jesse Doherty wrote:
> Maybe mention it's based on local machine time, and is subject to skew and
> changing.

Done.

https://codereview.chromium.org/511623002/diff/1/components/metrics/daily_int...
components/metrics/daily_interval.h:25: // DailyInterval.
On 2014/08/29 15:24:15, Jesse Doherty wrote:
> Mention that RegisterPref had to be called with the same pref_name.

Done.

https://codereview.chromium.org/511623002/diff/1/components/metrics/daily_int...
components/metrics/daily_interval.h:36: // Check if a day has elapsed.
On 2014/08/29 15:24:15, Jesse Doherty wrote:
> Mention the event is called when the day has elapsed.

Done.

[jwd, 2014-09-05 19:54:03]

lgtm

[Alexei Svitkine (slow), 2014-09-05 20:00:32]

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

[Alexei Svitkine (slow), 2014-09-05 20:00:33]

Can you associate a crbug since this is a non-trivial change?

https://codereview.chromium.org/511623002/diff/20001/components/metrics/daily...
File components/metrics/daily_interval.cc (right):

https://codereview.chromium.org/511623002/diff/20001/components/metrics/daily...
components/metrics/daily_interval.cc:88: DailyObserver, observers_,
OnDailyInterval());
Nit: Does this need to wrap?

https://codereview.chromium.org/511623002/diff/20001/components/metrics/daily...
File components/metrics/daily_interval.h (right):

https://codereview.chromium.org/511623002/diff/20001/components/metrics/daily...
components/metrics/daily_interval.h:22: public:
Nit: Indent 1 more, here and private below.

https://codereview.chromium.org/511623002/diff/20001/components/metrics/daily...
File components/metrics/daily_observer.h (right):

https://codereview.chromium.org/511623002/diff/20001/components/metrics/daily...
components/metrics/daily_observer.h:15: public:
Nit: Indent 1 more, same below.

[Steven Holte, 2014-09-05 21:15:08]

https://codereview.chromium.org/511623002/diff/20001/components/metrics/daily...
File components/metrics/daily_interval.cc (right):

https://codereview.chromium.org/511623002/diff/20001/components/metrics/daily...
components/metrics/daily_interval.cc:88: DailyObserver, observers_,
OnDailyInterval());
On 2014/09/05 20:00:33, Alexei Svitkine wrote:
> Nit: Does this need to wrap?

Done.

https://codereview.chromium.org/511623002/diff/20001/components/metrics/daily...
File components/metrics/daily_interval.h (right):

https://codereview.chromium.org/511623002/diff/20001/components/metrics/daily...
components/metrics/daily_interval.h:22: public:
On 2014/09/05 20:00:33, Alexei Svitkine wrote:
> Nit: Indent 1 more, here and private below.

Done.

https://codereview.chromium.org/511623002/diff/20001/components/metrics/daily...
File components/metrics/daily_observer.h (right):

https://codereview.chromium.org/511623002/diff/20001/components/metrics/daily...
components/metrics/daily_observer.h:15: public:
On 2014/09/05 20:00:33, Alexei Svitkine wrote:
> Nit: Indent 1 more, same below.

Done.

[Steven Holte, 2014-09-05 22:16:03]

holte@chromium.org changed reviewers:
+ jam@chromium.org

[Steven Holte, 2014-09-05 22:16:04]

+jam for chrome_browser_main.cc

[Alexei Svitkine (slow), 2014-09-08 15:34:21]

Looks good generally, just some minor style comments.

https://codereview.chromium.org/511623002/diff/60001/components/metrics/daily...
File components/metrics/daily_interval.cc (right):

https://codereview.chromium.org/511623002/diff/60001/components/metrics/daily...
components/metrics/daily_interval.cc:23: void RecordIntervalTypeHistogram(const
char* pref_name, IntervalType t) {
Nit: pass |pref_name| by const std::string& (just to this function) and then in
the code below, you can just do:

base::Histogram::FactoryGet(
    "DailyInterval.IntervalType." + pref_name,

https://codereview.chromium.org/511623002/diff/60001/components/metrics/daily...
components/metrics/daily_interval.cc:71: if (days_elapsed > 1) {
Shouldn't this be >=? otherwise it's a 2-day interval, no?

(Or add a comment if it really should be >1 so that readers of the code like
myself aren't confused.)

If it should be >=, add a test to exercise this.

https://codereview.chromium.org/511623002/diff/60001/components/metrics/daily...
File components/metrics/daily_interval.h (right):

https://codereview.chromium.org/511623002/diff/60001/components/metrics/daily...
components/metrics/daily_interval.h:33: static void
RegisterPref(PrefRegistrySimple* registry, const char* pref_name);
Nit: I think in other metrics files we order static methods at the end, before
the private section.

https://codereview.chromium.org/511623002/diff/60001/components/metrics/daily...
File components/metrics/daily_interval_unittest.cc (right):

https://codereview.chromium.org/511623002/diff/60001/components/metrics/daily...
components/metrics/daily_interval_unittest.cc:25: };
Nit: Unnecessary semicolon. Also, below.

https://codereview.chromium.org/511623002/diff/60001/components/rappor/rappor...
File components/rappor/rappor_service.h (right):

https://codereview.chromium.org/511623002/diff/60001/components/rappor/rappor...
components/rappor/rappor_service.h:45: RapporService(PrefService* pref_service);
Add explicit.

https://codereview.chromium.org/511623002/diff/60001/components/rappor/rappor...
components/rappor/rappor_service.h:49: void
AddDailyMetric(metrics::DailyObserver* observer);
Method name and its comment don't appear to be consistent. Method name implies
that this is adding a metric, but comment talks about an observer.

Either make the comment more clear w.r.t. to the metric name or vice versa.
Also, document ownership of the param and mention why it's ok to add without
having a remove method.

https://codereview.chromium.org/511623002/diff/60001/components/rappor/rappor...
components/rappor/rappor_service.h:73: // doesn't already exist.  The cohort
should be persistent.
Nit: I think we're using a single space between sentences in this code. Same for
the comment below.

https://codereview.chromium.org/511623002/diff/60001/components/rappor/rappor...
components/rappor/rappor_service.h:108: // lifetime of the DailyInterval object.
Nit: Since this is documenting a private member, I don't think the second
sentence is necessary since you already mention this in the ctor comment. I'd
also tweak the first sentence so it fits in a single line.

https://codereview.chromium.org/511623002/diff/60001/components/rappor/rappor...
components/rappor/rappor_service.h:120: // A daily interval tracker.
I'd expand this comment to mention the purpose (i.e. mentioning that it's for
metrics that should be recorded once a day.)

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

https://codereview.chromium.org/511623002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:3353: +<histogram
name="DailyInterval.IntervalType" enum="DailyIntervalIntervalType">
I'd use the "Rappor." prefix for this histogram so that we're not introducing a
new top-level name.

[jam, 2014-09-12 04:05:54]

On 2014/09/05 22:16:04, Steven Holte wrote:
> +jam for chrome_browser_main.cc

lgtm

[Steven Holte, 2014-09-12 21:23:32]

https://codereview.chromium.org/511623002/diff/60001/components/metrics/daily...
File components/metrics/daily_interval.cc (right):

https://codereview.chromium.org/511623002/diff/60001/components/metrics/daily...
components/metrics/daily_interval.cc:23: void RecordIntervalTypeHistogram(const
char* pref_name, IntervalType t) {
On 2014/09/08 15:34:20, Alexei Svitkine wrote:
> Nit: pass |pref_name| by const std::string& (just to this function) and then
in
> the code below, you can just do:
> 
> base::Histogram::FactoryGet(
>     "DailyInterval.IntervalType." + pref_name,

Changed this to a parameter to the DailyInterval constructor, to allow more
sensible metric names.

https://codereview.chromium.org/511623002/diff/60001/components/metrics/daily...
components/metrics/daily_interval.cc:71: if (days_elapsed > 1) {
On 2014/09/08 15:34:20, Alexei Svitkine wrote:
> Shouldn't this be >=? otherwise it's a 2-day interval, no?
> 
> (Or add a comment if it really should be >1 so that readers of the code like
> myself aren't confused.)
> 
> If it should be >=, add a test to exercise this.

Done.  Also editted test to be more precise.

https://codereview.chromium.org/511623002/diff/60001/components/metrics/daily...
File components/metrics/daily_interval.h (right):

https://codereview.chromium.org/511623002/diff/60001/components/metrics/daily...
components/metrics/daily_interval.h:33: static void
RegisterPref(PrefRegistrySimple* registry, const char* pref_name);
On 2014/09/08 15:34:20, Alexei Svitkine wrote:
> Nit: I think in other metrics files we order static methods at the end, before
> the private section.

Done.

https://codereview.chromium.org/511623002/diff/60001/components/metrics/daily...
File components/metrics/daily_interval_unittest.cc (right):

https://codereview.chromium.org/511623002/diff/60001/components/metrics/daily...
components/metrics/daily_interval_unittest.cc:25: };
On 2014/09/08 15:34:20, Alexei Svitkine wrote:
> Nit: Unnecessary semicolon. Also, below.

Done.

https://codereview.chromium.org/511623002/diff/60001/components/rappor/rappor...
File components/rappor/rappor_service.h (right):

https://codereview.chromium.org/511623002/diff/60001/components/rappor/rappor...
components/rappor/rappor_service.h:45: RapporService(PrefService* pref_service);
On 2014/09/08 15:34:21, Alexei Svitkine wrote:
> Add explicit.

Done.

https://codereview.chromium.org/511623002/diff/60001/components/rappor/rappor...
components/rappor/rappor_service.h:49: void
AddDailyMetric(metrics::DailyObserver* observer);
On 2014/09/08 15:34:21, Alexei Svitkine wrote:
> Method name and its comment don't appear to be consistent. Method name implies
> that this is adding a metric, but comment talks about an observer.
> 
> Either make the comment more clear w.r.t. to the metric name or vice versa.
> Also, document ownership of the param and mention why it's ok to add without
> having a remove method.

Changed the ownership model so that RapporService/DailyInterval just owns all of
it's observers.

https://codereview.chromium.org/511623002/diff/60001/components/rappor/rappor...
components/rappor/rappor_service.h:73: // doesn't already exist.  The cohort
should be persistent.
On 2014/09/08 15:34:21, Alexei Svitkine wrote:
> Nit: I think we're using a single space between sentences in this code. Same
for
> the comment below.

Done.

https://codereview.chromium.org/511623002/diff/60001/components/rappor/rappor...
components/rappor/rappor_service.h:108: // lifetime of the DailyInterval object.
On 2014/09/08 15:34:20, Alexei Svitkine wrote:
> Nit: Since this is documenting a private member, I don't think the second
> sentence is necessary since you already mention this in the ctor comment. I'd
> also tweak the first sentence so it fits in a single line.

Done.

https://codereview.chromium.org/511623002/diff/60001/components/rappor/rappor...
components/rappor/rappor_service.h:120: // A daily interval tracker.
On 2014/09/08 15:34:20, Alexei Svitkine wrote:
> I'd expand this comment to mention the purpose (i.e. mentioning that it's for
> metrics that should be recorded once a day.)

Done.

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

https://codereview.chromium.org/511623002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:3353: +<histogram
name="DailyInterval.IntervalType" enum="DailyIntervalIntervalType">
On 2014/09/08 15:34:21, Alexei Svitkine wrote:
> I'd use the "Rappor." prefix for this histogram so that we're not introducing
a
> new top-level name.

Done.

[Alexei Svitkine (slow), 2014-09-15 17:50:38]

https://codereview.chromium.org/511623002/diff/80001/components/metrics/daily...
File components/metrics/daily_observer.h (right):

https://codereview.chromium.org/511623002/diff/80001/components/metrics/daily...
components/metrics/daily_observer.h:12: // DailyObserver receives notifications
from a DailyInterval.
For an interface this simple, I suggest defining it inline in daily_interval.h,
similar to what we do here:

https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/field...

(Then it will be DailyInterval::Observer).

https://codereview.chromium.org/511623002/diff/80001/components/rappor/rappor...
File components/rappor/rappor_service.h (right):

https://codereview.chromium.org/511623002/diff/80001/components/rappor/rappor...
components/rappor/rappor_service.h:49: // take ownership of all observers.
If ownership is taken, pass the param by scoped_ptr (then in fact you don't need
this second sentence of the comment).

[Steven Holte, 2014-09-15 20:17:10]

https://codereview.chromium.org/511623002/diff/80001/components/metrics/daily...
File components/metrics/daily_observer.h (right):

https://codereview.chromium.org/511623002/diff/80001/components/metrics/daily...
components/metrics/daily_observer.h:12: // DailyObserver receives notifications
from a DailyInterval.
On 2014/09/15 17:50:38, Alexei Svitkine wrote:
> For an interface this simple, I suggest defining it inline in
daily_interval.h,
> similar to what we do here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/field...
> 
> (Then it will be DailyInterval::Observer).

Done.

https://codereview.chromium.org/511623002/diff/80001/components/rappor/rappor...
File components/rappor/rappor_service.h (right):

https://codereview.chromium.org/511623002/diff/80001/components/rappor/rappor...
components/rappor/rappor_service.h:49: // take ownership of all observers.
On 2014/09/15 17:50:38, Alexei Svitkine wrote:
> If ownership is taken, pass the param by scoped_ptr (then in fact you don't
need
> this second sentence of the comment).

Done.

[Alexei Svitkine (slow), 2014-09-15 20:29:41]

https://codereview.chromium.org/511623002/diff/100001/components/metrics/dail...
File components/metrics/daily_interval.h (right):

https://codereview.chromium.org/511623002/diff/100001/components/metrics/dail...
components/metrics/daily_interval.h:19: // chrome is restarted more frequently. 
It is based on local machine time, so
Nit: This is in a component, so don't say "Chrome" since the component can be
used by different programs - you can say "the program".

https://codereview.chromium.org/511623002/diff/100001/components/metrics/dail...
components/metrics/daily_interval.h:20: // it could be fired more often if the
clock is changed.
In the description, mention how this class is intended to be used (e.g. that
CheckInterval() should is expected to be called by the caller in place of
recording the metric, etc).

https://codereview.chromium.org/511623002/diff/100001/components/metrics/dail...
components/metrics/daily_interval.h:23: // DailyObserver receives notifications
from a DailyInterval.
Nit: Update comment (no longer named DailyObserver).

https://codereview.chromium.org/511623002/diff/100001/components/metrics/dail...
components/metrics/daily_interval.h:80: // True if we've started checking for
events.
Comment seems to not match the field (it's a time field, not a bool).

[Steven Holte, 2014-09-15 21:27:04]

Renamed DailyInterval to DailyEvent, which is more consistent.

https://codereview.chromium.org/511623002/diff/100001/components/metrics/dail...
File components/metrics/daily_interval.h (right):

https://codereview.chromium.org/511623002/diff/100001/components/metrics/dail...
components/metrics/daily_interval.h:19: // chrome is restarted more frequently. 
It is based on local machine time, so
On 2014/09/15 20:29:41, Alexei Svitkine wrote:
> Nit: This is in a component, so don't say "Chrome" since the component can be
> used by different programs - you can say "the program".

Done.

https://codereview.chromium.org/511623002/diff/100001/components/metrics/dail...
components/metrics/daily_interval.h:20: // it could be fired more often if the
clock is changed.
On 2014/09/15 20:29:41, Alexei Svitkine wrote:
> In the description, mention how this class is intended to be used (e.g. that
> CheckInterval() should is expected to be called by the caller in place of
> recording the metric, etc).

Done.

https://codereview.chromium.org/511623002/diff/100001/components/metrics/dail...
components/metrics/daily_interval.h:23: // DailyObserver receives notifications
from a DailyInterval.
On 2014/09/15 20:29:41, Alexei Svitkine wrote:
> Nit: Update comment (no longer named DailyObserver).

Done.

https://codereview.chromium.org/511623002/diff/100001/components/metrics/dail...
components/metrics/daily_interval.h:80: // True if we've started checking for
events.
On 2014/09/15 20:29:41, Alexei Svitkine wrote:
> Comment seems to not match the field (it's a time field, not a bool).

Done.

[Alexei Svitkine (slow), 2014-09-15 21:32:32]

LGTM with a few more nits.

https://codereview.chromium.org/511623002/diff/120001/components/metrics/dail...
File components/metrics/daily_event.cc (right):

https://codereview.chromium.org/511623002/diff/120001/components/metrics/dail...
components/metrics/daily_event.cc:24: IntervalType t) {
Nit: t -> type

https://codereview.chromium.org/511623002/diff/120001/components/metrics/dail...
File components/metrics/daily_event.h (right):

https://codereview.chromium.org/511623002/diff/120001/components/metrics/dail...
components/metrics/daily_event.h:46: // Caller is responsible for ensuring
|pref_service| outlives the DailyEvent.
Nit: Also, pref_name. Update comment and mention the same on the field comment.

https://codereview.chromium.org/511623002/diff/120001/components/metrics/dail...
components/metrics/daily_event.h:58: // Check if a day has elapsed. If it has,
OnDailyEvent will be called on
Nit: Check -> Checks for consistency with verb tenses in other comments.

https://codereview.chromium.org/511623002/diff/120001/components/metrics/dail...
components/metrics/daily_event.h:66: // Handle an interval elapsing.
Nit: Handle -> Handles

https://codereview.chromium.org/511623002/diff/120001/components/metrics/dail...
components/metrics/daily_event.h:80: // List of observers
Nit: Add a .

[Steven Holte, 2014-09-15 21:43:09]

https://codereview.chromium.org/511623002/diff/120001/components/metrics/dail...
File components/metrics/daily_event.cc (right):

https://codereview.chromium.org/511623002/diff/120001/components/metrics/dail...
components/metrics/daily_event.cc:24: IntervalType t) {
On 2014/09/15 21:32:32, Alexei Svitkine wrote:
> Nit: t -> type

Done.

https://codereview.chromium.org/511623002/diff/120001/components/metrics/dail...
File components/metrics/daily_event.h (right):

https://codereview.chromium.org/511623002/diff/120001/components/metrics/dail...
components/metrics/daily_event.h:46: // Caller is responsible for ensuring
|pref_service| outlives the DailyEvent.
On 2014/09/15 21:32:32, Alexei Svitkine wrote:
> Nit: Also, pref_name. Update comment and mention the same on the field
comment.

Done.

https://codereview.chromium.org/511623002/diff/120001/components/metrics/dail...
components/metrics/daily_event.h:58: // Check if a day has elapsed. If it has,
OnDailyEvent will be called on
On 2014/09/15 21:32:32, Alexei Svitkine wrote:
> Nit: Check -> Checks for consistency with verb tenses in other comments.

Done.

https://codereview.chromium.org/511623002/diff/120001/components/metrics/dail...
components/metrics/daily_event.h:66: // Handle an interval elapsing.
On 2014/09/15 21:32:32, Alexei Svitkine wrote:
> Nit: Handle -> Handles

Done.

https://codereview.chromium.org/511623002/diff/120001/components/metrics/dail...
components/metrics/daily_event.h:80: // List of observers
On 2014/09/15 21:32:32, Alexei Svitkine wrote:
> Nit: Add a .

Done.

[Steven Holte, 2014-09-15 21:44:03]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2014-09-15 22:04:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/511623002/180001

[commit-bot: I haz the power, 2014-09-16 00:47:30]

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

[commit-bot: I haz the power, 2014-09-16 00:47:32]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Steven Holte, 2014-09-19 21:13:28]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2014-09-19 21:15:04]

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

[commit-bot: I haz the power, 2014-09-19 21:59:13]

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

[commit-bot: I haz the power, 2014-09-19 21:59:14]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-09-19 21:59:50]

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

[commit-bot: I haz the power, 2014-09-19 21:59:52]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Steven Holte, 2014-09-19 23:22:27]

On 2014/09/19 21:59:52, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_gn_rel on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Hm, this looks like a linker error, but I don't understand why it would be in
just this one build.  It looks like
librappor.a isn't seeing the contents of metrics/daily_event.cc, but is still
seeing metrics/metrics_hashes.cc.

Any ideas what I'm missing?


FAILED: /mnt/scratch0/b_used/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -Wl,--fatal-warnings
-m64 -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -pthread
-B../../third_party/binutils/Linux_x64/Release/bin -fuse-ld=gold -Wl,--icf=none
-Wl,-O1 -Wl,--as-needed -Wl,--gc-sections -Wl,-rpath=\$ORIGIN/ -Wl,-rpath-link=
-Wl,--disable-new-dtags -o ./chrome -Wl,--start-group @./chrome.rsp 
-Wl,--end-group  -ldl -lpangocairo-1.0 -lpango-1.0 -lcairo -lgobject-2.0
-lglib-2.0 -lX11 -lXcomposite -lXcursor -lXdamage -lXext -lXfixes -lXi -lXrender
-lXss -lXtst -ludev -lrt -lcups -lgssapi_krb5 -lkrb5 -lk5crypto -lcom_err
-lgcrypt -lz -lpthread -lcrypt -lm -lgnome-keyring -ldbus-1 -lfontconfig
-lgmodule-2.0 -lgthread-2.0 -lnss3 -lnssutil3 -lsmime3 -lplds4 -lplc4 -lnspr4
-lgconf-2 -lresolv -lgio-2.0 -lfreetype -lXrandr -lasound -lexpat -lcrypto -lcap
-lgtk-x11-2.0 -lgdk-x11-2.0 -latk-1.0 -lpangoft2-1.0 -lgdk_pixbuf-2.0 
obj/components/rappor/librappor.a(librappor.rappor_service.o):../../components/rappor/rappor_service.cc:function
rappor::RapporService::RapporService(PrefService*): error: undefined reference
to 'metrics::DailyEvent::DailyEvent(PrefService*, char const*, std::string
const&)'
obj/components/rappor/librappor.a(librappor.rappor_service.o):../../components/rappor/rappor_service.cc:function
rappor::RapporService::~RapporService(): error: undefined reference to
'metrics::DailyEvent::~DailyEvent()'
obj/components/rappor/librappor.a(librappor.rappor_service.o):../../components/rappor/rappor_service.cc:function
rappor::RapporService::OnLogInterval(): error: undefined reference to
'metrics::DailyEvent::CheckInterval()'
obj/components/rappor/librappor.a(librappor.rappor_service.o):../../components/rappor/rappor_service.cc:function
rappor::RapporService::RegisterPrefs(PrefRegistrySimple*): error: undefined
reference to 'metrics::DailyEvent::RegisterPref(PrefRegistrySimple*, char
const*)'

[Ilya Sherman, 2014-09-20 00:04:38]

On 2014/09/19 23:22:27, Steven Holte wrote:
> On 2014/09/19 21:59:52, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_gn_rel on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> Hm, this looks like a linker error, but I don't understand why it would be in
> just this one build.  It looks like
> librappor.a isn't seeing the contents of metrics/daily_event.cc, but is still
> seeing metrics/metrics_hashes.cc.
> 
> Any ideas what I'm missing?
> 
> 
> FAILED: /mnt/scratch0/b_used/build/goma/gomacc
> ../../third_party/llvm-build/Release+Asserts/bin/clang++ -Wl,--fatal-warnings
> -m64 -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -pthread
> -B../../third_party/binutils/Linux_x64/Release/bin -fuse-ld=gold
-Wl,--icf=none
> -Wl,-O1 -Wl,--as-needed -Wl,--gc-sections -Wl,-rpath=\$ORIGIN/
-Wl,-rpath-link=
> -Wl,--disable-new-dtags -o ./chrome -Wl,--start-group @./chrome.rsp 
> -Wl,--end-group  -ldl -lpangocairo-1.0 -lpango-1.0 -lcairo -lgobject-2.0
> -lglib-2.0 -lX11 -lXcomposite -lXcursor -lXdamage -lXext -lXfixes -lXi
-lXrender
> -lXss -lXtst -ludev -lrt -lcups -lgssapi_krb5 -lkrb5 -lk5crypto -lcom_err
> -lgcrypt -lz -lpthread -lcrypt -lm -lgnome-keyring -ldbus-1 -lfontconfig
> -lgmodule-2.0 -lgthread-2.0 -lnss3 -lnssutil3 -lsmime3 -lplds4 -lplc4 -lnspr4
> -lgconf-2 -lresolv -lgio-2.0 -lfreetype -lXrandr -lasound -lexpat -lcrypto
-lcap
> -lgtk-x11-2.0 -lgdk-x11-2.0 -latk-1.0 -lpangoft2-1.0 -lgdk_pixbuf-2.0 
>
obj/components/rappor/librappor.a(librappor.rappor_service.o):../../components/rappor/rappor_service.cc:function
> rappor::RapporService::RapporService(PrefService*): error: undefined reference
> to 'metrics::DailyEvent::DailyEvent(PrefService*, char const*, std::string
> const&)'
>
obj/components/rappor/librappor.a(librappor.rappor_service.o):../../components/rappor/rappor_service.cc:function
> rappor::RapporService::~RapporService(): error: undefined reference to
> 'metrics::DailyEvent::~DailyEvent()'
>
obj/components/rappor/librappor.a(librappor.rappor_service.o):../../components/rappor/rappor_service.cc:function
> rappor::RapporService::OnLogInterval(): error: undefined reference to
> 'metrics::DailyEvent::CheckInterval()'
>
obj/components/rappor/librappor.a(librappor.rappor_service.o):../../components/rappor/rappor_service.cc:function
> rappor::RapporService::RegisterPrefs(PrefRegistrySimple*): error: undefined
> reference to 'metrics::DailyEvent::RegisterPref(PrefRegistrySimple*, char
> const*)'

It looks like you need to update //components/metrics/BUILD.gn to parallel your
update of the .gypi file.

[Steven Holte, 2014-09-20 00:35:12]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2014-09-20 00:35:53]

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

[commit-bot: I haz the power, 2014-09-20 02:37:18]

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

[commit-bot: I haz the power, 2014-09-20 02:37:20]

Exceeded time limit waiting for builds to trigger.

[Steven Holte, 2014-09-22 19:13:41]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2014-09-22 19:14:14]

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

[commit-bot: I haz the power, 2014-09-22 19:15:11]

Committed patchset #12 (id:220001) as 72e370f82adf050c98732db8b38f034bf647e261

[commit-bot: I haz the power, 2014-09-22 19:15:44]

Patchset 12 (id:??) landed as
https://crrev.com/07637b015c74b180c1163f38c1458ec41647644e
Cr-Commit-Position: refs/heads/master@{#296025}