Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1824713002:
Break down HTTPCache.Result metric by resource type (Closed)
|
Created:
4 years, 9 months ago by jkarlin Modified:
4 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBreak down HTTPCache.Result metric by resource type
We'd like to know how often different resource types are cached and
revalidated, so this CL adds cache pattern metrics for common resource
types. Note that the resource type is a guess based on the URL's
path. There will be many missed resources but I anticipate the
fraction of false positives will be low.
BUG=596569
Committed: https://crrev.com/5152e48e43339dbe3013b647c00bfff4867eb087
Cr-Commit-Position: refs/heads/master@{#383319}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase #Patch Set 3 : Address comments from PS1 #
Total comments: 4
Patch Set 4 : Add ecmascript #Messages
Total messages: 19 (6 generated)
jkarlin@chromium.org changed reviewers: + rkaplow@chromium.org, ttuttle@chromium.org
ttuttle: PTAL at net/ rkaplot: PTAL at histograms.xml Thanks!
https://codereview.chromium.org/1824713002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1824713002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:16800: +<histogram name="HttpCache.Pattern.CSS" enum="HttpCachePattern"> can you switch to using the histogram_suffix pattern? This will keep this much more concise as well as making it faster to add to. https://codereview.chromium.org/1824713002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:16827: + For each http cache transaction for a URL whose path ends with '.jpg', the i would change this comment to mention it also acts as a catchall
https://codereview.chromium.org/1824713002/diff/1/net/http/http_cache_transac... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1824713002/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2707: Have you thought about examining the content-type header? It might be more reliable.
Patchset #3 (id:40001) has been deleted
Thanks! PTAL. https://codereview.chromium.org/1824713002/diff/1/net/http/http_cache_transac... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1824713002/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.cc:2707: On 2016/03/22 15:12:47, Julia Tuttle (deprecated) wrote: > Have you thought about examining the content-type header? It might be more > reliable. Yes, that's better. Thanks! https://codereview.chromium.org/1824713002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1824713002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:16800: +<histogram name="HttpCache.Pattern.CSS" enum="HttpCachePattern"> On 2016/03/22 15:02:05, rkaplow wrote: > can you switch to using the histogram_suffix pattern? This will keep this much > more concise as well as making it faster to add to. Done. Thanks! https://codereview.chromium.org/1824713002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:16827: + For each http cache transaction for a URL whose path ends with '.jpg', the On 2016/03/22 15:02:05, rkaplow wrote: > i would change this comment to mention it also acts as a catchall this is now "HttpCache.Pattern.Image"
Patchset #3 (id:60001) has been deleted
lgtm
https://codereview.chromium.org/1824713002/diff/80001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1824713002/diff/80001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:2709: if (GetResponseInfo()->headers && Do you have a sense from local testing that these are covering things well? https://codereview.chromium.org/1824713002/diff/80001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:2728: } else if (base::EndsWith(mime_type, "javascript", Did you want to check for ecmascript as well?
Julia: PTAL, thanks! https://codereview.chromium.org/1824713002/diff/80001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1824713002/diff/80001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:2709: if (GetResponseInfo()->headers && On 2016/03/23 19:28:48, Julia Tuttle (deprecated) wrote: > Do you have a sense from local testing that these are covering things well? Nothing extensive, but it seems to have good coverage on my local build. https://codereview.chromium.org/1824713002/diff/80001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:2728: } else if (base::EndsWith(mime_type, "javascript", On 2016/03/23 19:28:48, Julia Tuttle (deprecated) wrote: > Did you want to check for ecmascript as well? Done.
On 2016/03/24 11:09:00, jkarlin wrote: > Julia: PTAL, thanks! > > https://codereview.chromium.org/1824713002/diff/80001/net/http/http_cache_tra... > File net/http/http_cache_transaction.cc (right): > > https://codereview.chromium.org/1824713002/diff/80001/net/http/http_cache_tra... > net/http/http_cache_transaction.cc:2709: if (GetResponseInfo()->headers && > On 2016/03/23 19:28:48, Julia Tuttle (deprecated) wrote: > > Do you have a sense from local testing that these are covering things well? > > Nothing extensive, but it seems to have good coverage on my local build. > > https://codereview.chromium.org/1824713002/diff/80001/net/http/http_cache_tra... > net/http/http_cache_transaction.cc:2728: } else if (base::EndsWith(mime_type, > "javascript", > On 2016/03/23 19:28:48, Julia Tuttle (deprecated) wrote: > > Did you want to check for ecmascript as well? > > Done. Ping Julia!
On 2016/03/25 17:08:29, jkarlin wrote: > On 2016/03/24 11:09:00, jkarlin wrote: > > Julia: PTAL, thanks! > > > > > https://codereview.chromium.org/1824713002/diff/80001/net/http/http_cache_tra... > > File net/http/http_cache_transaction.cc (right): > > > > > https://codereview.chromium.org/1824713002/diff/80001/net/http/http_cache_tra... > > net/http/http_cache_transaction.cc:2709: if (GetResponseInfo()->headers && > > On 2016/03/23 19:28:48, Julia Tuttle (deprecated) wrote: > > > Do you have a sense from local testing that these are covering things well? > > > > Nothing extensive, but it seems to have good coverage on my local build. > > > > > https://codereview.chromium.org/1824713002/diff/80001/net/http/http_cache_tra... > > net/http/http_cache_transaction.cc:2728: } else if (base::EndsWith(mime_type, > > "javascript", > > On 2016/03/23 19:28:48, Julia Tuttle (deprecated) wrote: > > > Did you want to check for ecmascript as well? > > > > Done. > > Ping Julia! Sorry, was out sick yesterday. lgtm now!
Thanks! Hope you're feeling better.
The CQ bit was checked by jkarlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/1824713002/#ps100001 (title: "Add ecmascript")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1824713002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1824713002/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Break down HTTPCache.Result metric by resource type We'd like to know how often different resource types are cached and revalidated, so this CL adds cache pattern metrics for common resource types. Note that the resource type is a guess based on the URL's path. There will be many missed resources but I anticipate the fraction of false positives will be low. BUG=596569 ========== to ========== Break down HTTPCache.Result metric by resource type We'd like to know how often different resource types are cached and revalidated, so this CL adds cache pattern metrics for common resource types. Note that the resource type is a guess based on the URL's path. There will be many missed resources but I anticipate the fraction of false positives will be low. BUG=596569 Committed: https://crrev.com/5152e48e43339dbe3013b647c00bfff4867eb087 Cr-Commit-Position: refs/heads/master@{#383319} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5152e48e43339dbe3013b647c00bfff4867eb087 Cr-Commit-Position: refs/heads/master@{#383319} |
