Add ExitFunnel to prepare for instrumenting browser exits.

Add ExitFunnel class to browser_watcher, along with tests
and metrics reporting.

BUG=412384
Committed: https://crrev.com/deaea6e5790c7e11f179b55aafda79e4b221ecb4
Cr-Commit-Position: refs/heads/master@{#308184}

[Sigurður Ásgeirsson, 2014-12-10 20:30:54]

Patchset #1 (id:1) has been deleted

[Sigurður Ásgeirsson, 2014-12-10 20:34:23]

siggi@chromium.org changed reviewers:
+ erik@erikwright.com

[Sigurður Ásgeirsson, 2014-12-10 20:35:30]

siggi@chromium.org changed reviewers:
+ erikwright@chromium.org
- erik@erikwright.com

[Sigurður Ásgeirsson, 2014-12-10 20:35:31]

Hey Erik,

PTAL - though I think this'll do with a bit more testing before submission. The
actual instrumentation will follow in a separate CL.

Siggi

[erikwright (departed), 2014-12-11 15:00:47]

Please decouple the reading and writing of data from the other code.

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
File components/browser_watcher/exit_funnel_win.cc (right):

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/exit_funnel_win.cc:9: #include
"base/strings/string16.h"
dup from header

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/exit_funnel_win.cc:12: #include "base/win/registry.h"
dup from header

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/exit_funnel_win.cc:37: base::StringAppendF(&key_name,
L"\\%d-%lld", pid,
Personally I find this harder to read than:

base::string16 key_name = registry_path + L"\\" + base::UintToString(pid) + L"-"
+ base::UintToString(base::Time::FromFileTime(...).ToInternalValue();

But you might reasonably disagree.

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
File components/browser_watcher/exit_funnel_win.h (right):

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/exit_funnel_win.h:25: // Records |event_name| at the
current time in registry for the process
the registry

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
File components/browser_watcher/exit_funnel_win_unittest.cc (right):

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/exit_funnel_win_unittest.cc:51:
base::StringPrintf(L"%ls\\%ls", kRegistryPath, subkey_name.c_str()));
It really seems like there should be a single implementation of this reading
code shared between this test and the production code that actually reads this
value.

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
File components/browser_watcher/watcher_metrics_provider_win.cc (right):

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.cc:120: int64 min_time =
std::numeric_limits<int64>::max();
Everything in this method needs to be extracted out into something tested. It's
probably the thing that the unit test should use.

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
File components/browser_watcher/watcher_metrics_provider_win_unittest.cc
(right):

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win_unittest.cc:61:
base::StringPrintf(L"%ls\\%d-%d", kRegistryPath, pid, pid);
Why dup the writing? You're testing (and overly coupling your tests to)
implementation.

You should have a reader and a writer. And the public contract of the two is
that what I write with the Writer can be read from the Reader. Your tests
shouldn't concern themselves with how that data is stored under the hood.

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win_unittest.cc:124:
provider.ProvideStabilityMetrics(NULL);
Testing that the histograms are actually written is good, but it's going to be
inconvenient to get really thorough coverage of your reading code if that's the
only way to do it.

Take the reading code out of the WatcherMetricsProvider, test the hell out of
it, and then you can easily get high coverage of the WatcherMetricsProvider by
making the simple verification that it does in fact send histograms.

[Sigurður Ásgeirsson, 2014-12-12 16:21:40]

Please take another look?

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
File components/browser_watcher/exit_funnel_win.cc (right):

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/exit_funnel_win.cc:9: #include
"base/strings/string16.h"
On 2014/12/11 15:00:46, erikwright wrote:
> dup from header

Done.

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/exit_funnel_win.cc:12: #include "base/win/registry.h"
On 2014/12/11 15:00:46, erikwright wrote:
> dup from header

Done.

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/exit_funnel_win.cc:37: base::StringAppendF(&key_name,
L"\\%d-%lld", pid,
On 2014/12/11 15:00:46, erikwright wrote:
> Personally I find this harder to read than:
> 
> base::string16 key_name = registry_path + L"\\" + base::UintToString(pid) +
L"-"
> + base::UintToString(base::Time::FromFileTime(...).ToInternalValue();
> 
> But you might reasonably disagree.

Acknowledged.

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
File components/browser_watcher/exit_funnel_win.h (right):

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/exit_funnel_win.h:25: // Records |event_name| at the
current time in registry for the process
On 2014/12/11 15:00:46, erikwright wrote:
> the registry

Done.

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
File components/browser_watcher/exit_funnel_win_unittest.cc (right):

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/exit_funnel_win_unittest.cc:51:
base::StringPrintf(L"%ls\\%ls", kRegistryPath, subkey_name.c_str()));
On 2014/12/11 15:00:46, erikwright wrote:
> It really seems like there should be a single implementation of this reading
> code shared between this test and the production code that actually reads this
> value.

The behavior is subtly different, and in any case there isn't enough code there
to worry about reuse.

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
File components/browser_watcher/watcher_metrics_provider_win.cc (right):

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.cc:120: int64 min_time =
std::numeric_limits<int64>::max();
On 2014/12/11 15:00:46, erikwright wrote:
> Everything in this method needs to be extracted out into something tested.
It's
> probably the thing that the unit test should use.

Maybe so, though the tests also want to verify that the data contract isn't
violated across versions.
In any case, I think it's good enough for what we need it for.

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
File components/browser_watcher/watcher_metrics_provider_win_unittest.cc
(right):

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win_unittest.cc:61:
base::StringPrintf(L"%ls\\%d-%d", kRegistryPath, pid, pid);
On 2014/12/11 15:00:46, erikwright wrote:
> Why dup the writing? You're testing (and overly coupling your tests to)
> implementation.
> 
> You should have a reader and a writer. And the public contract of the two is
> that what I write with the Writer can be read from the Reader. Your tests
> shouldn't concern themselves with how that data is stored under the hood.

There's really not enough code there to worry about reuse. Also, making the
writing reusable for testing means to allow specifying the time written.

https://codereview.chromium.org/792163002/diff/20001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win_unittest.cc:124:
provider.ProvideStabilityMetrics(NULL);
On 2014/12/11 15:00:46, erikwright wrote:
> Testing that the histograms are actually written is good, but it's going to be
> inconvenient to get really thorough coverage of your reading code if that's
the
> only way to do it.
> 
> Take the reading code out of the WatcherMetricsProvider, test the hell out of
> it, and then you can easily get high coverage of the WatcherMetricsProvider by
> making the simple verification that it does in fact send histograms.

I added some testing to verify that the metrics provider can read what the exit
funnel writes. I'd like to ship this as-is, rather than sinking further work
into refining it. This code won't live long, and we only need confidence that
it's not reporting rubbish.

[erikwright (departed), 2014-12-12 16:58:29]

LGTM with a few cleanups.

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
File components/browser_watcher/exit_funnel_win.h (right):

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
components/browser_watcher/exit_funnel_win.h:5: #ifndef
COMPONENTS_BROWSER_WATCHER_END_SESSION_WIN_H_
fix this guard

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
components/browser_watcher/exit_funnel_win.h:40: };
DISALLOW_...

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
File components/browser_watcher/exit_funnel_win_unittest.cc (right):

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
components/browser_watcher/exit_funnel_win_unittest.cc:43:
base::StringToUint(it.Name(), &pid);
Is this relying on parsing the string part until a non-digit character and
ignoring the creation time that follows? If so, please document that.

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
components/browser_watcher/exit_funnel_win_unittest.cc:101:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(500));
Is there any reason to make this much more than the resolution of
base::Time::Now()? I would reduce this to 50ms to reduce the impact on test
execution times.

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
File components/browser_watcher/watcher_metrics_provider_win.cc (right):

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.cc:136: for (size_t i =
0; i < events.size(); ++i) {
Since you are recording the duration from the first event, deleting an earlier
event will change how you report later events, which could corrupt data if
something systematic prevented deletion of some keys.

It would be more technically correct to sort the keys in order of time and
delete in descending order.

[erikwright (departed), 2014-12-12 17:17:19]

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
File components/browser_watcher/watcher_metrics_provider_win.cc (right):

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.cc:136: for (size_t i =
0; i < events.size(); ++i) {
On 2014/12/12 16:58:29, erikwright wrote:
> Since you are recording the duration from the first event, deleting an earlier
> event will change how you report later events, which could corrupt data if
> something systematic prevented deletion of some keys.
> 
> It would be more technically correct to sort the keys in order of time and
> delete in descending order.

This is optional, FYI. Up to you to decide if it compromises your data gathering
enough to warrant addressing.

[Sigurður Ásgeirsson, 2014-12-12 20:21:58]

Patchset #4 (id:80001) has been deleted

[Sigurður Ásgeirsson, 2014-12-12 20:24:37]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2014-12-12 20:25:14]

Thanks! I'm going to see if I can land this before or during the weekend...

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
File components/browser_watcher/exit_funnel_win.h (right):

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
components/browser_watcher/exit_funnel_win.h:5: #ifndef
COMPONENTS_BROWSER_WATCHER_END_SESSION_WIN_H_
On 2014/12/12 16:58:29, erikwright wrote:
> fix this guard

Done.

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
components/browser_watcher/exit_funnel_win.h:40: };
On 2014/12/12 16:58:29, erikwright wrote:
> DISALLOW_...

Ah - thanks.

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
File components/browser_watcher/exit_funnel_win_unittest.cc (right):

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
components/browser_watcher/exit_funnel_win_unittest.cc:43:
base::StringToUint(it.Name(), &pid);
On 2014/12/12 16:58:29, erikwright wrote:
> Is this relying on parsing the string part until a non-digit character and
> ignoring the creation time that follows? If so, please document that.

Done.

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
components/browser_watcher/exit_funnel_win_unittest.cc:101:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(500));
On 2014/12/12 16:58:29, erikwright wrote:
> Is there any reason to make this much more than the resolution of
> base::Time::Now()? I would reduce this to 50ms to reduce the impact on test
> execution times.

Done.

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
File components/browser_watcher/watcher_metrics_provider_win.cc (right):

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.cc:136: for (size_t i =
0; i < events.size(); ++i) {
On 2014/12/12 16:58:29, erikwright wrote:
> Since you are recording the duration from the first event, deleting an earlier
> event will change how you report later events, which could corrupt data if
> something systematic prevented deletion of some keys.
> 
> It would be more technically correct to sort the keys in order of time and
> delete in descending order.

Acknowledged.

https://codereview.chromium.org/792163002/diff/60001/components/browser_watch...
components/browser_watcher/watcher_metrics_provider_win.cc:136: for (size_t i =
0; i < events.size(); ++i) {
On 2014/12/12 17:17:19, erikwright wrote:
> On 2014/12/12 16:58:29, erikwright wrote:
> > Since you are recording the duration from the first event, deleting an
earlier
> > event will change how you report later events, which could corrupt data if
> > something systematic prevented deletion of some keys.
> > 
> > It would be more technically correct to sort the keys in order of time and
> > delete in descending order.
> 
> This is optional, FYI. Up to you to decide if it compromises your data
gathering
> enough to warrant addressing.

Thanks - I figure failure to delete a value will be persistent, so it's gonna
come out in the wash...

[commit-bot: I haz the power, 2014-12-12 20:25:56]

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

[commit-bot: I haz the power, 2014-12-12 22:41:56]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2014-12-12 22:42:47]

Patchset 4 (id:??) landed as
https://crrev.com/deaea6e5790c7e11f179b55aafda79e4b221ecb4
Cr-Commit-Position: refs/heads/master@{#308184}