Add Net.ProxyScriptFetchSuccessDuration UMA to record how long PAC script fetches take.

Add Net.ProxyScriptFetchSuccessDuration UMA to record how long PAC script fetches take.

Currently PAC script fetches time out after 5 minutes. It's possible that this was a much higher value than needed.

This limit is also problematic in certain cases. For example, if a PAC script is retrieved over HTTPS, and the computer recently joined a network which has a captive portal that blackholes all HTTPS traffic then that user will be unable to sign in to the captive portal with Chrome while waiting for the limit to be hit.

BUG=251682
Committed: https://crrev.com/27d2e5e5e2eb429943a27b4c23cf120733ea39a7
Cr-Commit-Position: refs/heads/master@{#338314}

[cbentzel, 2015-07-09 20:43:38]

cbentzel@chromium.org changed reviewers:
+ ellyjones@chromium.org, eroman@chromium.org

[cbentzel, 2015-07-09 20:43:39]

I also considered measuring this in the ProxyScriptDecider, but kept it here.
This will not include the DHCP proxy fetches.

[cbentzel, 2015-07-09 23:18:07]

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

[commit-bot: I haz the power, 2015-07-09 23:20:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224343002/1

[commit-bot: I haz the power, 2015-07-09 23:52:04]

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

[commit-bot: I haz the power, 2015-07-09 23:52:05]

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

(exceeded global retry quota)

[cbentzel, 2015-07-10 13:16:13]

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

[commit-bot: I haz the power, 2015-07-10 13:16:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224343002/20001

[commit-bot: I haz the power, 2015-07-10 13:44:52]

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

[commit-bot: I haz the power, 2015-07-10 13:44:53]

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

(exceeded global retry quota)

[Elly Fong-Jones, 2015-07-10 15:28:12]

lgtm

[cbentzel, 2015-07-10 16:17:36]

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

[cbentzel, 2015-07-10 16:17:37]

+asvitkine for histograms.xml

[cbentzel, 2015-07-10 16:17:38]

+asvitkine for histograms.xml

[cbentzel, 2015-07-10 16:21:37]

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

[commit-bot: I haz the power, 2015-07-10 16:21:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1224343002/40001

[Alexei Svitkine (slow), 2015-07-10 16:22:35]

lgtm

https://codereview.chromium.org/1224343002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1224343002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:21768: +<histogram
name="Net.ProxyScriptFetchSuccessDuration" units="milliseconds">
Nit: Optional suggestion:

Name it: Net.ProxyScript.FetchSuccessDuration

Makes a bit more readable and lets you later add more metrics related to proxy
script while keeping a structure. (If you make the change, make sure to change
both the code and the xml).

[cbentzel, 2015-07-10 16:25:23]

On 2015/07/10 16:22:35, Alexei Svitkine wrote:
> lgtm
> 
>
https://codereview.chromium.org/1224343002/diff/40001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1224343002/diff/40001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:21768: +<histogram
> name="Net.ProxyScriptFetchSuccessDuration" units="milliseconds">
> Nit: Optional suggestion:
> 
> Name it: Net.ProxyScript.FetchSuccessDuration
> 
> Makes a bit more readable and lets you later add more metrics related to proxy
> script while keeping a structure. (If you make the change, make sure to change
> both the code and the xml).

Yes, this is consistent with other histogram names in this area. But went with
Net.ProxySriptFetcher.SuccessDuration since this is really a ProxyScriptFetcher
related histogram.

[cbentzel, 2015-07-10 16:27:48]

The CQ bit was checked by cbentzel@chromium.org

[cbentzel, 2015-07-10 16:27:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
ellyjones@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1224343002/#ps60001
(title: "Rename UMA to Net.ProxyScriptFetcher.SuccessDuration")

[commit-bot: I haz the power, 2015-07-10 16:28:31]

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

[commit-bot: I haz the power, 2015-07-10 17:40:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-10 17:41:15]

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

[eroman, 2015-07-13 21:39:05]

lgtm

https://codereview.chromium.org/1224343002/diff/60001/net/proxy/proxy_script_...
File net/proxy/proxy_script_fetcher_impl.cc (right):

https://codereview.chromium.org/1224343002/diff/60001/net/proxy/proxy_script_...
net/proxy/proxy_script_fetcher_impl.cc:138: fetch_start_time_ =
base::Time::Now();
I suggest using base::TimeTicks over base::Time for measuring elapsed times.

base::Time is influenced from changing the system clock, so it is more likely to
measure walltime deltas incorrectly.

While base::TimeTicks is better in this regard, it has inconsistencies with how
time lapses measured across system sleep/suspend work, because unfortunately
last I looked we didn't use a consistent monotonic clock across platforms (ticks
will stand still on some platforms, but increase on others while sleeping).

... Even so, that is not likely to impact this histogram since fetches
interrupted by a suspend/sleep will almost surely fail (either aborted or
network error).

https://codereview.chromium.org/1224343002/diff/60001/net/proxy/proxy_script_...
net/proxy/proxy_script_fetcher_impl.cc:291:
UMA_HISTOGRAM_TIMES("Net.ProxyScriptFetcher.SuccessDuration",
I see you considered putting it in ProxyScriptDecider. This should be fine to
start getting some data.

Note that an advantage of ProxyScriptDecider is we might be rejecting slow
successful fetches in ProxyScriptDecider::DoVerifyPacScript().

For instance a hypothetical situation would be a captive portal that makes the
proxy script fetch really slow, however it still completes with an HTTP 200
code. The page served up however is an HTML page rather than a PAC script, so
DoVerifyPacScript() subsequently fails and we fallback to not using the script.

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

https://codereview.chromium.org/1224343002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:21772: +    time spent doing proxy
auto-discovery, or failed attempts at retrieiving PAC
typo: retrieiving --> retrieving

https://codereview.chromium.org/1224343002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:21773: +    scripts.
Additionally, this does not include the time spent for loading data: URLs.
However it _does_ include the time spent for file:// URLs