Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2009773007:
Add AntiVirus information to the system profile. (Closed)
|
Created:
4 years, 7 months ago by Will Harris Modified:
4 years, 6 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, 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 AntiVirus information to the system profile.
Collection happens after Google Update data has been collected and
before drive metrics data, on the FILE thread.
BUG=615154
TEST=unit_tests
TEST=manual, check system profile contains AV information via logging.
Committed: https://crrev.com/c768983fac1178f0219e1f7aab38200c7d87d8b4
Cr-Commit-Position: refs/heads/master@{#398597}
Patch Set 1 #Patch Set 2 : fix tabs #Patch Set 3 : rejig task posting #Patch Set 4 : better test #Patch Set 5 : full test #Patch Set 6 : clang fix. check task always runs even on win7 #Patch Set 7 : rebase and fix clang #Patch Set 8 : hash the product name and version #
Total comments: 19
Patch Set 9 : rebase #Patch Set 10 : code review fixes #
Total comments: 26
Patch Set 11 : code review fixes part 2 #
Total comments: 12
Patch Set 12 : code review fixes part 3 #Patch Set 13 : add constexpr member definition #Patch Set 14 : no need for const constexpr #Patch Set 15 : rebase #Messages
Total messages: 27 (9 generated)
Description was changed from ========== Add AntiVirus information to the system profile. Collection happens after Google Update data has been collected and before drive metrics data, on the FILE thread. BUG=615154 ========== to ========== Add AntiVirus information to the system profile. Collection happens after Google Update data has been collected and before drive metrics data, on the FILE thread. BUG=615154 TEST=Run Chrome, wait a bit, verify base64 encoded AV data is in the saved_system_profile pref in Local State. ==========
Description was changed from ========== Add AntiVirus information to the system profile. Collection happens after Google Update data has been collected and before drive metrics data, on the FILE thread. BUG=615154 TEST=Run Chrome, wait a bit, verify base64 encoded AV data is in the saved_system_profile pref in Local State. ========== to ========== Add AntiVirus information to the system profile. Collection happens after Google Update data has been collected and before drive metrics data, on the FILE thread. Design doc - https://docs.google.com/document/d/1Rk6lBD7Geb4XYZUjgXsNb0JB2XLKjrhoBAxSF6GHl... BUG=615154 TEST=Run Chrome, wait a bit, verify base64 encoded AV data is in the saved_system_profile pref in Local State. ==========
Description was changed from ========== Add AntiVirus information to the system profile. Collection happens after Google Update data has been collected and before drive metrics data, on the FILE thread. Design doc - https://docs.google.com/document/d/1Rk6lBD7Geb4XYZUjgXsNb0JB2XLKjrhoBAxSF6GHl... BUG=615154 TEST=Run Chrome, wait a bit, verify base64 encoded AV data is in the saved_system_profile pref in Local State. ========== to ========== Add AntiVirus information to the system profile. Collection happens after Google Update data has been collected and before drive metrics data, on the FILE thread. BUG=615154 TEST=Run Chrome, wait a bit, verify base64 encoded AV data is in the saved_system_profile pref in Local State. ==========
wfh@chromium.org changed reviewers: + asvitkine@chromium.org
Description was changed from ========== Add AntiVirus information to the system profile. Collection happens after Google Update data has been collected and before drive metrics data, on the FILE thread. BUG=615154 TEST=Run Chrome, wait a bit, verify base64 encoded AV data is in the saved_system_profile pref in Local State. ========== to ========== Add AntiVirus information to the system profile. Collection happens after Google Update data has been collected and before drive metrics data, on the FILE thread. BUG=615154 TEST=unit_tests TEST=manual, check system profile contains AV information via logging. ==========
PTAL If you're happy with the proto I will draft a second internal CL for that.
https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:35: namespace metrics { Only things in components/metrics should be in the metrics namespace. https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:44: chrome::GetChannel() == version_info::Channel::UNKNOWN) { I'd rather we don't default unknown to true. Canary is easy to reason about, but unknown isn't. A default of not sending is safer. https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:49: base::CompareCase::INSENSITIVE_ASCII); Use base::Feature API instead - which is now the best practice for new experiment controls. go/feature-api-announcement. Although I'm also a bit skeptical we need this. If we keep it, maybe add a comment mentioning that the expectation is that it's disabled for majority of users. https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:90: product->set_product_name_hash(HashName(product_name)); How about doing all this logic once (as opposed to for every log like you do now), so this loop just copies the entries? In fact, you can even use the protobuf type for the intermediate storage and not need an extra struct. https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.h (right): https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.h:10: #include <iwscapi.h> Nit: No blank lines between the different system headers. https://codereview.chromium.org/2009773007/diff/140001/components/metrics/pro... File components/metrics/proto/system_profile.proto (right): https://codereview.chromium.org/2009773007/diff/140001/components/metrics/pro... components/metrics/proto/system_profile.proto:797: // The product name e.g. "System Center Endpoint Protection". Expand comment to mention that it might not be recorded. Same for the other string. https://codereview.chromium.org/2009773007/diff/140001/components/metrics/pro... components/metrics/proto/system_profile.proto:798: optional string product_name = 1; Nit: Add empty lines between the fields.
https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:44: chrome::GetChannel() == version_info::Channel::UNKNOWN) { On 2016/06/01 20:42:35, Alexei Svitkine (slow) wrote: > I'd rather we don't default unknown to true. Canary is easy to reason about, but > unknown isn't. A default of not sending is safer. I think the tests/bots run as unknown. if I make it Canary only, we will lose a bit of test coverage.
https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:44: chrome::GetChannel() == version_info::Channel::UNKNOWN) { On 2016/06/01 23:57:08, Will Harris wrote: > On 2016/06/01 20:42:35, Alexei Svitkine (slow) wrote: > > I'd rather we don't default unknown to true. Canary is easy to reason about, > but > > unknown isn't. A default of not sending is safer. > > I think the tests/bots run as unknown. if I make it Canary only, we will lose a > bit of test coverage. Then I suggest your tests to explicitly enable the feature, rather than relying on the channel.
Patchset #9 (id:160001) has been deleted
all done. PTAL. https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:35: namespace metrics { On 2016/06/01 20:42:34, Alexei Svitkine (slow) wrote: > Only things in components/metrics should be in the metrics namespace. Happy to move out of namespace but there seems to be some inconsistency in chrome/browser/metrics here. the following files have classes declared in the metrics namespace: android_metrics_provider.h chromeos_metrics_provider.h chrome_metrics_service_client.h extensions_metrics_provider.h leak_detector_controller.h perf/perf_provider_chromeos.h perf/windowed_incognito_observer.h thread_watcher_report_hang.h should these be fixed as well? https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:44: chrome::GetChannel() == version_info::Channel::UNKNOWN) { On 2016/06/02 15:17:18, Alexei Svitkine (slow) wrote: > On 2016/06/01 23:57:08, Will Harris wrote: > > On 2016/06/01 20:42:35, Alexei Svitkine (slow) wrote: > > > I'd rather we don't default unknown to true. Canary is easy to reason about, > > but > > > unknown isn't. A default of not sending is safer. > > > > I think the tests/bots run as unknown. if I make it Canary only, we will lose > a > > bit of test coverage. > > Then I suggest your tests to explicitly enable the feature, rather than relying > on the channel. Done. https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:49: base::CompareCase::INSENSITIVE_ASCII); On 2016/06/01 20:42:35, Alexei Svitkine (slow) wrote: > Use base::Feature API instead - which is now the best practice for new > experiment controls. go/feature-api-announcement. > > Although I'm also a bit skeptical we need this. If we keep it, maybe add a > comment mentioning that the expectation is that it's disabled for majority of > users. Done. https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:90: product->set_product_name_hash(HashName(product_name)); On 2016/06/01 20:42:35, Alexei Svitkine (slow) wrote: > How about doing all this logic once (as opposed to for every log like you do > now), so this loop just copies the entries? In fact, you can even use the > protobuf type for the intermediate storage and not need an extra struct. I wanted to have the FillAntiVirusProducts be callable from other areas of the code without a dependency on the proto, also I wanted a clear difference between the data collected (which is dependent on the MS apis) and the data put on the wire. I think unless you have strong views on this I'd prefer to leave it as it is. https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.h (right): https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.h:10: #include <iwscapi.h> On 2016/06/01 20:42:35, Alexei Svitkine (slow) wrote: > Nit: No blank lines between the different system headers. Done. https://codereview.chromium.org/2009773007/diff/140001/components/metrics/pro... File components/metrics/proto/system_profile.proto (right): https://codereview.chromium.org/2009773007/diff/140001/components/metrics/pro... components/metrics/proto/system_profile.proto:797: // The product name e.g. "System Center Endpoint Protection". On 2016/06/01 20:42:35, Alexei Svitkine (slow) wrote: > Expand comment to mention that it might not be recorded. Same for the other > string. Done. https://codereview.chromium.org/2009773007/diff/140001/components/metrics/pro... components/metrics/proto/system_profile.proto:798: optional string product_name = 1; On 2016/06/01 20:42:35, Alexei Svitkine (slow) wrote: > Nit: Add empty lines between the fields. Done.
https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:35: namespace metrics { On 2016/06/02 20:36:26, Will Harris wrote: > On 2016/06/01 20:42:34, Alexei Svitkine (slow) wrote: > > Only things in components/metrics should be in the metrics namespace. > > Happy to move out of namespace but there seems to be some inconsistency in > chrome/browser/metrics here. the following files have classes declared in the > metrics namespace: > > android_metrics_provider.h > chromeos_metrics_provider.h > chrome_metrics_service_client.h > extensions_metrics_provider.h > leak_detector_controller.h > perf/perf_provider_chromeos.h > perf/windowed_incognito_observer.h > thread_watcher_report_hang.h > > should these be fixed as well? Sigh. I didn't realize this many things were inconsistent. Well, I guess it doesn't really matter - if we clean this up later, we'll need to do it to all of these classes anyways - what's one more? https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:90: product->set_product_name_hash(HashName(product_name)); On 2016/06/02 20:36:27, Will Harris wrote: > On 2016/06/01 20:42:35, Alexei Svitkine (slow) wrote: > > How about doing all this logic once (as opposed to for every log like you do > > now), so this loop just copies the entries? In fact, you can even use the > > protobuf type for the intermediate storage and not need an extra struct. > > I wanted to have the FillAntiVirusProducts be callable from other areas of the > code without a dependency on the proto, also I wanted a clear difference between > the data collected (which is dependent on the MS apis) and the data put on the > wire. > > I think unless you have strong views on this I'd prefer to leave it as it is. I think it's inefficient to do this logic over and over again. I would at least generate a proto once that we then copy each time, without needing to do extra string conversions and hashing. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:122: FillAntiVirusProducts(&av_products); Do you want to report that AV queries failed somehow? For example, how do you differentiate "no AVs" vs. "error querying AVs"? Both seem like they would appear as an empty AV list with your current impl. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:189: if (FAILED(result)) Should we skip individual entries where we have a failure as opposed to returning false? Actually, with your current impl it seems you just end early and whatever was earlier would still be added. Doesn't seem optimal. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:198: metrics::SystemProfileProto::AntiVirusState_MAX) { Set to OTHER here instead? https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:233: av_product.product_version = version_info_win->product_version(); Can you make GetProductionVersion() a helper function to make the body of this loop easier to follow? https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.h (right): https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.h:29: "ReportFullAVProductDetails"; Nit: Just declare the Feature here if you're exporting something anyway and use .name on it in the test code. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.h:34: ~AntiVirusMetricsProvider() override; Nit: Add an empty line below. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.h:51: static AvProductList GetAntiVirusProductsOnFileThread(); Nit: Can this be in the anon namespace instead? https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win_unittest.cc (right): https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win_unittest.cc:27: void setFeatureEnabled(bool enabled) { Nit: set -> Set https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win_unittest.cc:118: if (product.product_name == L"Windows Defender") { Nit: Make this a constant at the top of the file. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win_unittest.cc:132: internalRunTest(true); I think you're looking for TEST_P() - please use that instead of re-inventing your own.
another round, thanks for being patient :) https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/140001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:35: namespace metrics { On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > On 2016/06/02 20:36:26, Will Harris wrote: > > On 2016/06/01 20:42:34, Alexei Svitkine (slow) wrote: > > > Only things in components/metrics should be in the metrics namespace. > > > > Happy to move out of namespace but there seems to be some inconsistency in > > chrome/browser/metrics here. the following files have classes declared in the > > metrics namespace: > > > > android_metrics_provider.h > > chromeos_metrics_provider.h > > chrome_metrics_service_client.h > > extensions_metrics_provider.h > > leak_detector_controller.h > > perf/perf_provider_chromeos.h > > perf/windowed_incognito_observer.h > > thread_watcher_report_hang.h > > > > should these be fixed as well? > > Sigh. I didn't realize this many things were inconsistent. Well, I guess it > doesn't really matter - if we clean this up later, we'll need to do it to all of > these classes anyways - what's one more? I'll leave this new code outside metrics namespace, since I've already made the change. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:122: FillAntiVirusProducts(&av_products); On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > Do you want to report that AV queries failed somehow? > > For example, how do you differentiate "no AVs" vs. "error querying AVs"? Both > seem like they would appear as an empty AV list with your current impl. added a histogram to report failures. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:189: if (FAILED(result)) On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > Should we skip individual entries where we have a failure as opposed to > returning false? > > Actually, with your current impl it seems you just end early and whatever was > earlier would still be added. Doesn't seem optimal. I'm making this fail, and if we get lots of errors in the new histogram I can re-evaluate. These functions really shouldn't be failing at all. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:198: metrics::SystemProfileProto::AntiVirusState_MAX) { On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > Set to OTHER here instead? should be covered by RESULT_PRODUCT_STATE_INVALID. Again, this function should never return anything other than the documented set of values. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:233: av_product.product_version = version_info_win->product_version(); On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > Can you make GetProductionVersion() a helper function to make the body of this > loop easier to follow? Done. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.h (right): https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.h:29: "ReportFullAVProductDetails"; On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > Nit: Just declare the Feature here if you're exporting something anyway and use > .name on it in the test code. Done. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.h:34: ~AntiVirusMetricsProvider() override; On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > Nit: Add an empty line below. Done. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.h:51: static AvProductList GetAntiVirusProductsOnFileThread(); On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > Nit: Can this be in the anon namespace instead? moving it to anonymous would mean I'd have to declare the return type public which I'd rather not do, unless I'm missing some trick here. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win_unittest.cc (right): https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win_unittest.cc:27: void setFeatureEnabled(bool enabled) { On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > Nit: set -> Set Done. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win_unittest.cc:105: TEST_F(AntiVirusMetricsProviderTest, GetAvProducts) { this test wasn't adding any value any more so I removed it, it's subsumed by the other test anyway. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win_unittest.cc:118: if (product.product_name == L"Windows Defender") { On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > Nit: Make this a constant at the top of the file. Done, but put it closest to first use. https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win_unittest.cc:132: internalRunTest(true); On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > I think you're looking for TEST_P() - please use that instead of re-inventing > your own. I did not know about TEST_P. That's cool. Thanks!
Thanks, almost there! Can you prepare an internal CL with the same proto changes in the mean time? (The internal copy is the master copy, so we require that landed first before the Chromium proto change.) https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:198: metrics::SystemProfileProto::AntiVirusState_MAX) { On 2016/06/02 23:57:17, Will Harris wrote: > On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > > Set to OTHER here instead? > > should be covered by RESULT_PRODUCT_STATE_INVALID. Again, this function should > never return anything other than the documented set of values. I still don't buy this - but maybe I'm missing something - so let me walk through my thoughts. Here is the scenario: Microsoft releases Windows 10.5 (pardon my ignorance of what the versioning process of new Windows is). They expand the enum to provide 3 more values. The existing enums still have the same values. So the static asserts above are still OK. (And in fact those asserts only compare against the header files of the SDK we're compiling against - e.g. if we're still compiling against win10 sdk then the static asserts won't even catch anything). Anyway, so we now have 3 new enum values. The API can start returning these. It won't return RESULT_PRODUCT_STATE_INVALID because it's a valid new state. Then, it will hit this condition and so the question is how to handle it. (And I think using a special OTHER value is a reasonable way.) Now, maybe I'm wrong. For example, some APIs let you pass in the version of the API you're compiled against when using it, and then the library has special logic to only return you values you expect. Is that the case here? (If so, I missed it.) https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:117: *product = std::move(av_product); This doesn't look right to me. This function will be called multiple times (each time a UMA log is cut), so if you std::move() then the next time it's called, av_products_ will be a list of empty products. https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:137: UMA_HISTOGRAM_ENUMERATION("AntiVirusMetricsProvider.Result", Nit: Prefix the name with "UMA." https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.h (right): https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.h:29: static constexpr const base::Feature kFeature = { kFeature is not very informative. How about kReportNamesFeature? https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.h:41: // Called to gather metrics, before calling ProvideSystemProfileMetrics. Nit: This comment explains how its used, but how its used is outside of the class implementation - so it should be formulated as "how it should be used". Also, mention that it gathers the data asynchronously and calls done_callback when done. https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.h:63: typedef std::vector<AvProduct> AvProductList; Nit: I don't think having this typedef is very useful. The one above it makes sense, but I think spelling out std::vector<AvProduct> is more clear. https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:452: content::BrowserThread::FILE)); Any reason to use FILE instead of the worker pool? I think worker pool is generally preferred these days.
are you looking at the right ps? https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:198: metrics::SystemProfileProto::AntiVirusState_MAX) { On 2016/06/03 15:48:33, Alexei Svitkine (slow) wrote: > On 2016/06/02 23:57:17, Will Harris wrote: > > On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > > > Set to OTHER here instead? > > > > should be covered by RESULT_PRODUCT_STATE_INVALID. Again, this function should > > never return anything other than the documented set of values. > > I still don't buy this - but maybe I'm missing something - so let me walk > through my thoughts. > > Here is the scenario: > > Microsoft releases Windows 10.5 (pardon my ignorance of what the versioning > process of new Windows is). They expand the enum to provide 3 more values. > > The existing enums still have the same values. So the static asserts above are > still OK. (And in fact those asserts only compare against the header files of > the SDK we're compiling against - e.g. if we're still compiling against win10 > sdk then the static asserts won't even catch anything). > > Anyway, so we now have 3 new enum values. The API can start returning these. It > won't return RESULT_PRODUCT_STATE_INVALID because it's a valid new state. Then, > it will hit this condition and so the question is how to handle it. (And I think > using a special OTHER value is a reasonable way.) > > Now, maybe I'm wrong. For example, some APIs let you pass in the version of the > API you're compiled against when using it, and then the library has special > logic to only return you values you expect. Is that the case here? (If so, I > missed it.) the latest code uses metrics::SystemProfileProto_AntiVirusState_IsValid to check, which would trip if new enums were added.
https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:198: metrics::SystemProfileProto::AntiVirusState_MAX) { On 2016/06/03 16:01:21, Will Harris wrote: > On 2016/06/03 15:48:33, Alexei Svitkine (slow) wrote: > > On 2016/06/02 23:57:17, Will Harris wrote: > > > On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > > > > Set to OTHER here instead? > > > > > > should be covered by RESULT_PRODUCT_STATE_INVALID. Again, this function > should > > > never return anything other than the documented set of values. > > > > I still don't buy this - but maybe I'm missing something - so let me walk > > through my thoughts. > > > > Here is the scenario: > > > > Microsoft releases Windows 10.5 (pardon my ignorance of what the versioning > > process of new Windows is). They expand the enum to provide 3 more values. > > > > The existing enums still have the same values. So the static asserts above are > > still OK. (And in fact those asserts only compare against the header files of > > the SDK we're compiling against - e.g. if we're still compiling against win10 > > sdk then the static asserts won't even catch anything). > > > > Anyway, so we now have 3 new enum values. The API can start returning these. > It > > won't return RESULT_PRODUCT_STATE_INVALID because it's a valid new state. > Then, > > it will hit this condition and so the question is how to handle it. (And I > think > > using a special OTHER value is a reasonable way.) > > > > Now, maybe I'm wrong. For example, some APIs let you pass in the version of > the > > API you're compiled against when using it, and then the library has special > > logic to only return you values you expect. Is that the case here? (If so, I > > missed it.) > > the latest code uses metrics::SystemProfileProto_AntiVirusState_IsValid to > check, which would trip if new enums were added. I agree it's cleaner to use that to check, however I think the concern from my example still applies.
https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:198: metrics::SystemProfileProto::AntiVirusState_MAX) { On 2016/06/03 16:14:15, Alexei Svitkine (slow) wrote: > On 2016/06/03 16:01:21, Will Harris wrote: > > On 2016/06/03 15:48:33, Alexei Svitkine (slow) wrote: > > > On 2016/06/02 23:57:17, Will Harris wrote: > > > > On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > > > > > Set to OTHER here instead? > > > > > > > > should be covered by RESULT_PRODUCT_STATE_INVALID. Again, this function > > should > > > > never return anything other than the documented set of values. > > > > > > I still don't buy this - but maybe I'm missing something - so let me walk > > > through my thoughts. > > > > > > Here is the scenario: > > > > > > Microsoft releases Windows 10.5 (pardon my ignorance of what the versioning > > > process of new Windows is). They expand the enum to provide 3 more values. > > > > > > The existing enums still have the same values. So the static asserts above > are > > > still OK. (And in fact those asserts only compare against the header files > of > > > the SDK we're compiling against - e.g. if we're still compiling against > win10 > > > sdk then the static asserts won't even catch anything). > > > > > > Anyway, so we now have 3 new enum values. The API can start returning these. > > It > > > won't return RESULT_PRODUCT_STATE_INVALID because it's a valid new state. > > Then, > > > it will hit this condition and so the question is how to handle it. (And I > > think > > > using a special OTHER value is a reasonable way.) > > > > > > Now, maybe I'm wrong. For example, some APIs let you pass in the version of > > the > > > API you're compiled against when using it, and then the library has special > > > logic to only return you values you expect. Is that the case here? (If so, I > > > missed it.) > > > > the latest code uses metrics::SystemProfileProto_AntiVirusState_IsValid to > > check, which would trip if new enums were added. > > I agree it's cleaner to use that to check, however I think the concern from my > example still applies. Either way achieves the same end goal. with the OTHER member in the enum, if MS changes the API and adds new enums, then we get a load of OTHER, with the way it is is now, we stop getting (potentially invalid, since the API has changed, what else has changed?) data and instead get a lot of RESULT_PRODUCT_STATE_INVALID. Either way, we will need to investigate what has changed, and probably fix it. I prefer the RESULT_PRODUCT_STATE_INVALID way, and you prefer the OTHER way. It seems I don't feel as strongly about it as you are, so I'm happy to change it to the other way - although I don't agree, but you're the owner, so :) - but I don't think the problems you are outlining in your comment are something to be concerned about.
https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/200001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:198: metrics::SystemProfileProto::AntiVirusState_MAX) { On 2016/06/03 16:28:56, Will Harris wrote: > On 2016/06/03 16:14:15, Alexei Svitkine (slow) wrote: > > On 2016/06/03 16:01:21, Will Harris wrote: > > > On 2016/06/03 15:48:33, Alexei Svitkine (slow) wrote: > > > > On 2016/06/02 23:57:17, Will Harris wrote: > > > > > On 2016/06/02 21:46:57, Alexei Svitkine (slow) wrote: > > > > > > Set to OTHER here instead? > > > > > > > > > > should be covered by RESULT_PRODUCT_STATE_INVALID. Again, this function > > > should > > > > > never return anything other than the documented set of values. > > > > > > > > I still don't buy this - but maybe I'm missing something - so let me walk > > > > through my thoughts. > > > > > > > > Here is the scenario: > > > > > > > > Microsoft releases Windows 10.5 (pardon my ignorance of what the > versioning > > > > process of new Windows is). They expand the enum to provide 3 more values. > > > > > > > > The existing enums still have the same values. So the static asserts above > > are > > > > still OK. (And in fact those asserts only compare against the header files > > of > > > > the SDK we're compiling against - e.g. if we're still compiling against > > win10 > > > > sdk then the static asserts won't even catch anything). > > > > > > > > Anyway, so we now have 3 new enum values. The API can start returning > these. > > > It > > > > won't return RESULT_PRODUCT_STATE_INVALID because it's a valid new state. > > > Then, > > > > it will hit this condition and so the question is how to handle it. (And I > > > think > > > > using a special OTHER value is a reasonable way.) > > > > > > > > Now, maybe I'm wrong. For example, some APIs let you pass in the version > of > > > the > > > > API you're compiled against when using it, and then the library has > special > > > > logic to only return you values you expect. Is that the case here? (If so, > I > > > > missed it.) > > > > > > the latest code uses metrics::SystemProfileProto_AntiVirusState_IsValid to > > > check, which would trip if new enums were added. > > > > I agree it's cleaner to use that to check, however I think the concern from my > > example still applies. > > Either way achieves the same end goal. with the OTHER member in the enum, if MS > changes the API and adds new enums, then we get a load of OTHER, with the way it > is is now, we stop getting (potentially invalid, since the API has changed, what > else has changed?) data and instead get a lot of RESULT_PRODUCT_STATE_INVALID. > > Either way, we will need to investigate what has changed, and probably fix it. > > I prefer the RESULT_PRODUCT_STATE_INVALID way, and you prefer the OTHER way. It > seems I don't feel as strongly about it as you are, so I'm happy to change it to > the other way - although I don't agree, but you're the owner, so :) - but I > don't think the problems you are outlining in your comment are something to be > concerned about. OK. It does mean that we'll want to have this histogram forever and to monitor it, but I guess that's fine-ish.
getting closer... :) https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.cc (right): https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:117: *product = std::move(av_product); On 2016/06/03 15:48:33, Alexei Svitkine (slow) wrote: > This doesn't look right to me. This function will be called multiple times (each > time a UMA log is cut), so if you std::move() then the next time it's called, > av_products_ will be a list of empty products. so it turns out because I am using a const iterator the compiler knows to make a copy rather than a move, but I agree this is slightly misleading so I've changed it. Also made the type explicit to make it clear that product is a pointer. https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.cc:137: UMA_HISTOGRAM_ENUMERATION("AntiVirusMetricsProvider.Result", On 2016/06/03 15:48:33, Alexei Svitkine (slow) wrote: > Nit: Prefix the name with "UMA." Done. https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/antivirus_metrics_provider_win.h (right): https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.h:29: static constexpr const base::Feature kFeature = { On 2016/06/03 15:48:33, Alexei Svitkine (slow) wrote: > kFeature is not very informative. > > How about kReportNamesFeature? Done. https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.h:41: // Called to gather metrics, before calling ProvideSystemProfileMetrics. On 2016/06/03 15:48:33, Alexei Svitkine (slow) wrote: > Nit: This comment explains how its used, but how its used is outside of the > class implementation - so it should be formulated as "how it should be used". > Also, mention that it gathers the data asynchronously and calls done_callback > when done. I chose bad files to copy comments from :) Done. https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... chrome/browser/metrics/antivirus_metrics_provider_win.h:63: typedef std::vector<AvProduct> AvProductList; On 2016/06/03 15:48:33, Alexei Svitkine (slow) wrote: > Nit: I don't think having this typedef is very useful. The one above it makes > sense, but I think spelling out std::vector<AvProduct> is more clear. Done. https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2009773007/diff/220001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:452: content::BrowserThread::FILE)); On 2016/06/03 15:48:34, Alexei Svitkine (slow) wrote: > Any reason to use FILE instead of the worker pool? I think worker pool is > generally preferred these days. I was using the same thread that GetDriveMetrics runs on, but I've switched to the blocking pool. Thanks.
Looks good. Will wait for the internal proto cl to land first before l-g-t-m-ing.
lgtm
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2009773007/300001
Message was sent while issue was closed.
Description was changed from ========== Add AntiVirus information to the system profile. Collection happens after Google Update data has been collected and before drive metrics data, on the FILE thread. BUG=615154 TEST=unit_tests TEST=manual, check system profile contains AV information via logging. ========== to ========== Add AntiVirus information to the system profile. Collection happens after Google Update data has been collected and before drive metrics data, on the FILE thread. BUG=615154 TEST=unit_tests TEST=manual, check system profile contains AV information via logging. ==========
Message was sent while issue was closed.
Committed patchset #15 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Add AntiVirus information to the system profile. Collection happens after Google Update data has been collected and before drive metrics data, on the FILE thread. BUG=615154 TEST=unit_tests TEST=manual, check system profile contains AV information via logging. ========== to ========== Add AntiVirus information to the system profile. Collection happens after Google Update data has been collected and before drive metrics data, on the FILE thread. BUG=615154 TEST=unit_tests TEST=manual, check system profile contains AV information via logging. Committed: https://crrev.com/c768983fac1178f0219e1f7aab38200c7d87d8b4 Cr-Commit-Position: refs/heads/master@{#398597} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/c768983fac1178f0219e1f7aab38200c7d87d8b4 Cr-Commit-Position: refs/heads/master@{#398597} |
