Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1819503002:
Separate SBClientDownload.SignedOrWhitelistedDownload metric into two buckets, (Closed)
|
Created:
4 years, 9 months ago by Jialiu Lin Modified:
4 years, 9 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, grt+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSeparate SBClientDownload.SignedOrWhitelistedDownload metric into two buckets,
to distinguish whitelisted signature and whitelisted url.
BUG=595920
Committed: https://crrev.com/179d0904ce723736734a34ee0fdd0e567a3c4005
Cr-Commit-Position: refs/heads/master@{#382433}
Patch Set 1 #
Total comments: 6
Patch Set 2 : address mattm's comments #
Total comments: 4
Patch Set 3 : add another bucket #
Total comments: 4
Patch Set 4 : update histogram description #
Messages
Total messages: 25 (9 generated)
jialiul@chromium.org changed reviewers: + mattm@chromium.org
Hi mattm@, Do you mind if I change this UMA metric?
https://codereview.chromium.org/1819503002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/1819503002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/download_protection_service.cc:705: RecordCountOfWhitelistedDownload(SIGNATURE_WHITELIST); This needs to go inside the "if CertificateChainIsWhitelisted" block. (I have no idea why the old one wasn't.) https://codereview.chromium.org/1819503002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1819503002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:43555: + Record whenever an download matches certan whitelist (e.g. URL whitelist, or nit: an->a certan->a certain https://codereview.chromium.org/1819503002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:43556: + signiture whitelist). signature (also below)
Thanks, mattm! (sorry for my embarrassing typos....) https://codereview.chromium.org/1819503002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/1819503002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/download_protection_service.cc:705: RecordCountOfWhitelistedDownload(SIGNATURE_WHITELIST); On 2016/03/18 at 22:44:05, mattm wrote: > This needs to go inside the "if CertificateChainIsWhitelisted" block. (I have no idea why the old one wasn't.) Nice catch! I was blindly following.... haha https://codereview.chromium.org/1819503002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1819503002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:43555: + Record whenever an download matches certan whitelist (e.g. URL whitelist, or On 2016/03/18 at 22:44:05, mattm wrote: > nit: > an->a > certan->a certain done https://codereview.chromium.org/1819503002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:43556: + signiture whitelist). On 2016/03/18 at 22:44:05, mattm wrote: > signature (also below) done
lgtm
jialiul@chromium.org changed reviewers: + holte@chromium.org
jialiul@chromium.org changed reviewers: + asvitkine@chromium.org - holte@chromium.org
asvitkine@, could you take a look at the changes in histograms.xml? Thanks!
https://codereview.chromium.org/1819503002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1819503002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43556: + or signature whitelist). Nit: Mention when this is logged. Once per download when it completes? https://codereview.chromium.org/1819503002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:84188: +<enum name="WhitelistedDownloadType" type="int"> Have you considered recordering "does not match whitelist" as a bucket in the same metric? This way, you can track what percentage of downloads match these whitelist - as opposed to just absolute count which is likely not as useful. Or is that tracked in some other way?
Thanks, asvitkine@! https://codereview.chromium.org/1819503002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1819503002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43556: + or signature whitelist). On 2016/03/21 at 15:48:37, Alexei Svitkine (very slow) wrote: > Nit: Mention when this is logged. Once per download when it completes? Done https://codereview.chromium.org/1819503002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:84188: +<enum name="WhitelistedDownloadType" type="int"> On 2016/03/21 at 15:48:37, Alexei Svitkine (very slow) wrote: > Have you considered recordering "does not match whitelist" as a bucket in the same metric? This way, you can track what percentage of downloads match these whitelist - as opposed to just absolute count which is likely not as useful. Or is that tracked in some other way? Good idea! Although I guess it is probably can be computed by adding a bunch of metrics from different places, it is definitely much easier to simply add another bucket to this one.
lgtm
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org Link to the patchset: https://codereview.chromium.org/1819503002/#ps40001 (title: "add another bucket")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819503002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819503002/40001
The CQ bit was unchecked by mattm@chromium.org
sorry for unchecking, the last change made me realize the histograms description is unclear. https://codereview.chromium.org/1819503002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/1819503002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:719: RecordCountOfWhitelistedDownload(NO_WHITELIST_MATCH); I think this is a little unclear. There are downloads which don't get to this point in the process and thus don't get recorded in this histogram, so it should define more precisely what it means in histograms.xml. https://codereview.chromium.org/1819503002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1819503002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43556: + whitelist, or signature whitelist). Should more precisely define what this histogram means. Not all downloads will be recorded in this histogram.
Hi mattm@, Thanks! Let me know if my latest edits look good. https://codereview.chromium.org/1819503002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/1819503002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:719: RecordCountOfWhitelistedDownload(NO_WHITELIST_MATCH); On 2016/03/21 at 20:57:19, mattm wrote: > I think this is a little unclear. There are downloads which don't get to this point in the process and thus don't get recorded in this histogram, so it should define more precisely what it means in histograms.xml. metrics description updated. https://codereview.chromium.org/1819503002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1819503002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43556: + whitelist, or signature whitelist). On 2016/03/21 at 20:57:19, mattm wrote: > Should more precisely define what this histogram means. Not all downloads will be recorded in this histogram. You're right. Updated accordingly. Thanks!
lgtm
On 2016/03/21 at 22:12:24, mattm wrote: > lgtm Thanks, mattm!
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1819503002/#ps60001 (title: "update histogram description")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819503002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819503002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Separate SBClientDownload.SignedOrWhitelistedDownload metric into two buckets, to distinguish whitelisted signature and whitelisted url. BUG=595920 ========== to ========== Separate SBClientDownload.SignedOrWhitelistedDownload metric into two buckets, to distinguish whitelisted signature and whitelisted url. BUG=595920 Committed: https://crrev.com/179d0904ce723736734a34ee0fdd0e567a3c4005 Cr-Commit-Position: refs/heads/master@{#382433} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/179d0904ce723736734a34ee0fdd0e567a3c4005 Cr-Commit-Position: refs/heads/master@{#382433} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
