Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2347753002:
Adds histograms for tracking Software Reporter logs uploads in SRT Fetcher. (Closed)
DescriptionAdds histograms for tracking Software Reporter logs uploads in SRT Fetcher.
BUG=634434
Committed: https://crrev.com/c327e32928d28397da473e471c1fd77c8d2af638
Cr-Commit-Position: refs/heads/master@{#420516}
Patch Set 1 #Patch Set 2 : Rename Invocation::Flags and add boolean for logs upload to the invocation #Patch Set 3 : Fix comment in RunSwReporterAfterStartup #
Total comments: 28
Patch Set 4 : Addressing comments #Patch Set 5 : More comments #Patch Set 6 : More comments #
Total comments: 4
Patch Set 7 : No enum class for ftirelo #
Total comments: 10
Patch Set 8 : Registry error histogram and more comments #
Total comments: 6
Patch Set 9 : Improves comments in histograms.xml #
Total comments: 5
Patch Set 10 : Define constant #
Messages
Total messages: 55 (31 generated)
ftirelo@chromium.org changed reviewers: + csharp@chromium.org, grt@chromium.org, joenotcharles@chromium.org
Note: I'm also renaming SwReporterInvocation::flags to SwReporterInvocation::supported_behaviours (as a consequence, the enum was also renamed), since I found it hard to understand at first glance (I didn't know if these were behaviours supported or enabled, specially because experimental versions of the reporter can disable them).
lgtm with nits https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:122: // Schedules the software reporter to run after browser startup once it's I think this is still ambiguous - "after browser startup" still makes me assume it means after the next time Chrome is started. How about "after the current browser startup sequence is finished"? https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:115: // Used to send UMA information if Software Reporter logs uploads are enabled. "if" makes it sound like this is only sent if uploads are enabled. How about "Used to send UMA information showing whether Software Reporter logs uploads are enabled, or the reason why not." https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:125: // uppload logs, when logs are enabled. Replicated in the histograms.xml file, Typos: "Reporter's attempt". "upload" logs. https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:66: using ReporterBehaviours = uint32_t; This could just be called "Behaviours" and the flags start with BEHAVIOUR_, since they're already in the SwReporterInvocation class it's clear they're talking about the reporter. And the enum names get pretty long. https://codereview.chromium.org/2347753002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2347753002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:57766: + <summary>The result of the most Software Reporter logs upload.</summary> Should be "most recent".
Thanks. Joe's comments addressed. https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:122: // Schedules the software reporter to run after browser startup once it's On 2016/09/16 14:57:13, Joe Mason wrote: > I think this is still ambiguous - "after browser startup" still makes me assume > it means after the next time Chrome is started. How about "after the current > browser startup sequence is finished"? Done. https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:115: // Used to send UMA information if Software Reporter logs uploads are enabled. On 2016/09/16 14:57:13, Joe Mason wrote: > "if" makes it sound like this is only sent if uploads are enabled. How about > "Used to send UMA information showing whether Software Reporter logs uploads are > enabled, or the reason why not." Done. https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:125: // uppload logs, when logs are enabled. Replicated in the histograms.xml file, On 2016/09/16 14:57:13, Joe Mason wrote: > Typos: "Reporter's attempt". "upload" logs. Done. https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.h:66: using ReporterBehaviours = uint32_t; On 2016/09/16 14:57:13, Joe Mason wrote: > This could just be called "Behaviours" and the flags start with BEHAVIOUR_, > since they're already in the SwReporterInvocation class it's clear they're > talking about the reporter. And the enum names get pretty long. Done. https://codereview.chromium.org/2347753002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2347753002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:57766: + <summary>The result of the most Software Reporter logs upload.</summary> On 2016/09/16 14:57:13, Joe Mason wrote: > Should be "most recent". Done.
https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:122: // Schedules the software reporter to run after browser startup once it's On 2016/09/16 14:57:13, Joe Mason wrote: > I think this is still ambiguous - "after browser startup" still makes me assume > it means after the next time Chrome is started. How about "after the current > browser startup sequence is finished"? how about "...to run sometime after browser startup is complete"? the first line of code just after the comment is PostAterStartupTask, so i don't think the comment is too ambiguous. https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:60: L"Software\\Google\\Software Removal Tool"; is this key used by Google Chrome, Google Chrome SxS, and Chromium? do you want all three touching the same key? what would happen if beta and dev could be installed alongside stable, and someone had stable, beta, dev, and canary all running. would your metrics be sensible in this case? https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:127: enum SwReporterLogsUploadsResult { ...LogsUploadResult? https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:128: REPORTER_LOGS_UPLOADS_RESULT_SUCCESS = 0, ...LOGS_UPLOAD_RESULT...? https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:128: REPORTER_LOGS_UPLOADS_RESULT_SUCCESS = 0, it appears that the only value used is REPORTER_LOGS_UPLOADS_RESULT_MAX. if the values can't be defined in a central place accessible both here and by the reporter(s), then what do you think about defining the max here with a comment pointing the reader toward the definitive location for the values (which should also have a comment indicating that the max here must be updated in lock-step). https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:155: const char kLogsUploadsResultMetricName[] = "SoftwareReporter.LogsUploadResult"; Uploads -> Upload https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:362: KEY_QUERY_VALUE | KEY_SET_VALUE); combining the RegKey ctor with KEY_SET_VALUE will cause the ctor to create the key if it doesn't exist. since you're just pulling data out here, i think it makes sense to use the default ctor and explicitly call Open with the args you care about. https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:369: static_cast<SwReporterLogsUploadsResult>(logs_upload_result), cast this to a Sample rather than the enum since that's what RecordEnumerationHistogram takes? https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:732: SwReporterInvocation::REPORTER_BEHAVIOUR_SEND_REPORTER_LOGS && wdyt about moving the enum out of SwReporterInvocation and use an enum class instead so that this could be something like: SwReporterBehaviour::SEND_REPORTER_LOGS ?
Thanks for the excellent review! PTAnL https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/componen... File chrome/browser/component_updater/sw_reporter_installer_win.cc (right): https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/componen... chrome/browser/component_updater/sw_reporter_installer_win.cc:122: // Schedules the software reporter to run after browser startup once it's On 2016/09/16 15:59:20, grt (UTC plus 2) wrote: > On 2016/09/16 14:57:13, Joe Mason wrote: > > I think this is still ambiguous - "after browser startup" still makes me > assume > > it means after the next time Chrome is started. How about "after the current > > browser startup sequence is finished"? > > how about "...to run sometime after browser startup is complete"? the first line > of code just after the comment is PostAterStartupTask, so i don't think the > comment is too ambiguous. I kept the "current browser", because even though it's not ambiguous if you check the code, it can be confusing for people not familiar with the browser thread lib. https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:60: L"Software\\Google\\Software Removal Tool"; On 2016/09/16 15:59:20, grt (UTC plus 2) wrote: > is this key used by Google Chrome, Google Chrome SxS, and Chromium? do you want > all three touching the same key? what would happen if beta and dev could be > installed alongside stable, and someone had stable, beta, dev, and canary all > running. would your metrics be sensible in this case? Good catch! Thanks for noticing the issue! The SwReporter component is only enabled for Google Chrome and Google Chrome SxS, even though different forks of Chromium can remove this #ifdef: https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?l=666. Even though the problem is not relevant today for the stable channel, it can affect the stats for the Canary population, so the bug needs to be fixed. I opened a bug to track this (https://bugs.chromium.org/p/chromium/issues/detail?id=647763), but we will only be able to change Chromium once the change in the CCT/SRT reaches most users. https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:127: enum SwReporterLogsUploadsResult { On 2016/09/16 15:59:20, grt (UTC plus 2) wrote: > ...LogsUploadResult? Done. https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:128: REPORTER_LOGS_UPLOADS_RESULT_SUCCESS = 0, On 2016/09/16 15:59:20, grt (UTC plus 2) wrote: > ...LOGS_UPLOAD_RESULT...? Done. https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:128: REPORTER_LOGS_UPLOADS_RESULT_SUCCESS = 0, On 2016/09/16 15:59:20, grt (UTC plus 2) wrote: > it appears that the only value used is REPORTER_LOGS_UPLOADS_RESULT_MAX. if the > values can't be defined in a central place accessible both here and by the > reporter(s), then what do you think about defining the max here with a comment > pointing the reader toward the definitive location for the values (which should > also have a comment indicating that the max here must be updated in lock-step). Since there is no central place accessible by Chromium and by the reporter, I will use the histogram enum sent in this CL as the definitive location. I'll also change the reporter to use the histogram enum as a source. https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:155: const char kLogsUploadsResultMetricName[] = "SoftwareReporter.LogsUploadResult"; On 2016/09/16 15:59:20, grt (UTC plus 2) wrote: > Uploads -> Upload Done. https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:362: KEY_QUERY_VALUE | KEY_SET_VALUE); On 2016/09/16 15:59:21, grt (UTC plus 2) wrote: > combining the RegKey ctor with KEY_SET_VALUE will cause the ctor to create the > key if it doesn't exist. since you're just pulling data out here, i think it > makes sense to use the default ctor and explicitly call Open with the args you > care about. Thanks for the hint! Fixed here and in the other places where the same issue occurred. About using KEY_QUERY_VALUE, KEY_SET_VALUE, I'll leave it to when this refactoring bug is handled: https://bugs.chromium.org/p/chromium/issues/detail?id=641081&desc=2 https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:369: static_cast<SwReporterLogsUploadsResult>(logs_upload_result), On 2016/09/16 15:59:20, grt (UTC plus 2) wrote: > cast this to a Sample rather than the enum since that's what > RecordEnumerationHistogram takes? Done. https://codereview.chromium.org/2347753002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/srt_fetcher_win.cc:732: SwReporterInvocation::REPORTER_BEHAVIOUR_SEND_REPORTER_LOGS && On 2016/09/16 15:59:20, grt (UTC plus 2) wrote: > wdyt about moving the enum out of SwReporterInvocation and use an enum class > instead so that this could be something like: > SwReporterBehaviour::SEND_REPORTER_LOGS > ? Done.
The CQ bit was checked by ftirelo@chromium.org to run a CQ dry run
The CQ bit was unchecked by ftirelo@chromium.org
The CQ bit was checked by ftirelo@chromium.org to run a CQ dry run
The CQ bit was unchecked by ftirelo@chromium.org
The CQ bit was checked by ftirelo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2347753002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2347753002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:117: // Used to send UMA information showing whether Software Reporter logs uploads feel fee to ignore this if the terminology is consistent with the project: i find "foo logs uploads are enabled" hard to read! how about "foo logs uploading is enabled" or "uploading of foo logs is enabled"? https://codereview.chromium.org/2347753002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2347753002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.h:62: SwReporterBehaviours operator&(SwReporterBehaviours a, SwReporterBehaviours b); oh my. this is overcomplicating things. i'm sorry i suggested using enum class! anything is better than this!
PTAnL https://codereview.chromium.org/2347753002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2347753002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:117: // Used to send UMA information showing whether Software Reporter logs uploads On 2016/09/16 20:12:42, grt (UTC plus 2) wrote: > feel fee to ignore this if the terminology is consistent with the project: i > find "foo logs uploads are enabled" hard to read! how about "foo logs uploading > is enabled" or "uploading of foo logs is enabled"? Done. https://codereview.chromium.org/2347753002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2347753002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.h:62: SwReporterBehaviours operator&(SwReporterBehaviours a, SwReporterBehaviours b); On 2016/09/16 20:12:42, grt (UTC plus 2) wrote: > oh my. this is overcomplicating things. i'm sorry i suggested using enum class! > anything is better than this! Done. However, please notice that I kep BehaviourIsEnabled() in the anonymous namespace in srt_fetcher_win.cc, because I think the conditions are more readable that way.
The CQ bit was checked by ftirelo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm
lgtm https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:367: Before recording logs_upload_result, could you force it to have a value we recognize? Some of the Finch dashboards don't like the ExitCode histogram because it report values outside of the expected bounds (Although I'm not quite sure what value an out of bound value should be converted to) https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:664: bool BehaviourIsEnabled(SwReporterInvocation::Behaviours behaviours, What about having this as a member of SwReporterInvocation? Then you implicitly always have the first param https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.h:65: // supported. This comment is slightly out of date, since by default all behaviours are disabled and then turned on as required. https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.h:71: BEHAVIOUR_SEND_REPORTER_LOGS = 0x8, As discussed, this might be clearer if it is named "BEHAVIOUR_ALLOW_SEND_REPORTER_LOGS"
https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:367: On 2016/09/20 14:29:54, csharp wrote: > Before recording logs_upload_result, could you force it to have a value we > recognize? Some of the Finch dashboards don't like the ExitCode histogram > because it report values outside of the expected bounds > > (Although I'm not quite sure what value an out of bound value should be > converted to) Maybe limit it to 30 to allow possible values to grow without needing to change Chrome. And maybe instead of converting to a different value, just use another histogram to report the error
https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:367: On 2016/09/20 15:19:28, csharp wrote: > On 2016/09/20 14:29:54, csharp wrote: > > Before recording logs_upload_result, could you force it to have a value we > > recognize? Some of the Finch dashboards don't like the ExitCode histogram > > because it report values outside of the expected bounds > > > > (Although I'm not quite sure what value an out of bound value should be > > converted to) > > Maybe limit it to 30 to allow possible values to grow without needing to change > Chrome. > > And maybe instead of converting to a different value, just use another histogram > to report the error Done. https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:367: On 2016/09/20 14:29:54, csharp wrote: > Before recording logs_upload_result, could you force it to have a value we > recognize? Some of the Finch dashboards don't like the ExitCode histogram > because it report values outside of the expected bounds > > (Although I'm not quite sure what value an out of bound value should be > converted to) Done. https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:664: bool BehaviourIsEnabled(SwReporterInvocation::Behaviours behaviours, On 2016/09/20 14:29:54, csharp wrote: > What about having this as a member of SwReporterInvocation? Then you implicitly > always have the first param Done. https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.h (right): https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.h:65: // supported. On 2016/09/20 14:29:54, csharp wrote: > This comment is slightly out of date, since by default all behaviours are > disabled and then turned on as required. Done. https://codereview.chromium.org/2347753002/diff/110001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.h:71: BEHAVIOUR_SEND_REPORTER_LOGS = 0x8, On 2016/09/20 14:29:54, csharp wrote: > As discussed, this might be clearer if it is named > "BEHAVIOUR_ALLOW_SEND_REPORTER_LOGS" Done.
ftirelo@chromium.org changed reviewers: + jwd@chromium.org
The CQ bit was checked by ftirelo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ftirelo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM with comments. https://codereview.chromium.org/2347753002/diff/130001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2347753002/diff/130001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:57759: + disabled. Mention when it's recorded. https://codereview.chromium.org/2347753002/diff/130001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:57767: + The result of the most recent Software Reporter logs upload. Can you be specific about when this is recorded. https://codereview.chromium.org/2347753002/diff/130001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:57775: + Error encountered when reading the software reporter logs upload reult from nit:reult->result
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2347753002/diff/130001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2347753002/diff/130001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:57759: + disabled. On 2016/09/20 22:27:57, jwd wrote: > Mention when it's recorded. Done. https://codereview.chromium.org/2347753002/diff/130001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:57767: + The result of the most recent Software Reporter logs upload. On 2016/09/20 22:27:57, jwd wrote: > Can you be specific about when this is recorded. Done. https://codereview.chromium.org/2347753002/diff/130001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:57775: + Error encountered when reading the software reporter logs upload reult from On 2016/09/20 22:27:57, jwd wrote: > nit:reult->result Done.
ftirelo@chromium.org changed reviewers: + jialiul@chromium.org, sorin@google.com
Adding jialiul@ and sorin@ for OWNERS approvals.
The CQ bit was checked by ftirelo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with nits for chrome/browser/safe_browsing/... https://codereview.chromium.org/2347753002/diff/150001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2347753002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:163: const int kSwReporterLogsUploadResultMax = 30; You changed this constant from 7 to 30. A little bit confusing. Are you trying to reserve more histogram buckets for future use? I think you can simply increase this const whenever you modify SoftwareReporter.LogsUploadResult in histogram.xml. So maybe keep 7 here, add note in the description of SoftwareReporter.LogsUploadResult in histogram.xml, to remind others update this const if they try to modify SoftwareReporter.LogsUploadResult? https://codereview.chromium.org/2347753002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:363: RecordEnumerationHistogram("SoftwareReporter.Step", value, SW_REPORTER_MAX); nit: Maybe make "SoftwareReporter.Step" a const as well?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2347753002/diff/150001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2347753002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:163: const int kSwReporterLogsUploadResultMax = 30; On 2016/09/21 17:41:18, Jialiu Lin wrote: > You changed this constant from 7 to 30. A little bit confusing. Are you trying > to reserve more histogram buckets for future use? I think you can simply > increase this const whenever you modify SoftwareReporter.LogsUploadResult in > histogram.xml. > > So maybe keep 7 here, add note in the description of > SoftwareReporter.LogsUploadResult in histogram.xml, to remind others update this > const if they try to modify SoftwareReporter.LogsUploadResult? The idea of moving from 7 to 30 was to reserve more histograms for future use without the dependency on Chrome release. This is particularly important in this case, because if we need to change the Software Reporter to emit a new result value, we only need to change histograms.xml, and the change will take effect immediately everywhere. On the other hand, if the constant is hard-coded here, then such changes in the reporter will need to wait for the whole Chrome release process. Please also notice that this doesn't affect Chrome, which is only used for reporting whatever is sent by the reporter. https://codereview.chromium.org/2347753002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:363: RecordEnumerationHistogram("SoftwareReporter.Step", value, SW_REPORTER_MAX); On 2016/09/21 17:41:18, Jialiu Lin wrote: > nit: Maybe make "SoftwareReporter.Step" a const as well? Done.
ftirelo@chromium.org changed reviewers: + sorin@chromium.org - sorin@google.com
https://codereview.chromium.org/2347753002/diff/150001/chrome/browser/safe_br... File chrome/browser/safe_browsing/srt_fetcher_win.cc (right): https://codereview.chromium.org/2347753002/diff/150001/chrome/browser/safe_br... chrome/browser/safe_browsing/srt_fetcher_win.cc:163: const int kSwReporterLogsUploadResultMax = 30; On 2016/09/21 18:45:18, ftirelo wrote: > On 2016/09/21 17:41:18, Jialiu Lin wrote: > > You changed this constant from 7 to 30. A little bit confusing. Are you trying > > to reserve more histogram buckets for future use? I think you can simply > > increase this const whenever you modify SoftwareReporter.LogsUploadResult in > > histogram.xml. > > > > So maybe keep 7 here, add note in the description of > > SoftwareReporter.LogsUploadResult in histogram.xml, to remind others update > this > > const if they try to modify SoftwareReporter.LogsUploadResult? > > The idea of moving from 7 to 30 was to reserve more histograms for future use > without the dependency on Chrome release. > > This is particularly important in this case, because if we need to change the > Software Reporter to emit a new result value, we only need to change > histograms.xml, and the change will take effect immediately everywhere. On the > other hand, if the constant is hard-coded here, then such changes in the > reporter will need to wait for the whole Chrome release process. Please also > notice that this doesn't affect Chrome, which is only used for reporting > whatever is sent by the reporter. Thanks for clarifying this. SGTM.
lgtm component updater paths lgtm
The CQ bit was checked by ftirelo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, csharp@chromium.org, joenotcharles@chromium.org, jwd@chromium.org, jialiul@chromium.org Link to the patchset: https://codereview.chromium.org/2347753002/#ps170001 (title: "Define constant")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:170001)
Message was sent while issue was closed.
Description was changed from ========== Adds histograms for tracking Software Reporter logs uploads in SRT Fetcher. BUG=634434 ========== to ========== Adds histograms for tracking Software Reporter logs uploads in SRT Fetcher. BUG=634434 Committed: https://crrev.com/c327e32928d28397da473e471c1fd77c8d2af638 Cr-Commit-Position: refs/heads/master@{#420516} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c327e32928d28397da473e471c1fd77c8d2af638 Cr-Commit-Position: refs/heads/master@{#420516} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
