Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2107243003:
NQE: Record correlation metric in UMA (Closed)
DescriptionNQE: Record correlation metric in UMA
The 32-bit metric is recorded as a sparse histogram. Each
sample contains information on transport RTT, HTTP RTT,
bandwidth, and time taken to download the resource. Each
metric occupies 7 bits with first 4 bits left unset.
BUG=624597
Committed: https://crrev.com/07a33c8ce604eeec637abe035908a19f1772d797
Cr-Commit-Position: refs/heads/master@{#407072}
Patch Set 1 #
Total comments: 29
Patch Set 2 : Addressed kundaji's comments #
Total comments: 24
Patch Set 3 : Rebased #Patch Set 4 : Addressed bengr comments #
Total comments: 2
Patch Set 5 : Rebased #Patch Set 6 : Addressed bengr comments #Patch Set 7 : Add resource size #
Total comments: 8
Patch Set 8 : Addressed Ilya's comments #
Total comments: 4
Patch Set 9 : Addressed Ilya's comments #Patch Set 10 : Rebased #Patch Set 11 : Added more details to histogram description #
Messages
Total messages: 46 (23 generated)
Description was changed from ========== Record correlation metric in UMA BUG= ========== to ========== Record correlation metric in UMA BUG=624597 ==========
Description was changed from ========== Record correlation metric in UMA BUG=624597 ========== to ========== Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 8 bits. BUG=624597 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 8 bits. BUG=624597 ========== to ========== Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 7 bits. BUG=624597 ==========
Patchset #1 (id:40001) has been deleted
tbansal@chromium.org changed reviewers: + kundaji@chromium.org
kundaji: ptal. Thanks.
Description was changed from ========== Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 7 bits. BUG=624597 ========== to ========== Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 7 bits with first 4 bits as unset. BUG=624597 ==========
Description was changed from ========== Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 7 bits with first 4 bits as unset. BUG=624597 ========== to ========== Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 7 bits with first 4 bits are left unset. BUG=624597 ==========
Description was changed from ========== Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 7 bits with first 4 bits are left unset. BUG=624597 ========== to ========== Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 7 bits with first 4 bits left unset. BUG=624597 ==========
https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:214: // then it will be rounded down. This is confusing. Can you please rephrase it. How about: If trimmed metric value is greater than what can be fit in kBitsPerMetric bits, then the largest value that can be represented in kBitsPerMetric bits is returned. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:216: const int32_t kBitsPerMetric = 7; Add: static_assert(31 > kBitsPerMetric * 4, "4 metrics are being added to 32-bit int with first bit reserved for sign") https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:220: int32_t TrimAndRoundDown(int32_t metric) { Rename method to FitInKBitsPerMetricBits? https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:226: metric = (1 << kBitsPerMetric) - 1; Possible Refactor: int32_t largest_value_using_kBitsPerMetric_bits = (1 << kBitsPerMetric) - 1" Then use this variable in the if check above as well. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:645: if (base::RandDouble() >= correlation_logging_probability_) Shouldn't this be base::RandDouble() <= correlation_logging_probability_? https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:654: // did not go over the network. Could this be replaced with request->response_info().network_accessed? https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:666: return; Nit: Add new line. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:669: if (now - last_main_frame_request_ > base::TimeDelta::FromSeconds(15)) Why ignore older requests? https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:672: DCHECK_GE(now, load_timing_info.send_start); Not sure this is necessary. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:676: DCHECK_GE(31, kBitsPerMetric * 4); Prefer replacing with static assert above where kBitsPerMetric is defined. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:678: const int32_t transport_rtt_milliseconds = Does it help to extract estimated_quality_at_last_main_frame_.transport_rtt() to a temp variable? Say: "transport_rtt = estimated_quality_at_last_main_frame_.transport_rtt();" ? https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:708: int32_t sample = transport_rtt_milliseconds; Suggest adding DCHECK_EQ(0, transport_rtt_milliseconds | http_rtt_milliseconds | .. >> kBitsPerMetric). https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:709: sample = (sample << kBitsPerMetric) + http_rtt_milliseconds; Minor: | seems prefereable to +.
kundaji: ptal. Thanks. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:214: // then it will be rounded down. On 2016/06/30 17:49:44, kundaji wrote: > This is confusing. Can you please rephrase it. How about: If trimmed metric > value is greater than what can be fit in kBitsPerMetric bits, then the largest > value that can be represented in kBitsPerMetric bits is returned. Done. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:216: const int32_t kBitsPerMetric = 7; On 2016/06/30 17:49:44, kundaji wrote: > Add: > static_assert(31 > kBitsPerMetric * 4, "4 metrics are being added to 32-bit int > with first bit reserved for sign") Done. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:220: int32_t TrimAndRoundDown(int32_t metric) { On 2016/06/30 17:49:44, kundaji wrote: > Rename method to FitInKBitsPerMetricBits? Done. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:226: metric = (1 << kBitsPerMetric) - 1; On 2016/06/30 17:49:44, kundaji wrote: > Possible Refactor: > int32_t largest_value_using_kBitsPerMetric_bits = > (1 << kBitsPerMetric) - 1" > > Then use this variable in the if check above as well. Done. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:645: if (base::RandDouble() >= correlation_logging_probability_) On 2016/06/30 17:49:44, kundaji wrote: > Shouldn't this be base::RandDouble() <= correlation_logging_probability_? No. Say correlation_logging_probability_ is 1.0. Then, we never want to return. So, we always want the if conditional to be false. Since RandDouble returns double between [0.0, 1.0), we want to check if RandDouble < correlation_logging_probability_ https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:654: // did not go over the network. On 2016/06/30 17:49:44, kundaji wrote: > Could this be replaced with request->response_info().network_accessed? Done. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:666: return; On 2016/06/30 17:49:44, kundaji wrote: > Nit: Add new line. Done. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:669: if (now - last_main_frame_request_ > base::TimeDelta::FromSeconds(15)) On 2016/06/30 17:49:44, kundaji wrote: > Why ignore older requests? It is possible that network quality was recorded at t=0. Then, there is some background traffic which keeps going on for long-long time (say after 1-2 hours). There is no point of recording correlation for that traffic because network quality was not recently checked. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:672: DCHECK_GE(now, load_timing_info.send_start); On 2016/06/30 17:49:44, kundaji wrote: > Not sure this is necessary. It checks that we are not adding negative values to the histogram. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:676: DCHECK_GE(31, kBitsPerMetric * 4); On 2016/06/30 17:49:44, kundaji wrote: > Prefer replacing with static assert above where kBitsPerMetric is defined. Done. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:678: const int32_t transport_rtt_milliseconds = On 2016/06/30 17:49:44, kundaji wrote: > Does it help to extract estimated_quality_at_last_main_frame_.transport_rtt() to > a temp variable? Say: > "transport_rtt = estimated_quality_at_last_main_frame_.transport_rtt();" ? I am not sure. I wanted to declare it as a const variable. But, it may have reduced the readability slightly. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:708: int32_t sample = transport_rtt_milliseconds; On 2016/06/30 17:49:44, kundaji wrote: > Suggest adding DCHECK_EQ(0, transport_rtt_milliseconds | http_rtt_milliseconds | > .. >> kBitsPerMetric). Done. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:709: sample = (sample << kBitsPerMetric) + http_rtt_milliseconds; On 2016/06/30 17:49:44, kundaji wrote: > Minor: | seems prefereable to +. Done.
Description was changed from ========== Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 7 bits with first 4 bits left unset. BUG=624597 ========== to ========== NQE: Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 7 bits with first 4 bits left unset. BUG=624597 ==========
lgtm https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:645: if (base::RandDouble() >= correlation_logging_probability_) On 2016/07/01 00:16:00, tbansal1 wrote: > On 2016/06/30 17:49:44, kundaji wrote: > > Shouldn't this be base::RandDouble() <= correlation_logging_probability_? > > No. Say correlation_logging_probability_ is 1.0. Then, we never want to return. > So, we always want the if conditional to be false. Since RandDouble returns > double between [0.0, 1.0), we want to check if RandDouble < > correlation_logging_probability_ Misread as condition to execute below. You are right. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:669: if (now - last_main_frame_request_ > base::TimeDelta::FromSeconds(15)) On 2016/07/01 00:16:00, tbansal1 wrote: > On 2016/06/30 17:49:44, kundaji wrote: > > Why ignore older requests? > > It is possible that network quality was recorded at t=0. Then, there is some > background traffic which keeps going on for long-long time (say after 1-2 > hours). There is no point of recording correlation for that traffic because > network quality was not recently checked. Acknowledged. https://codereview.chromium.org/2107243003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:672: DCHECK_GE(now, load_timing_info.send_start); On 2016/07/01 00:16:01, tbansal1 wrote: > On 2016/06/30 17:49:44, kundaji wrote: > > Not sure this is necessary. > > It checks that we are not adding negative values to the histogram. Acknowledged. https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:230: static const int32_t kLargestValuePossible = (1 << kBitsPerMetric) - 1; Suggest moving up with the other constants, instead of redefining in every call.
https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:230: static const int32_t kLargestValuePossible = (1 << kBitsPerMetric) - 1; On 2016/07/01 18:26:26, kundaji wrote: > Suggest moving up with the other constants, instead of redefining in every call. As discussed offline, I would prefer to keep the scope restricted to function. Also, there are other examples of similar code: https://cs.chromium.org/search/?q=%5C+%5C+static%5C+const%5C+int%5C+k%5Ba-zA-...
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. Thanks.
https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:150: double GetDoubleValueForVariationParamWithDefaultValue( This really should live somewhere in the variations code. Is that possible? https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:211: // The least significant kTrimBits of the metric will be discarded. If trimmed If -> If the Also, I don't understand why you trim bits off a metric. You should explain this somewhere. https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:223: int32_t FitInKBitsPerMetricBits(int32_t metric) { I don't understand the point of this method. Is this some sort of optimization or some workaround due to some limitation of the UMA API? https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:651: // |correlation_logging_probability_| to reduce overhead. What is the overhead? https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.h:505: // Records correlation metric that can be used for computing the correlation Records -> Records a HTTP -> HTTP-layer transport -> transport-layer What does "time taken to fetch |request|" mean? https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.h:615: // requests. Why is it not recorded for all requests? If this is a performance optimization, say so, and also provide some intuitions as to why performance would suffer with a 1.0 probability. https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.h:616: const double correlation_logging_probability_; rename: correlation_uma_reporting_probability_; https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1761: // Verify that if the metric is larger 2^(kBitsPerMetric + kTrimBits), larger -> larger than https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1764: 1.0, base::TimeDelta::FromSeconds(10), 4095 >> kTrimBits, I guess there's no way to test a probability greater than 0 and less than 1? https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1789: // Start a main-frame request which should cause network quality estimator which -> that
bengr: ptal. Thanks. https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:150: double GetDoubleValueForVariationParamWithDefaultValue( On 2016/07/08 16:49:52, bengr wrote: > This really should live somewhere in the variations code. Is that possible? I am not sure, but I am planning to add a n_q_e_config class which will take away all this code to a different place :) https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:211: // The least significant kTrimBits of the metric will be discarded. If trimmed On 2016/07/08 16:49:52, bengr wrote: > If -> If the Done. > > Also, I don't understand why you trim bits off a metric. You should explain this > somewhere. I do say below in FitInKBitsPerMetricBits() function: "// Remove the last kTrimBits. This will allow the metric to fit within // kBitsPerMetric while losing only the least significant bits." https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:223: int32_t FitInKBitsPerMetricBits(int32_t metric) { On 2016/07/08 16:49:52, bengr wrote: > I don't understand the point of this method. Is this some sort of optimization > or some workaround due to some limitation of the UMA API? Yes, the goal is to record 4 32-bit samples in a single UMA sample (which is itself 31 bit). So, somehow we need to trim down the 4 metrics from 32 bits to 7 bits. I did check with the UMA team before doing this. https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:651: // |correlation_logging_probability_| to reduce overhead. On 2016/07/08 16:49:52, bengr wrote: > What is the overhead? Done. Added more comments. https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.h:505: // Records correlation metric that can be used for computing the correlation On 2016/07/08 16:49:52, bengr wrote: > Records -> Records a > HTTP -> HTTP-layer > transport -> transport-layer Done. > > What does "time taken to fetch |request|" mean? Re-worded. Not sure it is useful to explain more (code is pretty obvious). https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.h:615: // requests. On 2016/07/08 16:49:52, bengr wrote: > Why is it not recorded for all requests? If this is a performance optimization, > say so, and also provide some intuitions as to why performance would suffer with > a 1.0 probability. Added more comments. https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.h:616: const double correlation_logging_probability_; On 2016/07/08 16:49:52, bengr wrote: > rename: correlation_uma_reporting_probability_; Done. https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1761: // Verify that if the metric is larger 2^(kBitsPerMetric + kTrimBits), On 2016/07/08 16:49:52, bengr wrote: > larger -> larger than Done. https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1764: 1.0, base::TimeDelta::FromSeconds(10), 4095 >> kTrimBits, On 2016/07/08 16:49:52, bengr wrote: > I guess there's no way to test a probability greater than 0 and less than 1? Done. Moved random number to a different function, and virtualized it. https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1789: // Start a main-frame request which should cause network quality estimator On 2016/07/08 16:49:52, bengr wrote: > which -> that Done.
https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:223: int32_t FitInKBitsPerMetricBits(int32_t metric) { On 2016/07/08 17:45:51, tbansal1 wrote: > On 2016/07/08 16:49:52, bengr wrote: > > I don't understand the point of this method. Is this some sort of optimization > > or some workaround due to some limitation of the UMA API? > > Yes, the goal is to record 4 32-bit samples in a single UMA sample (which is > itself 31 bit). So, somehow we need to trim down the 4 metrics from 32 bits to 7 > bits. I did check with the UMA team before doing this. Why do you have to store 4 samples in one UMA sample? https://codereview.chromium.org/2107243003/diff/110001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2107243003/diff/110001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:589: EffectiveConnectionType effective_connection_type_at_last_main_frame_; This seems like a bit of a layering violation for net/ to know about main frames or their timings.
bengr: ptal. Thanks. https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2107243003/diff/60002/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:223: int32_t FitInKBitsPerMetricBits(int32_t metric) { On 2016/07/19 00:32:31, bengr wrote: > On 2016/07/08 17:45:51, tbansal1 wrote: > > On 2016/07/08 16:49:52, bengr wrote: > > > I don't understand the point of this method. Is this some sort of > optimization > > > or some workaround due to some limitation of the UMA API? > > > > Yes, the goal is to record 4 32-bit samples in a single UMA sample (which is > > itself 31 bit). So, somehow we need to trim down the 4 metrics from 32 bits to > 7 > > bits. I did check with the UMA team before doing this. > > Why do you have to store 4 samples in one UMA sample? Added comments. https://codereview.chromium.org/2107243003/diff/110001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2107243003/diff/110001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:589: EffectiveConnectionType effective_connection_type_at_last_main_frame_; On 2016/07/19 00:32:31, bengr wrote: > This seems like a bit of a layering violation for net/ to know about main frames > or their timings. ack'ed. I can move it to Chrome but it is not clear how important that is.
lgtm
Patchset #7 (id:170001) has been deleted
tbansal@chromium.org changed reviewers: + isherman@chromium.org
isherman: Can you PTAL at network_quality_estimator.cc Lines 827 - 837 in the modified file. I am recording a sparse histogram sample with 4-dimensional data (Context: http://shortn/_xdrRCvBkLG). Thanks! I am going to process the data manually using scripts.
isherman@chromium.org changed reviewers: + holte@chromium.org
+Steve, who'll likely have some good input on recording correlatable metrics. FWIW, we've been talking about prototyping adding explicit support to the UMA protocol for recording specific correlatable metrics. It didn't make our list of Q3 OKRs; but if you're willing to invest some time poking at the UMA internals, this work might be a good candidate for that approach. Also, if you do keep this as a histogram, please add it to histograms.xml, and document the histogram there. Also, please make sure to add it to the list of excluded-from-serverside-analysis histograms before landing this CL; since across all users, there would be *lots* of different samples for this one histogram. https://codereview.chromium.org/2107243003/diff/190001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2107243003/diff/190001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:247: // metrices in a single histogram sample, and ensures that all those metrices nit: s/metrices/metrics https://codereview.chromium.org/2107243003/diff/190001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:253: "reserved for sign"); FWIW, the sign bit is safe to use, as long as you are careful to decode the bit patterns correctly on the server-side. https://codereview.chromium.org/2107243003/diff/190001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:817: std::string histogram_name = "NQE.Correlation.ResourceLoadTime.0Kb_128Kb"; nit: I'd just inline this constant below, where it is used. https://codereview.chromium.org/2107243003/diff/190001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:835: sample = (sample << kBitsPerMetric) | resource_size; How many distinct values for this metric is each client likely to record? It seems like the answer is "a lot", in which case there could be a significant memory cost to recording this metric.
On 2016/07/21 21:38:08, Ilya Sherman wrote: > +Steve, who'll likely have some good input on recording correlatable metrics. > > FWIW, we've been talking about prototyping adding explicit support to the UMA > protocol for recording specific correlatable metrics. It didn't make our list > of Q3 OKRs; but if you're willing to invest some time poking at the UMA > internals, this work might be a good candidate for that approach. The histograms only support 32 bit samples. Modifying them to support multi-dimensional data seems non-trivial. Right? Trimming bits to store the multiple metrics in 32 bit sample is very hacky, and does not seem like a good approach for general purpose. > > Also, if you do keep this as a histogram, please add it to histograms.xml, and > document the histogram there. Done. > Also, please make sure to add it to the list of > excluded-from-serverside-analysis histograms before landing this CL; since > across all users, there would be *lots* of different samples for this one > histogram. Please see my comments elsewhere. This CL has no effect until the variation param is set. I will make sure to add the histogram to "excluded-from-serverside-analysis" before setting the param.
Ilya, PTAL https://codereview.chromium.org/2107243003/diff/190001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2107243003/diff/190001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:247: // metrices in a single histogram sample, and ensures that all those metrices On 2016/07/21 21:38:08, Ilya Sherman wrote: > nit: s/metrices/metrics Done. https://codereview.chromium.org/2107243003/diff/190001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:253: "reserved for sign"); On 2016/07/21 21:38:08, Ilya Sherman wrote: > FWIW, the sign bit is safe to use, as long as you are careful to decode the bit > patterns correctly on the server-side. Done. https://codereview.chromium.org/2107243003/diff/190001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:817: std::string histogram_name = "NQE.Correlation.ResourceLoadTime.0Kb_128Kb"; On 2016/07/21 21:38:08, Ilya Sherman wrote: > nit: I'd just inline this constant below, where it is used. Done. https://codereview.chromium.org/2107243003/diff/190001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:835: sample = (sample << kBitsPerMetric) | resource_size; On 2016/07/21 21:38:08, Ilya Sherman wrote: > How many distinct values for this metric is each client likely to record? It > seems like the answer is "a lot", in which case there could be a significant > memory cost to recording this metric. This could potentially be recorded once per HTTP resource (so ~100 per page load). However, this metric is recorded randomly with a probability that is configured by field trial variation param. See |correlation_uma_logging_probability_| above. By default, correlation_uma_logging_probability_ is 0.0. So, until I land the server CL to enable logging, this CL would have no effect. I plan to set it to not more than 0.01 (so approximately once per page loads).
On 2016/07/21 22:42:51, tbansal1 wrote: > On 2016/07/21 21:38:08, Ilya Sherman wrote: > > +Steve, who'll likely have some good input on recording correlatable metrics. > > > > FWIW, we've been talking about prototyping adding explicit support to the UMA > > protocol for recording specific correlatable metrics. It didn't make our list > > of Q3 OKRs; but if you're willing to invest some time poking at the UMA > > internals, this work might be a good candidate for that approach. > The histograms only support 32 bit samples. Modifying them to support > multi-dimensional data seems non-trivial. Right? > Trimming bits to store the multiple metrics in 32 bit sample is very hacky, and > does not seem like a good approach for general purpose. > > > > Also, if you do keep this as a histogram, please add it to histograms.xml, and > > document the histogram there. > Done. > > Also, please make sure to add it to the list of > > excluded-from-serverside-analysis histograms before landing this CL; since > > across all users, there would be *lots* of different samples for this one > > histogram. > Please see my comments elsewhere. This CL has no effect until the variation > param is > set. I will make sure to add the histogram to > "excluded-from-serverside-analysis" before > setting the param. I would be more comfortable with adding the exclusion ahead of time. It's easy to forget to do later, and the cost is relatively high -- we'd need to diagnose the issue, and then rerun our pipelines with the fix in place. Easier to just preempt this now =) https://codereview.chromium.org/2107243003/diff/210001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2107243003/diff/210001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:249: const int32_t kBitsPerMetric = 7; Did you want to set this to 8? (And update the code below to correctly use all of the bits, probably be performing bit-shift calculations on an unsigned int, and then casting to a signed int when logging the value [being careful to choose a cast that doesn't affect the bit structure]) https://codereview.chromium.org/2107243003/diff/210001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2107243003/diff/210001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35718: + sample. This metric is recorded randomly when a resource load finishes. Please document how the metric is constructed, and that this is intended to be analyzed via custom scripts rather than on the dashboard.
On 2016/07/21 22:42:51, tbansal1 wrote: > On 2016/07/21 21:38:08, Ilya Sherman wrote: > > +Steve, who'll likely have some good input on recording correlatable metrics. > > > > FWIW, we've been talking about prototyping adding explicit support to the UMA > > protocol for recording specific correlatable metrics. It didn't make our list > > of Q3 OKRs; but if you're willing to invest some time poking at the UMA > > internals, this work might be a good candidate for that approach. > The histograms only support 32 bit samples. Modifying them to support > multi-dimensional data seems non-trivial. Right? > Trimming bits to store the multiple metrics in 32 bit sample is very hacky, and > does not seem like a good approach for general purpose. For generalizing correlated metrics, we'd probably be looking at adding some new messages to the protobuf to record tuples (either a message encoding them, or just making it easier to add new messages that proto). So that solution wouldn't be aiming to modify the existing histogram recording code, but would involve adding code to plumb new data type into the metrics uploader. It would also require new analysis code, but your histogram will probably require that already. > > > > Also, if you do keep this as a histogram, please add it to histograms.xml, and > > document the histogram there. > Done. > > Also, please make sure to add it to the list of > > excluded-from-serverside-analysis histograms before landing this CL; since > > across all users, there would be *lots* of different samples for this one > > histogram. > Please see my comments elsewhere. This CL has no effect until the variation > param is > set. I will make sure to add the histogram to > "excluded-from-serverside-analysis" before > setting the param.
Ilya, PTAL. Thanks. https://codereview.chromium.org/2107243003/diff/210001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2107243003/diff/210001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:249: const int32_t kBitsPerMetric = 7; On 2016/07/21 23:39:51, Ilya Sherman wrote: > Did you want to set this to 8? (And update the code below to correctly use all > of the bits, probably be performing bit-shift calculations on an unsigned int, > and then casting to a signed int when logging the value [being careful to choose > a cast that doesn't affect the bit structure]) I want to keep the 4 bits unused. I might add EffectiveConnectonType enum later. https://cs.chromium.org/chromium/src/net/nqe/network_quality_estimator.h?rcl=... (or some other metric, may be). https://codereview.chromium.org/2107243003/diff/210001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2107243003/diff/210001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:35718: + sample. This metric is recorded randomly when a resource load finishes. On 2016/07/21 23:39:51, Ilya Sherman wrote: > Please document how the metric is constructed, and that this is intended to be > analyzed via custom scripts rather than on the dashboard. Done.
Metrics lgtm, thanks
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kundaji@chromium.org, bengr@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2107243003/#ps270001 (title: "Added more details to histogram description")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== NQE: Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 7 bits with first 4 bits left unset. BUG=624597 ========== to ========== NQE: Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 7 bits with first 4 bits left unset. BUG=624597 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:270001)
Message was sent while issue was closed.
Description was changed from ========== NQE: Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 7 bits with first 4 bits left unset. BUG=624597 ========== to ========== NQE: Record correlation metric in UMA The 32-bit metric is recorded as a sparse histogram. Each sample contains information on transport RTT, HTTP RTT, bandwidth, and time taken to download the resource. Each metric occupies 7 bits with first 4 bits left unset. BUG=624597 Committed: https://crrev.com/07a33c8ce604eeec637abe035908a19f1772d797 Cr-Commit-Position: refs/heads/master@{#407072} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/07a33c8ce604eeec637abe035908a19f1772d797 Cr-Commit-Position: refs/heads/master@{#407072} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
