Record field-trial information in stability file for crash analysis.

Record field-trial information in stability file for crash analysis.

BUG=620813
Review-Url: https://codereview.chromium.org/2666653002
Cr-Commit-Position: refs/heads/master@{#449013}
Committed: https://chromium.googlesource.com/chromium/src/+/33d92f4ec79c19ddc925b1a96adad8e4edbd8010

[bcwhite, 2017-01-30 19:15:57]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 19:16:24]

Dry run: 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-30 19:19:00]

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

[commit-bot: I haz the power, 2017-01-30 19:19:01]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[bcwhite, 2017-01-30 19:23:20]

bcwhite@chromium.org changed reviewers:
+ manzagop@chromium.org

[bcwhite, 2017-01-30 21:18:56]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 21:19:54]

Dry run: 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-30 21:23:13]

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

[commit-bot: I haz the power, 2017-01-30 21:23:14]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[manzagop (departed), 2017-01-30 22:09:40]

I want to make sure my understanding of Field trials is accurate.

IIUC:
- at startup it is already possible to know your full experiment configuration
based on your seed and your commandline
- *However* UMA uploads and crash reports only contain a *subset* of that
configuration, based on what was "registered" at the time, because an experiment
that is not registered has no effect.
https://cs.chromium.org/chromium/src/components/metrics/metrics_service.h?rcl...

Does this change record all experiments or the registered ones?

[chromium-reviews, 2017-01-31 13:24:36]

>
> IIUC:
> - at startup it is already possible to know your full experiment
> configuration
> based on your seed and your commandline
> - *However* UMA uploads and crash reports only contain a *subset* of that
> configuration, based on what was "registered" at the time, because an
> experiment
> that is not registered has no effect.
> https://cs.chromium.org/chromium/src/components/
> metrics/metrics_service.h?rcl=1485491011&l=238
>
> Does this change record all experiments or the registered ones?
>

What's recorded for field trials is the same information that's passed to
all subprocesses so it should be complete.

The global-user strings then record the actual lookups and their results.

  Brian
  bcwhite@google.com
-----------------------------------------------------------------------------------------



*Treat someone as they are and they will remain that way.Treat someone as
they can be and they will become that way.*

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bcwhite, 2017-01-31 14:19:15]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 14:19:25]

Dry run: 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-31 14:26:20]

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

[commit-bot: I haz the power, 2017-01-31 14:26:21]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[bcwhite, 2017-01-31 14:50:57]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 14:51:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-01-31 14:53:02]

Patchset #3 (id:40001) has been deleted

[bcwhite, 2017-01-31 14:53:14]

Patchset #2 (id:20001) has been deleted

[bcwhite, 2017-01-31 15:03:17]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 15:03:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-01-31 15:03:45]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-01-31 15:40:23]

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

[commit-bot: I haz the power, 2017-01-31 15:40:25]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[manzagop (departed), 2017-01-31 16:21:19]

(+Alexei to make sure we get this right)

IIUC:
>> - at startup it is already possible to know your full experiment
>> configuration
>> based on your seed and your commandline
>> - *However* UMA uploads and crash reports only contain a *subset* of that
>> configuration, based on what was "registered" at the time, because an
>> experiment
>> that is not registered has no effect.
>> https://cs.chromium.org/chromium/src/components/metrics/metr
>> ics_service.h?rcl=1485491011&l=238
>>
>> Does this change record all experiments or the registered ones?
>>
>
> What's recorded for field trials is the same information that's passed to
> all subprocesses so it should be complete.
>

I had a look at the code and this function dumps the registered_ field. My
understanding is this field grows throughout chrome's lifetime, so I don't
think a one time dump is enough. I'm guessing that's why there are calls to
AddToAllocatorWhileLocked in ActivateFieldTrialEntryWhileLocked
and OnGroupFinalized in addition to InstantiateFieldTrialAllocatorIfNeeded.

The global-user strings then record the actual lookups and their results.
>

 It seems not ideal to need to check a second structure to know about
experiments being active. Could we use the existing activated field from
the FieldTrialEntry struct [2]?

[2] https://cs.chromium.org/chromium/src/base/metrics/
field_trial.h?type=cs&l=155

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2017-01-31 16:32:06]

The 'activated' field should indeed be up to date.

It's true that the data can grow during Chrome's runtime, but this is
generally rare after startup - usually most trials are created by
VariationsService during startup.

Still, ideally we would use the most up to date version of that data (i.e.
the one in the actual shared memory segment rather than a copy of it). But
that would I guess require additional support - since it would be separate
from breadcrumbs itself.

On Tue, Jan 31, 2017 at 8:20 AM, Pierre-Antoine Manzagol <
manzagop@chromium.org> wrote:

> (+Alexei to make sure we get this right)
>
> IIUC:
>>> - at startup it is already possible to know your full experiment
>>> configuration
>>> based on your seed and your commandline
>>> - *However* UMA uploads and crash reports only contain a *subset* of that
>>> configuration, based on what was "registered" at the time, because an
>>> experiment
>>> that is not registered has no effect.
>>> https://cs.chromium.org/chromium/src/components/metrics/metr
>>> ics_service.h?rcl=1485491011&l=238
>>>
>>> Does this change record all experiments or the registered ones?
>>>
>>
>> What's recorded for field trials is the same information that's passed to
>> all subprocesses so it should be complete.
>>
>
> I had a look at the code and this function dumps the registered_ field. My
> understanding is this field grows throughout chrome's lifetime, so I don't
> think a one time dump is enough. I'm guessing that's why there are calls to
> AddToAllocatorWhileLocked in ActivateFieldTrialEntryWhileLocked
> and OnGroupFinalized in addition to InstantiateFieldTrialAlloca
> torIfNeeded.
>
> The global-user strings then record the actual lookups and their results.
>>
>
>  It seems not ideal to need to check a second structure to know about
> experiments being active. Could we use the existing activated field from
> the FieldTrialEntry struct [2]?
>
> [2] https://cs.chromium.org/chromium/src/base/metrics/field_
> trial.h?type=cs&l=155
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[manzagop (departed), 2017-01-31 16:45:23]

Right we want to have the complete up to date picture, as we hope this will
increase fidelity of experiment membership information in crashes and
stability reports.

Some more questions:
- Is there a difference between registered and activated?
- (assuming yes) what do the stability / crash reports include: activated
or registered?

On Tue, Jan 31, 2017 at 11:31 AM, Alexei Svitkine <asvitkine@chromium.org>
wrote:

> The 'activated' field should indeed be up to date.
>
> It's true that the data can grow during Chrome's runtime, but this is
> generally rare after startup - usually most trials are created by
> VariationsService during startup.
>
> Still, ideally we would use the most up to date version of that data (i.e.
> the one in the actual shared memory segment rather than a copy of it). But
> that would I guess require additional support - since it would be separate
> from breadcrumbs itself.
>
> On Tue, Jan 31, 2017 at 8:20 AM, Pierre-Antoine Manzagol <
> manzagop@chromium.org> wrote:
>
>> (+Alexei to make sure we get this right)
>>
>> IIUC:
>>>> - at startup it is already possible to know your full experiment
>>>> configuration
>>>> based on your seed and your commandline
>>>> - *However* UMA uploads and crash reports only contain a *subset* of
>>>> that
>>>> configuration, based on what was "registered" at the time, because an
>>>> experiment
>>>> that is not registered has no effect.
>>>> https://cs.chromium.org/chromium/src/components/metrics/metr
>>>> ics_service.h?rcl=1485491011&l=238
>>>>
>>>> Does this change record all experiments or the registered ones?
>>>>
>>>
>>> What's recorded for field trials is the same information that's passed
>>> to all subprocesses so it should be complete.
>>>
>>
>> I had a look at the code and this function dumps the registered_ field.
>> My understanding is this field grows throughout chrome's lifetime, so I
>> don't think a one time dump is enough. I'm guessing that's why there are
>> calls to AddToAllocatorWhileLocked in ActivateFieldTrialEntryWhileLocked
>> and OnGroupFinalized in addition to InstantiateFieldTrialAlloca
>> torIfNeeded.
>>
>> The global-user strings then record the actual lookups and their results.
>>>
>>
>>  It seems not ideal to need to check a second structure to know about
>> experiments being active. Could we use the existing activated field from
>> the FieldTrialEntry struct [2]?
>>
>> [2] https://cs.chromium.org/chromium/src/base/metrics/field_tria
>> l.h?type=cs&l=155
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2017-01-31 16:49:58]

There's a difference. Crash reports include activated.

On Jan 31, 2017 8:45 AM, "Pierre-Antoine Manzagol" <manzagop@chromium.org>
wrote:

> Right we want to have the complete up to date picture, as we hope this
> will increase fidelity of experiment membership information in crashes and
> stability reports.
>
> Some more questions:
> - Is there a difference between registered and activated?
> - (assuming yes) what do the stability / crash reports include: activated
> or registered?
>
> On Tue, Jan 31, 2017 at 11:31 AM, Alexei Svitkine <asvitkine@chromium.org>
> wrote:
>
>> The 'activated' field should indeed be up to date.
>>
>> It's true that the data can grow during Chrome's runtime, but this is
>> generally rare after startup - usually most trials are created by
>> VariationsService during startup.
>>
>> Still, ideally we would use the most up to date version of that data
>> (i.e. the one in the actual shared memory segment rather than a copy of
>> it). But that would I guess require additional support - since it would be
>> separate from breadcrumbs itself.
>>
>> On Tue, Jan 31, 2017 at 8:20 AM, Pierre-Antoine Manzagol <
>> manzagop@chromium.org> wrote:
>>
>>> (+Alexei to make sure we get this right)
>>>
>>> IIUC:
>>>>> - at startup it is already possible to know your full experiment
>>>>> configuration
>>>>> based on your seed and your commandline
>>>>> - *However* UMA uploads and crash reports only contain a *subset* of
>>>>> that
>>>>> configuration, based on what was "registered" at the time, because an
>>>>> experiment
>>>>> that is not registered has no effect.
>>>>> https://cs.chromium.org/chromium/src/components/metrics/metr
>>>>> ics_service.h?rcl=1485491011&l=238
>>>>>
>>>>> Does this change record all experiments or the registered ones?
>>>>>
>>>>
>>>> What's recorded for field trials is the same information that's passed
>>>> to all subprocesses so it should be complete.
>>>>
>>>
>>> I had a look at the code and this function dumps the registered_ field.
>>> My understanding is this field grows throughout chrome's lifetime, so I
>>> don't think a one time dump is enough. I'm guessing that's why there are
>>> calls to AddToAllocatorWhileLocked in ActivateFieldTrialEntryWhileLocked
>>> and OnGroupFinalized in addition to InstantiateFieldTrialAlloca
>>> torIfNeeded.
>>>
>>> The global-user strings then record the actual lookups and their results.
>>>>
>>>
>>>  It seems not ideal to need to check a second structure to know about
>>> experiments being active. Could we use the existing activated field from
>>> the FieldTrialEntry struct [2]?
>>>
>>> [2] https://cs.chromium.org/chromium/src/base/metrics/field_tria
>>> l.h?type=cs&l=155
>>>
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bcwhite, 2017-01-31 18:36:24]

It's true that only a single snapshot of the metrics information is captured and
stored.  However, the individual recordings of the checks will always show the
value at the time of the query.

That means that additions and changes will still get recorded, though only the
result they returned rather than the full field-trial/feature information.

[manzagop (departed), 2017-01-31 22:28:03]

On 2017/01/31 18:36:24, bcwhite wrote:
> It's true that only a single snapshot of the metrics information is captured
and
> stored.  However, the individual recordings of the checks will always show the
> value at the time of the query.
> 
> That means that additions and changes will still get recorded, though only the
> result they returned rather than the full field-trial/feature information.

Stability reports and crash reports seem to only have the hashed experiment and 
group, so it looks like we don't need some of the data in the snapshot (e.g. 
params). For the global user data, we'd need to add the experiment group in 
addition to the name, but I don't think I like it as a mechanism... It seems 
preferable to have a dedicated experiment recording, no?

Siggi's also asking about single-instancing the experiment set data. Let me
start
a thread.

[bcwhite, 2017-02-03 16:02:16]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 16:02:50]

Dry run: 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-02-03 16:09:48]

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

[commit-bot: I haz the power, 2017-02-03 16:09:49]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[bcwhite, 2017-02-03 16:11:23]

Implemented as discussed.  All field-trial activations are recorded in
global_data along with the active group name.

I've checked this on an official build and about 1.5KiB of strings are stored in
the breadcrumbs file.

Note that this only affects field-trials.  Features enabled on the command-line
via --enable-features do not get included this way but those that have
--force-field-trials=... do appear.

[bcwhite, 2017-02-03 16:13:29]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[bcwhite, 2017-02-03 16:13:37]

Patchset #3 (id:90001) has been deleted

[commit-bot: I haz the power, 2017-02-03 16:13:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-02-03 16:14:23]

Description was changed from

==========
Record field-trial information in stability file for crash analysis.

BUG=620813
==========

to

==========
Record field-trial information in stability file for crash analysis.

BUG=620813
==========

[bcwhite, 2017-02-03 16:14:23]

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

[commit-bot: I haz the power, 2017-02-03 16:42:43]

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

[commit-bot: I haz the power, 2017-02-03 16:42:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[manzagop (departed), 2017-02-03 16:56:59]

manzagop@chromium.org changed reviewers:
+ siggi@chromium.org

[manzagop (departed), 2017-02-03 16:57:00]

I've investigated how experiments are recorded to crash keys and I think Siggi's
concern is the delay from the observer mechanism posting a task.

Siggi, can you confim this is your concern?

[bcwhite, 2017-02-03 18:00:22]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 18:00:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-02-03 18:16:02]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[bcwhite, 2017-02-03 18:16:45]

Patchset #4 (id:130001) has been deleted

[commit-bot: I haz the power, 2017-02-03 18:16:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sigurður Ásgeirsson, 2017-02-03 18:48:53]

Yups, we want immediate recording, not re-posting.


On Fri, Feb 3, 2017 at 8:57 AM <manzagop@chromium.org> wrote:

> I've investigated how experiments are recorded to crash keys and I think
> Siggi's
> concern is the delay from the observer mechanism posting a task.
>
> Siggi, can you confim this is your concern?
>
> https://codereview.chromium.org/2666653002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bcwhite, 2017-02-03 18:49:06]

On 2017/02/03 16:57:00, manzagop wrote:
> I've investigated how experiments are recorded to crash keys and I think
Siggi's
> concern is the delay from the observer mechanism posting a task.
> 
> Siggi, can you confim this is your concern?

I've created a new version that records it immediately, no posted tasks.

[manzagop (departed), 2017-02-03 19:01:49]

Thanks, this is great! lgtm % question about GlobalUserData

We need to keep in mind we have a small blind spot for now at start because we
wait for the experiment state to decide whether we record to the stability file.

https://codereview.chromium.org/2666653002/diff/150001/base/debug/activity_tr...
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2666653002/diff/150001/base/debug/activity_tr...
base/debug/activity_tracker.cc:1291: // Destruction of the object will disable
tracking.
Update comment (no longer destroys).

https://codereview.chromium.org/2666653002/diff/150001/base/debug/activity_tr...
File base/debug/activity_tracker.h (right):

https://codereview.chromium.org/2666653002/diff/150001/base/debug/activity_tr...
base/debug/activity_tracker.h:937: ActivityUserData global_data_;
Do you mean for this to be of type GlobalUserData?

[bcwhite, 2017-02-03 19:06:56]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[bcwhite, 2017-02-03 19:07:04]

https://codereview.chromium.org/2666653002/diff/150001/base/debug/activity_tr...
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2666653002/diff/150001/base/debug/activity_tr...
base/debug/activity_tracker.cc:1291: // Destruction of the object will disable
tracking.
On 2017/02/03 19:01:49, manzagop wrote:
> Update comment (no longer destroys).

Done.

https://codereview.chromium.org/2666653002/diff/150001/base/debug/activity_tr...
File base/debug/activity_tracker.h (right):

https://codereview.chromium.org/2666653002/diff/150001/base/debug/activity_tr...
base/debug/activity_tracker.h:937: ActivityUserData global_data_;
On 2017/02/03 19:01:49, manzagop wrote:
> Do you mean for this to be of type GlobalUserData?

Oops!  Yes!

[commit-bot: I haz the power, 2017-02-03 19:07:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-02-03 19:08:04]

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

[bcwhite, 2017-02-03 19:08:04]

asvitkine@chromium.org: Please review changes in 

base/metrics/field_trial.cc
chrome/browser/chrome_browser_field_trials_desktop.cc

[commit-bot: I haz the power, 2017-02-03 20:09:11]

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

[commit-bot: I haz the power, 2017-02-03 20:09:12]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-02-06 17:46:12]

https://codereview.chromium.org/2666653002/diff/170001/base/debug/activity_tr...
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2666653002/diff/170001/base/debug/activity_tr...
base/debug/activity_tracker.cc:75: void Disable() {
What prevents this being called after Record() has already added something to
global_data()?

https://codereview.chromium.org/2666653002/diff/170001/base/debug/activity_tr...
base/debug/activity_tracker.cc:110: trial_group_map_.insert(std::make_pair(key,
group_name.as_string()));
group_name is being turned back into a string here, making the param being a
StringPiece worse than if it was a string ref. Use a string ref instead.

https://codereview.chromium.org/2666653002/diff/170001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2666653002/diff/170001/base/metrics/field_tri...
base/metrics/field_trial.cc:987: // to an observer may not get executed before a
crash.
While you're changing this, how about setting crash_keys from the same place?
This way, the problem will also be fixed there too.

https://codereview.chromium.org/2666653002/diff/170001/base/metrics/field_tri...
base/metrics/field_trial.cc:988:
base::debug::GlobalActivityTracker::RecordFieldTrial(
Is this API thread safe? Because this can be called from any thread....

If it is, please document it in the API's function comment.

[manzagop (departed), 2017-02-06 18:38:17]

https://codereview.chromium.org/2666653002/diff/170001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2666653002/diff/170001/base/metrics/field_tri...
base/metrics/field_trial.cc:987: // to an observer may not get executed before a
crash.
On 2017/02/06 17:46:12, Alexei Svitkine (very slow) wrote:
> While you're changing this, how about setting crash_keys from the same place?
> This way, the problem will also be fixed there too.

IIUC the problem with crash keys is the update leads to querying for all active
experiments, hashing them, and formatting them, which is deemed too expensive to
be inline experiment activation?

[Alexei Svitkine (slow), 2017-02-06 19:45:56]

I think it's inefficient, yes, but it's still all just computation stuff
that doesn't involve IO so I think it should still be fine to do it inline
as part of experiment activation.

Plus, we'll only do it as many times as there are active experiments - so
total 30 or so times each session.

On Mon, Feb 6, 2017 at 1:38 PM, <manzagop@chromium.org> wrote:

>
> https://codereview.chromium.org/2666653002/diff/170001/base/
> metrics/field_trial.cc
> File base/metrics/field_trial.cc (right):
>
> https://codereview.chromium.org/2666653002/diff/170001/base/
> metrics/field_trial.cc#newcode987
> base/metrics/field_trial.cc:987: // to an observer may not get executed
> before a crash.
> On 2017/02/06 17:46:12, Alexei Svitkine (very slow) wrote:
> > While you're changing this, how about setting crash_keys from the same
> place?
> > This way, the problem will also be fixed there too.
>
> IIUC the problem with crash keys is the update leads to querying for all
> active experiments, hashing them, and formatting them, which is deemed
> too expensive to be inline experiment activation?
>
> https://codereview.chromium.org/2666653002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bcwhite, 2017-02-07 15:42:54]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 15:43:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-02-07 15:58:59]

https://codereview.chromium.org/2666653002/diff/170001/base/debug/activity_tr...
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2666653002/diff/170001/base/debug/activity_tr...
base/debug/activity_tracker.cc:75: void Disable() {
On 2017/02/06 17:46:12, Alexei Svitkine (slow) wrote:
> What prevents this being called after Record() has already added something to
> global_data()?

Nothing.  Data before that point will be recorded, if a GlobalActivityTracker
got created, but not after.  No harm.

https://codereview.chromium.org/2666653002/diff/170001/base/debug/activity_tr...
base/debug/activity_tracker.cc:110: trial_group_map_.insert(std::make_pair(key,
group_name.as_string()));
On 2017/02/06 17:46:12, Alexei Svitkine (slow) wrote:
> group_name is being turned back into a string here, making the param being a
> StringPiece worse than if it was a string ref. Use a string ref instead.

This isn't the usual case, though.  Normal activity will run through the case
above which doesn't turn it back into a std::string.

However, since we know where it's being called from and that it's a std::string
there, might as well make use of that.

Done.

https://codereview.chromium.org/2666653002/diff/170001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2666653002/diff/170001/base/metrics/field_tri...
base/metrics/field_trial.cc:987: // to an observer may not get executed before a
crash.
Happy to do it, if it's desired, but I'll do it as a separate CL so if there is
a problem and it has to be reverted, it won't affect the rest.

https://codereview.chromium.org/2666653002/diff/170001/base/metrics/field_tri...
base/metrics/field_trial.cc:988:
base::debug::GlobalActivityTracker::RecordFieldTrial(
On 2017/02/06 17:46:12, Alexei Svitkine (slow) wrote:
> Is this API thread safe? Because this can be called from any thread....
> 
> If it is, please document it in the API's function comment.

Yes.  Done.

[Alexei Svitkine (slow), 2017-02-07 16:14:07]

https://codereview.chromium.org/2666653002/diff/170001/base/debug/activity_tr...
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2666653002/diff/170001/base/debug/activity_tr...
base/debug/activity_tracker.cc:75: void Disable() {
On 2017/02/07 15:58:58, bcwhite wrote:
> On 2017/02/06 17:46:12, Alexei Svitkine (slow) wrote:
> > What prevents this being called after Record() has already added something
to
> > global_data()?
> 
> Nothing.  Data before that point will be recorded, if a GlobalActivityTracker
> got created, but not after.  No harm.

Can you document that? In practice, I guess this means that whatever trials we
check before and including the trial that's controlling the Feature in question
will always be recorded?

https://codereview.chromium.org/2666653002/diff/190001/base/debug/activity_tr...
File base/debug/activity_tracker.h (right):

https://codereview.chromium.org/2666653002/diff/190001/base/debug/activity_tr...
base/debug/activity_tracker.h:792: // dumped into it. This call is completely
thread-safe.
Nit: Remove the word "completely". "thread safe" is enough to be clear.

https://codereview.chromium.org/2666653002/diff/190001/base/debug/activity_tr...
base/debug/activity_tracker.h:796: // Disable all filed trial recording. This
should be called when its known
field

https://codereview.chromium.org/2666653002/diff/190001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2666653002/diff/190001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials_desktop.cc:115:
base::debug::GlobalActivityTracker::DisableFieldTrialRecording();
I notice SetupStabilityDebugging() is only called on Windows.

So on other platforms we'll never hit this Disable call.

What will happen then - since the field_trial.cc code still runs on all the
platforms?

[commit-bot: I haz the power, 2017-02-07 16:48:06]

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

[commit-bot: I haz the power, 2017-02-07 16:48:07]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2017-02-07 17:14:55]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 17:15:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-02-07 17:39:31]

https://codereview.chromium.org/2666653002/diff/170001/base/debug/activity_tr...
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2666653002/diff/170001/base/debug/activity_tr...
base/debug/activity_tracker.cc:75: void Disable() {
On 2017/02/07 16:14:06, Alexei Svitkine (slow) wrote:
> On 2017/02/07 15:58:58, bcwhite wrote:
> > On 2017/02/06 17:46:12, Alexei Svitkine (slow) wrote:
> > > What prevents this being called after Record() has already added something
> to
> > > global_data()?
> > 
> > Nothing.  Data before that point will be recorded, if a
GlobalActivityTracker
> > got created, but not after.  No harm.
> 
> Can you document that? In practice, I guess this means that whatever trials we
> check before and including the trial that's controlling the Feature in
question
> will always be recorded?

They can't be recorded until a global tracker is created.  Generally, it'll
either be created or field-trial tracking will be disabled but not both.  The
result is that early field-trial activations will get held in temporary space
and then discarded when Disable is called.

https://codereview.chromium.org/2666653002/diff/190001/base/debug/activity_tr...
File base/debug/activity_tracker.h (right):

https://codereview.chromium.org/2666653002/diff/190001/base/debug/activity_tr...
base/debug/activity_tracker.h:792: // dumped into it. This call is completely
thread-safe.
On 2017/02/07 16:14:06, Alexei Svitkine (slow) wrote:
> Nit: Remove the word "completely". "thread safe" is enough to be clear.

Done.

https://codereview.chromium.org/2666653002/diff/190001/base/debug/activity_tr...
base/debug/activity_tracker.h:796: // Disable all filed trial recording. This
should be called when its known
On 2017/02/07 16:14:06, Alexei Svitkine (slow) wrote:
> field

Done.

https://codereview.chromium.org/2666653002/diff/190001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials_desktop.cc (right):

https://codereview.chromium.org/2666653002/diff/190001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials_desktop.cc:115:
base::debug::GlobalActivityTracker::DisableFieldTrialRecording();
On 2017/02/07 16:14:06, Alexei Svitkine (slow) wrote:
> I notice SetupStabilityDebugging() is only called on Windows.
> 
> So on other platforms we'll never hit this Disable call.
> 
> What will happen then - since the field_trial.cc code still runs on all the
> platforms?

Good catch.  Added call to disable on non-Win platforms.

[commit-bot: I haz the power, 2017-02-07 18:15:27]

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

[commit-bot: I haz the power, 2017-02-07 18:15:28]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-02-07 19:28:10]

https://codereview.chromium.org/2666653002/diff/210001/base/debug/activity_tr...
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2666653002/diff/210001/base/debug/activity_tr...
base/debug/activity_tracker.cc:82: void DumpCollectedActivity() {
Thinking about this more, I think we can simplify this much more by not having
this extra FieldTrialRecorder class.

Right now, you monitor for field trial activations even before the global object
has been created and then have this Dump call to synchronize it.

Instead, I propose the following:
  - When GlobalTracker is created, instead of the Dump call from this
intermediate object, it queries the current activated state from the FieldTrial
system.
  - Calls to base::debug::GlobalActivityTracker::RecordFieldTrial() are no-ops
before the GlobalTracker exists and once it does they start update the full
state.

This way, no need for FieldTrialRecorded and no need for extra intermediate
state or any special Disable functionality.

You can get the active field trial groups via GetActiveFieldTrialGroups() when
the activity tracker is created.

[bcwhite, 2017-02-07 20:50:43]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 20:51:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-02-07 20:55:32]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 20:56:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-02-07 21:17:30]

Patchset #8 (id:230001) has been deleted

[bcwhite, 2017-02-07 21:18:03]

https://codereview.chromium.org/2666653002/diff/210001/base/debug/activity_tr...
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2666653002/diff/210001/base/debug/activity_tr...
base/debug/activity_tracker.cc:82: void DumpCollectedActivity() {
On 2017/02/07 19:28:10, Alexei Svitkine (slow) wrote:
> Thinking about this more, I think we can simplify this much more by not having
> this extra FieldTrialRecorder class.
> 
> Right now, you monitor for field trial activations even before the global
object
> has been created and then have this Dump call to synchronize it.
> 
> Instead, I propose the following:
>   - When GlobalTracker is created, instead of the Dump call from this
> intermediate object, it queries the current activated state from the
FieldTrial
> system.
>   - Calls to base::debug::GlobalActivityTracker::RecordFieldTrial() are no-ops
> before the GlobalTracker exists and once it does they start update the full
> state.
> 
> This way, no need for FieldTrialRecorded and no need for extra intermediate
> state or any special Disable functionality.
> 
> You can get the active field trial groups via GetActiveFieldTrialGroups() when
> the activity tracker is created.

Done.

[Alexei Svitkine (slow), 2017-02-07 21:23:52]

lgtm % comment

https://codereview.chromium.org/2666653002/diff/250001/base/debug/activity_tr...
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2666653002/diff/250001/base/debug/activity_tr...
base/debug/activity_tracker.cc:1277: for (size_t i = 0; i <
active_groups.size(); ++i)
Nit: C++ for each loop

https://codereview.chromium.org/2666653002/diff/250001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2666653002/diff/250001/base/metrics/field_tri...
base/metrics/field_trial.cc:993: }
Nit: I prefer this extra logic to be inside GlobalActivityTracker - i.e. still
have a single static call from here like before and then do the null check
within it and call the instance method there.

The instance method can then be private.

[bcwhite, 2017-02-07 22:07:51]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[bcwhite, 2017-02-07 22:09:02]

https://codereview.chromium.org/2666653002/diff/250001/base/debug/activity_tr...
File base/debug/activity_tracker.cc (right):

https://codereview.chromium.org/2666653002/diff/250001/base/debug/activity_tr...
base/debug/activity_tracker.cc:1277: for (size_t i = 0; i <
active_groups.size(); ++i)
On 2017/02/07 21:23:52, Alexei Svitkine (slow) wrote:
> Nit: C++ for each loop

Done.  I'd just copied it from a unittest.

https://codereview.chromium.org/2666653002/diff/250001/base/metrics/field_tri...
File base/metrics/field_trial.cc (right):

https://codereview.chromium.org/2666653002/diff/250001/base/metrics/field_tri...
base/metrics/field_trial.cc:993: }
On 2017/02/07 21:23:52, Alexei Svitkine (slow) wrote:
> Nit: I prefer this extra logic to be inside GlobalActivityTracker - i.e. still
> have a single static call from here like before and then do the null check
> within it and call the instance method there.
> 
> The instance method can then be private.

It could but all the existing tracker interface operates as methods off of an
instance so I'd like to remain consistent with that.

[commit-bot: I haz the power, 2017-02-07 22:09:34]

Dry run: 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-02-08 00:05:46]

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

[commit-bot: I haz the power, 2017-02-08 00:05:47]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2017-02-08 16:36:18]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2017-02-08 16:36:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
manzagop@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2666653002/#ps270001
(title: "use c++ loop format")

[commit-bot: I haz the power, 2017-02-08 16:36:40]

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-02-08 16:43:21]

CQ is committing da patch.

Bot data: {"patchset_id": 270001, "attempt_start_ts": 1486571778921700,
"parent_rev": "013e6c4c3399fd6da7e11ab4a7b668f5b6f295c7", "commit_rev":
"33d92f4ec79c19ddc925b1a96adad8e4edbd8010"}

[commit-bot: I haz the power, 2017-02-08 16:44:00]

Description was changed from

==========
Record field-trial information in stability file for crash analysis.

BUG=620813
==========

to

==========
Record field-trial information in stability file for crash analysis.

BUG=620813

Review-Url: https://codereview.chromium.org/2666653002
Cr-Commit-Position: refs/heads/master@{#449013}
Committed:
https://chromium.googlesource.com/chromium/src/+/33d92f4ec79c19ddc925b1a96ada...
==========

[commit-bot: I haz the power, 2017-02-08 16:44:02]

Committed patchset #9 (id:270001) as
https://chromium.googlesource.com/chromium/src/+/33d92f4ec79c19ddc925b1a96ada...