Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2708853002:
Network traffic annotation added to feedback_uploader_chrome. (Closed)
|
Created:
3 years, 10 months ago by Ramin Halavati Modified:
3 years, 8 months ago CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to feedback_uploader_chrome.
Network traffic annotation is added to network request of
feedback_uploader_chrome.
BUG=656607
Review-Url: https://codereview.chromium.org/2708853002
Cr-Commit-Position: refs/heads/master@{#462021}
Committed: https://chromium.googlesource.com/chromium/src/+/dffcf95adcedc8a1e1e59a17695a2610664b4564
Patch Set 1 #
Total comments: 3
Patch Set 2 : Annotation updated. #
Total comments: 12
Patch Set 3 : Annotation updated. #
Total comments: 10
Patch Set 4 : Annotation updated. #Patch Set 5 : Annotation updated. #
Messages
Total messages: 35 (11 generated)
rhalavati@chromium.org changed reviewers: + afakhry@chromium.org, asanka@chromium.org
@asanka: Please review DEPS file as owner of /net. @afakhry: 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 1 file in feedback uploader and it's DEPS file and added annotation template to it. Please review them 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.
DEPS lgtm
https://codereview.chromium.org/2708853002/diff/1/components/feedback/feedbac... File components/feedback/feedback_uploader_chrome.cc (right): https://codereview.chromium.org/2708853002/diff/1/components/feedback/feedbac... components/feedback/feedback_uploader_chrome.cc:58: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER Why are these placeholders not set? I don't know how the NetworkTrafficAnnotationTag should be used, but looking at this example: https://cs.chromium.org/chromium/src/chrome/browser/apps/drive/drive_app_conv... The various fields are filled. Can you please explain why we just use "...", WEBSITE/GOOGLE_OWNED_SERVICE/OTHER, false/true and MANDATORY/RECOMMENDED/UNSET here without specifying anything in particular?
The CQ bit was checked by rhalavati@chromium.org
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 rhalavati@chromium.org
Comment addressed, sorry for the Trybot run by mistake. https://codereview.chromium.org/2708853002/diff/1/components/feedback/feedbac... File components/feedback/feedback_uploader_chrome.cc (right): https://codereview.chromium.org/2708853002/diff/1/components/feedback/feedbac... components/feedback/feedback_uploader_chrome.cc:58: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/25 02:37:37, afakhry wrote: > Why are these placeholders not set? I don't know how the > NetworkTrafficAnnotationTag should be used, but looking at this example: > https://cs.chromium.org/chromium/src/chrome/browser/apps/drive/drive_app_conv... > > The various fields are filled. Can you please explain why we just use "...", > WEBSITE/GOOGLE_OWNED_SERVICE/OTHER, false/true and MANDATORY/RECOMMENDED/UNSET > here without specifying anything in particular? I am sorry I wasn't clear in my initial message. I was asking you tho suggest appropriate text for all place holders.
On 2017/02/27 08:56:30, Ramin Halavati wrote: > Comment addressed, sorry for the Trybot run by mistake. > > https://codereview.chromium.org/2708853002/diff/1/components/feedback/feedbac... > File components/feedback/feedback_uploader_chrome.cc (right): > > https://codereview.chromium.org/2708853002/diff/1/components/feedback/feedbac... > components/feedback/feedback_uploader_chrome.cc:58: destination: > WEBSITE/GOOGLE_OWNED_SERVICE/OTHER > On 2017/02/25 02:37:37, afakhry wrote: > > Why are these placeholders not set? I don't know how the > > NetworkTrafficAnnotationTag should be used, but looking at this example: > > > https://cs.chromium.org/chromium/src/chrome/browser/apps/drive/drive_app_conv... > > > > The various fields are filled. Can you please explain why we just use "...", > > WEBSITE/GOOGLE_OWNED_SERVICE/OTHER, false/true and MANDATORY/RECOMMENDED/UNSET > > here without specifying anything in particular? > > I am sorry I wasn't clear in my initial message. I was asking you tho suggest > appropriate text for all place holders. Hi, A very friendly ping for this.
On 2017/03/09 07:28:16, Ramin Halavati wrote: > On 2017/02/27 08:56:30, Ramin Halavati wrote: > > Comment addressed, sorry for the Trybot run by mistake. > > > > > https://codereview.chromium.org/2708853002/diff/1/components/feedback/feedbac... > > File components/feedback/feedback_uploader_chrome.cc (right): > > > > > https://codereview.chromium.org/2708853002/diff/1/components/feedback/feedbac... > > components/feedback/feedback_uploader_chrome.cc:58: destination: > > WEBSITE/GOOGLE_OWNED_SERVICE/OTHER > > On 2017/02/25 02:37:37, afakhry wrote: > > > Why are these placeholders not set? I don't know how the > > > NetworkTrafficAnnotationTag should be used, but looking at this example: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/apps/drive/drive_app_conv... > > > > > > The various fields are filled. Can you please explain why we just use "...", > > > WEBSITE/GOOGLE_OWNED_SERVICE/OTHER, false/true and > MANDATORY/RECOMMENDED/UNSET > > > here without specifying anything in particular? > > > > I am sorry I wasn't clear in my initial message. I was asking you tho suggest > > appropriate text for all place holders. > > Hi, > > A very friendly ping for this. Sorry I was OOO the past 3 days. I will look at it later today.
https://codereview.chromium.org/2708853002/diff/1/components/feedback/feedbac... File components/feedback/feedback_uploader_chrome.cc (right): https://codereview.chromium.org/2708853002/diff/1/components/feedback/feedbac... components/feedback/feedback_uploader_chrome.cc:58: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/27 08:56:30, Ramin Halavati wrote: > On 2017/02/25 02:37:37, afakhry wrote: > > Why are these placeholders not set? I don't know how the > > NetworkTrafficAnnotationTag should be used, but looking at this example: > > > https://cs.chromium.org/chromium/src/chrome/browser/apps/drive/drive_app_conv... > > > > The various fields are filled. Can you please explain why we just use "...", > > WEBSITE/GOOGLE_OWNED_SERVICE/OTHER, false/true and MANDATORY/RECOMMENDED/UNSET > > here without specifying anything in particular? > > I am sorry I wasn't clear in my initial message. I was asking you tho suggest > appropriate text for all place holders. You can use something like this: net::NetworkTrafficAnnotationTag traffic_annotation = net::DefineNetworkTrafficAnnotation("chrome_feedback_report_app", R"( semantics { sender: "Chrome Feedbak Report App" description: "This uploads the feedback report to Google feedback " "server" trigger: "When the feedback report is being uploaded" data: "Contents of the report, user entered message, and useful " "debugging logs" destination: GOOGLE_OWNED_SERVICE } policy { })"); We don't really have any special policies.
Annotations updated, some inline comments added. Please review. https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... File components/feedback/feedback_uploader_chrome.cc (right): https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:56: "This uploads the feedback report to Google feedback server." May be it would be good if you describe what is the feedback about. This text should be clear enough so that a non-very-technical person finds out why this data is sent. https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:57: trigger: "When the feedback report is being uploaded." Is user asked for sending feedback? In which cases feedback is generated? https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:59: "Contents of the report, user entered message, and useful " Can you add details on content items? Is there any user specific data? https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:65: setting: "..." I didn't find any settings disabling feedback, is there? Or can we add this feedback is sent only by direct approval of the user.?
afakhry@chromium.org changed reviewers: + rkc@chromium.org
https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... File components/feedback/feedback_uploader_chrome.cc (right): https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:56: "This uploads the feedback report to Google feedback server." On 2017/03/11 09:42:54, Ramin Halavati wrote: > May be it would be good if you describe what is the feedback about. This text > should be clear enough so that a non-very-technical person finds out why this > data is sent. I'm not sure how to describe it in a simple way. This is the feedback report sent by the user when he presses Alt+Shift+i to report a bug or a feedback in general. Along with it is sent system logs that helps us to diagnose the issue. https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:57: trigger: "When the feedback report is being uploaded." On 2017/03/11 09:42:54, Ramin Halavati wrote: > Is user asked for sending feedback? In which cases feedback is generated? This is a user-generated event, as a result of pressing Alt+Shift+i. See https://support.google.com/chrome/answer/95315?hl=en https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:59: "Contents of the report, user entered message, and useful " On 2017/03/11 09:42:54, Ramin Halavati wrote: > Can you add details on content items? Is there any user specific data? We anonymize the logs to remove any user-private data. The user can view the system information before sending, and can choose to send the feedback report without the logs, the screenshot, or even his email address. https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:65: setting: "..." On 2017/03/11 09:42:54, Ramin Halavati wrote: > I didn't find any settings disabling feedback, is there? Or can we add this > feedback is sent only by direct approval of the user.? I'm not aware of any setting to disable feedback. Yes, we can add that. I vaguely remember that in kiosk devices, some feedback reports are sent periodically in the background, but I think for that to happen the user has to be opt-in. +rkc@ who might know more about this.
Comments addressed, please review. https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... File components/feedback/feedback_uploader_chrome.cc (right): https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:56: "This uploads the feedback report to Google feedback server." On 2017/03/14 01:36:42, afakhry wrote: > On 2017/03/11 09:42:54, Ramin Halavati wrote: > > May be it would be good if you describe what is the feedback about. This text > > should be clear enough so that a non-very-technical person finds out why this > > data is sent. > > I'm not sure how to describe it in a simple way. This is the feedback report > sent by the user when he presses Alt+Shift+i to report a bug or a feedback in > general. Along with it is sent system logs that helps us to diagnose the issue. Thank you, I think this description is quite good. I updated the text based on this. https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:57: trigger: "When the feedback report is being uploaded." On 2017/03/14 01:36:42, afakhry wrote: > On 2017/03/11 09:42:54, Ramin Halavati wrote: > > Is user asked for sending feedback? In which cases feedback is generated? > > This is a user-generated event, as a result of pressing Alt+Shift+i. See > https://support.google.com/chrome/answer/95315?hl=en Done. https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:59: "Contents of the report, user entered message, and useful " On 2017/03/14 01:36:42, afakhry wrote: > On 2017/03/11 09:42:54, Ramin Halavati wrote: > > Can you add details on content items? Is there any user specific data? > > We anonymize the logs to remove any user-private data. The user can view the > system information before sending, and can choose to send the feedback report > without the logs, the screenshot, or even his email address. Done. https://codereview.chromium.org/2708853002/diff/20001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:65: setting: "..." On 2017/03/14 01:36:42, afakhry wrote: > On 2017/03/11 09:42:54, Ramin Halavati wrote: > > I didn't find any settings disabling feedback, is there? Or can we add this > > feedback is sent only by direct approval of the user.? > > I'm not aware of any setting to disable feedback. > Yes, we can add that. > I vaguely remember that in kiosk devices, some feedback reports are sent > periodically in the background, but I think for that to happen the user has to > be opt-in. +rkc@ who might know more about this. Acknowledged.
lgtm
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org
Thank you. battre@, msramek@: Do you have any comments?
https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... File components/feedback/feedback_uploader_chrome.cc (right): https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:57: "general. Along with their text, system logs that helps in " nit: s/their/text/the free-form text they entered/ nit: help (3d person plural) https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:59: "the report to Google feedback server." nit: Feedback https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:63: "User entered message and useful debugging logs. The logs are " nit: User-entered ("User entered" reads as a subject and predicate). What exactly is meant by logs here? Does that refer to raw UMA (in which case I would say "metrics")? Or ARC++ logcat?
https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... File components/feedback/feedback_uploader_chrome.cc (right): https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:63: "User entered message and useful debugging logs. The logs are " On 2017/03/14 22:20:59, msramek wrote: > nit: User-entered ("User entered" reads as a subject and predicate). > > What exactly is meant by logs here? Does that refer to raw UMA (in which case I > would say "metrics")? Or ARC++ logcat? All sorts of logs, UI logs, chrome logs, kernel logs, auto update engine logs, ARC++ logs, ... etc.
Comments addressed, please review. https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... File components/feedback/feedback_uploader_chrome.cc (right): https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:57: "general. Along with their text, system logs that helps in " On 2017/03/14 22:20:59, msramek wrote: > nit: s/their/text/the free-form text they entered/ > nit: help (3d person plural) Done. https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:59: "the report to Google feedback server." On 2017/03/14 22:20:59, msramek wrote: > nit: Feedback Done. https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:63: "User entered message and useful debugging logs. The logs are " On 2017/03/15 03:01:32, afakhry wrote: > On 2017/03/14 22:20:59, msramek wrote: > > nit: User-entered ("User entered" reads as a subject and predicate). > > > > What exactly is meant by logs here? Does that refer to raw UMA (in which case > I > > would say "metrics")? Or ARC++ logcat? > > All sorts of logs, UI logs, chrome logs, kernel logs, auto update engine logs, > ARC++ logs, ... etc. Done.
https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... File components/feedback/feedback_uploader_chrome.cc (right): https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:63: "User entered message and useful debugging logs. The logs are " On 2017/03/15 06:08:01, Ramin Halavati wrote: > On 2017/03/15 03:01:32, afakhry wrote: > > On 2017/03/14 22:20:59, msramek wrote: > > > nit: User-entered ("User entered" reads as a subject and predicate). > > > > > > What exactly is meant by logs here? Does that refer to raw UMA (in which > case > > I > > > would say "metrics")? Or ARC++ logcat? > > > > All sorts of logs, UI logs, chrome logs, kernel logs, auto update engine logs, > > ARC++ logs, ... etc. > > Done. The reason why I'm asking is that I see two checkboxes: "Include screenshot" and "Send system information [and metrics]". The last sentence claims that sending logs can be disabled, but there is no such chcekbox. Do logs belong under the "system information" checkbox? Because this sentence and the next make it sound like they are two different things, and I also don't see any logs when I click on "system information".
https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... File components/feedback/feedback_uploader_chrome.cc (right): https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:63: "User entered message and useful debugging logs. The logs are " On 2017/03/15 09:41:11, msramek wrote: > On 2017/03/15 06:08:01, Ramin Halavati wrote: > > On 2017/03/15 03:01:32, afakhry wrote: > > > On 2017/03/14 22:20:59, msramek wrote: > > > > nit: User-entered ("User entered" reads as a subject and predicate). > > > > > > > > What exactly is meant by logs here? Does that refer to raw UMA (in which > > case > > > I > > > > would say "metrics")? Or ARC++ logcat? > > > > > > All sorts of logs, UI logs, chrome logs, kernel logs, auto update engine > logs, > > > ARC++ logs, ... etc. > > > > Done. > > The reason why I'm asking is that I see two checkboxes: "Include screenshot" and > "Send system information [and metrics]". > > The last sentence claims that sending logs can be disabled, but there is no such > chcekbox. Do logs belong under the "system information" checkbox? Because this > sentence and the next make it sound like they are two different things, and I > also don't see any logs when I click on "system information". If the user unchecks the entry for "system information and metrics", no logs will be sent with the report. There should be all sorts of logs! If you are running on Chrome OS, open system information and look for "Profile [0] chrome_user_log", "syslog", "cheets_log", "ui_log", ... etc. as a few examples.
Annotation updated, please review. https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... File components/feedback/feedback_uploader_chrome.cc (right): https://codereview.chromium.org/2708853002/diff/40001/components/feedback/fee... components/feedback/feedback_uploader_chrome.cc:63: "User entered message and useful debugging logs. The logs are " On 2017/03/15 16:00:11, afakhry wrote: > On 2017/03/15 09:41:11, msramek wrote: > > On 2017/03/15 06:08:01, Ramin Halavati wrote: > > > On 2017/03/15 03:01:32, afakhry wrote: > > > > On 2017/03/14 22:20:59, msramek wrote: > > > > > nit: User-entered ("User entered" reads as a subject and predicate). > > > > > > > > > > What exactly is meant by logs here? Does that refer to raw UMA (in which > > > case > > > > I > > > > > would say "metrics")? Or ARC++ logcat? > > > > > > > > All sorts of logs, UI logs, chrome logs, kernel logs, auto update engine > > logs, > > > > ARC++ logs, ... etc. > > > > > > Done. > > > > The reason why I'm asking is that I see two checkboxes: "Include screenshot" > and > > "Send system information [and metrics]". > > > > The last sentence claims that sending logs can be disabled, but there is no > such > > chcekbox. Do logs belong under the "system information" checkbox? Because this > > sentence and the next make it sound like they are two different things, and I > > also don't see any logs when I click on "system information". > > If the user unchecks the entry for "system information and metrics", no logs > will be sent with the report. > > There should be all sorts of logs! If you are running on Chrome OS, open system > information and look for "Profile [0] chrome_user_log", "syslog", "cheets_log", > "ui_log", ... etc. as a few examples. Acknowledged.
LGTM, thanks for the expanding the text in the last patchset :)
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2708853002/#ps80001 (title: "Annotation updated.")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by rhalavati@chromium.org
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": 80001, "attempt_start_ts": 1491385810916980,
"parent_rev": "8bd5d6a15f00ab291c675c2e49b19275d263b6bc", "commit_rev":
"dffcf95adcedc8a1e1e59a17695a2610664b4564"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to feedback_uploader_chrome. Network traffic annotation is added to network request of feedback_uploader_chrome. BUG=656607 ========== to ========== Network traffic annotation added to feedback_uploader_chrome. Network traffic annotation is added to network request of feedback_uploader_chrome. BUG=656607 Review-Url: https://codereview.chromium.org/2708853002 Cr-Commit-Position: refs/heads/master@{#462021} Committed: https://chromium.googlesource.com/chromium/src/+/dffcf95adcedc8a1e1e59a17695a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/dffcf95adcedc8a1e1e59a17695a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
