Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2444023002:
Histogram for VpdUpdateSuccess (Closed)
DescriptionHistogram for VpdUpdateSuccess
Chrome OS implementation in https://chromium-review.googlesource.com/#/c/406830/
BUG=653814, 659029
Committed: https://crrev.com/a33fef8835031daa276de93b71f759f7dba0c441
Cr-Commit-Position: refs/heads/master@{#430254}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Changed the histogram to be sparse #
Total comments: 4
Patch Set 3 : Fixed review comment #
Total comments: 2
Patch Set 4 : Fixed review comments and merged with other CL #
Total comments: 1
Patch Set 5 : Fixed review comments #Messages
Total messages: 27 (11 generated)
Description was changed from ========== Histogram for VpdUpdateSuccess ChromeOs implementation in https://chromium-review.googlesource.com/#/c/400881/ BUG=653814 ========== to ========== Histogram for VpdUpdateSuccess ChromeOs implementation in https://chromium-review.googlesource.com/#/c/400881/ BUG=653814 ==========
igorcov@chromium.org changed reviewers: + jochen@chromium.org, tnagel@chromium.org
Please review the addition of new histogram.
tnagel@chromium.org changed reviewers: + holte@chromium.org - jochen@chromium.org
-jochen +holte Jochen only does UseCounter reviews.
https://codereview.chromium.org/2444023002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2444023002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13172: +<histogram name="Enterprise.VpdUpdateSuccess" enum="BooleanSuccess"> Nit: The "Success" part of the histogram name is redundant since the histogram type already conveys that information. https://codereview.chromium.org/2444023002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13174: + <summary>ChromeOS Only - Whether VPD update succeeded.</summary> The correct spelling is Chrome OS. To be in line with other histograms, I'd suggest a wording like "Whether Chrome OS VPD update succeeded." or "Whether VPD update succeeded (Chrome OS)."
lgtm
I've changed the histogram to be spase. Will update also the code on Chrome OS side. Please take a look again. https://codereview.chromium.org/2444023002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2444023002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13172: +<histogram name="Enterprise.VpdUpdateSuccess" enum="BooleanSuccess"> On 2016/10/24 18:27:59, Thiemo Nagel (slow) wrote: > Nit: The "Success" part of the histogram name is redundant since the histogram > type already conveys that information. Done.
Description was changed from ========== Histogram for VpdUpdateSuccess ChromeOs implementation in https://chromium-review.googlesource.com/#/c/400881/ BUG=653814 ========== to ========== Histogram for VpdUpdateSuccess Chrome OS implementation in https://chromium-https://chromium-review.googlesource.com/#/c/406830/ BUG=653814 ==========
Description was changed from ========== Histogram for VpdUpdateSuccess Chrome OS implementation in https://chromium-https://chromium-review.googlesource.com/#/c/406830/ BUG=653814 ========== to ========== Histogram for VpdUpdateSuccess Chrome OS implementation in https://chromium-review.googlesource.com/#/c/406830/ BUG=653814 ==========
derat@chromium.org changed reviewers: + derat@chromium.org
https://codereview.chromium.org/2444023002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2444023002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13172: +<histogram name="Enterprise.VpdUpdateStatus"> if this is an enum, please add an enum="VpdUpdateStatus" attribute and an <enum name="VpdUpdateStatus" type="int"> stanza describing the different values
https://codereview.chromium.org/2444023002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2444023002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13172: +<histogram name="Enterprise.VpdUpdateStatus"> On 2016/11/02 14:16:49, Daniel Erat wrote: > if this is an enum, please add an enum="VpdUpdateStatus" attribute and an <enum > name="VpdUpdateStatus" type="int"> stanza describing the different values looked more closely at the other side and saw that this is siginfo_t.si_status. maybe update the summary to say, "The status code (exit code or signal) from ..."?
https://codereview.chromium.org/2444023002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2444023002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13172: +<histogram name="Enterprise.VpdUpdateStatus"> On 2016/11/02 14:19:17, Daniel Erat wrote: > On 2016/11/02 14:16:49, Daniel Erat wrote: > > if this is an enum, please add an enum="VpdUpdateStatus" attribute and an > <enum > > name="VpdUpdateStatus" type="int"> stanza describing the different values > > looked more closely at the other side and saw that this is siginfo_t.si_status. > maybe update the summary to say, "The status code (exit code or signal) from > ..."? Done. https://codereview.chromium.org/2444023002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13172: +<histogram name="Enterprise.VpdUpdateStatus"> On 2016/11/02 14:16:49, Daniel Erat wrote: > if this is an enum, please add an enum="VpdUpdateStatus" attribute and an <enum > name="VpdUpdateStatus" type="int"> stanza describing the different values Done.
lgtm thanks!
Steven, does it make sense to specify an enum here to attach labels to the well known and expected exit codes, 0, 3..11, 256, 257, 258? Or would that hide unexpected error codes from the dashboard?
https://codereview.chromium.org/2444023002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2444023002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13173: + <owner>igorcov@chromium.org</owner> Could you maybe add me as another owner?
Merged with https://codereview.chromium.org/2460933002/. Please take a look again. https://codereview.chromium.org/2444023002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2444023002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13173: + <owner>igorcov@chromium.org</owner> On 2016/11/02 17:22:37, Thiemo Nagel (slow) wrote: > Could you maybe add me as another owner? Done.
Description was changed from ========== Histogram for VpdUpdateSuccess Chrome OS implementation in https://chromium-review.googlesource.com/#/c/406830/ BUG=653814 ========== to ========== Histogram for VpdUpdateSuccess Chrome OS implementation in https://chromium-review.googlesource.com/#/c/406830/ BUG=653814, 659029 ==========
lgtm https://codereview.chromium.org/2444023002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2444023002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13176: + The status code from sanity check of RW_VPD read and matching the contents is this also an exit code or signal? if so, please add the same parenthetical expression that VpdUpdateStatus has.
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, derat@chromium.org Link to the patchset: https://codereview.chromium.org/2444023002/#ps80001 (title: "Fixed review comments")
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 ========== Histogram for VpdUpdateSuccess Chrome OS implementation in https://chromium-review.googlesource.com/#/c/406830/ BUG=653814, 659029 ========== to ========== Histogram for VpdUpdateSuccess Chrome OS implementation in https://chromium-review.googlesource.com/#/c/406830/ BUG=653814, 659029 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Histogram for VpdUpdateSuccess Chrome OS implementation in https://chromium-review.googlesource.com/#/c/406830/ BUG=653814, 659029 ========== to ========== Histogram for VpdUpdateSuccess Chrome OS implementation in https://chromium-review.googlesource.com/#/c/406830/ BUG=653814, 659029 Committed: https://crrev.com/a33fef8835031daa276de93b71f759f7dba0c441 Cr-Commit-Position: refs/heads/master@{#430254} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a33fef8835031daa276de93b71f759f7dba0c441 Cr-Commit-Position: refs/heads/master@{#430254} |
