Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2020353002:
Record NQE accuracy at main frame requests (Closed)
DescriptionRecord NQE accuracy at main frame requests
At each main frame request, NQE estimates are preserved
locally. Then, the observed network quality is computed
from the observations received in the next 15, 30 and 60
seconds. The preserved estimates are compared against the
observed estimates, and the computed accuracy is recorded
using UMA.
Design doc here: https://docs.google.com/document/d/18dA6DmUdh8DxwWutR-hFcd2K4yVJvqDIot6291etf6E/view
BUG=615551
Committed: https://crrev.com/599e0cae220ae619be4039ef8cbb45069e188038
Cr-Commit-Position: refs/heads/master@{#398428}
Patch Set 1 : #
Total comments: 16
Patch Set 2 : Addressed bengr comments #
Total comments: 4
Patch Set 3 : Addressed asvitkine comments #
Total comments: 2
Patch Set 4 : Rebased #Patch Set 5 : Addressed asvitkine comments #
Messages
Total messages: 35 (20 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Record NQE accuracy BUG= ========== to ========== Record NQE accuracy at main frame requests At each main frame request, NQE estimates are preserved locally. Then, the observed network quality is computed from the observations received in the next 15, 30 and 60 seconds. The preserved estimates are compared against the observed estimates, and the computed accuracy is recorded using UMA. Design doc here: https://docs.google.com/document/d/18dA6DmUdh8DxwWutR-hFcd2K4yVJvqDIot6291etf... BUG=615551 ==========
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. Thanks.
https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:183: return "5100_Infinity"; How about: const char* const kSuffixes[] = { "0_20", "20_60", ...} for (int i = 0; i < arraylen(kSuffixes) - 1; ++i) { if (rtt_milliseconds <= 20 * 2^i) return kSuffixes[i]; return kSuffixes[arraylen(kSuffixes) - 1]; Alternatively, you could use StringPrintf("%d_%d", prev, curr) Also, should the second interval be 21_60? Do these suffixes need to be in sync with histograms.xml. Say so if so. https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:516: // Post the tasks which will run in future and record the estimation in -> in the https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:518: // task execution. Say why you post instead of executing inline. https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:194: virtual bool GetTransportRTTEstimate(base::TimeDelta* rtt) const Add to the comment that this is virtualized for testing. https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:230: virtual bool GetRecentTransportRTTMedian(const base::TimeTicks& start_time, Add to the comment that this is virtualized for testing. https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:314: // Virtualized for testing. Returns the list of intervals at which accuracy of nits: which -> which the Move the first sentence to the end. https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:540: nqe::internal::NetworkQuality estimated_quality_main_frame_; estimated_quality_at_last_main_frame_request_? or estimated_quality_at_last_main_frame_? https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:167: bool GetTransportRTTEstimate(base::TimeDelta* rtt) const override { Add comments saying what you're overriding.
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
bengr: ptal. https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:183: return "5100_Infinity"; On 2016/06/01 16:55:29, bengr wrote: > How about: > const char* const kSuffixes[] = { "0_20", "20_60", ...} > for (int i = 0; i < arraylen(kSuffixes) - 1; ++i) { > if (rtt_milliseconds <= 20 * 2^i) > return kSuffixes[i]; > return kSuffixes[arraylen(kSuffixes) - 1]; Done. > > > Alternatively, you could use StringPrintf("%d_%d", prev, curr) I think it is easier to understand the distribution of buckets if the strings are explicit. > > > Also, should the second interval be 21_60? May be but that might be confusing too. e.g., it is not clear which histogram should be used if actual_rtt is 20.5 milliseconds. I suppose it does not matter, but if you feel strongly, I can change. > > Do these suffixes need to be in sync with histograms.xml. Say so if so. Done. https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:516: // Post the tasks which will run in future and record the estimation On 2016/06/01 16:55:29, bengr wrote: > in -> in the Done. https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:518: // task execution. On 2016/06/01 16:55:29, bengr wrote: > Say why you post instead of executing inline. Done. https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:194: virtual bool GetTransportRTTEstimate(base::TimeDelta* rtt) const On 2016/06/01 16:55:29, bengr wrote: > Add to the comment that this is virtualized for testing. Done. https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:230: virtual bool GetRecentTransportRTTMedian(const base::TimeTicks& start_time, On 2016/06/01 16:55:29, bengr wrote: > Add to the comment that this is virtualized for testing. Done. https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:314: // Virtualized for testing. Returns the list of intervals at which accuracy of On 2016/06/01 16:55:30, bengr wrote: > nits: > which -> which the > Move the first sentence to the end. Done. https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:540: nqe::internal::NetworkQuality estimated_quality_main_frame_; On 2016/06/01 16:55:29, bengr wrote: > estimated_quality_at_last_main_frame_request_? or > estimated_quality_at_last_main_frame_? Done. https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2020353002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:167: bool GetTransportRTTEstimate(base::TimeDelta* rtt) const override { On 2016/06/01 16:55:30, bengr wrote: > Add comments saying what you're overriding. Done.
lgtm
tbansal@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine: PTAL at histograms.xml.
https://codereview.chromium.org/2020353002/diff/160001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2020353002/diff/160001/net/nqe/network_qualit... net/nqe/network_quality_estimator_unittest.cc:183: } Nit: bad indent? https://codereview.chromium.org/2020353002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2020353002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:34491: + Records the difference between the observed HTTP RTT and the actual HTTP I find these histogram names super confusing - and the descriptions also confuse me. :\ In the names, you have the terms "estimated" and "actual". But in the descriptions you have "observed", "actual", "estimated". Can we avoid these inconsistencies? i.e. have the names correspond more closely to the descriptions? The description also use the word "predicted" - is this different as well from estimated, or are some of these synonyms? What's the difference between observed and actual? For example, if a histogram is measuring the difference between two things, maybe that's how it should be called? e.g. DiffActualVsEstimate? Looking at your code, maybe you're just using the two names because histograms don't support logging negatives. If so, maybe it would be clearer to just have suffixes for the same histogram name? e.g. .Positive vs. .Negative?
Patchset #3 (id:180001) has been deleted
Patchset #3 (id:200001) has been deleted
asvitkine: ptal. Thanks. https://codereview.chromium.org/2020353002/diff/160001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2020353002/diff/160001/net/nqe/network_qualit... net/nqe/network_quality_estimator_unittest.cc:183: } On 2016/06/03 20:01:07, Alexei Svitkine (slow) wrote: > Nit: bad indent? Done, not sure why git cl format did not catch that. https://codereview.chromium.org/2020353002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2020353002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:34491: + Records the difference between the observed HTTP RTT and the actual HTTP On 2016/06/03 20:01:07, Alexei Svitkine (slow) wrote: > I find these histogram names super confusing - and the descriptions also confuse > me. :\ > > In the names, you have the terms "estimated" and "actual". But in the > descriptions you have "observed", "actual", "estimated". Fixed it to use "estimated" and "observed" every where. There is one function (not touched by this CL) that uses "actual" but that function and the corresponding histograms will go away in the next CL. > > Can we avoid these inconsistencies? i.e. have the names correspond more closely > to the descriptions? The description also use the word "predicted" - is this > different as well from estimated, or are some of these synonyms? What's the > difference between observed and actual? observed = actual estimated = predicted But, now it should all be "estimated" and "observed". > > For example, if a histogram is measuring the difference between two things, > maybe that's how it should be called? e.g. DiffActualVsEstimate? > > Looking at your code, maybe you're just using the two names because histograms > don't support logging negatives. If so, maybe it would be clearer to just have > suffixes for the same histogram name? e.g. .Positive vs. .Negative? Using the suffix now.
Patchset #3 (id:220001) has been deleted
Patchset #3 (id:240001) has been deleted
lgtm https://codereview.chromium.org/2020353002/diff/260001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2020353002/diff/260001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:561: const base::TimeDelta& measuring_duration) const { Nit: I think TimeDeltas should be passed by value. Check the header comment.
https://codereview.chromium.org/2020353002/diff/260001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2020353002/diff/260001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:561: const base::TimeDelta& measuring_duration) const { On 2016/06/06 15:11:55, Alexei Svitkine (slow) wrote: > Nit: I think TimeDeltas should be passed by value. Check the header comment. Done.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2020353002/#ps300001 (title: "Addressed asvitkine comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020353002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tbansal@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2020353002/300001
Message was sent while issue was closed.
Description was changed from ========== Record NQE accuracy at main frame requests At each main frame request, NQE estimates are preserved locally. Then, the observed network quality is computed from the observations received in the next 15, 30 and 60 seconds. The preserved estimates are compared against the observed estimates, and the computed accuracy is recorded using UMA. Design doc here: https://docs.google.com/document/d/18dA6DmUdh8DxwWutR-hFcd2K4yVJvqDIot6291etf... BUG=615551 ========== to ========== Record NQE accuracy at main frame requests At each main frame request, NQE estimates are preserved locally. Then, the observed network quality is computed from the observations received in the next 15, 30 and 60 seconds. The preserved estimates are compared against the observed estimates, and the computed accuracy is recorded using UMA. Design doc here: https://docs.google.com/document/d/18dA6DmUdh8DxwWutR-hFcd2K4yVJvqDIot6291etf... BUG=615551 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Record NQE accuracy at main frame requests At each main frame request, NQE estimates are preserved locally. Then, the observed network quality is computed from the observations received in the next 15, 30 and 60 seconds. The preserved estimates are compared against the observed estimates, and the computed accuracy is recorded using UMA. Design doc here: https://docs.google.com/document/d/18dA6DmUdh8DxwWutR-hFcd2K4yVJvqDIot6291etf... BUG=615551 ========== to ========== Record NQE accuracy at main frame requests At each main frame request, NQE estimates are preserved locally. Then, the observed network quality is computed from the observations received in the next 15, 30 and 60 seconds. The preserved estimates are compared against the observed estimates, and the computed accuracy is recorded using UMA. Design doc here: https://docs.google.com/document/d/18dA6DmUdh8DxwWutR-hFcd2K4yVJvqDIot6291etf... BUG=615551 Committed: https://crrev.com/599e0cae220ae619be4039ef8cbb45069e188038 Cr-Commit-Position: refs/heads/master@{#398428} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/599e0cae220ae619be4039ef8cbb45069e188038 Cr-Commit-Position: refs/heads/master@{#398428} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
