Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1281103002:
Add UMA metrics to log attachment broker errors. (Closed)
DescriptionAdd UMA metrics to log attachment broker errors.
BUG=493414
Committed: https://crrev.com/16f9112db34bc9fb02eba1830ca5643786591e6b
Cr-Commit-Position: refs/heads/master@{#350312}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : Compile errors. #Patch Set 5 : Rebase. #Patch Set 6 : Remove unused enum. #
Total comments: 6
Patch Set 7 : Comments from isherman. #
Total comments: 4
Patch Set 8 : Rebase. #Patch Set 9 : Comments from isherman #
Messages
Total messages: 37 (15 generated)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1281103002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1281103002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1281103002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1281103002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1281103002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1281103002/80001
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1281103002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1281103002/100001
erikchen@chromium.org changed reviewers: + tsepez@chromium.org
tsepez: Please review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1281103002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1281103002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
erikchen@chromium.org changed reviewers: + isherman@chromium.org
isherman: PTAL
https://codereview.chromium.org/1281103002/diff/100001/ipc/attachment_broker_... File ipc/attachment_broker_privileged.h (right): https://codereview.chromium.org/1281103002/diff/100001/ipc/attachment_broker_... ipc/attachment_broker_privileged.h:40: // These match tools/metrics/histograms.xml nit: Please document that this enum should be treated as append-only https://codereview.chromium.org/1281103002/diff/100001/ipc/attachment_broker_... ipc/attachment_broker_privileged.h:47: MAX_VALUE nit: "MAX_VALUE" is a little unclear out of context. Perhaps "ERROR_MAX"? https://codereview.chromium.org/1281103002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1281103002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60793: +<enum name="IPCAttachmentBrokerPrivilegedBrokerAttachmentError" type="int"> I'd recommend adding either a "success" or an "attempt" bucket, so that you have a baseline to compare to and be able to compute an error rate.
isherman: PTAL https://codereview.chromium.org/1281103002/diff/100001/ipc/attachment_broker_... File ipc/attachment_broker_privileged.h (right): https://codereview.chromium.org/1281103002/diff/100001/ipc/attachment_broker_... ipc/attachment_broker_privileged.h:40: // These match tools/metrics/histograms.xml On 2015/08/11 21:22:16, Ilya Sherman wrote: > nit: Please document that this enum should be treated as append-only Done. https://codereview.chromium.org/1281103002/diff/100001/ipc/attachment_broker_... ipc/attachment_broker_privileged.h:47: MAX_VALUE On 2015/08/11 21:22:16, Ilya Sherman wrote: > nit: "MAX_VALUE" is a little unclear out of context. Perhaps "ERROR_MAX"? Done. https://codereview.chromium.org/1281103002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1281103002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60793: +<enum name="IPCAttachmentBrokerPrivilegedBrokerAttachmentError" type="int"> On 2015/08/11 21:22:16, Ilya Sherman wrote: > I'd recommend adding either a "success" or an "attempt" bucket, so that you have > a baseline to compare to and be able to compute an error rate. I added a success bucket.
https://codereview.chromium.org/1281103002/diff/120001/ipc/attachment_broker_... File ipc/attachment_broker_privileged.cc (right): https://codereview.chromium.org/1281103002/diff/120001/ipc/attachment_broker_... ipc/attachment_broker_privileged.cc:9: #include "base/metrics/histogram.h" nit: histogram_macros https://codereview.chromium.org/1281103002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1281103002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63826: + </int> Is this label the "success" case? If so, I'd recommend moving it to be first in the list. That way, if you ever add additional error cases, it won't get stuck in the middle. If this isn't the success case, then I'd recommend adding a success case, so that you understand the error rate, as well as the relative frequency of different types of errors.
isherman: PTAL https://codereview.chromium.org/1281103002/diff/120001/ipc/attachment_broker_... File ipc/attachment_broker_privileged.cc (right): https://codereview.chromium.org/1281103002/diff/120001/ipc/attachment_broker_... ipc/attachment_broker_privileged.cc:9: #include "base/metrics/histogram.h" On 2015/09/21 20:34:26, Ilya Sherman wrote: > nit: histogram_macros Done. https://codereview.chromium.org/1281103002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1281103002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63826: + </int> On 2015/09/21 20:34:26, Ilya Sherman wrote: > Is this label the "success" case? If so, I'd recommend moving it to be first in > the list. That way, if you ever add additional error cases, it won't get stuck > in the middle. > > If this isn't the success case, then I'd recommend adding a success case, so > that you understand the error rate, as well as the relative frequency of > different types of errors. It's the success case. I moved it to 0.
LGTM, thanks.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1281103002/#ps160001 (title: "Comments from isherman")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1281103002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1281103002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1281103002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1281103002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/16f9112db34bc9fb02eba1830ca5643786591e6b Cr-Commit-Position: refs/heads/master@{#350312} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
