Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2703363002:
Network traffic annotation added to NetMetricsLogUploader. (Closed)
|
Created:
3 years, 10 months ago by Ramin Halavati Modified:
3 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org, net-reviews_chromium.org, msramek Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to NetMetricsLogUploader.
Network traffic annotation is added to network request of
NetMetricsLogUploader.
BUG=656607
Review-Url: https://codereview.chromium.org/2703363002
Cr-Commit-Position: refs/heads/master@{#456024}
Committed: https://chromium.googlesource.com/chromium/src/+/5cba00e049c9802bb5a403d73ee8b184701e21c2
Patch Set 1 #
Total comments: 2
Patch Set 2 : Annotation updated. #
Total comments: 6
Patch Set 3 : Annotation updated. #
Total comments: 2
Patch Set 4 : nits #
Total comments: 2
Patch Set 5 : nits #
Total comments: 2
Patch Set 6 : nits #
Total comments: 13
Patch Set 7 : nits #Patch Set 8 : nits #Patch Set 9 : nits #
Total comments: 9
Patch Set 10 : Annotation updated. #
Total comments: 6
Patch Set 11 : Comments addressed. #Patch Set 12 : nits #Patch Set 13 : nits #
Total comments: 2
Patch Set 14 : Check added to ensure correct annotation based on service type. #
Total comments: 1
Patch Set 15 : Comment addressed. #Messages
Total messages: 53 (15 generated)
rhalavati@chromium.org changed reviewers: + holte@chromium.org
We are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit the requests and configure Chrome in a way that meets their security policies and expectations. Furthermore, it allows us to generate better debugging data in chrome://net-internals and measure bandwidth consumption on a per-request-type basis. I've modified NetMetricsLogUploader and added annotation template to it. Please review it and suggest the required answers for the empty parts. Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/cl... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
holte@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine https://codereview.chromium.org/2703363002/diff/1/components/metrics/net/net_... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/1/components/metrics/net/net_... components/metrics/net/net_metrics_log_uploader.cc:30: net::NetworkTrafficAnnotationTag traffic_annotation = net::DefineNetworkTrafficAnnotation("metrics_report", R"( + semantics { + sender: "metrics" + description: "Report of usage statistics and crash-related data about Google Chrome." + trigger: "Reports are automatically generated." + data: "A protocol buffer with usage statistics and crash related data." + destination: GOOGLE_OWNED_SERVICE + } + policy { + cookies_allowed: false + setting: "This is controlled by Settings > Advanced Settings > Privacy > Automatically send usage statistics and crash reports to Google." + policy { + MetricsReportingEnabled { + policy_options {mode: MANDATORY} + value: false + } + } + })");
Annotation updated, please review. https://codereview.chromium.org/2703363002/diff/1/components/metrics/net/net_... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/1/components/metrics/net/net_... components/metrics/net/net_metrics_log_uploader.cc:30: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/02/21 22:45:03, Steven Holte wrote: > net::DefineNetworkTrafficAnnotation("metrics_report", R"( > + semantics { > + sender: "metrics" > + description: "Report of usage statistics and crash-related data about > Google Chrome." > + trigger: "Reports are automatically generated." > + data: "A protocol buffer with usage statistics and crash related > data." > + destination: GOOGLE_OWNED_SERVICE > + } > + policy { > + cookies_allowed: false > + setting: > "This is controlled by Settings > Advanced Settings > Privacy > > Automatically send usage statistics and crash reports to Google." > + policy { > + MetricsReportingEnabled { > + policy_options {mode: MANDATORY} > + value: false > + } > + } > + })"); Done. https://codereview.chromium.org/2703363002/diff/20001/components/metrics/net/... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/20001/components/metrics/net/... components/metrics/net/net_metrics_log_uploader.cc:36: "Chrome." Would you like to add any details here, like example, a link to a specification doc, etc.? https://codereview.chromium.org/2703363002/diff/20001/components/metrics/net/... components/metrics/net/net_metrics_log_uploader.cc:37: trigger: "Reports are automatically generated." Would you like to add any occasion, interval, etc., specifying why and when they are generated?
https://codereview.chromium.org/2703363002/diff/20001/components/metrics/net/... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/20001/components/metrics/net/... components/metrics/net/net_metrics_log_uploader.cc:36: "Chrome." On 2017/02/22 11:48:08, Ramin Halavati wrote: > Would you like to add any details here, like example, a link to a specification > doc, etc.? The best links might be this one: https://support.google.com/chrome/answer/96817 or maybe https://www.google.com/chrome/browser/privacy/ the relevant text from privacy doc is: Usage statistics contain information such as preferences, button clicks, and memory usage. Usage statistics do not include web page URLs or personal information. I'm not sure that we'd want to include that directly here, because I worry it would not be kept up to date as accurately. https://codereview.chromium.org/2703363002/diff/20001/components/metrics/net/... components/metrics/net/net_metrics_log_uploader.cc:37: trigger: "Reports are automatically generated." On 2017/02/22 11:48:09, Ramin Halavati wrote: > Would you like to add any occasion, interval, etc., specifying why and when they > are generated? Currently the reports are generated on startup, and approximately every 5 minutes (though I believe there is a different interval for devices on cellular connections). I'm not sure that we won't change that interval, so I'd be a bit worried about this getting out of date. Maybe it could say "on startup and at intervals while chrome is running."
Annotation updated, please review. https://codereview.chromium.org/2703363002/diff/20001/components/metrics/net/... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/20001/components/metrics/net/... components/metrics/net/net_metrics_log_uploader.cc:36: "Chrome." On 2017/02/22 20:10:46, Steven Holte wrote: > On 2017/02/22 11:48:08, Ramin Halavati wrote: > > Would you like to add any details here, like example, a link to a > specification > > doc, etc.? > > The best links might be this one: > > https://support.google.com/chrome/answer/96817 > or > maybe https://www.google.com/chrome/browser/privacy/ > > the relevant text from privacy doc is: > > Usage statistics contain information such as preferences, button clicks, and > memory usage. Usage statistics do not include web page URLs or personal > information. > > I'm not sure that we'd want to include that directly here, because I worry it > would not be kept up to date as accurately. Thanks. https://codereview.chromium.org/2703363002/diff/20001/components/metrics/net/... components/metrics/net/net_metrics_log_uploader.cc:37: trigger: "Reports are automatically generated." On 2017/02/22 20:10:47, Steven Holte wrote: > On 2017/02/22 11:48:09, Ramin Halavati wrote: > > Would you like to add any occasion, interval, etc., specifying why and when > they > > are generated? > > Currently the reports are generated on startup, and approximately every 5 > minutes (though I believe there is a different interval for devices on cellular > connections). I'm not sure that we won't change that interval, so I'd be a bit > worried about this getting out of date. Maybe it could say "on startup and at > intervals while chrome is running." Done.
lgtm https://codereview.chromium.org/2703363002/diff/40001/components/metrics/net/... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/40001/components/metrics/net/... components/metrics/net/net_metrics_log_uploader.cc:43: "while chrome is running." Capitalize Chrome
lgtm
rhalavati@chromium.org changed reviewers: + battre@chromium.org
+battre@ Comments addressed. https://codereview.chromium.org/2703363002/diff/40001/components/metrics/net/... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/40001/components/metrics/net/... components/metrics/net/net_metrics_log_uploader.cc:43: "while chrome is running." On 2017/02/23 19:32:25, Alexei Svitkine (slow) wrote: > Capitalize Chrome Done.
Alexei will UKMs use a different uploader? In that case: LGTM https://codereview.chromium.org/2703363002/diff/60001/components/metrics/net/... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/60001/components/metrics/net/... components/metrics/net/net_metrics_log_uploader.cc:51: "Users can enable or disable this feature by stoping " by disabling
+rkaplow On Fri, Feb 24, 2017 at 2:59 AM, <battre@chromium.org> wrote: > Alexei will UKMs use a different uploader? In that case: LGTM > Actually, this class is used by both - so we'll need to make the distinction here. Luckily, Rob has a CL to add plumbing to provide this information to this class - so this CL should be rebased on top of his CL to do so (https://codereview.chromium.org/2708293002/) and then provide a different struct for UKM. > > > https://codereview.chromium.org/2703363002/diff/60001/ > components/metrics/net/net_metrics_log_uploader.cc > File components/metrics/net/net_metrics_log_uploader.cc (right): > > https://codereview.chromium.org/2703363002/diff/60001/ > components/metrics/net/net_metrics_log_uploader.cc#newcode51 > components/metrics/net/net_metrics_log_uploader.cc:51: "Users can enable > or disable this feature by stoping " > by disabling > > https://codereview.chromium.org/2703363002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
rkaplow@chromium.org changed reviewers: + rkaplow@chromium.org
My CL is submitted which splits the data from this between UMA and UKM. When this is updated can it handle both?
asvltkine@, rkaplow@, Updates done to match the new CL, please review again. https://codereview.chromium.org/2703363002/diff/60001/components/metrics/net/... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/60001/components/metrics/net/... components/metrics/net/net_metrics_log_uploader.cc:51: "Users can enable or disable this feature by stoping " On 2017/02/24 07:59:33, battre wrote: > by disabling Done.
https://codereview.chromium.org/2703363002/diff/80001/components/metrics/net/... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/80001/components/metrics/net/... components/metrics/net/net_metrics_log_uploader.cc:35: net::DefineNetworkTrafficAnnotation("metrics_report", R"( This is only correct for the MetricsLogUploader::UMA case. You need to provide different descriptions for the MetricsLogUploader::UKM case. Rob and Dominic should be able to help you formulate these.
Annotation for two cases separated, please advise differences. https://codereview.chromium.org/2703363002/diff/80001/components/metrics/net/... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/80001/components/metrics/net/... components/metrics/net/net_metrics_log_uploader.cc:35: net::DefineNetworkTrafficAnnotation("metrics_report", R"( On 2017/02/27 15:42:56, Alexei Svitkine (slow) wrote: > This is only correct for the MetricsLogUploader::UMA case. You need to provide > different descriptions for the MetricsLogUploader::UKM case. > > Rob and Dominic should be able to help you formulate these. I separate the two cases.
https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:39: net::NetworkTrafficAnnotationTag traffic_annotation = Can you declare NetworkTrafficAnnotationTag traffic_annotation outside of the switch and assign to it within? Then, the URLFetcher::Create() code can be common below the switch. You also won't need the {}'s. https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:115: default: { DCHECK(false); } Compiler will error when an enum is not covered by the switch, so no need to add this case.
https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:81: sender: "Metrics Log Uploader" how is the sender field used? https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:83: "Report of usage statistics and crash-related data about " this is not correct for UKM. This is the draft of the current policy wording: https://docs.google.com/document/d/1hKVEH7DjVMeu5xgHde4XsNABh4L2QluL7PmeaoDcY... Would you like me to suggest wording for this section? https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:100: "Users can enable or disable this feature by disabling " this is also not fully correct, you should mention this can also be disabling by disabling History Sync (or Sync)
Annotation updated, please review. https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:39: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/02/27 18:24:08, Alexei Svitkine (slow) wrote: > Can you declare NetworkTrafficAnnotationTag traffic_annotation outside of the > switch and assign to it within? Then, the URLFetcher::Create() code can be > common below the switch. You also won't need the {}'s. That makes the clang tool that extracts annotations quite more complicated as it should follow two paths. https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:81: sender: "Metrics Log Uploader" On 2017/02/27 22:52:58, rkaplow (slow) wrote: > how is the sender field used? > It's used in the report stating which module is making this request. It does not need to be unique. https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:83: "Report of usage statistics and crash-related data about " On 2017/02/27 22:52:58, rkaplow (slow) wrote: > this is not correct for UKM. > This is the draft of the current policy wording: > https://docs.google.com/document/d/1hKVEH7DjVMeu5xgHde4XsNABh4L2QluL7PmeaoDcY... > > Would you like me to suggest wording for this section? Yes please, thank you. https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:100: "Users can enable or disable this feature by disabling " On 2017/02/27 22:52:58, rkaplow (slow) wrote: > this is also not fully correct, you should mention this can also be disabling by > disabling History Sync (or Sync) Could you please suggest wording for this one as well? https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:115: default: { DCHECK(false); } On 2017/02/27 18:24:08, Alexei Svitkine (slow) wrote: > Compiler will error when an enum is not covered by the switch, so no need to add > this case. Done.
https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:39: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/02/28 08:55:39, Ramin Halavati wrote: > On 2017/02/27 18:24:08, Alexei Svitkine (slow) wrote: > > Can you declare NetworkTrafficAnnotationTag traffic_annotation outside of the > > switch and assign to it within? Then, the URLFetcher::Create() code can be > > common below the switch. You also won't need the {}'s. > > That makes the clang tool that extracts annotations quite more complicated as it > should follow two paths. I see. Please add a comment about this - so that someone doesn't refactor this in the future.
https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:39: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/02/28 16:01:50, Alexei Svitkine (slow) wrote: > On 2017/02/28 08:55:39, Ramin Halavati wrote: > > On 2017/02/27 18:24:08, Alexei Svitkine (slow) wrote: > > > Can you declare NetworkTrafficAnnotationTag traffic_annotation outside of > the > > > switch and assign to it within? Then, the URLFetcher::Create() code can be > > > common below the switch. You also won't need the {}'s. > > > > That makes the clang tool that extracts annotations quite more complicated as > it > > should follow two paths. > > I see. Please add a comment about this - so that someone doesn't refactor this > in the future. Why is this the case? Didn't we discuss that we could have even static global const objects for the traffic annotation?
Comment addressed, please review and suggest changes for the UKM part. https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:39: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/02/28 16:06:14, battre wrote: > On 2017/02/28 16:01:50, Alexei Svitkine (slow) wrote: > > On 2017/02/28 08:55:39, Ramin Halavati wrote: > > > On 2017/02/27 18:24:08, Alexei Svitkine (slow) wrote: > > > > Can you declare NetworkTrafficAnnotationTag traffic_annotation outside of > > the > > > > switch and assign to it within? Then, the URLFetcher::Create() code can be > > > > common below the switch. You also won't need the {}'s. > > > > > > That makes the clang tool that extracts annotations quite more complicated > as > > it > > > should follow two paths. > > > > I see. Please add a comment about this - so that someone doesn't refactor this > > in the future. > > Why is this the case? Didn't we discuss that we could have even static global > const objects for the traffic annotation? This is a different issue, I had changed it this way because our current report follows up the annotation down to the function that uses it and if two different annotations are given to one function, it should follow this path through case and if blocks as well. But as you mentioned before this is not an important part of the report and we can drop it for cases that clang tool cannot find it. So I will change the code to the way Alexei suggested.
On 2017/03/01 05:52:50, Ramin Halavati wrote: > Comment addressed, please review and suggest changes for the UKM part. > > https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... > File components/metrics/net/net_metrics_log_uploader.cc (right): > > https://codereview.chromium.org/2703363002/diff/100001/components/metrics/net... > components/metrics/net/net_metrics_log_uploader.cc:39: > net::NetworkTrafficAnnotationTag traffic_annotation = > On 2017/02/28 16:06:14, battre wrote: > > On 2017/02/28 16:01:50, Alexei Svitkine (slow) wrote: > > > On 2017/02/28 08:55:39, Ramin Halavati wrote: > > > > On 2017/02/27 18:24:08, Alexei Svitkine (slow) wrote: > > > > > Can you declare NetworkTrafficAnnotationTag traffic_annotation outside > of > > > the > > > > > switch and assign to it within? Then, the URLFetcher::Create() code can > be > > > > > common below the switch. You also won't need the {}'s. > > > > > > > > That makes the clang tool that extracts annotations quite more complicated > > as > > > it > > > > should follow two paths. > > > > > > I see. Please add a comment about this - so that someone doesn't refactor > this > > > in the future. > > > > Why is this the case? Didn't we discuss that we could have even static global > > const objects for the traffic annotation? > > This is a different issue, I had changed it this way because our current report > follows up the annotation down to the function that uses it and if two different > annotations are given to one function, it should follow this path through case > and if blocks as well. But as you mentioned before this is not an important part > of the report and we can drop it for cases that clang tool cannot find it. So I > will change the code to the way Alexei suggested. Sorry, I cannot parse what you say.
https://codereview.chromium.org/2703363002/diff/160001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/160001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:79: sender: "Metrics Log Uploader" you mention it doesn't need to be unique, but seems odd for it to be the same. Maybe UKM Log Uploader https://codereview.chromium.org/2703363002/diff/160001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:81: "Report of usage statistics and crash-related data about " Report of usage statistics that are keyed by URLs to Chromium, sent only if the profile has History Sync. This includes information about the web pages you visit and your usage of them, such as page load speed. This will also include URLs and statistics related to downloaded files. If Extension Sync is enabled, these statistics will also include information about the extensions that have been installed from Chrome Web Store. Google only stores usage statistics associated with published extensions, and URLs that are known by Google’s search index. Is my suggestion. However this hasn't been published to the whitepaper yet. Dominic - what is your suggestion here for timing this. I would also add the "see more" bit, but that doesn't exist yet. https://codereview.chromium.org/2703363002/diff/160001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:89: "intervals while Chromium is running." ... running with Sync enabled. https://codereview.chromium.org/2703363002/diff/160001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:91: "A protocol buffer with usage statistics and crash related " "A protocol buffer with usage statistics and associated URLs. https://codereview.chromium.org/2703363002/diff/160001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:98: "Users can enable or disable this feature by disabling " Users can enable or disable this feature by disabling 'Automatically send usage statistics and crash reports to Google' in Chromium's settings under Advanced Settings, Privacy. This is only enabled if all active profiles have History/Extension Sync enabled without a Sync passphrase.
Annotation updated, please review. https://codereview.chromium.org/2703363002/diff/160001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/160001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:79: sender: "Metrics Log Uploader" On 2017/03/02 00:18:28, rkaplow (slow) wrote: > you mention it doesn't need to be unique, but seems odd for it to be the same. > > Maybe UKM Log Uploader Done. https://codereview.chromium.org/2703363002/diff/160001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:89: "intervals while Chromium is running." On 2017/03/02 00:18:28, rkaplow (slow) wrote: > ... running with Sync enabled. Done. https://codereview.chromium.org/2703363002/diff/160001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:91: "A protocol buffer with usage statistics and crash related " On 2017/03/02 00:18:28, rkaplow (slow) wrote: > "A protocol buffer with usage statistics and associated URLs. Done. https://codereview.chromium.org/2703363002/diff/160001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:98: "Users can enable or disable this feature by disabling " On 2017/03/02 00:18:28, rkaplow (slow) wrote: > Users can enable or disable this feature by disabling 'Automatically send usage > statistics and crash reports to Google' in Chromium's settings under Advanced > Settings, Privacy. This is only enabled if all active profiles have > History/Extension Sync enabled without a Sync passphrase. Done.
lgtm this lgtm but I'd like to have dominic read the wording of this and sign off
I have some comments but LGTM afterwards. https://codereview.chromium.org/2703363002/diff/180001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/180001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:36: NO_TRAFFIC_ANNOTATION_YET; I think that this is problematic because it will keep generating a warning about a missing annotation. https://codereview.chromium.org/2703363002/diff/180001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:50: "statistics and crash reports'." Add at the end: Usage statistics are tied to a pseudonymous machine identifier and not to your email address. https://codereview.chromium.org/2703363002/diff/180001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:90: "by Google’s search index." Add at the end: Add: Usage statistics are tied to a pseudonymous machine identifier and not to your email address.
Comments addressed, landing. https://codereview.chromium.org/2703363002/diff/180001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/180001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:36: NO_TRAFFIC_ANNOTATION_YET; On 2017/03/08 14:19:31, battre wrote: > I think that this is problematic because it will keep generating a warning about > a missing annotation. You are right, moved it to anonymous function. https://codereview.chromium.org/2703363002/diff/180001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:50: "statistics and crash reports'." On 2017/03/08 14:19:31, battre wrote: > Add at the end: Usage statistics are tied to a pseudonymous machine identifier > and not to your email address. Done. https://codereview.chromium.org/2703363002/diff/180001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:90: "by Google’s search index." On 2017/03/08 14:19:31, battre wrote: > Add at the end: Add: Usage statistics are tied to a pseudonymous machine > identifier and not to your email address. Done.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, holte@chromium.org, rkaplow@chromium.org, battre@chromium.org Link to the patchset: https://codereview.chromium.org/2703363002/#ps240001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2703363002/diff/240001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/240001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:19: if (service_type == metrics::MetricsLogUploader::UMA) { Please use a switch, so that if a new type is added, it forces an update to the switch since compiler requires all enum entries to be covered by the switch if there's no default case.
The CQ bit was unchecked by asvitkine@chromium.org
https://codereview.chromium.org/2703363002/diff/240001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/240001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:19: if (service_type == metrics::MetricsLogUploader::UMA) { On 2017/03/08 16:02:19, Alexei Svitkine (very slow) wrote: > Please use a switch, so that if a new type is added, it forces an update to the > switch since compiler requires all enum entries to be covered by the switch if > there's no default case. If we add a switch, we again need NO_TRAFFIC_ANNOTATION_YET flag for 'default' case which causes the problem that Dominic noted. Is it sufficient if I add a DCHECK to the else part checking if the type is ukm?
On 2017/03/08 17:48:07, Ramin Halavati wrote: > https://codereview.chromium.org/2703363002/diff/240001/components/metrics/net... > File components/metrics/net/net_metrics_log_uploader.cc (right): > > https://codereview.chromium.org/2703363002/diff/240001/components/metrics/net... > components/metrics/net/net_metrics_log_uploader.cc:19: if (service_type == > metrics::MetricsLogUploader::UMA) { > On 2017/03/08 16:02:19, Alexei Svitkine (very slow) wrote: > > Please use a switch, so that if a new type is added, it forces an update to > the > > switch since compiler requires all enum entries to be covered by the switch if > > there's no default case. > > If we add a switch, we again need NO_TRAFFIC_ANNOTATION_YET flag for 'default' > case which causes the problem that Dominic noted. Is it sufficient if I add a > DCHECK to the else part checking if the type is ukm? you shouldn't need a default case, can see discussion in https://codereview.chromium.org/2708293002/
On 2017/03/08 18:09:59, rkaplow (slow) wrote: > On 2017/03/08 17:48:07, Ramin Halavati wrote: > > > https://codereview.chromium.org/2703363002/diff/240001/components/metrics/net... > > File components/metrics/net/net_metrics_log_uploader.cc (right): > > > > > https://codereview.chromium.org/2703363002/diff/240001/components/metrics/net... > > components/metrics/net/net_metrics_log_uploader.cc:19: if (service_type == > > metrics::MetricsLogUploader::UMA) { > > On 2017/03/08 16:02:19, Alexei Svitkine (very slow) wrote: > > > Please use a switch, so that if a new type is added, it forces an update to > > the > > > switch since compiler requires all enum entries to be covered by the switch > if > > > there's no default case. > > > > If we add a switch, we again need NO_TRAFFIC_ANNOTATION_YET flag for 'default' > > case which causes the problem that Dominic noted. Is it sufficient if I add a > > DCHECK to the else part checking if the type is ukm? > > you shouldn't need a default case, can see discussion in > https://codereview.chromium.org/2708293002/ Shouldn't all cases in GetNetworkTrafficAnnotation function have a return value? In your code, you have an initialization value if neither of switch cases happen. But I don't see how we can have that in GetNetworkTrafficAnnotation function. Could you please elaborate how you are suggesting it?
On 2017/03/08 18:15:02, Ramin Halavati wrote: > On 2017/03/08 18:09:59, rkaplow (slow) wrote: > > On 2017/03/08 17:48:07, Ramin Halavati wrote: > > > > > > https://codereview.chromium.org/2703363002/diff/240001/components/metrics/net... > > > File components/metrics/net/net_metrics_log_uploader.cc (right): > > > > > > > > > https://codereview.chromium.org/2703363002/diff/240001/components/metrics/net... > > > components/metrics/net/net_metrics_log_uploader.cc:19: if (service_type == > > > metrics::MetricsLogUploader::UMA) { > > > On 2017/03/08 16:02:19, Alexei Svitkine (very slow) wrote: > > > > Please use a switch, so that if a new type is added, it forces an update > to > > > the > > > > switch since compiler requires all enum entries to be covered by the > switch > > if > > > > there's no default case. > > > > > > If we add a switch, we again need NO_TRAFFIC_ANNOTATION_YET flag for > 'default' > > > case which causes the problem that Dominic noted. Is it sufficient if I add > a > > > DCHECK to the else part checking if the type is ukm? > > > > you shouldn't need a default case, can see discussion in > > https://codereview.chromium.org/2708293002/ > > Shouldn't all cases in GetNetworkTrafficAnnotation function have a return value? > In your code, you have an initialization value if neither of switch cases > happen. But I don't see how we can have that in GetNetworkTrafficAnnotation > function. Could you please elaborate how you are suggesting it? I see. Not sure the best way, possible adding an UNREACHABLE() and just returning an empty net::DefineNetworkTrafficAnnotation outside of the switch as suggested https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/switch...
On 2017/03/08 18:30:04, rkaplow (slow) wrote: > On 2017/03/08 18:15:02, Ramin Halavati wrote: > > On 2017/03/08 18:09:59, rkaplow (slow) wrote: > > > On 2017/03/08 17:48:07, Ramin Halavati wrote: > > > > > > > > > > https://codereview.chromium.org/2703363002/diff/240001/components/metrics/net... > > > > File components/metrics/net/net_metrics_log_uploader.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2703363002/diff/240001/components/metrics/net... > > > > components/metrics/net/net_metrics_log_uploader.cc:19: if (service_type == > > > > metrics::MetricsLogUploader::UMA) { > > > > On 2017/03/08 16:02:19, Alexei Svitkine (very slow) wrote: > > > > > Please use a switch, so that if a new type is added, it forces an update > > to > > > > the > > > > > switch since compiler requires all enum entries to be covered by the > > switch > > > if > > > > > there's no default case. > > > > > > > > If we add a switch, we again need NO_TRAFFIC_ANNOTATION_YET flag for > > 'default' > > > > case which causes the problem that Dominic noted. Is it sufficient if I > add > > a > > > > DCHECK to the else part checking if the type is ukm? > > > > > > you shouldn't need a default case, can see discussion in > > > https://codereview.chromium.org/2708293002/ > > > > Shouldn't all cases in GetNetworkTrafficAnnotation function have a return > value? > > In your code, you have an initialization value if neither of switch cases > > happen. But I don't see how we can have that in GetNetworkTrafficAnnotation > > function. Could you please elaborate how you are suggesting it? > > I see. Not sure the best way, possible adding an UNREACHABLE() and just > returning an empty net::DefineNetworkTrafficAnnotation outside of the switch as > suggested > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/switch... As we extract annotations by a clang tool, having the empty annotation even if it is unreachable will trigger a warning that something should be annotated here. The only solution that I think of is adding a DCHECK in the else part of the GetNetworkTrafficAnnotation function that checks if the state is UKM. In this case, if a new state is added in future, it will catch it.
The CQ bit was checked by rhalavati@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...
On 2017/03/08 18:36:12, Ramin Halavati wrote: > On 2017/03/08 18:30:04, rkaplow (slow) wrote: > > On 2017/03/08 18:15:02, Ramin Halavati wrote: > > > On 2017/03/08 18:09:59, rkaplow (slow) wrote: > > > > On 2017/03/08 17:48:07, Ramin Halavati wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2703363002/diff/240001/components/metrics/net... > > > > > File components/metrics/net/net_metrics_log_uploader.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2703363002/diff/240001/components/metrics/net... > > > > > components/metrics/net/net_metrics_log_uploader.cc:19: if (service_type > == > > > > > metrics::MetricsLogUploader::UMA) { > > > > > On 2017/03/08 16:02:19, Alexei Svitkine (very slow) wrote: > > > > > > Please use a switch, so that if a new type is added, it forces an > update > > > to > > > > > the > > > > > > switch since compiler requires all enum entries to be covered by the > > > switch > > > > if > > > > > > there's no default case. > > > > > > > > > > If we add a switch, we again need NO_TRAFFIC_ANNOTATION_YET flag for > > > 'default' > > > > > case which causes the problem that Dominic noted. Is it sufficient if I > > add > > > a > > > > > DCHECK to the else part checking if the type is ukm? > > > > > > > > you shouldn't need a default case, can see discussion in > > > > https://codereview.chromium.org/2708293002/ > > > > > > Shouldn't all cases in GetNetworkTrafficAnnotation function have a return > > value? > > > In your code, you have an initialization value if neither of switch cases > > > happen. But I don't see how we can have that in GetNetworkTrafficAnnotation > > > function. Could you please elaborate how you are suggesting it? > > > > I see. Not sure the best way, possible adding an UNREACHABLE() and just > > returning an empty net::DefineNetworkTrafficAnnotation outside of the switch > as > > suggested > > > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/switch... > > As we extract annotations by a clang tool, having the empty annotation even if > it is unreachable will trigger a warning that something should be annotated > here. > The only solution that I think of is adding a DCHECK in the else part of the > GetNetworkTrafficAnnotation function that checks if the state is UKM. In this > case, if a new state is added in future, it will catch it. I added a DCHECK and a note on why the code does not use a switch. Please review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2703363002/diff/260001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/2703363002/diff/260001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:55: DCHECK(service_type == metrics::MetricsLogUploader::UKM); Can this be DCHECK_EQ?
Thank you. Comment addressed, landing.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, holte@chromium.org, battre@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2703363002/#ps280001 (title: "Comment addressed.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1489132298839250,
"parent_rev": "3cfa3f891108a0b22cdf04122cbde1f85d9e87e7", "commit_rev":
"5cba00e049c9802bb5a403d73ee8b184701e21c2"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to NetMetricsLogUploader. Network traffic annotation is added to network request of NetMetricsLogUploader. BUG=656607 ========== to ========== Network traffic annotation added to NetMetricsLogUploader. Network traffic annotation is added to network request of NetMetricsLogUploader. BUG=656607 Review-Url: https://codereview.chromium.org/2703363002 Cr-Commit-Position: refs/heads/master@{#456024} Committed: https://chromium.googlesource.com/chromium/src/+/5cba00e049c9802bb5a403d73ee8... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/5cba00e049c9802bb5a403d73ee8... |
