|
|
Created:
3 years, 7 months ago by Ramin Halavati Modified:
3 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPartial Protobuf for Traffic Annotation.
In many use cases, it's better to define some part of a network traffic
annotation in one function and the reset of it in another one. like when a
function makes the initial request and controls policies, and then passes it
to another one to set/unset the cookies and perform the actual request. This CL
adds required definitions to make partial annotations.
BUG=656607
patch from issue 2815293002 at patchset 160001 (http://crrev.com/2815293002#ps160001)
Review-Url: https://codereview.chromium.org/2869373002
Cr-Commit-Position: refs/heads/master@{#474602}
Committed: https://chromium.googlesource.com/chromium/src/+/99c6fcef9cff05b0df3a536623f8d8251c2f2a6b
Patch Set 1 #Patch Set 2 : Types changed to struct. #Patch Set 3 : Partial Test tag added. #Patch Set 4 : Runtime checks added for partial annotations. #
Total comments: 4
Patch Set 5 : Comments addressed. #Patch Set 6 : unique ids changed to hash codes. #Patch Set 7 : Hash function changed to avoid overflow. #Patch Set 8 : Hashed unique ids added to auditor outputs. #Patch Set 9 : Merged. #
Total comments: 13
Patch Set 10 : Comments addressed. #
Total comments: 5
Patch Set 11 : Comments addressed. #
Total comments: 13
Patch Set 12 : Comments addressed. #Patch Set 13 : Hash function updated. New report added. #
Total comments: 5
Patch Set 14 : Comment addressed, logging header added. #Patch Set 15 : net/BUILD.gn updated. #Patch Set 16 : Type changed to int32, Names changed. #Messages
Total messages: 65 (38 generated)
rhalavati@chromium.org changed reviewers: + battre@chromium.org, dcheng@chromium.org, msramek@chromium.org
Daniel, Dominic, I've enclosed another version of partial annotations with 4 functions and 2 partial and complete types. We can check the following items when DECHECK_IS_ON: - Annotation passed to network functions are complete and syntactically correct. - Partial annotation passed to completing functions has a correct reference to the one completing it. Please review.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
On 2017/05/10 13:09:58, Ramin Halavati wrote: > Daniel, Dominic, > > I've enclosed another version of partial annotations with 4 functions and 2 > partial and complete types. We can check the following items when DECHECK_IS_ON: > - Annotation passed to network functions are complete and syntactically > correct. > - Partial annotation passed to completing functions has a correct reference to > the one completing it. > > Please review. Update on possible tests: 1- A trybot based on clang tool to check syntax of all annotations and coverage of all required functions. Plus checking if each combination of partial and completing annotations have all required fields. 2- A presubmit check (voluntarily) for syntax checking and coverage checking. 3- A runtime check for when DCHECKs are on, that tests that a partial annotation is completed using a completing annotation with correct id. Here is a sample usage: https://codereview.chromium.org/2888763004/
Patchset #4 (id:100001) has been deleted
rhalavati@chromium.org changed reviewers: + asanka@chromium.org
Asanka, Daniel, I would be delighted to have your approval on this. :)
LGTM Sorry for the delay, traveling + a series of laptop failures so I missed a few reviews =( https://codereview.chromium.org/2869373002/diff/120001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2869373002/diff/120001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:84: LOG_ASSERT(!strcmp(unique_id, partial_annotation.completing_id)); Out of curiosity, why LOG_ASSERT instead of DCHECK? https://codereview.chromium.org/2869373002/diff/120001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2869373002/diff/120001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:183: // their arfuments, and their ancestor function (when possible). Nit: arguments
Thank you very much Daniel, comments addressed. Asanka, Martin, Dominic, Please review. https://codereview.chromium.org/2869373002/diff/120001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2869373002/diff/120001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:84: LOG_ASSERT(!strcmp(unique_id, partial_annotation.completing_id)); On 2017/05/23 05:13:35, dcheng wrote: > Out of curiosity, why LOG_ASSERT instead of DCHECK? Done. https://codereview.chromium.org/2869373002/diff/120001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2869373002/diff/120001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:183: // their arfuments, and their ancestor function (when possible). On 2017/05/23 05:13:35, dcheng wrote: > Nit: arguments Done.
On 2017/05/23 06:38:12, Ramin Halavati wrote: > Thank you very much Daniel, comments addressed. > > Asanka, Martin, Dominic, > Please review. > > https://codereview.chromium.org/2869373002/diff/120001/net/traffic_annotation... > File net/traffic_annotation/network_traffic_annotation.h (right): > > https://codereview.chromium.org/2869373002/diff/120001/net/traffic_annotation... > net/traffic_annotation/network_traffic_annotation.h:84: > LOG_ASSERT(!strcmp(unique_id, partial_annotation.completing_id)); > On 2017/05/23 05:13:35, dcheng wrote: > > Out of curiosity, why LOG_ASSERT instead of DCHECK? > > Done. > > https://codereview.chromium.org/2869373002/diff/120001/tools/clang/traffic_an... > File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp > (right): > > https://codereview.chromium.org/2869373002/diff/120001/tools/clang/traffic_an... > tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:183: > // their arfuments, and their ancestor function (when possible). > On 2017/05/23 05:13:35, dcheng wrote: > > Nit: arguments > > Done. I changed the values type back to the hash value of the strings mostly because serialization in mojom would become very tricky (if not impossible) with our former pointer to constant string approach). This change would also have the benefit of keeping less memory (unique_ids are also optimized off). I will add a checking in unittest for duplicate hash values.
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...
Patchset #6 (id:160001) has been deleted
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
+Helen
xunjieli@chromium.org changed reviewers: + xunjieli@chromium.org
Thanks! a couple of nits and a question on how to run the clang tool. https://codereview.chromium.org/2869373002/diff/240001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2869373002/diff/240001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:10: // Recuresively compute hash code of the given string as a constant expression. s/Recuresively/Recursively https://codereview.chromium.org/2869373002/diff/240001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:34: struct PartialNetworkTrafficAnnotationTag { Can you add a comment here on what |completing_id_hash_code| is and why it is only defined in debug build? https://codereview.chromium.org/2869373002/diff/240001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:76: // specify the unique id of the annotation that will complete it. In case of Could you re-word the last part to mention |completion_id|? e.g. In case of ExtendNetworkTrafficAnnotation, |completion_id| is the unique id of the annotation that will complete it. In the case of ExpandNetworkTrafficAnnotation, |completion_id| is the group id of the completing annotations. https://codereview.chromium.org/2869373002/diff/240001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:108: // branched into several annotations. In this case, group_id is a common id that nit: Could you add a pair of pipes for "group_id" ? s/group_id/|group_id| This is to have a consistent commenting style. https://codereview.chromium.org/2869373002/diff/240001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2869373002/diff/240001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:3: `tools/traffic_annotation/auditor/traffic_annotaion_auditor.py`. Refer to it for There is a typo in the script name. s/annotaion/annotation https://codereview.chromium.org/2869373002/diff/240001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:18: Run `traffic_annotation_extractor --help` for parameters help. Could you include the path to |traffic_annotation_extractor| ? Is this a build artifact or are you referring to the python script, traffic_annotation_auditor?
Patchset #10 (id:260001) has been deleted
Patchset #10 (id:280001) has been deleted
Thank you Helen, comments addressed, please review. https://codereview.chromium.org/2869373002/diff/240001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2869373002/diff/240001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:10: // Recuresively compute hash code of the given string as a constant expression. On 2017/05/23 15:17:49, xunjieli wrote: > s/Recuresively/Recursively Done. https://codereview.chromium.org/2869373002/diff/240001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:34: struct PartialNetworkTrafficAnnotationTag { On 2017/05/23 15:17:49, xunjieli wrote: > Can you add a comment here on what |completing_id_hash_code| is and why it is > only defined in debug build? Done. https://codereview.chromium.org/2869373002/diff/240001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:76: // specify the unique id of the annotation that will complete it. In case of On 2017/05/23 15:17:49, xunjieli wrote: > Could you re-word the last part to mention |completion_id|? > > e.g. > In case of ExtendNetworkTrafficAnnotation, |completion_id| is the unique id of > the annotation that will complete it. In the case of > ExpandNetworkTrafficAnnotation, |completion_id| is the group id of the > completing annotations. Done. https://codereview.chromium.org/2869373002/diff/240001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:108: // branched into several annotations. In this case, group_id is a common id that On 2017/05/23 15:17:49, xunjieli wrote: > nit: Could you add a pair of pipes for "group_id" ? > s/group_id/|group_id| > > This is to have a consistent commenting style. Done. https://codereview.chromium.org/2869373002/diff/240001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2869373002/diff/240001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:3: `tools/traffic_annotation/auditor/traffic_annotaion_auditor.py`. Refer to it for On 2017/05/23 15:17:49, xunjieli wrote: > There is a typo in the script name. > s/annotaion/annotation Done. https://codereview.chromium.org/2869373002/diff/240001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:18: Run `traffic_annotation_extractor --help` for parameters help. On 2017/05/23 15:17:49, xunjieli wrote: > Could you include the path to |traffic_annotation_extractor| ? > Is this a build artifact or are you referring to the python script, > traffic_annotation_auditor? The default path for clang tools output is third_party/llvm-build/Release+Asserts/bin, but I suspect that it can be changed. Would you suggest that I add it here? It is build using the script mentioned on lines 7-8.
I like the part about storing a hash code. I thinks it makes it easier to do runtime analysis of annotation usage. One question: Are we able to guarantee that we won't have hash collision in the future? Can we have the clang tool to generate a file that contains string unique id to its hash value? And check in that file to the codebase? We don't compile the mapping in the binary but can make the file go through the code review process. Whenever someone adds a new annotation, a presubmit check should verify the newly generated mapping doesn't have a hash collision and that mapping is stable for the duration of a runtime anaylsis. https://codereview.chromium.org/2869373002/diff/240001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2869373002/diff/240001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:18: Run `traffic_annotation_extractor --help` for parameters help. On 2017/05/23 16:30:43, Ramin Halavati wrote: > On 2017/05/23 15:17:49, xunjieli wrote: > > Could you include the path to |traffic_annotation_extractor| ? > > Is this a build artifact or are you referring to the python script, > > traffic_annotation_auditor? > > The default path for clang tools output is > third_party/llvm-build/Release+Asserts/bin, but I suspect that it can be > changed. Would you suggest that I add it here? It is build using the script > mentioned on lines 7-8. Yes please add the path here. You could mention the path might be different. I had a hard time finding traffic_annotation_extractor. I suspect it's the same for people who haven't used clang tools before. https://codereview.chromium.org/2869373002/diff/300001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2869373002/diff/300001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:18: Run `traffic_annotation_extractor --help` for parameters help. Instead of doing --help, could you give an example of a full command? I looked at the help message, it looks like -p and sources are needed, but when I tried to do that, it gives me "Compile command not found." I think the README.md file should give detailed instructions that require minimal effort from the user. https://codereview.chromium.org/2869373002/diff/300001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:28: - Line 7-: Serialized protobuf of the annotation. nit: there is an extra dash after 7.
Thanks Helen, comments addressed, please review. Yes, the plan is to have hash collision checking in unittest. The unittest runs the clang tool over the entire repository, extracts all annotations and checks for their syntax and unique id collisions. Also checks that each partial annotation and its completing part(s) have all required fields. Presubmit check does not check the whole repository for speed issues and just goes through changed files that have specific keywords like annotation functions (the complete check takes around 30 minutes on a Z840 workstation). I will add the report you mentioned to the auditor. https://codereview.chromium.org/2869373002/diff/300001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2869373002/diff/300001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:18: Run `traffic_annotation_extractor --help` for parameters help. On 2017/05/23 18:37:10, xunjieli wrote: > Instead of doing --help, could you give an example of a full command? > > I looked at the help message, it looks like -p and sources are needed, but when > I tried to do that, it gives me "Compile command not found." I think the > README.md file should give detailed instructions that require minimal effort > from the user. Done. https://codereview.chromium.org/2869373002/diff/300001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:28: - Line 7-: Serialized protobuf of the annotation. On 2017/05/23 18:37:09, xunjieli wrote: > nit: there is an extra dash after 7. It means lines 7 to the end of file. Would you suggest removing it?
net/ LGTM > Presubmit check does not check the whole repository for speed issues and just > goes through changed files that have specific keywords like annotation functions > (the complete check takes around 30 minutes on a Z840 workstation). I think we should still have the mapping (of string unique_ids to their hash codes) checked into the codebase. I am fine with doing it in a separate CL. The Presubmit check doesn't need to run over the entire repository, it can just check changed files, and if there is a new usage, additionally check the mapping file to make sure the id doesn't already exist. I'd assume that'll run pretty quickly. Let me know if I'm missing anything. https://codereview.chromium.org/2869373002/diff/300001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2869373002/diff/300001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:28: - Line 7-: Serialized protobuf of the annotation. On 2017/05/23 18:56:08, Ramin Halavati wrote: > On 2017/05/23 18:37:09, xunjieli wrote: > > nit: there is an extra dash after 7. > > It means lines 7 to the end of file. Would you suggest removing it? I see. Up to you. I wasn't sure what the - was for. https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:76: // these cases, we prefer that both parts of the annotation appear in context nit: per style guide, could you avoid using first person pronouns? https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:149: // // "call_by_fooX"). nit: s/call_by_fooX/call_by_foo1 https://codereview.chromium.org/2869373002/diff/320001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2869373002/diff/320001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:21: -p=out/Debug components/spellcheck/browser/spelling_service_client.cc` Is there a way to run the tool over a directory recursively? The tool doesn't like when I supplied a directory.
I'm fine with this overall approach. It might be nice to document how collision resistant we expect this hash to be/what algorithm it's derived from. Also, do these hashes need to be immutable, or is it OK for the implementation to change over time? https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:13: return (uint32_t)((recursive_hash<N - 1>(str) * 31 + str[N - 1]) % 138547331); Nit: Use static_cast C++ style casts for consistency with Chromium/Google style https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:20: return (uint32_t)(*str); We could just specialize <0> to avoid the cast here (and it would handle "" more gracefully... though whether we want to do that or not is a separate question) https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:24: #define COMPUTE_STRING_HASH(S) recursive_hash<sizeof(S)>(S) I think it might actually be possible to template the recursive_hash helpers on char (&str)[N] and specialize 0 for the end case. However, if we keep the macro approach, please #undef COMPUTE_STRING_HASH at the end of this file to avoid polluting the global namespace. https://codereview.chromium.org/2869373002/diff/320001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2869373002/diff/320001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:21: -p=out/Debug components/spellcheck/browser/spelling_service_client.cc` On 2017/05/23 23:51:50, xunjieli wrote: > Is there a way to run the tool over a directory recursively? > The tool doesn't like when I supplied a directory. Generally it's easier to run the tool with the run_tool.py script, which abstracts away the details of building the compile DB, etc, and provides a platform agnostic interface for running it. Can we do that here?
On 2017/05/24 03:39:52, dcheng wrote: > I'm fine with this overall approach. It might be nice to document how collision > resistant we expect this hash to be/what algorithm it's derived from. Also, do > these hashes need to be immutable, or is it OK for the implementation to change > over time? > > https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... > File net/traffic_annotation/network_traffic_annotation.h (right): > > https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... > net/traffic_annotation/network_traffic_annotation.h:13: return > (uint32_t)((recursive_hash<N - 1>(str) * 31 + str[N - 1]) % 138547331); > Nit: Use static_cast C++ style casts for consistency with Chromium/Google style > > https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... > net/traffic_annotation/network_traffic_annotation.h:20: return (uint32_t)(*str); > We could just specialize <0> to avoid the cast here (and it would handle "" more > gracefully... though whether we want to do that or not is a separate question) > > https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... > net/traffic_annotation/network_traffic_annotation.h:24: #define > COMPUTE_STRING_HASH(S) recursive_hash<sizeof(S)>(S) > I think it might actually be possible to template the recursive_hash helpers on > char (&str)[N] and specialize 0 for the end case. > > However, if we keep the macro approach, please #undef COMPUTE_STRING_HASH at the > end of this file to avoid polluting the global namespace. > > https://codereview.chromium.org/2869373002/diff/320001/tools/clang/traffic_an... > File tools/clang/traffic_annotation_extractor/README.md (right): > > https://codereview.chromium.org/2869373002/diff/320001/tools/clang/traffic_an... > tools/clang/traffic_annotation_extractor/README.md:21: -p=out/Debug > components/spellcheck/browser/spelling_service_client.cc` > On 2017/05/23 23:51:50, xunjieli wrote: > > Is there a way to run the tool over a directory recursively? > > The tool doesn't like when I supplied a directory. > > Generally it's easier to run the tool with the run_tool.py script, which > abstracts away the details of building the compile DB, etc, and provides a > platform agnostic interface for running it. Can we do that here? (Also, there's no need to wait for another LGTM from me after addressing these comments)
Daniel, Helen, All comments addressed, thank you very much. I changed the denominator of hash function to the largest prime below the threshold, and added a new report for unique ids and hash values. I can change this later to a json or any other suitable format. Helen, Regarding keeping all hash codes in an online db and checking it during presubmit, we might do it later, but now there are still a lot of complications around how to test annotations and I'd rather not add a new angle to it. Doing it in a complete sweep in trybot is much easier now but it sure would be a good next addition. I will add you to current discussions on this issue. Daniel, Regarding hash code documentations and collision resistance, the function is not driven from a known algorithm (AFAIK) and it doesn't have any collision on current 130 annotations. I don't know what to add or how to analyze it. Any suggestions? https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:13: return (uint32_t)((recursive_hash<N - 1>(str) * 31 + str[N - 1]) % 138547331); On 2017/05/24 03:39:52, dcheng wrote: > Nit: Use static_cast C++ style casts for consistency with Chromium/Google style Done. https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:20: return (uint32_t)(*str); On 2017/05/24 03:39:52, dcheng wrote: > We could just specialize <0> to avoid the cast here (and it would handle "" more > gracefully... though whether we want to do that or not is a separate question) If we do that, all hash codes will have an extra multiplication by 31. Also we would need an extra check somewhere to prevent passing empty strings. But none of them are important if it seems better that way. https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:24: #define COMPUTE_STRING_HASH(S) recursive_hash<sizeof(S)>(S) On 2017/05/24 03:39:52, dcheng wrote: > I think it might actually be possible to template the recursive_hash helpers on > char (&str)[N] and specialize 0 for the end case. > > However, if we keep the macro approach, please #undef COMPUTE_STRING_HASH at the > end of this file to avoid polluting the global namespace. I couldn't find a replacement, added the #undef. https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:76: // these cases, we prefer that both parts of the annotation appear in context On 2017/05/23 23:51:50, xunjieli wrote: > nit: per style guide, could you avoid using first person pronouns? Aren't we? https://codereview.chromium.org/2869373002/diff/320001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:149: // // "call_by_fooX"). On 2017/05/23 23:51:50, xunjieli wrote: > nit: s/call_by_fooX/call_by_foo1 Added foo2 to clear the example. https://codereview.chromium.org/2869373002/diff/320001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2869373002/diff/320001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:21: -p=out/Debug components/spellcheck/browser/spelling_service_client.cc` On 2017/05/24 03:39:52, dcheng wrote: > On 2017/05/23 23:51:50, xunjieli wrote: > > Is there a way to run the tool over a directory recursively? > > The tool doesn't like when I supplied a directory. > > Generally it's easier to run the tool with the run_tool.py script, which > abstracts away the details of building the compile DB, etc, and provides a > platform agnostic interface for running it. Can we do that here? Done.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry, I have an issue with the words Extend vs. Expand. They are the same thing to me. https://codereview.chromium.org/2869373002/diff/360001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2869373002/diff/360001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:46: // if intended completing part is added to a partial network annotation. if "an" https://codereview.chromium.org/2869373002/diff/360001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:84: // 'ExtendNetworkTrafficAnnotation' or 'ExpandNetworkTrafficAnnotation' Hm, I think that extend and expand are synonymous. https://codereview.chromium.org/2869373002/diff/360001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2869373002/diff/360001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:39: // Name of the function including this line. E.g., in the following code, s/including this line/calling the network annotation function/
LGTM % naming
Patchset #14 (id:380001) has been deleted
Thank you Dominic, comment addressed. What about the followings? CompleteNetworkTrafficAnnotation and CompleteBranchedNetworkTrafficAnnotation https://codereview.chromium.org/2869373002/diff/360001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2869373002/diff/360001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:46: // if intended completing part is added to a partial network annotation. On 2017/05/24 12:12:51, battre wrote: > if "an" Done. https://codereview.chromium.org/2869373002/diff/360001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:84: // 'ExtendNetworkTrafficAnnotation' or 'ExpandNetworkTrafficAnnotation' On 2017/05/24 12:12:51, battre wrote: > Hm, I think that extend and expand are synonymous. Acknowledged.
On 2017/05/24 12:27:28, Ramin Halavati wrote: > Thank you Dominic, comment addressed. > > What about the followings? > > CompleteNetworkTrafficAnnotation > and > CompleteBranchedNetworkTrafficAnnotation BranchedCompleteNetworkTrafficAnnotation?
On 2017/05/24 12:39:49, battre wrote: > On 2017/05/24 12:27:28, Ramin Halavati wrote: > > Thank you Dominic, comment addressed. > > > > What about the followings? > > > > CompleteNetworkTrafficAnnotation > > and > > CompleteBranchedNetworkTrafficAnnotation > > BranchedCompleteNetworkTrafficAnnotation? LGTM! Any objections?
On 2017/05/24 12:41:29, Ramin Halavati wrote: > On 2017/05/24 12:39:49, battre wrote: > > On 2017/05/24 12:27:28, Ramin Halavati wrote: > > > Thank you Dominic, comment addressed. > > > > > > What about the followings? > > > > > > CompleteNetworkTrafficAnnotation > > > and > > > CompleteBranchedNetworkTrafficAnnotation > > > > BranchedCompleteNetworkTrafficAnnotation? > > LGTM! > > Any objections? CompleteNetworkTrafficAnnotation and BranchedCompleteNetworkTrafficAnnotation SGTM. > Regarding hash code documentations and collision resistance, the function is not > driven from a known algorithm (AFAIK) and it doesn't have any collision on > current 130 annotations. I don't know what to add or how to analyze it. Any > suggestions? Let's try not to change the hash function unless absolutely necessary. We could add a comment to recommend the user to choose a new string unique id that doesn't map to an existing hash code. If there is an existing hash code, choose a slightly different string unique id. We can enforce that using either unit test or presubmit.
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...
Description was changed from ========== Partial Protobuf for Traffic Annotation. (V.4) In many use cases, it's better to define some part of a network traffic annotation in one function and the reset of it in another one. like when a function makes the initial request and controls policies, and then passes it to another one to set/unset the cookies and perform the actual request. This CL adds required definitions to make partial annotations. BUG=656607 patch from issue 2815293002 at patchset 160001 (http://crrev.com/2815293002#ps160001) ========== to ========== Partial Protobuf for Traffic Annotation. In many use cases, it's better to define some part of a network traffic annotation in one function and the reset of it in another one. like when a function makes the initial request and controls policies, and then passes it to another one to set/unset the cookies and perform the actual request. This CL adds required definitions to make partial annotations. BUG=656607 patch from issue 2815293002 at patchset 160001 (http://crrev.com/2815293002#ps160001) ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
One more question. Can we use signed int32 instead of unsigned int32 for the hash code? UMA samples are in the range of signed int32 (see https://codereview.chromium.org/2897143003/#msg14). If you want to keep it unsigned, we will need to enforce some additional check.
On 2017/05/24 17:26:04, xunjieli wrote: > One more question. Can we use signed int32 instead of unsigned int32 for the > hash code? > UMA samples are in the range of signed int32 (see > https://codereview.chromium.org/2897143003/#msg14). > If you want to keep it unsigned, we will need to enforce some additional check. Signed overflow is undefined, which could be a problem. I suppose we could cast the final result.
On 2017/05/24 06:56:13, Ramin Halavati wrote: > Daniel, Helen, > All comments addressed, thank you very much. > > I changed the denominator of hash function to the largest prime below the > threshold, and added a new report for unique ids and hash values. I can change > this later to a json or any other suitable format. > > > Helen, > Regarding keeping all hash codes in an online db and checking it during > presubmit, we might do it later, but now there are still a lot of complications > around how to test annotations and I'd rather not add a new angle to it. Doing > it in a complete sweep in trybot is much easier now but it sure would be a good > next addition. I will add you to current discussions on this issue. > > > Daniel, > Regarding hash code documentations and collision resistance, the function is not > driven from a known algorithm (AFAIK) and it doesn't have any collision on > current 130 annotations. I don't know what to add or how to analyze it. Any > suggestions? I think it'd also be good to document if the computed result of this hash function must never change. Some of the followup comments by Helen seem to imply that we don't want to change this (which makes me wonder... what happens if people just change the tag string anyway)? On 2017/05/24 12:41:29, Ramin Halavati wrote: > On 2017/05/24 12:39:49, battre wrote: > > On 2017/05/24 12:27:28, Ramin Halavati wrote: > > > Thank you Dominic, comment addressed. > > > > > > What about the followings? > > > > > > CompleteNetworkTrafficAnnotation > > > and > > > CompleteBranchedNetworkTrafficAnnotation > > > > BranchedCompleteNetworkTrafficAnnotation? > > LGTM! > > Any objections? The name seems fine to me.
Patchset #16 (id:440001) has been deleted
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...
Thank you. Type changed to int32_t, completing function names updated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, xunjieli@chromium.org, battre@chromium.org Link to the patchset: https://codereview.chromium.org/2869373002/#ps460001 (title: "Type changed to int32, Names changed.")
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": 460001, "attempt_start_ts": 1495698594036450, "parent_rev": "cbecd589a9ca1e66959de5bc3b6016ff1adb5d21", "commit_rev": "99c6fcef9cff05b0df3a536623f8d8251c2f2a6b"}
Message was sent while issue was closed.
Description was changed from ========== Partial Protobuf for Traffic Annotation. In many use cases, it's better to define some part of a network traffic annotation in one function and the reset of it in another one. like when a function makes the initial request and controls policies, and then passes it to another one to set/unset the cookies and perform the actual request. This CL adds required definitions to make partial annotations. BUG=656607 patch from issue 2815293002 at patchset 160001 (http://crrev.com/2815293002#ps160001) ========== to ========== Partial Protobuf for Traffic Annotation. In many use cases, it's better to define some part of a network traffic annotation in one function and the reset of it in another one. like when a function makes the initial request and controls policies, and then passes it to another one to set/unset the cookies and perform the actual request. This CL adds required definitions to make partial annotations. BUG=656607 patch from issue 2815293002 at patchset 160001 (http://crrev.com/2815293002#ps160001) Review-Url: https://codereview.chromium.org/2869373002 Cr-Commit-Position: refs/heads/master@{#474602} Committed: https://chromium.googlesource.com/chromium/src/+/99c6fcef9cff05b0df3a536623f8... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:460001) as https://chromium.googlesource.com/chromium/src/+/99c6fcef9cff05b0df3a536623f8... |