Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1657233002:
SafeBrowsing: Create a custom histogram enum for V4 GetHash results. (Closed)
|
Created:
4 years, 10 months ago by kcarattini Modified:
4 years, 10 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, chrome-apps-syd-reviews_chromium.org, vakh (use Gerrit instead) Base URL:
https://chromium.googlesource.com/chromium/src.git@sb-v4-4 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSafeBrowsing: Create a custom histogram enum for V4 GetHash results.
None of the data collection code for this histogram is live yet so this change should not affect any live metrics.
BUG=561867, 543161
Committed: https://crrev.com/16697f40e07b93472ddfff75f65a5cc8567d3de7
Cr-Commit-Position: refs/heads/master@{#373410}
Patch Set 1 #Patch Set 2 : Remove TODO #
Total comments: 2
Patch Set 3 : Fix comments #
Total comments: 2
Patch Set 4 : Rebase #
Depends on Patchset: Messages
Total messages: 23 (10 generated)
kcarattini@chromium.org changed reviewers: + nparker@chromium.org
Note that this remove the empty/hit/miss results buckets, as well as the 204 response code bucket. Thanks! Kendra
kcarattini@chromium.org changed reviewers: + rkaplow@chromium.org
rkaplow: please review histograms.xml
Description was changed from ========== SafeBrowsing: Create a custom histogram enum for V4 GetHash results. BUG=561867,543161 ========== to ========== SafeBrowsing: Create a custom histogram enum for V4 GetHash results. None of the data collection code for this histogram is live yet so this change should not affect any live metrics. BUG=561867,543161 ==========
lgtm https://codereview.chromium.org/1657233002/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_protocol_manager.h (right): https://codereview.chromium.org/1657233002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_protocol_manager.h:91: // prefix, while 204 is an empty response indicating that the fix comments since 204 isn't relevant anymore. Same below.
Thanks, Nathan! rkaplow: please review histograms.xml https://codereview.chromium.org/1657233002/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_protocol_manager.h (right): https://codereview.chromium.org/1657233002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_protocol_manager.h:91: // prefix, while 204 is an empty response indicating that the On 2016/02/02 22:56:14, nparker wrote: > fix comments since 204 isn't relevant anymore. Same below. Done.
https://codereview.chromium.org/1657233002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1657233002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40692: +<histogram name="SafeBrowsing.GetV4HashResult" can you create a new histogram and obsolete the current one? The new version will be incompatible. As well, can you verify no other histograms are using the SB2GetHashResult enum since you're making changes to the enum in the code.
i just noticed the comment about the data wasn't being collected yet - so it is ok you're breaking that histogram. What about stuff like SB2.GetHashResultDownload?
https://codereview.chromium.org/1657233002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1657233002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40692: +<histogram name="SafeBrowsing.GetV4HashResult" On 2016/02/03 22:51:05, rkaplow wrote: > can you create a new histogram and obsolete the current one? The new version > will be incompatible. > > As well, can you verify no other histograms are using the SB2GetHashResult enum > since you're making changes to the enum in the code. Since the code is not live this histogram never collected any data, so it is fine to change the enum. There is another histogram using SB2GetHashResult which is why I have not removed it.
lgtm
The CQ bit was checked by kcarattini@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org Link to the patchset: https://codereview.chromium.org/1657233002/#ps40001 (title: "Fix comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1657233002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1657233002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kcarattini@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/1657233002/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1657233002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1657233002/60001
Message was sent while issue was closed.
Description was changed from ========== SafeBrowsing: Create a custom histogram enum for V4 GetHash results. None of the data collection code for this histogram is live yet so this change should not affect any live metrics. BUG=561867,543161 ========== to ========== SafeBrowsing: Create a custom histogram enum for V4 GetHash results. None of the data collection code for this histogram is live yet so this change should not affect any live metrics. BUG=561867,543161 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== SafeBrowsing: Create a custom histogram enum for V4 GetHash results. None of the data collection code for this histogram is live yet so this change should not affect any live metrics. BUG=561867,543161 ========== to ========== SafeBrowsing: Create a custom histogram enum for V4 GetHash results. None of the data collection code for this histogram is live yet so this change should not affect any live metrics. BUG=561867,543161 Committed: https://crrev.com/16697f40e07b93472ddfff75f65a5cc8567d3de7 Cr-Commit-Position: refs/heads/master@{#373410} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/16697f40e07b93472ddfff75f65a5cc8567d3de7 Cr-Commit-Position: refs/heads/master@{#373410} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
