Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1212773002:
Add prefetch UMA metrics. (Closed)
|
Created:
5 years, 5 months ago by twifkak Modified:
4 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, wifiprefetch-reviews_google.com, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd prefetch UMA metrics.
Add Precache.Fetch.ResponseBytes and Precache.Fetch.PercentCompleted,
recorded by PrecacheFetcher. Tested using HistogramTester.
BUG=499532
Committed: https://crrev.com/0d97553cdb341dbdf4a40ac1c0dee17eb44e39af
Cr-Commit-Position: refs/heads/master@{#338361}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fix histograms and s/int64/int64_t/. #Patch Set 3 : s/respective //. #
Total comments: 11
Patch Set 4 : Comment and style tweaks, and adding bengr as histogram owner. #Patch Set 5 : Rebase. #Patch Set 6 : Tweak percent_completed calculation. #
Total comments: 2
Patch Set 7 : Fix for CloudExternalDataManagerBaseTest.CacheCorruption. #
Messages
Total messages: 31 (8 generated)
twifkak@chromium.org changed reviewers: + bengr@chromium.org, jar@chromium.org, mef@chromium.org
mef@chromium.org: Please review changes in net/url_request/ bengr@chromium.org: Please review changes in components/precache/ jar@chromium.org: Please review changes in tools/metrics/histograms/
twifkak@chromium.org changed reviewers: + mpearson@google.com - jar@chromium.org
-jar, +mpearson, assuming that (doing other things) means (reassign) mpearson: Please review histograms.xml (at a minimum; more if you like).
twifkak@chromium.org changed reviewers: + mpearson@chromium.org - mpearson@google.com
https://codereview.chromium.org/1212773002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/1212773002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:182: UMA_HISTOGRAM_COUNTS_100("Precache.Fetch.PercentCompleted", Why not UMA_HISTOGRAM_PERCENTAGE? https://codereview.chromium.org/1212773002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:184: UMA_HISTOGRAM_COUNTS("Precache.Fetch.ResponseBytes", total_response_bytes_); This maxs out at 1,000,000. Is that enough range for you? https://codereview.chromium.org/1212773002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1212773002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:31307: + The percent of manifests for which all respective resources have been respective? I don't know what this word means in the context of this sentence. If it means something, you need to explain it or choose a better word. Otherwise remove it.
https://codereview.chromium.org/1212773002/diff/1/net/url_request/test_url_fe... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/1212773002/diff/1/net/url_request/test_url_fe... net/url_request/test_url_fetcher_factory.h:311: int64 response_bytes_; according to base/basictypes.h it should be int64_t and include <stdint.h>
https://codereview.chromium.org/1212773002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/1212773002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:182: UMA_HISTOGRAM_COUNTS_100("Precache.Fetch.PercentCompleted", On 2015/06/26 17:03:37, Mark P wrote: > Why not UMA_HISTOGRAM_PERCENTAGE? Because I didn't know any better. https://codereview.chromium.org/1212773002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.cc:184: UMA_HISTOGRAM_COUNTS("Precache.Fetch.ResponseBytes", total_response_bytes_); On 2015/06/26 17:03:37, Mark P wrote: > This maxs out at 1,000,000. Is that enough range for you? Eek, no. Fixed. https://codereview.chromium.org/1212773002/diff/1/net/url_request/test_url_fe... File net/url_request/test_url_fetcher_factory.h (right): https://codereview.chromium.org/1212773002/diff/1/net/url_request/test_url_fe... net/url_request/test_url_fetcher_factory.h:311: int64 response_bytes_; On 2015/06/26 17:27:36, mef wrote: > according to base/basictypes.h it should be int64_t and include <stdint.h> Done. https://codereview.chromium.org/1212773002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1212773002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:31307: + The percent of manifests for which all respective resources have been On 2015/06/26 17:03:37, Mark P wrote: > respective? > I don't know what this word means in the context of this sentence. > If it means something, you need to explain it or choose a better word. > Otherwise remove it. Done.
histograms.xml lgtm
net/url_request/* lgtm
https://codereview.chromium.org/1212773002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.h (right): https://codereview.chromium.org/1212773002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:126: int total_response_bytes_; Are we concerned about overflow? https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher.cc:31: // The 99%ile number of bytes we expect to receive during prefetch, and the max 99%ile -> 99th percentile and the max -> which will be used as the maximum for the xxx histogram https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher.cc:185: num_manifest_urls_to_fetch_ * 100); Can num_manifest_urls_to_fetch_ be 0? https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... File components/precache/core/precache_fetcher_unittest.cc (right): https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher_unittest.cc:162: { Why the scope? https://codereview.chromium.org/1212773002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1212773002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31305: + <owner>twifkak@chromium.org</owner> Please add me as an owner on all precache histograms.
https://codereview.chromium.org/1212773002/diff/1/components/precache/core/pr... File components/precache/core/precache_fetcher.h (right): https://codereview.chromium.org/1212773002/diff/1/components/precache/core/pr... components/precache/core/precache_fetcher.h:126: int total_response_bytes_; On 2015/06/30 19:26:19, bengr wrote: > Are we concerned about overflow? An int should be able to represent up to ~2G, so I'm not concerned. If it does occur, it should show up in the histogram as a large count in bucket 0 and a histogram which does not appear to decrease to zero count in the upper buckets -- i.e. aliasing. But please correct me if my analysis is wrong. https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher.cc:31: // The 99%ile number of bytes we expect to receive during prefetch, and the max On 2015/06/30 19:26:19, bengr wrote: > 99%ile -> 99th percentile > > and the max -> which will be used as the maximum for the xxx histogram Done. https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher.cc:185: num_manifest_urls_to_fetch_ * 100); On 2015/06/30 19:26:19, bengr wrote: > Can num_manifest_urls_to_fetch_ be 0? Yes, but if num_manifests_urls_to_fetch_ is 0, so is manifests_completed. This is tested by PrecacheFetcherTest.Cancel. https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... File components/precache/core/precache_fetcher_unittest.cc (right): https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher_unittest.cc:162: { On 2015/06/30 19:26:19, bengr wrote: > Why the scope? Documented on line 169. How should I document that instead? https://codereview.chromium.org/1212773002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1212773002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31305: + <owner>twifkak@chromium.org</owner> On 2015/06/30 19:26:19, bengr wrote: > Please add me as an owner on all precache histograms. Done.
https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher.cc:185: num_manifest_urls_to_fetch_ * 100); On 2015/06/30 20:23:51, twifkak wrote: > On 2015/06/30 19:26:19, bengr wrote: > > Can num_manifest_urls_to_fetch_ be 0? > > Yes, but if num_manifests_urls_to_fetch_ is 0, so is manifests_completed. This > is tested by PrecacheFetcherTest.Cancel. DCHECK(num_manifest_urls_to_fetch_ >= manifests_completed) ? https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... File components/precache/core/precache_fetcher_unittest.cc (right): https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher_unittest.cc:162: { On 2015/06/30 20:23:51, twifkak wrote: > On 2015/06/30 19:26:19, bengr wrote: > > Why the scope? > > Documented on line 169. How should I document that instead? Acknowledged.
https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/1212773002/diff/40001/components/precache/cor... components/precache/core/precache_fetcher.cc:185: num_manifest_urls_to_fetch_ * 100); On 2015/07/06 17:31:08, bengr wrote: > On 2015/06/30 20:23:51, twifkak wrote: > > On 2015/06/30 19:26:19, bengr wrote: > > > Can num_manifest_urls_to_fetch_ be 0? > > > > Yes, but if num_manifests_urls_to_fetch_ is 0, so is manifests_completed. This > > is tested by PrecacheFetcherTest.Cancel. > > DCHECK(num_manifest_urls_to_fetch_ >= manifests_completed) ? OK, added a different DCHECK, but changed the conditional to make it clear there's no SIGFPE.
https://codereview.chromium.org/1212773002/diff/100001/components/precache/co... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/1212773002/diff/100001/components/precache/co... components/precache/core/precache_fetcher.cc:145: int64 current, I think you want to use int64_t. See https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types
https://codereview.chromium.org/1212773002/diff/100001/components/precache/co... File components/precache/core/precache_fetcher.cc (right): https://codereview.chromium.org/1212773002/diff/100001/components/precache/co... components/precache/core/precache_fetcher.cc:145: int64 current, On 2015/07/08 19:52:40, bengr wrote: > I think you want to use int64_t. See > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types > I'm implementing the net::URLFetcherDelegate interface. I think relying on the fact that int64 is a typedef for int64_t in order for it to compile is worse than using a deprecated type. When they change the interface, they can change all implementors.
On 2015/07/08 19:59:03, twifkak wrote: > https://codereview.chromium.org/1212773002/diff/100001/components/precache/co... > File components/precache/core/precache_fetcher.cc (right): > > https://codereview.chromium.org/1212773002/diff/100001/components/precache/co... > components/precache/core/precache_fetcher.cc:145: int64 current, > On 2015/07/08 19:52:40, bengr wrote: > > I think you want to use int64_t. See > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types > > > > I'm implementing the net::URLFetcherDelegate interface. I think relying on the > fact that int64 is a typedef for int64_t in order for it to compile is worse > than using a deprecated type. When they change the interface, they can change > all implementors. I see. lgtm.
The CQ bit was checked by twifkak@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1212773002/#ps100001 (title: "Tweak percent_completed calculation.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1212773002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
mef: PTAL. Patch set 7 fixes a bug in FakeURLFetcher whereby a client relies on the ability to delete the URLFetcher from inside OnURLFetchDownloadProgress, to cancel the fetch.
OK, release cut is happening Real Soon Now, and I need this in. I think the test util change in patch set 7 is fairly minor (and not at all user-impacting), so I'm going to TBR it and commit now. PTAL when you get a chance.
The CQ bit was checked by twifkak@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org, bengr@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1212773002/#ps120001 (title: "Fix for CloudExternalDataManagerBaseTest.CacheCorruption.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1212773002/120001
On 2015/07/10 20:14:03, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1212773002/120001 still lgtm
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0d97553cdb341dbdf4a40ac1c0dee17eb44e39af Cr-Commit-Position: refs/heads/master@{#338361} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
