Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2155783002:
Batch pageload metrics pingbacks (Closed)
DescriptionBatch pageload metrics pingbacks
Currently, each page load through data_reduction_proxy will create a
pingback, and sends those pingbacks one at a time. The proto has
infrastructure to batch those page load metrics that will reduce the
request overhead for cases that are slow to send the pingback, but have
multiple page load metrics trying to be reported around the same time.
BUG=628721
Committed: https://crrev.com/6d456bbb39361b2031da5bda446f1418dfd14cfe
Cr-Commit-Position: refs/heads/master@{#406142}
Patch Set 1 #
Total comments: 18
Patch Set 2 : tbansal comments #
Total comments: 1
Patch Set 3 : tbansal comment #
Messages
Total messages: 26 (16 generated)
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
ryansturm@chromium.org changed reviewers: + tbansal@chromium.org
tbansal: PTAL
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.
nits.. https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:120: DCHECK_EQ(data_to_send_->pageloads_size(), 1); Expect field should come first. So, reverse the order. Similar comment on line 126 below. https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:129: data_to_send_.reset(new RecordPageloadMetricsRequest()); Instead of resetting, use Clear(). https://cs.chromium.org/chromium/src/out/Debug/gen/components/data_reduction_... https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:140: return fetcher; Unrelated to this CL: Can this function update |current_fetcher_| directly, and return void. I do not see much use in this returning back a class member back to other functions which are also part of the same class. https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h (right): https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:8: #include <list> Not needed anymore. https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:65: // |url_request_context_|. The max retires is set to 5. s/retires/retries/ https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:79: std::unique_ptr<RecordPageloadMetricsRequest> data_to_send_; Just change to: RecordPageloadMetricsRequest data_to_send_; Also, would it be easier to read if the variable is renamed to match class name. e.g. may be metrics_request_; https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc (right): https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc:202: for (int i = 0; i < 2; i++) { s/int/size_t/ s/i++/++i https://codereview.chromium.org/2155783002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2155783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:7754: + or not queued based on the reporting fraction. It is generally useful to specify when a metric is recorded. May be add: Recorded once per pageload. https://codereview.chromium.org/2155783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:7763: + failed at being sent to the server. May be add: Recorded every time a ping back request is attempted.
The CQ bit was checked by ryansturm@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...
https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:120: DCHECK_EQ(data_to_send_->pageloads_size(), 1); On 2016/07/18 17:42:00, tbansal1 wrote: > Expect field should come first. > So, reverse the order. > > Similar comment on line 126 below. I'll change this one, but for line 126, everywhere I look has the DCHECK_GE comparing actual with expected in that order. https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:129: data_to_send_.reset(new RecordPageloadMetricsRequest()); On 2016/07/18 17:42:00, tbansal1 wrote: > Instead of resetting, use Clear(). > > https://cs.chromium.org/chromium/src/out/Debug/gen/components/data_reduction_... Done. https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:140: return fetcher; On 2016/07/18 17:42:00, tbansal1 wrote: > Unrelated to this CL: Can this function update |current_fetcher_| directly, and > return void. I do not see much use in this returning back a class member back to > other functions which are also part of the same class. Done. https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h (right): https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:8: #include <list> On 2016/07/18 17:42:00, tbansal1 wrote: > Not needed anymore. Done. https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:65: // |url_request_context_|. The max retires is set to 5. On 2016/07/18 17:42:00, tbansal1 wrote: > s/retires/retries/ Done. https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:79: std::unique_ptr<RecordPageloadMetricsRequest> data_to_send_; On 2016/07/18 17:42:00, tbansal1 wrote: > Just change to: > RecordPageloadMetricsRequest data_to_send_; > > Also, would it be easier to read if the variable is renamed to match class name. > e.g. may be metrics_request_; Done. https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc (right): https://codereview.chromium.org/2155783002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client_unittest.cc:202: for (int i = 0; i < 2; i++) { On 2016/07/18 17:42:00, tbansal1 wrote: > s/int/size_t/ > s/i++/++i Done. https://codereview.chromium.org/2155783002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2155783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:7754: + or not queued based on the reporting fraction. On 2016/07/18 17:42:00, tbansal1 wrote: > It is generally useful to specify when a metric is recorded. May be add: > Recorded once per pageload. Done. https://codereview.chromium.org/2155783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:7763: + failed at being sent to the server. On 2016/07/18 17:42:00, tbansal1 wrote: > May be add: > Recorded every time a ping back request is attempted. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tbansal: PTAL
lgtm % nit below. https://codereview.chromium.org/2155783002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc (right): https://codereview.chromium.org/2155783002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:121: void DataReductionProxyPingbackClient::CreateFetcherForDataAndStart() { DCHECK current_fetcher_ is null.
ryansturm@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine: PTAL @ histograms.xml, thanks
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
lgtm
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 ryansturm@chromium.org
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org Link to the patchset: https://codereview.chromium.org/2155783002/#ps40001 (title: "tbansal comment")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Batch pageload metrics pingbacks Currently, each page load through data_reduction_proxy will create a pingback, and sends those pingbacks one at a time. The proto has infrastructure to batch those page load metrics that will reduce the request overhead for cases that are slow to send the pingback, but have multiple page load metrics trying to be reported around the same time. BUG=628721 ========== to ========== Batch pageload metrics pingbacks Currently, each page load through data_reduction_proxy will create a pingback, and sends those pingbacks one at a time. The proto has infrastructure to batch those page load metrics that will reduce the request overhead for cases that are slow to send the pingback, but have multiple page load metrics trying to be reported around the same time. BUG=628721 Committed: https://crrev.com/6d456bbb39361b2031da5bda446f1418dfd14cfe Cr-Commit-Position: refs/heads/master@{#406142} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6d456bbb39361b2031da5bda446f1418dfd14cfe Cr-Commit-Position: refs/heads/master@{#406142} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
