Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903133002:
[IndexedDB] Adding uma for transaction aborts (Closed)
Description[IndexedDB] Adding uma for transaction aborts
R=jsbell
BUG=520256
Review-Url: https://codereview.chromium.org/2903133002
Cr-Commit-Position: refs/heads/master@{#475647}
Committed: https://chromium.googlesource.com/chromium/src/+/4df5f9a8a6e3322faf4c1e5cada87d499ba13e9c
Patch Set 1 #
Total comments: 10
Patch Set 2 : comments #
Total comments: 2
Patch Set 3 : comment #
Messages
Total messages: 33 (17 generated)
Josh, can you PTAL?
I don't think doing this in the front end will be sufficient. The timeout aborts occur when the renderer is not processing the transaction, which we suspected meant it's hung and unlikely to recover in a reasonable time frame. So we'd never get the stats. I think we (also?) want logic on the back end to tick a histogram when we abort the tx due to the timeout.
https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/en... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/en... tools/metrics/histograms/enums.xml:18645: +<enum name="IDBWebException" type="int"> I'd remove 'Web' here - doesn't add anything https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:80685: + Recorded when a transaction is aborted, specifically recording the reason Note that it's recorded on the front end.
I think you missed something - this is happening in the backend :) On Thu, May 25, 2017, 9:52 AM <jsbell@chromium.org> wrote: > > > https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/en... > File tools/metrics/histograms/enums.xml (right): > > > https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/en... > tools/metrics/histograms/enums.xml:18645: +<enum name="IDBWebException" > type="int"> > I'd remove 'Web' here - doesn't add anything > > > https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/hi... > File tools/metrics/histograms/histograms.xml (right): > > > https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:80685: + Recorded when a > > transaction is aborted, specifically recording the reason > Note that it's recorded on the front end. > > https://codereview.chromium.org/2903133002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/25 16:58:48, dmurph wrote: > I think you missed something - this is happening in the backend :) Damnit... 'blink::' threw me and I literally hadn't started drinking the cup of coffee next to me yet. *hangs head in shame* I'll take another look.
okay, lgtm with some nits Again, sorry about that. Thanks for your patience (and humor) https://codereview.chromium.org/2903133002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2903133002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_transaction.cc:43: UmaIDBExceptionConstraintError, Give all of these explicit values since they are used in UMA. https://codereview.chromium.org/2903133002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_transaction.cc:53: UmaIDBException TranslateExceptionCodeToUmaEnum(uint16_t code) { Could drop 'Translate' from the name here, but it's fine. https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/en... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/en... tools/metrics/histograms/enums.xml:18645: +<enum name="IDBWebException" type="int"> On 2017/05/25 16:52:24, jsbell wrote: > I'd remove 'Web' here - doesn't add anything This comment stands. https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:80685: + Recorded when a transaction is aborted, specifically recording the reason On 2017/05/25 16:52:24, jsbell wrote: > Note that it's recorded on the front end. make that 'back end'
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! Np - you look busy today :) https://codereview.chromium.org/2903133002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_transaction.cc (right): https://codereview.chromium.org/2903133002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_transaction.cc:43: UmaIDBExceptionConstraintError, On 2017/05/25 17:08:58, jsbell wrote: > Give all of these explicit values since they are used in UMA. Done. https://codereview.chromium.org/2903133002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_transaction.cc:53: UmaIDBException TranslateExceptionCodeToUmaEnum(uint16_t code) { On 2017/05/25 17:08:58, jsbell wrote: > Could drop 'Translate' from the name here, but it's fine. Done. https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/en... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/en... tools/metrics/histograms/enums.xml:18645: +<enum name="IDBWebException" type="int"> On 2017/05/25 17:08:58, jsbell wrote: > On 2017/05/25 16:52:24, jsbell wrote: > > I'd remove 'Web' here - doesn't add anything > > This comment stands. Done. https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2903133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:80685: + Recorded when a transaction is aborted, specifically recording the reason On 2017/05/25 17:08:58, jsbell wrote: > On 2017/05/25 16:52:24, jsbell wrote: > > Note that it's recorded on the front end. > > make that 'back end' Done.
dmurph@chromium.org changed reviewers: + mpearson@chromium.org
+Mark for histograms - can you please take a look? Thanks :)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm modulo nit --mark https://codereview.chromium.org/2903133002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2903133002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:80684: + Recorded on the browser side (back end) when a transaction is aborted, nit: add IndexedDB before transaction
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2903133002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2903133002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:80684: + Recorded on the browser side (back end) when a transaction is aborted, On 2017/05/25 19:49:48, Mark P wrote: > nit: add IndexedDB before transaction Done.
The CQ bit was unchecked by dmurph@chromium.org
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2903133002/#ps40001 (title: "comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496167771074370,
"parent_rev": "0af6d95f72025aae5c137b54acfcb9b700a66bda", "commit_rev":
"4df5f9a8a6e3322faf4c1e5cada87d499ba13e9c"}
Message was sent while issue was closed.
Description was changed from ========== [IndexedDB] Adding uma for transaction aborts R=jsbell BUG=520256 ========== to ========== [IndexedDB] Adding uma for transaction aborts R=jsbell BUG=520256 Review-Url: https://codereview.chromium.org/2903133002 Cr-Commit-Position: refs/heads/master@{#475647} Committed: https://chromium.googlesource.com/chromium/src/+/4df5f9a8a6e3322faf4c1e5cada8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/4df5f9a8a6e3322faf4c1e5cada8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
