NQE: Add accuracy histogram for external estimate provider

NQE: Add accuracy histogram for external estimate provider

BUG=627647
Committed: https://crrev.com/4a0414454caede887f31926179c28eaa4f935dba
Cr-Commit-Position: refs/heads/master@{#407623}

[tbansal1, 2016-07-12 23:22:39]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-07-12 23:25:29]

Description was changed from

==========
NQE: Add accuracy histogram for external estimate provider

BUG=
==========

to

==========
NQE: Add accuracy histogram for external estimate provider

BUG=627647
==========

[tbansal1, 2016-07-12 23:28:10]

tbansal@chromium.org changed reviewers:
+ bengr@chromium.org

[tbansal1, 2016-07-12 23:28:11]

bengr: ptal. Thanks.

[bengr, 2016-07-20 23:51:28]

https://codereview.chromium.org/2145613003/diff/20001/net/nqe/network_quality...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2145613003/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:666: base::HistogramBase* histogram =
base::Histogram::FactoryGet(
You should have tests for histograms.

https://codereview.chromium.org/2145613003/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:677: // Add histogram for accuracy of
external estimate provider.
for -> to evaluate the 
of -> of the

https://codereview.chromium.org/2145613003/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:686: estimated_observed_diff_milliseconds
>= 0 ? "Positive." : "Negative.";
You should probably create a helper called GetSignSuffix() and use it
everywhere.

https://codereview.chromium.org/2145613003/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:688: base::HistogramBase* histogram =
base::Histogram::FactoryGet(
You get similar histograms all over this method. Consider refactoring into a
helper.

https://codereview.chromium.org/2145613003/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:689:
"NQE.ExternalEstimateProvider.RTT.Accuracy."
Here and everywhere in this method, consider using StringPrintf.

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

https://codereview.chromium.org/2145613003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:35581: +    Records the difference
between the HTTP RTT estimated by external estimate
by -> by the

https://codereview.chromium.org/2145613003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:35582: +    provider and the observed
HTTP RTT. HTTP RTT estimated by the external
HTTP RTT -> The HTTP RTT

[tbansal1, 2016-07-21 18:48:26]

Patchset #2 (id:40001) has been deleted

[tbansal1, 2016-07-21 21:42:21]

Patchset #2 (id:60001) has been deleted

[tbansal1, 2016-07-21 22:00:36]

Patchset #2 (id:80001) has been deleted

[tbansal1, 2016-07-21 22:00:51]

bengr: ptal. Thanks.

https://codereview.chromium.org/2145613003/diff/20001/net/nqe/network_quality...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2145613003/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:666: base::HistogramBase* histogram =
base::Histogram::FactoryGet(
On 2016/07/20 23:51:28, bengr wrote:
> You should have tests for histograms.
Done. I completely forgot that I already have harness for external estimate
provider in the tests.

https://codereview.chromium.org/2145613003/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:677: // Add histogram for accuracy of
external estimate provider.
On 2016/07/20 23:51:28, bengr wrote:
> for -> to evaluate the 
> of -> of the

Done.

https://codereview.chromium.org/2145613003/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:686: estimated_observed_diff_milliseconds
>= 0 ? "Positive." : "Negative.";
On 2016/07/20 23:51:28, bengr wrote:
> You should probably create a helper called GetSignSuffix() and use it
> everywhere.

Done.

https://codereview.chromium.org/2145613003/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:688: base::HistogramBase* histogram =
base::Histogram::FactoryGet(
On 2016/07/20 23:51:28, bengr wrote:
> You get similar histograms all over this method. Consider refactoring into a
> helper.

Done.

https://codereview.chromium.org/2145613003/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:689:
"NQE.ExternalEstimateProvider.RTT.Accuracy."
On 2016/07/20 23:51:28, bengr wrote:
> Here and everywhere in this method, consider using StringPrintf.

Done.

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

https://codereview.chromium.org/2145613003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:35581: +    Records the difference
between the HTTP RTT estimated by external estimate
On 2016/07/20 23:51:28, bengr wrote:
> by -> by the

Done.

https://codereview.chromium.org/2145613003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:35582: +    provider and the observed
HTTP RTT. HTTP RTT estimated by the external
On 2016/07/20 23:51:28, bengr wrote:
> HTTP RTT -> The HTTP RTT

Done.

[bengr, 2016-07-22 17:32:57]

Lgtm with nit.

https://codereview.chromium.org/2145613003/diff/100001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2145613003/diff/100001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:220: void RecordAccuracyRTT(const char*
prefix,
I would switch the wording of all of these to put "Accuracy" last. E.g.,
"RecordRTTAccuracy"

[tbansal1, 2016-07-22 21:47:45]

tbansal@chromium.org changed reviewers:
+ rkaplow@chromium.org

[tbansal1, 2016-07-22 21:47:46]

rkaplow: ptal at histograms.xml. Thanks.

https://codereview.chromium.org/2145613003/diff/100001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2145613003/diff/100001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:220: void RecordAccuracyRTT(const char*
prefix,
On 2016/07/22 17:32:57, bengr wrote:
> I would switch the wording of all of these to put "Accuracy" last. E.g.,
> "RecordRTTAccuracy"

Done.

[rkaplow, 2016-07-25 18:36:29]

lgtm

[tbansal1, 2016-07-25 20:29:25]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-07-25 20:29:26]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2145613003/#ps160001
(title: "Rebased")

[commit-bot: I haz the power, 2016-07-25 20:30:00]

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, 2016-07-25 20:39:54]

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

[commit-bot: I haz the power, 2016-07-25 20:39:55]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[tbansal1, 2016-07-25 21:50:05]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2016-07-25 21:50:33]

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, 2016-07-25 23:04:13]

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

[commit-bot: I haz the power, 2016-07-25 23:04:23]

Failed to apply the patch.

[commit-bot: I haz the power, 2016-07-25 23:04:51]

Description was changed from

==========
NQE: Add accuracy histogram for external estimate provider

BUG=627647
==========

to

==========
NQE: Add accuracy histogram for external estimate provider

BUG=627647

Committed: https://crrev.com/4a0414454caede887f31926179c28eaa4f935dba
Cr-Commit-Position: refs/heads/master@{#407623}
==========

[commit-bot: I haz the power, 2016-07-25 23:04:52]

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