Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2722223002:
Separate collection logic from the extraction of the report (Closed)
|
Created:
3 years, 9 months ago by manzagop (departed) Modified:
3 years, 9 months ago Reviewers:
Sigurður Ásgeirsson CC:
chromium-reviews, bcwhite Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSeparate collection logic from the extraction of the report
Separate collection logic (identifying files, registering reports with crashpad, calling the minidump writer) from the actual extraction of the report proto from disk.
BUG=691595
Review-Url: https://codereview.chromium.org/2722223002
Cr-Commit-Position: refs/heads/master@{#455232}
Committed: https://chromium.googlesource.com/chromium/src/+/6d901940e770f2494539d0bf177ae6cd027a8b19
Patch Set 1 #
Total comments: 4
Patch Set 2 : impl -> extractor #Patch Set 3 : Add a TODO #
Total comments: 7
Patch Set 4 : Address Siggi's comments #
Total comments: 2
Patch Set 5 : Merge #Patch Set 6 : Address Siggi's comment #Patch Set 7 : Relocate CollectThread in file #
Total comments: 4
Patch Set 8 : Merge #Patch Set 9 : Address Siggi's comments #
Messages
Total messages: 43 (26 generated)
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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 ========== PostmortemReportCollector: extract collection to _impl files Collection is growing more complex. Pull it out to _impl files to manage cognitive load. BUG=691595 ========== to ========== PostmortemReportCollector: extract collection to _impl files Collection is growing more complex. Pull it out actual collection to *_impl files to reduce cognitive load. PostmortemReportCollector still handles finding the stability files, registering a crash with the database and invoking WriteDump. BUG=691595 ==========
manzagop@chromium.org changed reviewers: + siggi@chromium.org
Hi! Could you have a look? This is shuffling code. Thanks! Pierre
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2722223002/diff/20001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.h (right): https://codereview.chromium.org/2722223002/diff/20001/components/browser_watc... components/browser_watcher/postmortem_report_collector.h:24: #include "components/browser_watcher/postmortem_report_collector_impl.h" like we discussed, the thing you're extracting here probably wants to be named "XXXExtractor", or the like as it is about extracting the crumbs from a file, into a proto. https://codereview.chromium.org/2722223002/diff/20001/components/browser_watc... File components/browser_watcher/postmortem_report_collector_impl.h (right): https://codereview.chromium.org/2722223002/diff/20001/components/browser_watc... components/browser_watcher/postmortem_report_collector_impl.h:29: // Collects a stability report from a stability file. Extracts? Also, maybe you want to pass in the Analyzer, rather than a file path. That way, I'd guess this thing will work to scrape from a crashed process too?
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
I've renamed the file. https://codereview.chromium.org/2722223002/diff/20001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.h (right): https://codereview.chromium.org/2722223002/diff/20001/components/browser_watc... components/browser_watcher/postmortem_report_collector.h:24: #include "components/browser_watcher/postmortem_report_collector_impl.h" On 2017/03/01 20:50:30, Sigurður Ásgeirsson wrote: > like we discussed, the thing you're extracting here probably wants to be named > "XXXExtractor", or the like as it is about extracting the crumbs from a file, > into a proto. Done. https://codereview.chromium.org/2722223002/diff/20001/components/browser_watc... File components/browser_watcher/postmortem_report_collector_impl.h (right): https://codereview.chromium.org/2722223002/diff/20001/components/browser_watc... components/browser_watcher/postmortem_report_collector_impl.h:29: // Collects a stability report from a stability file. On 2017/03/01 20:50:30, Sigurður Ásgeirsson wrote: > Extracts? Done. > Also, maybe you want to pass in the Analyzer, rather than a file path. That way, > I'd guess this thing will work to scrape from a crashed process too? I think that will happen from the filename (it's derived from pid+creation time).
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 ========== PostmortemReportCollector: extract collection to _impl files Collection is growing more complex. Pull it out actual collection to *_impl files to reduce cognitive load. PostmortemReportCollector still handles finding the stability files, registering a crash with the database and invoking WriteDump. BUG=691595 ========== to ========== Split PostmortemReportCollector Separate the logic of collection (identifying files, registering reports with crashpad, calling the minidump writer) from the actual extraction of the report proto from disk. BUG=691595 ==========
Description was changed from ========== Split PostmortemReportCollector Separate the logic of collection (identifying files, registering reports with crashpad, calling the minidump writer) from the actual extraction of the report proto from disk. BUG=691595 ========== to ========== Separate collection logic from the extraction of the report Separate collection logic (identifying files, registering reports with crashpad, calling the minidump writer) from the actual extraction of the report proto from disk. BUG=691595 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On Wed, Mar 1, 2017 at 4:49 PM <manzagop@chromium.org> wrote: > I've renamed the file. > > > > > https://codereview.chromium.org/2722223002/diff/20001/components/browser_watc... > File components/browser_watcher/postmortem_report_collector.h (right): > > > https://codereview.chromium.org/2722223002/diff/20001/components/browser_watc... > components/browser_watcher/postmortem_report_collector.h:24: #include > "components/browser_watcher/postmortem_report_collector_impl.h" > On 2017/03/01 20:50:30, Sigurður Ásgeirsson wrote: > > like we discussed, the thing you're extracting here probably wants to > be named > > "XXXExtractor", or the like as it is about extracting the crumbs from > a file, > > into a proto. > > Done. > > > > https://codereview.chromium.org/2722223002/diff/20001/components/browser_watc... > File components/browser_watcher/postmortem_report_collector_impl.h > (right): > > > https://codereview.chromium.org/2722223002/diff/20001/components/browser_watc... > components/browser_watcher/postmortem_report_collector_impl.h:29: // > Collects a stability report from a stability file. > On 2017/03/01 20:50:30, Sigurður Ásgeirsson wrote: > > Extracts? > > Done. > > > > Also, maybe you want to pass in the Analyzer, rather than a file path. > That way, > > I'd guess this thing will work to scrape from a crashed process too? > > I think that will happen from the filename (it's derived from > pid+creation time). > > Hmmm, but would your thing be easier to test if you pass it the interface, instead of the file name? You should at least be able to create an anonymous mapping and elide a temp file/dir creation? > https://codereview.chromium.org/2722223002/ > -- 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.
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
> > > Also, maybe you want to pass in the Analyzer, rather than a file path. > > That way, > > > I'd guess this thing will work to scrape from a crashed process too? > > > > I think that will happen from the filename (it's derived from > > pid+creation time). > > > > Hmmm, but would your thing be easier to test if you pass it the interface, > instead of the file name? You should at least be able to create an > anonymous mapping and elide a temp file/dir creation? Added a TODO. I'm hoping to relocate the tests to a separate file as a follow up CL as well, to get this CL in earlier and avoid merge pain.
A couple of moar nits. https://codereview.chromium.org/2722223002/diff/60001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.h (right): https://codereview.chromium.org/2722223002/diff/60001/components/browser_watc... components/browser_watcher/postmortem_report_collector.h:32: // crash report with the crash datapbase and calling into code to write out nit: datapbase->database. https://codereview.chromium.org/2722223002/diff/60001/components/browser_watc... File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2722223002/diff/60001/components/browser_watc... components/browser_watcher/postmortem_report_extractor.cc:117: char code_identifier[17]; Ugh :/ https://codereview.chromium.org/2722223002/diff/60001/components/browser_watc... components/browser_watcher/postmortem_report_extractor.cc:127: snprintf(code_identifier, sizeof(code_identifier), "%08X%zx", I'd sprint for a StringPrintf here and below. Fixed-size buffers on stack are ... ughy.
Thanks! Another look? https://codereview.chromium.org/2722223002/diff/60001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.h (right): https://codereview.chromium.org/2722223002/diff/60001/components/browser_watc... components/browser_watcher/postmortem_report_collector.h:32: // crash report with the crash datapbase and calling into code to write out On 2017/03/06 19:17:10, Sigurður Ásgeirsson wrote: > nit: datapbase->database. Done. https://codereview.chromium.org/2722223002/diff/60001/components/browser_watc... File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2722223002/diff/60001/components/browser_watc... components/browser_watcher/postmortem_report_extractor.cc:117: char code_identifier[17]; On 2017/03/06 19:17:10, Sigurður Ásgeirsson wrote: > Ugh :/ Acknowledged. https://codereview.chromium.org/2722223002/diff/60001/components/browser_watc... components/browser_watcher/postmortem_report_extractor.cc:127: snprintf(code_identifier, sizeof(code_identifier), "%08X%zx", On 2017/03/06 19:17:10, Sigurður Ásgeirsson wrote: > I'd sprint for a StringPrintf here and below. Fixed-size buffers on stack are > ... ughy. Done. Can you say more?
https://codereview.chromium.org/2722223002/diff/60001/components/browser_watc... File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2722223002/diff/60001/components/browser_watc... components/browser_watcher/postmortem_report_extractor.cc:127: snprintf(code_identifier, sizeof(code_identifier), "%08X%zx", On 2017/03/06 21:29:39, manzagop wrote: > On 2017/03/06 19:17:10, Sigurður Ásgeirsson wrote: > > I'd sprint for a StringPrintf here and below. Fixed-size buffers on stack are > > ... ughy. > > Done. Can you say more? https://www.google.ca/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8#q=s... Probably doesn't apply here, but not the sort of thing we do lightly anymore. Should cause review angst if ever spotted. https://codereview.chromium.org/2722223002/diff/80001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.h (right): https://codereview.chromium.org/2722223002/diff/80001/components/browser_watc... components/browser_watcher/postmortem_report_collector.h:30: // Handles postmortem report collection by establishing the set of stability nit: Nice to use a numbered enumeration or a bullet list for this sort of thing.
The CQ bit was checked by manzagop@chromium.org to run a CQ dry run
Thanks! PTAL. https://codereview.chromium.org/2722223002/diff/80001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.h (right): https://codereview.chromium.org/2722223002/diff/80001/components/browser_watc... components/browser_watcher/postmortem_report_collector.h:30: // Handles postmortem report collection by establishing the set of stability On 2017/03/07 14:46:01, Sigurður Ásgeirsson wrote: > nit: Nice to use a numbered enumeration or a bullet list for this sort of thing. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
You have several DO NOT SUBMIT commends in there, do your really want me to review right now? On Tue, Mar 7, 2017 at 11:14 AM <manzagop@chromium.org> wrote: > Thanks! PTAL. > > > > > https://codereview.chromium.org/2722223002/diff/80001/components/browser_watc... > File components/browser_watcher/postmortem_report_collector.h (right): > > > https://codereview.chromium.org/2722223002/diff/80001/components/browser_watc... > components/browser_watcher/postmortem_report_collector.h:30: // Handles > postmortem report collection by establishing the set of stability > On 2017/03/07 14:46:01, Sigurður Ásgeirsson wrote: > > nit: Nice to use a numbered enumeration or a bullet list for this sort > of thing. > > Done. > > https://codereview.chromium.org/2722223002/ > -- 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> You have several DO NOT SUBMIT commends in there, do your really want me to > review right now? Oops, forgot about those. They were meant to minimize the diff for the review. I've now swapped the two function's order in the file.
The CQ bit was checked by manzagop@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...
So ready for review now? On Tue, Mar 7, 2017 at 11:28 AM <manzagop@chromium.org> wrote: > > You have several DO NOT SUBMIT commends in there, do your really want me > to > > review right now? > > Oops, forgot about those. They were meant to minimize the diff for the > review. > > I've now swapped the two function's order in the file. > > https://codereview.chromium.org/2722223002/ > -- 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.
Yes, please! On Tue, Mar 7, 2017 at 11:42 AM, Sigurður Ásgeirsson <siggi@chromium.org> wrote: > So ready for review now? > > On Tue, Mar 7, 2017 at 11:28 AM <manzagop@chromium.org> wrote: > >> > You have several DO NOT SUBMIT commends in there, do your really want >> me to >> > review right now? >> >> Oops, forgot about those. They were meant to minimize the diff for the >> review. >> >> I've now swapped the two function's order in the file. >> >> https://codereview.chromium.org/2722223002/ >> > -- 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm - though I think you'll conflict with https://codereview.chromium.org/2715903003/. https://codereview.chromium.org/2722223002/diff/140001/components/browser_wat... File components/browser_watcher/postmortem_report_collector.h (right): https://codereview.chromium.org/2722223002/diff/140001/components/browser_wat... components/browser_watcher/postmortem_report_collector.h:32: // - calling into code to extract a report protocol buffer ubernit: I think for our purposes "calling into code" is implied :). https://codereview.chromium.org/2722223002/diff/140001/components/browser_wat... File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2722223002/diff/140001/components/browser_wat... components/browser_watcher/postmortem_report_extractor.cc:183: // Collect user data nit: comments end in a full stop.
Thanks for the review! Going ahead with submission. https://codereview.chromium.org/2722223002/diff/140001/components/browser_wat... File components/browser_watcher/postmortem_report_collector.h (right): https://codereview.chromium.org/2722223002/diff/140001/components/browser_wat... components/browser_watcher/postmortem_report_collector.h:32: // - calling into code to extract a report protocol buffer On 2017/03/07 18:45:02, Sigurður Ásgeirsson wrote: > ubernit: I think for our purposes "calling into code" is implied :). Done. https://codereview.chromium.org/2722223002/diff/140001/components/browser_wat... File components/browser_watcher/postmortem_report_extractor.cc (right): https://codereview.chromium.org/2722223002/diff/140001/components/browser_wat... components/browser_watcher/postmortem_report_extractor.cc:183: // Collect user data On 2017/03/07 18:45:02, Sigurður Ásgeirsson wrote: > nit: comments end in a full stop. Done.
The CQ bit was checked by manzagop@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from siggi@chromium.org Link to the patchset: https://codereview.chromium.org/2722223002/#ps180001 (title: "Address Siggi's comments")
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": 180001, "attempt_start_ts": 1488915601660750,
"parent_rev": "2ba9b1f20d4f158b5cdeb359d9ab7d836e09858a", "commit_rev":
"6d901940e770f2494539d0bf177ae6cd027a8b19"}
Message was sent while issue was closed.
Description was changed from ========== Separate collection logic from the extraction of the report Separate collection logic (identifying files, registering reports with crashpad, calling the minidump writer) from the actual extraction of the report proto from disk. BUG=691595 ========== to ========== Separate collection logic from the extraction of the report Separate collection logic (identifying files, registering reports with crashpad, calling the minidump writer) from the actual extraction of the report proto from disk. BUG=691595 Review-Url: https://codereview.chromium.org/2722223002 Cr-Commit-Position: refs/heads/master@{#455232} Committed: https://chromium.googlesource.com/chromium/src/+/6d901940e770f2494539d0bf177a... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/6d901940e770f2494539d0bf177a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
