Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2339193003:
A collector for postmortem reports (Closed)
|
Created:
4 years, 3 months ago by manzagop (departed) Modified:
4 years, 3 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. |
DescriptionA collector for postmortem reports
PostmortemReportCollector handles the collection of stability reports,
their packaging to minidump format and registration to the Crashpad
database for reporting.
BUG=620813
Committed: https://crrev.com/49e10c6d112f14337814ed9c844031987925fc7a
Cr-Commit-Position: refs/heads/master@{#419593}
Patch Set 1 #
Total comments: 21
Patch Set 2 : Address first round of comments #
Total comments: 10
Patch Set 3 : Address Siggi's second round #
Total comments: 8
Patch Set 4 : Address nits #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 28 (9 generated)
manzagop@chromium.org changed reviewers: + siggi@chromium.org
Here's the report collector, broken out of https://crrev.com/2128683002 . Please have a look!
Nice - I didn't read through the tests in detail yet, but here are my initial comments. https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector.cc:86: CrashReportDatabase::NewReport* new_report; nullme? https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector.cc:101: maybe you want to defer the file delete and bail to this point? https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector.cc:156: DCHECK(thread_analyzer->IsValid()); why is this - do they only become invalid on a race? https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... File components/browser_watcher/postmortem_report_collector.h (right): https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector.h:33: // data in |debug_dir| using |debug_file_pattern|. If data is found, the docs look outdated. https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector.h:41: // the stability debug information files. what does it return? https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector.h:48: private: is this the recommended way to do things? Another way to do this is to have this protected, and test against a derived TestingXXX instance ? https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... File components/browser_watcher/postmortem_report_collector_unittest.cc (right): https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector_unittest.cc:50: class MockCrashReportDatabase : public CrashReportDatabase { ah, this is an interface - sweet! https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector_unittest.cc:119: arg.SerializeToString(&actual_serialized); is the serialized protobuf canonical? ISTR an upcoming change where e.g. maps will iterate randomly, which doesn't mean they won't serialize canonically, but I just don't know... https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector_unittest.cc:125: TEST(PostmortemReportCollectorTest, CollectAndSubmitForUpload) { IMHO there ought to be a test for the "stuck file" case also. https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector_unittest.cc:140: std::unique_ptr<MockCrashReportDatabase> database( nit: is there a reason why this can't be a plain-old local?
Thanks! Ready for another round. https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector.cc:86: CrashReportDatabase::NewReport* new_report; On 2016/09/14 18:04:31, Sigurður Ásgeirsson wrote: > nullme? Done. https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector.cc:101: On 2016/09/14 18:04:31, Sigurður Ásgeirsson wrote: > maybe you want to defer the file delete and bail to this point? Done, but I also added a deletion at l.80 for the empty / corrupted files. https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector.cc:156: DCHECK(thread_analyzer->IsValid()); On 2016/09/14 18:04:30, Sigurður Ásgeirsson wrote: > why is this - do they only become invalid on a race? An analyzer is valid or invalid from construction and the iteration is only over valid analyzers. I've revised the comment in activity_analyzer.h to say so. https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... File components/browser_watcher/postmortem_report_collector.h (right): https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector.h:33: // data in |debug_dir| using |debug_file_pattern|. If data is found, On 2016/09/14 18:04:31, Sigurður Ásgeirsson wrote: > the docs look outdated. Removed. Done. https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector.h:41: // the stability debug information files. On 2016/09/14 18:04:31, Sigurður Ásgeirsson wrote: > what does it return? Done. https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector.h:48: private: On 2016/09/14 18:04:31, Sigurður Ásgeirsson wrote: > is this the recommended way to do things? Another way to do this is to have this > protected, and test against a derived TestingXXX instance ? Dunno. I started with what you suggest, but changed it in https://crrev.com/2128683002 due to a mild preference by scottmg@. This is more verbose in this file, but closer to the intent wrt access... Both approaches require no changes if you prefix tests with FLAKY_ or DISABLED_. Do you have a strong preference? https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... File components/browser_watcher/postmortem_report_collector_unittest.cc (right): https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector_unittest.cc:50: class MockCrashReportDatabase : public CrashReportDatabase { On 2016/09/14 18:04:31, Sigurður Ásgeirsson wrote: > ah, this is an interface - sweet! Acknowledged. https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector_unittest.cc:119: arg.SerializeToString(&actual_serialized); On 2016/09/14 18:04:31, Sigurður Ásgeirsson wrote: > is the serialized protobuf canonical? ISTR an upcoming change where e.g. maps > will iterate randomly, which doesn't mean they won't serialize canonically, but > I just don't know... You're right, there is no deterministic iteration requirement for maps. There's a few options: 1) Build proper generic proto matcher. I don't think we can do this with the lite runtime which doesn't have reflection. 2) Code a serialization to string that is deterministic. That is doable, and might actually be provided by protobufs down the line. Not sure how much work it would be. 3) Hand craft a matcher for StabilityReport 4) leave this as is with a note/todo I've gone with 4, but tell me if you think we should do more. Probably 3) would be the next option in terms of required work. https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector_unittest.cc:125: TEST(PostmortemReportCollectorTest, CollectAndSubmitForUpload) { On 2016/09/14 18:04:32, Sigurður Ásgeirsson wrote: > IMHO there ought to be a test for the "stuck file" case also. Done. https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector_unittest.cc:140: std::unique_ptr<MockCrashReportDatabase> database( On 2016/09/14 18:04:31, Sigurður Ásgeirsson wrote: > nit: is there a reason why this can't be a plain-old local? Done.
LGTM with a couple of optional nits. https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... File components/browser_watcher/postmortem_report_collector.h (right): https://codereview.chromium.org/2339193003/diff/1/components/browser_watcher/... components/browser_watcher/postmortem_report_collector.h:48: private: On 2016/09/15 15:07:38, manzagop wrote: > On 2016/09/14 18:04:31, Sigurður Ásgeirsson wrote: > > is this the recommended way to do things? Another way to do this is to have > this > > protected, and test against a derived TestingXXX instance ? > > Dunno. I started with what you suggest, but changed it in > https://crrev.com/2128683002 due to a mild preference by scottmg@. This is more > verbose in this file, but closer to the intent wrt access... Both approaches > require no changes if you prefix tests with FLAKY_ or DISABLED_. > > Do you have a strong preference? I have no strong preference. https://codereview.chromium.org/2339193003/diff/20001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2339193003/diff/20001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:27: // TODO(manzagop): throttling and graceful handling of too much data. I think it makes sense for throttling to be centralized in the uploader? https://codereview.chromium.org/2339193003/diff/20001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:60: std::unique_ptr<StabilityReport> report_proto = Collect(file); it would IMHO be much more readable to scoop the body of this loop into a function. Early return is somehow much more readable (in my opinion) than continue... https://codereview.chromium.org/2339193003/diff/20001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:93: // TODO(manzagop): metrics for the number of non-deletable files. is there any reason you can't collect these metrics here for reporting by the user, and/or just UMA them directly? https://codereview.chromium.org/2339193003/diff/20001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:151: while (thread_analyzer) { could write this as a for(;;)?
Addressed comments. You may want to have another look as there were some non-trivial changes. https://codereview.chromium.org/2339193003/diff/20001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2339193003/diff/20001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:27: // TODO(manzagop): throttling and graceful handling of too much data. On 2016/09/15 18:44:10, Sigurður Ásgeirsson wrote: > I think it makes sense for throttling to be centralized in the uploader? I've rewritten the TODO to more clearly express the concern is about the collection taking too long on a critical path (e.g. starting chrome). I've also moved the TODO to the .h. https://codereview.chromium.org/2339193003/diff/20001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:60: std::unique_ptr<StabilityReport> report_proto = Collect(file); On 2016/09/15 18:44:10, Sigurður Ásgeirsson wrote: > it would IMHO be much more readable to scoop the body of this loop into a > function. > Early return is somehow much more readable (in my opinion) than continue... +1, and it's a nicely self contained unit. Done. https://codereview.chromium.org/2339193003/diff/20001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:93: // TODO(manzagop): metrics for the number of non-deletable files. On 2016/09/15 18:44:10, Sigurður Ásgeirsson wrote: > is there any reason you can't collect these metrics here for reporting by the > user, and/or just UMA them directly? Done. https://codereview.chromium.org/2339193003/diff/20001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:151: while (thread_analyzer) { On 2016/09/15 18:44:10, Sigurður Ásgeirsson wrote: > could write this as a for(;;)? Done, but note I've left the init statement empty as I think it better highlights it's common to get no analyzer (in the clean exit case).
manzagop@chromium.org changed reviewers: + bcwhite@chromium.org, scottmg@chromium.org
Hi Brian, Scott, I've broken out the report collector from https://crrev.com/2128683002 to this CL. Brian: there shouldn't be changes wrt ActivityAnalyzer use from when you reviewed, but a quick peek may still be a good idea. Scott: this has the interaction with the Crashpad database, which wasn't yet in https://crrev.com/2128683002 when you made your pass. Thanks! Pierre
lgtm https://codereview.chromium.org/2339193003/diff/20001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2339193003/diff/20001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:151: while (thread_analyzer) { On 2016/09/16 16:37:30, manzagop wrote: > On 2016/09/15 18:44:10, Sigurður Ásgeirsson wrote: > > could write this as a for(;;)? > > Done, but note I've left the init statement empty as I think it better > highlights it's common to get no analyzer (in the clean exit case). I like it - I often use the same idiom, mainly because multi-line for(;;) clauses annoy me :).
Crashpad interaction lgtm. https://codereview.chromium.org/2339193003/diff/40001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2339193003/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:136: database_status = report_database->FinishedWritingCrashReport( It'll take up to 15 minutes for this to get "noticed" right now, but I guess that's OK. https://codereview.chromium.org/2339193003/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:188: MinidumpInfo mindump_info; mindump -> minidump https://codereview.chromium.org/2339193003/diff/40001/components/browser_watc... File components/browser_watcher/postmortem_report_collector_unittest.cc (right): https://codereview.chromium.org/2339193003/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector_unittest.cc:169: // requesting Weird comment wrapping here. https://codereview.chromium.org/2339193003/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector_unittest.cc:203: EXPECT_CALL(database_, FinishedWritingCrashReport(&crashpad_report_, _)) I think I probably would have just used a real database in a tempdir and verified the contents of the reports. But if prefer mockymocks that's fine too.
LGTM for ActivityTracker interface.
Thanks for the reviews! https://codereview.chromium.org/2339193003/diff/20001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2339193003/diff/20001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:151: while (thread_analyzer) { On 2016/09/16 17:01:02, Sigurður Ásgeirsson wrote: > On 2016/09/16 16:37:30, manzagop wrote: > > On 2016/09/15 18:44:10, Sigurður Ásgeirsson wrote: > > > could write this as a for(;;)? > > > > Done, but note I've left the init statement empty as I think it better > > highlights it's common to get no analyzer (in the clean exit case). > > I like it - I often use the same idiom, mainly because multi-line for(;;) > clauses annoy me :). Acknowledged. https://codereview.chromium.org/2339193003/diff/40001/components/browser_watc... File components/browser_watcher/postmortem_report_collector.cc (right): https://codereview.chromium.org/2339193003/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:136: database_status = report_database->FinishedWritingCrashReport( On 2016/09/16 17:07:45, scottmg wrote: > It'll take up to 15 minutes for this to get "noticed" right now, but I guess > that's OK. Great point! I've added a comment. https://codereview.chromium.org/2339193003/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector.cc:188: MinidumpInfo mindump_info; On 2016/09/16 17:07:45, scottmg wrote: > mindump -> minidump Done. https://codereview.chromium.org/2339193003/diff/40001/components/browser_watc... File components/browser_watcher/postmortem_report_collector_unittest.cc (right): https://codereview.chromium.org/2339193003/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector_unittest.cc:169: // requesting On 2016/09/16 17:07:45, scottmg wrote: > Weird comment wrapping here. Done. https://codereview.chromium.org/2339193003/diff/40001/components/browser_watc... components/browser_watcher/postmortem_report_collector_unittest.cc:203: EXPECT_CALL(database_, FinishedWritingCrashReport(&crashpad_report_, _)) On 2016/09/16 17:07:45, scottmg wrote: > I think I probably would have just used a real database in a tempdir and > verified the contents of the reports. But if prefer mockymocks that's fine too. Acknowledged.
manzagop@chromium.org changed reviewers: + rkaplow@chromium.org, sky@chromium.org
Hi Rob, Scott, I'm looking for an OWNERS' review: - Rob: for the histograms.xml change - Scott: for the DEPS change that adds components/version_info Please have a look! Thanks! Pierre
Description was changed from ========== A collector for postmortem reports PostmortemReportCollector handles the collection of stability reports, their packaging to minidump format and registration to the Crashpad database for reporting. BUG=620813 ========== to ========== A collector for postmortem reports PostmortemReportCollector handles the collection of stability reports, their packaging to minidump format and registration to the Crashpad database for reporting. BUG=620813 ==========
rkaplow@chromium.org changed reviewers: + gayane@chromium.org
LGTM https://codereview.chromium.org/2339193003/diff/60001/components/browser_watc... File components/browser_watcher/DEPS (right): https://codereview.chromium.org/2339193003/diff/60001/components/browser_watc... components/browser_watcher/DEPS:3: "+components/version_info", version_info depends upon ui/base. So, this changes makes me mildly nervous. But I can't readily see any problems.
lgtm
Thanks for the review! Friendly ping Rob for tools/metrics OWNERS. Thanks! https://codereview.chromium.org/2339193003/diff/60001/components/browser_watc... File components/browser_watcher/DEPS (right): https://codereview.chromium.org/2339193003/diff/60001/components/browser_watc... components/browser_watcher/DEPS:3: "+components/version_info", On 2016/09/16 21:23:38, sky wrote: > version_info depends upon ui/base. So, this changes makes me mildly nervous. But > I can't readily see any problems. Acknowledged.
lgtm
On 2016/09/19 21:14:31, rkaplow wrote: > lgtm Thanks!
The CQ bit was checked by manzagop@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bcwhite@chromium.org, siggi@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2339193003/#ps60001 (title: "Address nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== A collector for postmortem reports PostmortemReportCollector handles the collection of stability reports, their packaging to minidump format and registration to the Crashpad database for reporting. BUG=620813 ========== to ========== A collector for postmortem reports PostmortemReportCollector handles the collection of stability reports, their packaging to minidump format and registration to the Crashpad database for reporting. BUG=620813 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== A collector for postmortem reports PostmortemReportCollector handles the collection of stability reports, their packaging to minidump format and registration to the Crashpad database for reporting. BUG=620813 ========== to ========== A collector for postmortem reports PostmortemReportCollector handles the collection of stability reports, their packaging to minidump format and registration to the Crashpad database for reporting. BUG=620813 Committed: https://crrev.com/49e10c6d112f14337814ed9c844031987925fc7a Cr-Commit-Position: refs/heads/master@{#419593} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/49e10c6d112f14337814ed9c844031987925fc7a Cr-Commit-Position: refs/heads/master@{#419593} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
