Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1511993002:
Add initial version of performance regression sheriffing documentation. (Closed)
DescriptionAdd initial version of performance regression sheriffing documentation.
To make it easier to preview, I've uploaded the initial version to
https://gist.github.com/anniesullie/f15fcf998e2276c6d5aa
This documenation would eventually live at:
https://chromium.googlesource.com/chromium/src/+/master/tools/perf/docs
And would be made easier to find via:
* go links
* bit.ly links
* link or iframe from current perf sheriff documentation.
Committed: https://crrev.com/79dba48ba7d332baf4d1133d768b3ff4496220dc
Cr-Commit-Position: refs/heads/master@{#364726}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Addressed review comments #Patch Set 3 : Addressed review comments #Messages
Total messages: 15 (4 generated)
sullivan@chromium.org changed reviewers: + rschoen@chromium.org
Hello perf sheriffs, I'm working on new documentation for the split rotation. These are the docs for performance regression triaging. Comments and requests for clarification are welcome from everyone. Thanks, Annie
Just wanted to make sure that people saw there are two docs to review. The perfbot sheriffing one got several comments, but not this one, which is about regression sheriffing.
https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... File tools/perf/docs/perf_regression_sheriffing.md (right): https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:18: In the upper right corner, sign in with your Google account. The page shows I'd make a bigger deal out of signing in, or at least explain why we have to do it. I've seen a number of bugs get closed because people didn't realize they weren't signed in, and saw no alerts. https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:32: * If the alert is **shifted right or left** a little, click on it and use the I don't think "shifted" is clear here. Maybe you want to say "if the alert is visibly to the left or the right of the actual regression" https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:50: * Otherwise click "new bug". Be sure to see the [test owner](go/perf-owners) Do you need to include "http://" on the go link? Also, did you mean cc instead of see? https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:60: ###<a name="followup"></a> Follow up on Performance Regressions I think it would be good to include a first paragraph in this section about following-up on performance regressions *during your shift*. That is, once you've triaged all the alerts, go back through the bugs you've filed. Have the bisects come back/failed? Take the next step toward figuring out what happened. https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:85: <!-- Unresolved issues: This comment didn't work - I see it in the preview. But I'm not sure why?
Review comments addressed and preview updated. PTAL https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... File tools/perf/docs/perf_regression_sheriffing.md (right): https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:18: In the upper right corner, sign in with your Google account. The page shows On 2015/12/09 18:58:56, RyanS wrote: > I'd make a bigger deal out of signing in, or at least explain why we have to do > it. I've seen a number of bugs get closed because people didn't realize they > weren't signed in, and saw no alerts. Done. https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:32: * If the alert is **shifted right or left** a little, click on it and use the On 2015/12/09 18:58:56, RyanS wrote: > I don't think "shifted" is clear here. Maybe you want to say "if the alert is > visibly to the left or the right of the actual regression" Thanks, I couldn't think of good wording here :) https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:50: * Otherwise click "new bug". Be sure to see the [test owner](go/perf-owners) On 2015/12/09 18:58:55, RyanS wrote: > Do you need to include "http://" on the go link? > > Also, did you mean cc instead of see? Done. https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:60: ###<a name="followup"></a> Follow up on Performance Regressions On 2015/12/09 18:58:56, RyanS wrote: > I think it would be good to include a first paragraph in this section about > following-up on performance regressions *during your shift*. That is, once > you've triaged all the alerts, go back through the bugs you've filed. Have the > bisects come back/failed? Take the next step toward figuring out what happened. Done. https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:60: ###<a name="followup"></a> Follow up on Performance Regressions On 2015/12/09 18:58:56, RyanS wrote: > I think it would be good to include a first paragraph in this section about > following-up on performance regressions *during your shift*. That is, once > you've triaged all the alerts, go back through the bugs you've filed. Have the > bisects come back/failed? Take the next step toward figuring out what happened. Done. https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:85: <!-- Unresolved issues: On 2015/12/09 18:58:56, RyanS wrote: > This comment didn't work - I see it in the preview. But I'm not sure why? Done.
picksi@google.com changed reviewers: + picksi@google.com
Nice doc! A couple of thoughts from me. https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... File tools/perf/docs/perf_regression_sheriffing.md (right): https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:55: 5. **Optionally, bisect**. The perf dashboard will automatically kick off a It's not clear to me whether we should lean in favor of kicking off a bisect for edge cases (i.e. if we suspect it may be noise), or if the extra load on the bisect bots is bad. It might be worth adding a note about how to weigh up the cost/benefit of kicking off bisects (or just say it is always worth kicking off a bisect, [if it is!]). https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:65: certain that a specific CL caused a performance regression, it is okay to revert Do we need to be more assertive than 'it is okay to revert'? Should we actively say that the CL 'must'(?) 'should' be reverted 'if the owner is not available to fix it'? I personally have a lot of psychological resistance to reverting CLs and any I would seize any get-out clause to avoid doing it!
https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... File tools/perf/docs/perf_regression_sheriffing.md (right): https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:55: 5. **Optionally, bisect**. The perf dashboard will automatically kick off a On 2015/12/10 10:50:24, picksi wrote: > It's not clear to me whether we should lean in favor of kicking off a bisect for > edge cases (i.e. if we suspect it may be noise), or if the extra load on the > bisect bots is bad. It might be worth adding a note about how to weigh up the > cost/benefit of kicking off bisects (or just say it is always worth kicking off > a bisect, [if it is!]). Done. https://codereview.chromium.org/1511993002/diff/1/tools/perf/docs/perf_regres... tools/perf/docs/perf_regression_sheriffing.md:65: certain that a specific CL caused a performance regression, it is okay to revert On 2015/12/10 10:50:24, picksi wrote: > Do we need to be more assertive than 'it is okay to revert'? Should we actively > say that the CL 'must'(?) 'should' be reverted 'if the owner is not available to > fix it'? I personally have a lot of psychological resistance to reverting CLs > and any I would seize any get-out clause to avoid doing it! Done.
Great! Thanks :)
lgtm, thanks Annie!
The CQ bit was checked by sullivan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1511993002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1511993002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add initial version of performance regression sheriffing documentation. To make it easier to preview, I've uploaded the initial version to https://gist.github.com/anniesullie/f15fcf998e2276c6d5aa This documenation would eventually live at: https://chromium.googlesource.com/chromium/src/+/master/tools/perf/docs And would be made easier to find via: * go links * bit.ly links * link or iframe from current perf sheriff documentation. ========== to ========== Add initial version of performance regression sheriffing documentation. To make it easier to preview, I've uploaded the initial version to https://gist.github.com/anniesullie/f15fcf998e2276c6d5aa This documenation would eventually live at: https://chromium.googlesource.com/chromium/src/+/master/tools/perf/docs And would be made easier to find via: * go links * bit.ly links * link or iframe from current perf sheriff documentation. Committed: https://crrev.com/79dba48ba7d332baf4d1133d768b3ff4496220dc Cr-Commit-Position: refs/heads/master@{#364726} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/79dba48ba7d332baf4d1133d768b3ff4496220dc Cr-Commit-Position: refs/heads/master@{#364726} |
