Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
191383003:
First cut of chrome-proxy (data reduction proxy) measurements. (Closed)
|
Created:
6 years, 9 months ago by bolian Modified:
6 years, 8 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, telemetry+watch_chromium.org, klundberg Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFirst cut of chrome-proxy (data reduction proxy) measurements.
BUG=320748
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260431
Patch Set 1 #Patch Set 2 : . #
Total comments: 32
Patch Set 3 : addressed comments (split out separate CLs, etc) #Patch Set 4 : Added unit tests for network and chrome_proxy metrics. #Patch Set 5 : . #
Total comments: 6
Patch Set 6 : Factor out loading.py change and network.py; metric_unittest.py renaming. #Patch Set 7 : . #Patch Set 8 : change ref head. #Patch Set 9 : moved out test_page_measurement_results.py to network.py CL. #
Total comments: 14
Patch Set 10 : Addressed comments. #Patch Set 11 : fix private member access in tests. #Patch Set 12 : fix rebasing. #
Messages
Total messages: 26 (0 generated)
Hello, This is the first cut of the data reduction proxy (officially called chrome proxy) telemetry measurements. I will add more meat later. The measurements can be divided into 3 categories: latency, data saving, and correctness. Let me what you think. Thanks, Bolian
ping. I think the Tony could look at the structure, whether it is following Telemetry test practices and Ben could look at test details (mainly in tools/perf/metrics/chrome_proxy.py)
Some comments to get started on. Parts of this can be factored out to separate CLs and are ready to land. https://codereview.chromium.org/191383003/diff/20001/tools/perf/measurements/... File tools/perf/measurements/chrome_proxy.py (right): https://codereview.chromium.org/191383003/diff/20001/tools/perf/measurements/... tools/perf/measurements/chrome_proxy.py:11: class ChromeProxyLatency(page_measurement.PageMeasurement): Rather than creating a new measurement, I recommend we use the PageCycler Measurement with --cold-load-percent=100. https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... File tools/perf/metrics/chrome_proxy.py (right): https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. 2014 https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:5: import logging nit: alphabetize https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:22: class ChromeProxyLatency(Metric): Let's remove this Metric, fold your new results into LoadingMetric, make all the result names consistent in that file, and mark all results unimportant except the overall load_time. In case you are wondering, no bots depend on LoadingMetric, so we can change anything there that we want. https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:273: results.Add('original_content_length', 'bytes', original_content_length) Let's generalize this a bit and have a Network(Metric) which reports resource_from_cache, resources_other and content_length. Then if a proxy is detected it will also report the savings. https://codereview.chromium.org/191383003/diff/20001/tools/perf/page_sets/chr... File tools/perf/page_sets/chrome_proxy/top_20.json (right): https://codereview.chromium.org/191383003/diff/20001/tools/perf/page_sets/chr... tools/perf/page_sets/chrome_proxy/top_20.json:2: "description": "Pages hand-picked for Chrome Proxy tests.", We'll need to check in a recording along with the page set even if you plan to hit the network because all page sets should be composable with measurements. It is probably easiest to just land the page set in a patch by itself. See: http://www.chromium.org/developers/telemetry/record_a_page_set https://codereview.chromium.org/191383003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/inspector_network.py (right): https://codereview.chromium.org/191383003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:85: return self._served_from_cache Let's break this out into a new CL and land it along with a unittest (or modification to the existing test). https://codereview.chromium.org/191383003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/actions/navigate.py (right): https://codereview.chromium.org/191383003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/actions/navigate.py:17: if hasattr(self, 'timeout_seconds') and self.timeout_seconds: Please break this out to a new CL and mail to dtu. He is doing significant work on this API and I want to make sure he's happy w/ the new attr.
https://codereview.chromium.org/191383003/diff/20001/tools/perf/measurements/... File tools/perf/measurements/chrome_proxy.py (right): https://codereview.chromium.org/191383003/diff/20001/tools/perf/measurements/... tools/perf/measurements/chrome_proxy.py:10: I'm a little confused about the file naming. What's the difference between measurements, benchmarks, and metrics? https://codereview.chromium.org/191383003/diff/20001/tools/perf/measurements/... tools/perf/measurements/chrome_proxy.py:44: """Base class for all chrome proxy correctness measurement.""" nit: "measurements". Also, why "measurement" and not "tests"? It seems strange to call a test a measurement. https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... File tools/perf/metrics/chrome_proxy.py (right): https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:13: # All chrome_proxy metrics are Chrome only. Is there a check somewhere that Chrome is being tested? https://codereview.chromium.org/191383003/diff/20001/tools/perf/page_sets/chr... File tools/perf/page_sets/chrome_proxy/bypass.json (right): https://codereview.chromium.org/191383003/diff/20001/tools/perf/page_sets/chr... tools/perf/page_sets/chrome_proxy/bypass.json:9: "url": "http://aws1.mdw.la/piatek/bypass-demo", We should probably make copies of all of these URLs so that the tests don't break when someone modifies one of them. E.g., we could put them in aws1.mdw.la/tests/. Also, if we really need to depend on aws, we should have a domain that isn't mdw.la.
https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... File tools/perf/metrics/chrome_proxy.py (right): https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:138: decoded = base64.b64decode(body) Why do you decode a base64 encoded body? https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:143: if ChromeProxyResponse.ShouldGzipContent(content_type): I don't understand. What if it is possible to gzip the resource, but the origin doesn't gzip it? https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:169: if resp.url.startswith('https') or resp.url.startswith('data:'): This reminds me. We should have an integration test that verifies that the chrome proxy isn't bypassed when a data url or resource from cache is loaded that doesn't have the via header. https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:188: def IsValidByViaHeader(self): I don't know why you need a function like this one. We should test that Chrome does the right thing in the presence/absence of the via header, not that the response has a via header. I guess you could use this as a proxy that Chrome is doing the right thing. E.g., if you load a page with 5 resources and you expect all 5 to be proxied, then you should see the header 5 times.
Hello Tony and Ben, I am still working on the unit tests for metrics/network and metrics/chrome_proxy. But just want to sent this out to make sure we agree on the major pieces in place. Thanks, Bolian https://codereview.chromium.org/191383003/diff/20001/tools/perf/measurements/... File tools/perf/measurements/chrome_proxy.py (right): https://codereview.chromium.org/191383003/diff/20001/tools/perf/measurements/... tools/perf/measurements/chrome_proxy.py:10: I am trying to fit into the Telemetry test structure. The relationship of a benchmark, measurement, page set, etc can be found here http://www.chromium.org/developers/telemetry (see Telemetry Benchmarks section). As for metrics, my understanding is that they calculated results for a measurement. https://codereview.chromium.org/191383003/diff/20001/tools/perf/measurements/... tools/perf/measurements/chrome_proxy.py:11: class ChromeProxyLatency(page_measurement.PageMeasurement): PageCycler measurement does not call LoadingMetric which is what I want here. Should I create a separate loading measurement? https://codereview.chromium.org/191383003/diff/20001/tools/perf/measurements/... tools/perf/measurements/chrome_proxy.py:44: """Base class for all chrome proxy correctness measurement.""" Done. A measurement is a page set in Telemetry terminology. https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... File tools/perf/metrics/chrome_proxy.py (right): https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. On 2014/03/12 02:08:37, tonyg wrote: > 2014 Done. https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:5: import logging On 2014/03/12 02:08:37, tonyg wrote: > nit: alphabetize Done. https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:13: # All chrome_proxy metrics are Chrome only. No. But an exception will be thrown if a non-Chrome test uses this metric, because there will be no timeline event returned. https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:22: class ChromeProxyLatency(Metric): On 2014/03/12 02:08:37, tonyg wrote: > Let's remove this Metric, fold your new results into LoadingMetric, make all the > result names consistent in that file, and mark all results unimportant except > the overall load_time. > > In case you are wondering, no bots depend on LoadingMetric, so we can change > anything there that we want. Done. https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:138: decoded = base64.b64decode(body) Added comments for that. The binary body (like that of an image) that Telemetry got is base64 encoded, I need to get the actual length. https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:143: if ChromeProxyResponse.ShouldGzipContent(content_type): Fixed and added comments. The logic now is that if the headers say it is compressed, compress it. The reason is that the body Telemetry got is always decompressed. https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:169: if resp.url.startswith('https') or resp.url.startswith('data:'): I think cached resource has Via header and "data" url may not go through via header checking. I will add tests for them in next batch. https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:188: def IsValidByViaHeader(self): Yes, I am using this to tell whether Chrome proxy is used. On the other hand, since we reply so heavily on the Via header, we want to have specific test for that. For example, run a set of pages 10 times and all responses have Via header. https://codereview.chromium.org/191383003/diff/20001/tools/perf/metrics/chrom... tools/perf/metrics/chrome_proxy.py:273: results.Add('original_content_length', 'bytes', original_content_length) Done. Added a new network metric. https://codereview.chromium.org/191383003/diff/20001/tools/perf/page_sets/chr... File tools/perf/page_sets/chrome_proxy/bypass.json (right): https://codereview.chromium.org/191383003/diff/20001/tools/perf/page_sets/chr... tools/perf/page_sets/chrome_proxy/bypass.json:9: "url": "http://aws1.mdw.la/piatek/bypass-demo", Good point. I think we will end up have a dedicated set of pages for telemetry tests, but for now I just use these. https://codereview.chromium.org/191383003/diff/20001/tools/perf/page_sets/chr... File tools/perf/page_sets/chrome_proxy/top_20.json (right): https://codereview.chromium.org/191383003/diff/20001/tools/perf/page_sets/chr... tools/perf/page_sets/chrome_proxy/top_20.json:2: "description": "Pages hand-picked for Chrome Proxy tests.", Split to a separate CL. https://codereview.chromium.org/191383003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/inspector_network.py (right): https://codereview.chromium.org/191383003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/inspector_network.py:85: return self._served_from_cache On 2014/03/12 02:08:37, tonyg wrote: > Let's break this out into a new CL and land it along with a unittest (or > modification to the existing test). Done. https://codereview.chromium.org/191383003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/actions/navigate.py (right): https://codereview.chromium.org/191383003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/actions/navigate.py:17: if hasattr(self, 'timeout_seconds') and self.timeout_seconds: Split out as https://codereview.chromium.org/202483006/
Added unit tests. Please take a look.
https://codereview.chromium.org/191383003/diff/80001/tools/perf/metrics/loadi... File tools/perf/metrics/loading.py (right): https://codereview.chromium.org/191383003/diff/80001/tools/perf/metrics/loadi... tools/perf/metrics/loading.py:19: # All numbers in milliseconds. Let's factor just this file out into a separate patch. It should be ready to land with the nits below. Sorry to be a pain about that, but the tiny patches make it so that if we break or change existing tests then the problems are very easy to diagnose. https://codereview.chromium.org/191383003/diff/80001/tools/perf/metrics/loadi... tools/perf/metrics/loading.py:22: results.Add('page_load', 'ms', page_load) Naming convention suggestion: # NavigationStart relative markers end with "start": 'fetch_start' 'request_start' 'dom_content_loaded_start' 'window_load_start' # Phase measurements end with "duration": 'domain_lookup_duration' 'connect_duration' 'request_duration' 'response_duration' WDYT?
network_metric.py and its unittest will be ready to land in a separate patch if you want to break it out. https://codereview.chromium.org/191383003/diff/80001/tools/perf/metrics/metri... File tools/perf/metrics/metric_unittest.py (right): https://codereview.chromium.org/191383003/diff/80001/tools/perf/metrics/metri... tools/perf/metrics/metric_unittest.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. This file isn't named correctly, the name makes it sound like it is a unittest for metrics. What about test_page_measurement_results.py?
Thanks, I factored out below two CLs as suggested to be reviewed separately: loading.py: https://codereview.chromium.org/199653007/ network*.py: https://codereview.chromium.org/211133004 Thanks, Bolian https://codereview.chromium.org/191383003/diff/80001/tools/perf/metrics/loadi... File tools/perf/metrics/loading.py (right): https://codereview.chromium.org/191383003/diff/80001/tools/perf/metrics/loadi... tools/perf/metrics/loading.py:19: # All numbers in milliseconds. Done. Np at all, that is good. Split out: https://codereview.chromium.org/199653007/ https://codereview.chromium.org/191383003/diff/80001/tools/perf/metrics/loadi... tools/perf/metrics/loading.py:22: results.Add('page_load', 'ms', page_load) Doen. much better. https://codereview.chromium.org/191383003/diff/80001/tools/perf/metrics/metri... File tools/perf/metrics/metric_unittest.py (right): https://codereview.chromium.org/191383003/diff/80001/tools/perf/metrics/metri... tools/perf/metrics/metric_unittest.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. sounds good. Done.
Please ignore tools/perf/metrics/loading.py tools/perf/metrics/network.py tools/perf/metrics/network_unittest.py They are reviewed separately, but I am not sure how to hide them here while still use them (diffbase?)
Got it now. Changed ref head.
https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... File tools/perf/metrics/chrome_proxy.py (right): https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... tools/perf/metrics/chrome_proxy.py:10: pass Shouldn't this have an implementation? https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... tools/perf/metrics/chrome_proxy.py:14: CHROME_PROXY_VIA_HEADER_OLD = '1.1 Chrome Compression Proxy' OLD -> DEPRECATED https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... tools/perf/metrics/chrome_proxy.py:25: if resp.url.startswith('https') or resp.url.startswith('data:'): Is there any way to do something similar for incognito? https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... File tools/perf/metrics/chrome_proxy_unittest.py (right): https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... tools/perf/metrics/chrome_proxy_unittest.py:101: def testChromePrxoyMetricForDataSaving(self): Proxy https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... tools/perf/metrics/chrome_proxy_unittest.py:113: def testChromePrxoyMetricForHeaderValidation(self): Proxy https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... tools/perf/metrics/chrome_proxy_unittest.py:131: def testChromePrxoyMetricForBypass(self): Proxy https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... tools/perf/metrics/chrome_proxy_unittest.py:149: def testChromePrxoyMetricForSafebrowsing(self): Proxy
Thanks! I also update how the events are organized in unitest. https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... File tools/perf/metrics/chrome_proxy.py (right): https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... tools/perf/metrics/chrome_proxy.py:10: pass No, it will just take an error message when thrown. https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... tools/perf/metrics/chrome_proxy.py:14: CHROME_PROXY_VIA_HEADER_OLD = '1.1 Chrome Compression Proxy' On 2014/03/26 22:45:55, bengr1 wrote: > OLD -> DEPRECATED Done. https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... tools/perf/metrics/chrome_proxy.py:25: if resp.url.startswith('https') or resp.url.startswith('data:'): Not sure how to know it is incognito from Telemetry for now. But I will add an item in the test spreadsheet for this. https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... File tools/perf/metrics/chrome_proxy_unittest.py (right): https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... tools/perf/metrics/chrome_proxy_unittest.py:101: def testChromePrxoyMetricForDataSaving(self): On 2014/03/26 22:45:55, bengr1 wrote: > Proxy Done. https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... tools/perf/metrics/chrome_proxy_unittest.py:113: def testChromePrxoyMetricForHeaderValidation(self): On 2014/03/26 22:45:55, bengr1 wrote: > Proxy Done. https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... tools/perf/metrics/chrome_proxy_unittest.py:131: def testChromePrxoyMetricForBypass(self): On 2014/03/26 22:45:55, bengr1 wrote: > Proxy Done. https://codereview.chromium.org/191383003/diff/210001/tools/perf/metrics/chro... tools/perf/metrics/chrome_proxy_unittest.py:149: def testChromePrxoyMetricForSafebrowsing(self): On 2014/03/26 22:45:55, bengr1 wrote: > Proxy Done.
Rubber stamp lgtm. Please make sure Ben is happy with the actual functionality here before landing.
lgtm
The CQ bit was checked by bolian@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bolian@chromium.org/191383003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The CQ bit was checked by bolian@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bolian@chromium.org/191383003/260001
The CQ bit was unchecked by cmp@chromium.org
The CQ bit was checked by cmp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bolian@chromium.org/191383003/260001
Message was sent while issue was closed.
Change committed as 260431 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
