Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
970953002:
Clarifications to network bug triage process. (Closed)
|
Created:
5 years, 9 months ago by Randy Smith (Not in Mondays) Modified:
5 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, laforge Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClarifications to network bug triage process.
Specfiically:
* Highlight top level responsibilities.
* Clarify detailed responsibilities around data gathering, filing crasher bugs, and monitoring Gasper/UMA.
BUG=None
R=mmenke@chromium.org
Committed: https://crrev.com/9ae51ac13fe63dc697425d3bb1c17965d71783f1
Cr-Commit-Position: refs/heads/master@{#319903}
Patch Set 1 #Patch Set 2 : Results of self review. #
Total comments: 13
Patch Set 3 : Incorporated comments. #Patch Set 4 : Incorporated crash feedback. #
Total comments: 7
Patch Set 5 : Incorporate all comments except for Needs-Feedback rewrite. #Patch Set 6 : Reorganized dealing with Needs-Feedback label #Messages
Total messages: 16 (4 generated)
Matt: I'm phrasing some suggestions around the network bug triage process in the form of a CL; PTAL? (I'm also going to give net-dev a heads up in case other folks want to comment.)
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... File net/docs/bug-triage-suggested-workflow.txt (right): https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:31: judgement when filing bugs. There is more needed here - for example, examining third-party modules for signs of malware (known and unknown) More importantly, however, the TPMs are already executing this crasher triage as part of their TPM duties, and already providing a high quality signal/noise filtering. That is, considering population of distribution to population of crashes. https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:33: * The backtrace Do not do this unless you've confirmed with the TPMs on the privacy implications. This _used_ to be practice and was stopped. https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:49: Please run these last two steps by the TPMs.
https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... File net/docs/bug-triage-suggested-workflow.txt (right): https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:31: judgement when filing bugs. On 2015/03/02 19:01:09, Ryan Sleevi wrote: > There is more needed here - for example, examining third-party modules for signs > of malware (known and unknown) > > More importantly, however, the TPMs are already executing this crasher triage as > part of their TPM duties, and already providing a high quality signal/noise > filtering. That is, considering population of distribution to population of > crashes. I'd take issue with "high quality signal/noise filtering". I've repeatedly seen something called a "top-10 crasher" when only one user had 3 or 4 crashes in a couple hours of Canary data. I really don't feel like we're doing a good or consistent job of catching crashers. We're doing a worse job of investigating crashers, but that's beyond the scope of this document. :( https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:78: the reporter to make forward progress on the bug. IF it is, request nit: IF -> If https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage.txt File net/docs/bug-triage.txt (right): https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage.txt#... net/docs/bug-triage.txt:66: appropriate label if so. Note that if no more specifical label "no more specifical label" -> "no label more specific than"
Conversation with TPMs still in process, so this isn't final. But all comments incorporated. https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... File net/docs/bug-triage-suggested-workflow.txt (right): https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:31: judgement when filing bugs. On 2015/03/02 19:01:09, Ryan Sleevi wrote: > There is more needed here - for example, examining third-party modules for signs > of malware (known and unknown) So I thought about doing that, but I have no clue how to actually write that section. Want to suggest some text/pop up your own CL? > More importantly, however, the TPMs are already executing this crasher triage as > part of their TPM duties, and already providing a high quality signal/noise > filtering. That is, considering population of distribution to population of > crashes. Conversation with TPMs in progress; I'll reflect the results in this CL. https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:31: judgement when filing bugs. On 2015/03/02 19:15:13, mmenke wrote: > On 2015/03/02 19:01:09, Ryan Sleevi wrote: > > There is more needed here - for example, examining third-party modules for > signs > > of malware (known and unknown) > > > > More importantly, however, the TPMs are already executing this crasher triage > as > > part of their TPM duties, and already providing a high quality signal/noise > > filtering. That is, considering population of distribution to population of > > crashes. > > I'd take issue with "high quality signal/noise filtering". I've repeatedly seen > something called a "top-10 crasher" when only one user had 3 or 4 crashes in a > couple hours of Canary data. > > I really don't feel like we're doing a good or consistent job of catching > crashers. We're doing a worse job of investigating crashers, but that's beyond > the scope of this document. :( Acknowledged. https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:33: * The backtrace On 2015/03/02 19:01:09, Ryan Sleevi wrote: > Do not do this unless you've confirmed with the TPMs on the privacy > implications. This _used_ to be practice and was stopped. Conversation in process. https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:49: On 2015/03/02 19:01:09, Ryan Sleevi wrote: > Please run these last two steps by the TPMs. These steps are ok by them (laforge@ specifically). https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:78: the reporter to make forward progress on the bug. IF it is, request On 2015/03/02 19:15:13, mmenke wrote: > nit: IF -> If Done. https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage.txt File net/docs/bug-triage.txt (right): https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage.txt#... net/docs/bug-triage.txt:66: appropriate label if so. Note that if no more specifical label On 2015/03/02 19:15:13, mmenke wrote: > "no more specifical label" -> "no label more specific than" Done.
rdsmith@chromium.org changed required reviewers: + mmenke@chromium.org
TPM input incorporated. Anyone who wants to review, please review (Matt primary). https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... File net/docs/bug-triage-suggested-workflow.txt (right): https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:33: * The backtrace On 2015/03/03 15:08:01, rdsmith wrote: > On 2015/03/02 19:01:09, Ryan Sleevi wrote: > > Do not do this unless you've confirmed with the TPMs on the privacy > > implications. This _used_ to be practice and was stopped. > > Conversation in process. Refined contexts under which adding a backtrace is appropriate.
On 2015/03/04 22:22:21, rdsmith wrote: > TPM input incorporated. Anyone who wants to review, please review (Matt > primary). > > https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... > File net/docs/bug-triage-suggested-workflow.txt (right): > > https://codereview.chromium.org/970953002/diff/20001/net/docs/bug-triage-sugg... > net/docs/bug-triage-suggested-workflow.txt:33: * The backtrace > On 2015/03/03 15:08:01, rdsmith wrote: > > On 2015/03/02 19:01:09, Ryan Sleevi wrote: > > > Do not do this unless you've confirmed with the TPMs on the privacy > > > implications. This _used_ to be practice and was stopped. > > > > Conversation in process. > > Refined contexts under which adding a backtrace is appropriate. LGTM
https://codereview.chromium.org/970953002/diff/60001/net/docs/bug-triage-sugg... File net/docs/bug-triage-suggested-workflow.txt (right): https://codereview.chromium.org/970953002/diff/60001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:3: * Choose the platforms and releases for which you will be Think this would be clearer as "For each platform, look through the releases for which..." And then you can remove the last sentence of the paragraph https://codereview.chromium.org/970953002/diff/60001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:20: narrowly labeled network buckets). I don't think the "(and in such a way..." part of this description adds anything. https://codereview.chromium.org/970953002/diff/60001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:84: that information and mark the bug Needs-Feedback. The first bullet seems redundant with the "Dealing with old bugs" section at the bottom, and the second bullet is covered in "Investigating Cr-Internals-Network bugs". Open to reorganizing, but don't want to be too redundant, since that inevitably leads to one copy getting outdated.
Matt: PTAL? https://codereview.chromium.org/970953002/diff/60001/net/docs/bug-triage-sugg... File net/docs/bug-triage-suggested-workflow.txt (right): https://codereview.chromium.org/970953002/diff/60001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:3: * Choose the platforms and releases for which you will be On 2015/03/05 15:49:28, mmenke wrote: > Think this would be clearer as "For each platform, look through the releases for > which..." And then you can remove the last sentence of the paragraph Done. https://codereview.chromium.org/970953002/diff/60001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:20: narrowly labeled network buckets). On 2015/03/05 15:49:28, mmenke wrote: > I don't think the "(and in such a way..." part of this description adds > anything. Done. https://codereview.chromium.org/970953002/diff/60001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:84: that information and mark the bug Needs-Feedback. On 2015/03/05 15:49:28, mmenke wrote: > The first bullet seems redundant with the "Dealing with old bugs" section at the > bottom, and the second bullet is covered in "Investigating Cr-Internals-Network > bugs". Open to reorganizing, but don't want to be too redundant, since that > inevitably leads to one copy getting outdated. Hmmm. This brings up a tricky set of questions, that I"m not sure how to resolve: * I don't personally believe that people will ever get to the "Dealing with old bugs" section (or rarely) since it's best effort, and there's a fair amount of stuff in the active responsibilities section. * I'd like to have sections in "suggested-workflow" that map pretty directly from the responsibilities listed in bug-triage.txt; this section was absent. Proposal: Both here and in bug-triage.txt, combine "Request data about recent unassigned Cr-Internals-Network bugs" in with "Investigate each recent Cr-Internals-Network issue", but make sure that the information in both bullet points above is included in that (some redundancy with "old bugs" as you note). I'll do that in a separate PS so that it can be examined and reverted easily; let me know what you think.
LGTM. Sorry for slowness on this one - Kept on forgetting about it. https://codereview.chromium.org/970953002/diff/60001/net/docs/bug-triage-sugg... File net/docs/bug-triage-suggested-workflow.txt (right): https://codereview.chromium.org/970953002/diff/60001/net/docs/bug-triage-sugg... net/docs/bug-triage-suggested-workflow.txt:84: that information and mark the bug Needs-Feedback. On 2015/03/06 00:21:35, rdsmith wrote: > On 2015/03/05 15:49:28, mmenke wrote: > > The first bullet seems redundant with the "Dealing with old bugs" section at > the > > bottom, and the second bullet is covered in "Investigating > Cr-Internals-Network > > bugs". Open to reorganizing, but don't want to be too redundant, since that > > inevitably leads to one copy getting outdated. > > Hmmm. This brings up a tricky set of questions, that I"m not sure how to > resolve: > * I don't personally believe that people will ever get to the "Dealing with old > bugs" section (or rarely) since it's best effort, and there's a fair amount of > stuff in the active responsibilities section. > * I'd like to have sections in "suggested-workflow" that map pretty directly > from the responsibilities listed in bug-triage.txt; this section was absent. > > Proposal: Both here and in bug-triage.txt, combine "Request data about recent > unassigned Cr-Internals-Network bugs" in with "Investigate each recent > Cr-Internals-Network issue", but make sure that the information in both bullet > points above is included in that (some redundancy with "old bugs" as you note). > I'll do that in a separate PS so that it can be examined and reverted easily; > let me know what you think. SGTM
The CQ bit was checked by rdsmith@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from laforge@google.com Link to the patchset: https://codereview.chromium.org/970953002/#ps100001 (title: "Reorganized dealing with Needs-Feedback label")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/970953002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9ae51ac13fe63dc697425d3bb1c17965d71783f1 Cr-Commit-Position: refs/heads/master@{#319903} |
