Add metrics for DHCP WPAD feature.

Add metrics for DHCP WPAD feature. Fix memory overwrite.

BUG=18575
TEST=net_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86421

[Jói, 2011-05-17 20:52:04]

Hi Eric,

Last change to round this out and let it run behind the flag for a few days. 
This one should be an easier review than the last one I hope :D

Cheers,
Jói

[eroman, 2011-05-17 21:23:49]

LGTM.

@jar: could you take a look over the new histograms as well? To give some
context, this is measuring some timings for a new feature (DHCP-based proxy
auto-detect). Thanks.

[Jói, 2011-05-18 18:41:07]

Jim, I'm going to update this before you take a look.

Cheers,
Jói

[jar (doing other things), 2011-05-18 18:59:05]

On 2011/05/18 18:41:07, Jói wrote:
> Jim, I'm going to update this before you take a look.
> 
> Cheers,
> Jói

OK... I will hold off looking.

Please ping me when it is ready.

Thanks,

Jim

[Jói, 2011-05-18 19:52:03]

I've uploaded a new version that is ready for you to take a look.

Cheers,
Jói


On 2011/05/18 18:59:05, jar wrote:
> On 2011/05/18 18:41:07, Jói wrote:
> > Jim, I'm going to update this before you take a look.
> > 
> > Cheers,
> > Jói
> 
> OK... I will hold off looking.
> 
> Please ping me when it is ready.
> 
> Thanks,
> 
> Jim

[eroman%chromium.org, 2011-05-18 20:33:27]

lgtm

http://codereview.chromium.org/6975027/diff/7006/net/proxy/dhcp_proxy_script_...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/6975027/diff/7006/net/proxy/dhcp_proxy_script_...
net/proxy/dhcp_proxy_script_fetcher_win.cc:50: unsigned long
EnsureGetAdaptersAddressesError(unsigned long error_code) {
It shouldn't be necessary to map unexpected errorst o ERROR_GEN_FAILURE, since
in your enumeration list you have already made the buckets of size 1. If the
error falls outside of the expected ranges, we will see it regardless (just
won't know it's exact value.)

http://codereview.chromium.org/6975027/diff/7006/net/proxy/dhcp_proxy_script_...
net/proxy/dhcp_proxy_script_fetcher_win.cc:134:
UMA_HISTOGRAM_TIMES("Net.DhcpWpadCancelTime",
I am not sure how useful the cancelation info will be, but ok.

[Jói, 2011-05-19 14:39:49]

I've uploaded a new version to respond to Eric's review comments.

Jim, please take a look when you have time.  I'm trying to get the feature
enabled by default with metrics enabled as soon as possible, so that we have
enough data before the M13 branch point whether to turn it off for the branch or
not.

Cheers,
Jói

http://codereview.chromium.org/6975027/diff/7006/net/proxy/dhcp_proxy_script_...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/6975027/diff/7006/net/proxy/dhcp_proxy_script_...
net/proxy/dhcp_proxy_script_fetcher_win.cc:50: unsigned long
EnsureGetAdaptersAddressesError(unsigned long error_code) {
On 2011/05/18 20:33:27, eroman%chromium.org wrote:
> It shouldn't be necessary to map unexpected errorst o ERROR_GEN_FAILURE, since
> in your enumeration list you have already made the buckets of size 1. If the
> error falls outside of the expected ranges, we will see it regardless (just
> won't know it's exact value.)

Ah, I misunderstood how the UMA enumerations worked. Fixed that.

http://codereview.chromium.org/6975027/diff/7006/net/proxy/dhcp_proxy_script_...
net/proxy/dhcp_proxy_script_fetcher_win.cc:134:
UMA_HISTOGRAM_TIMES("Net.DhcpWpadCancelTime",
On 2011/05/18 20:33:27, eroman%chromium.org wrote:
> I am not sure how useful the cancelation info will be, but ok.

Yeah - once I start seeing the stats I'll remove things that aren't providing
extra information, but would like to have more for now rather than less.

[jar (doing other things), 2011-05-19 23:25:38]

I suspect common code could help handle the two comments below very efficiently.
 The macros should be used to generate simple C style arrays (in both cases),
and then a helper function should turn the C style array into a vector, with the
extra cutoffs added (given a C style array and its array_size).

If you want, you could push that helper function into the histograms.* file, as
I suspect it will be useful for other folks.

Other than that, the general use of histograms looks fine.

Thanks.

http://codereview.chromium.org/6975027/diff/9002/net/base/net_errors.cc
File net/base/net_errors.cc (right):

http://codereview.chromium.org/6975027/diff/9002/net/base/net_errors.cc#newco...
net/base/net_errors.cc:31: #define NET_ERROR(label, value)
all_error_codes.push_back(-(value));
Although this was copied....  it might be better to push two cutoffs per error
code, so that IF there is an intermediate value, it is not accidentally lumped
with the previous bucket.  Note that the underlying HISTOGRAM will get rid of
dups, so you don't need to worry about that eventuality.  The code might then
look like:

#define NET_ERROR(laber, value) all_error_codes.push_back(-(value)) ; \
all_error_codes.push_back(1 - (value));

IF you do make this change, you may want to rename any existing users of this
list so that they don't conflate the existing uploads (though you can always
split them out by version).

http://codereview.chromium.org/6975027/diff/9002/net/proxy/dhcp_proxy_script_...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/6975027/diff/9002/net/proxy/dhcp_proxy_script_...
net/proxy/dhcp_proxy_script_fetcher_win.cc:39: errors.push_back(error_code + 1);
This approach of translating a C style array to a vector is IMO very good.  The
other approach may be notably more expensive (using a push_back for each item in
the macro list).  You might think of migrating to this approach consistently.

[Jói, 2011-05-20 16:56:23]

Hi Jim,

If it's alright by you, I will add the function you suggested, and use
it in my own code, but leave changing the enumeration in
resource_dispatcher_host.cc for a separate change that I'll send to
appropriate reviewers, since it will mean a change in the existing
histogram there (at least its semantics, and maybe as you suggested,
its name).  This will go at the top of my to-do list, just want to not
block on that approval as I'm trying to get the DHCP WPAD metrics
landed as soon as possible so I can collect them for a while before
the M13 branch point.

I'll upload a new version shortly.

Cheers,
Jói



On Thu, May 19, 2011 at 7:25 PM,  <jar@chromium.org> wrote:
> I suspect common code could help handle the two comments below very
> efficiently.
>  The macros should be used to generate simple C style arrays (in both
> cases),
> and then a helper function should turn the C style array into a vector, with
> the
> extra cutoffs added (given a C style array and its array_size).
>
> If you want, you could push that helper function into the histograms.* file,
> as
> I suspect it will be useful for other folks.
>
> Other than that, the general use of histograms looks fine.
>
> Thanks.
>
>
> http://codereview.chromium.org/6975027/diff/9002/net/base/net_errors.cc
> File net/base/net_errors.cc (right):
>
>
http://codereview.chromium.org/6975027/diff/9002/net/base/net_errors.cc#newco...
> net/base/net_errors.cc:31: #define NET_ERROR(label, value)
> all_error_codes.push_back(-(value));
> Although this was copied....  it might be better to push two cutoffs per
> error code, so that IF there is an intermediate value, it is not
> accidentally lumped with the previous bucket.  Note that the underlying
> HISTOGRAM will get rid of dups, so you don't need to worry about that
> eventuality.  The code might then look like:
>
> #define NET_ERROR(laber, value) all_error_codes.push_back(-(value)) ; \
> all_error_codes.push_back(1 - (value));
>
> IF you do make this change, you may want to rename any existing users of
> this list so that they don't conflate the existing uploads (though you
> can always split them out by version).
>
>
http://codereview.chromium.org/6975027/diff/9002/net/proxy/dhcp_proxy_script_...
> File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):
>
>
http://codereview.chromium.org/6975027/diff/9002/net/proxy/dhcp_proxy_script_...
> net/proxy/dhcp_proxy_script_fetcher_win.cc:39:
> errors.push_back(error_code + 1);
> This approach of translating a C style array to a vector is IMO very
> good.  The other approach may be notably more expensive (using a
> push_back for each item in the macro list).  You might think of
> migrating to this approach consistently.
>
> http://codereview.chromium.org/6975027/
>

[Jói, 2011-05-20 18:24:45]

Hi Jim,

I've responded to your comments.  I also fixed a memory overwrite that I found
while testing these changes (PageHeap for the win!).

I will follow up with a change to switch other users of CUSTOM_ENUMERATION over
to using the new CustomHistogram::ArrayToCustomRanges function as appropriate
(it looks like there are more than just the resource_dispatcher_host.cc one).

Cheers,
Jói

[jar (doing other things), 2011-05-23 19:37:52]

http://codereview.chromium.org/6975027/diff/15001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/6975027/diff/15001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:89: void
DhcpProxyScriptFetcherWin::CancelImpl(bool user_initiated) {
Please switch to using an enumerated type, rather than a boolean here to
increase readability.  I found it very confusing to read CancelImpl(false), and
thought it was saying we don't really want to cancel :-/, and so I was forced to
read header files (or implementation) to be able to read the code.

http://codereview.chromium.org/6975027/diff/15001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.h (right):

http://codereview.chromium.org/6975027/diff/15001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:119: base::TimeTicks
fetch_start_time_;
nit: Data members should be private, and accessors and mutators can be public or
protected.

I'm surprised to see a pile of such declarations here... and that should be
changed sometime... but you probably shouldn't add to the violations (the
corrected style is very helpful in debugging).

[Jói, 2011-05-24 00:58:38]

Thanks for the review Jim. I think I've fixed what you asked for (readability
and making the new member private), and I've added further cleanup of scope
declarations to my TODO list.  Hope that covers it.

Cheers,
Jói

http://codereview.chromium.org/6975027/diff/15001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/6975027/diff/15001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:89: void
DhcpProxyScriptFetcherWin::CancelImpl(bool user_initiated) {
On 2011/05/23 19:37:52, jar wrote:
> Please switch to using an enumerated type, rather than a boolean here to
> increase readability.  I found it very confusing to read CancelImpl(false),
and
> thought it was saying we don't really want to cancel :-/, and so I was forced
to
> read header files (or implementation) to be able to read the code.

I don't think an enumerated type would be warranted since this is not a public
interface.  However, I agree with the spirit of the comment, that as is, the
"CancelImpl" name gives you no clue as to what the boolean argument does.  I
have split this up now so that the Cancel() function does the UMA bit, and
CancelImpl takes no parameters.  Hope that clears it up.

http://codereview.chromium.org/6975027/diff/15001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.h (right):

http://codereview.chromium.org/6975027/diff/15001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:119: base::TimeTicks
fetch_start_time_;
On 2011/05/23 19:37:52, jar wrote:
> nit: Data members should be private, and accessors and mutators can be public
or
> protected.
> 
> I'm surprised to see a pile of such declarations here... and that should be
> changed sometime... but you probably shouldn't add to the violations (the
> corrected style is very helpful in debugging).

I've made this member private (as well as the implicit constructors). I've also
added a TODO in this file for me to clean this up better, and added that close
to the top of my TODO list (right after the item about moving other users of
UMA_HISTOGRAM_CUSTOM_ENUMERATION that are transforming from a static array of
values to using the new helper function).

[jar (doing other things), 2011-05-24 01:12:03]

LGTM