Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2324493005:
[Offline Pages] Adds event logs for requests dropped due to number of start or complete attempts. (Closed)
|
Created:
4 years, 3 months ago by dougarnett Modified:
4 years, 3 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Adds event logs for requests dropped due to number of start or complete attempts.
BUG=645155
Committed: https://crrev.com/32febbf7cb75de2bcbd277cdbc994749ce9e6d48
Cr-Commit-Position: refs/heads/master@{#418064}
Patch Set 1 #Patch Set 2 : Fix up unittests #Patch Set 3 : Merge #
Total comments: 11
Patch Set 4 : Return enum value as string if no specific switch case for it #Patch Set 5 : Addresses petewil feedback #Patch Set 6 : Fixed a new comment and also made the new comments a bit less fragile to specific logger method nam… #
Total comments: 6
Patch Set 7 : Addresses dimich feedback #Messages
Total messages: 47 (35 generated)
The CQ bit was checked by dougarnett@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: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by dougarnett@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: Exceeded global retry quota
The CQ bit was checked by dougarnett@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: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dougarnett@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...
dougarnett@chromium.org changed reviewers: + petewil@chromium.org
Also shortened the event log messages a bit.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:199: event_logger_.RecordDroppedSavePageRequest( This is logging to the offline-internals page, not UMA, if I understand correctly. Did we want UMA instead? https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator_event_logger.cc (right): https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator_event_logger.cc:36: static std::string BackgroundSavePageResultToString( This is a bit fragile - what happens if we add a new enum value? At the least, we should have a comment where BackgroundSavePageResult is defined that this needs to be updated if we add a new value. At best, there may be another way to solve this that can't get out of sync. Whatever solution we apply should also be applied to OfflinerRequestStatusToString while we are at it. https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... components/offline_pages/background/request_picker.cc:264: // TODO(dougarnett): Plumb in event logger and RecordDroppedSavePageRequest This seems to my untrained eyes like a small amount of code, any reason not to do it now? (it being larger than it seems to me might be a good reason, for instance).
https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:199: event_logger_.RecordDroppedSavePageRequest( On 2016/09/09 17:16:19, Pete Williamson wrote: > This is logging to the offline-internals page, not UMA, if I understand > correctly. Did we want UMA instead? Per our conversation, this is OK as logging, not UMA, but let's open a bug to add UMA for this.
The CQ bit was checked by dougarnett@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...
dougarnett@chromium.org changed reviewers: + dimich@chromium.org
Adding Dmitry as second reviewer wrt event logging philosophy https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:199: event_logger_.RecordDroppedSavePageRequest( On 2016/09/09 22:08:31, Pete Williamson wrote: > On 2016/09/09 17:16:19, Pete Williamson wrote: > > This is logging to the offline-internals page, not UMA, if I understand > > correctly. Did we want UMA instead? > > Per our conversation, this is OK as logging, not UMA, but let's open a bug to > add UMA for this. Opened 645679 for UMA. The point of this event logging is for advanced user to see why hir requests are dropped per policy settings. This can make it easier to do manual evals with standard build. https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator_event_logger.cc (right): https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator_event_logger.cc:36: static std::string BackgroundSavePageResultToString( On 2016/09/09 17:16:19, Pete Williamson wrote: > This is a bit fragile - what happens if we add a new enum value? At the least, > we should have a comment where BackgroundSavePageResult is defined that this > needs to be updated if we add a new value. At best, there may be another way to > solve this that can't get out of sync. > > Whatever solution we apply should also be applied to > OfflinerRequestStatusToString while we are at it. How about just returning the int value of enum with no specific case? Drop the DCHECKs? https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... components/offline_pages/background/request_picker.cc:264: // TODO(dougarnett): Plumb in event logger and RecordDroppedSavePageRequest On 2016/09/09 17:16:19, Pete Williamson wrote: > This seems to my untrained eyes like a small amount of code, any reason not to > do it now? (it being larger than it seems to me might be a good reason, for > instance). Really just have a design question here about should we just pass the event logger into the constructor. Should I go ahead and do that?
https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator_event_logger.cc (right): https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator_event_logger.cc:36: static std::string BackgroundSavePageResultToString( On 2016/09/09 23:58:21, dougarnett wrote: > On 2016/09/09 17:16:19, Pete Williamson wrote: > > This is a bit fragile - what happens if we add a new enum value? At the > least, > > we should have a comment where BackgroundSavePageResult is defined that this > > needs to be updated if we add a new value. At best, there may be another way > to > > solve this that can't get out of sync. > > > > Whatever solution we apply should also be applied to > > OfflinerRequestStatusToString while we are at it. > > How about just returning the int value of enum with no specific case? > Drop the DCHECKs? that's OK, but it won't make the logging very legible. I think I'd prefer adding a comment in RequestNotifier::BackgroundSavePageResult. https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... components/offline_pages/background/request_picker.cc:264: // TODO(dougarnett): Plumb in event logger and RecordDroppedSavePageRequest On 2016/09/09 23:58:21, dougarnett wrote: > On 2016/09/09 17:16:19, Pete Williamson wrote: > > This seems to my untrained eyes like a small amount of code, any reason not to > > do it now? (it being larger than it seems to me might be a good reason, for > > instance). > > Really just have a design question here about should we just pass the event > logger into the constructor. Should I go ahead and do that? Yes, please, if it isn't a lot of work. If it is, we can make a bug and do it later.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dougarnett@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 checked by dougarnett@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...
https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator_event_logger.cc (right): https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator_event_logger.cc:36: static std::string BackgroundSavePageResultToString( On 2016/09/10 00:14:13, Pete Williamson wrote: > On 2016/09/09 23:58:21, dougarnett wrote: > > On 2016/09/09 17:16:19, Pete Williamson wrote: > > > This is a bit fragile - what happens if we add a new enum value? At the > > least, > > > we should have a comment where BackgroundSavePageResult is defined that this > > > needs to be updated if we add a new value. At best, there may be another > way > > to > > > solve this that can't get out of sync. > > > > > > Whatever solution we apply should also be applied to > > > OfflinerRequestStatusToString while we are at it. > > > > How about just returning the int value of enum with no specific case? > > Drop the DCHECKs? > > that's OK, but it won't make the logging very legible. I think I'd prefer > adding a comment in RequestNotifier::BackgroundSavePageResult. Done https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2324493005/diff/40001/components/offline_page... components/offline_pages/background/request_picker.cc:264: // TODO(dougarnett): Plumb in event logger and RecordDroppedSavePageRequest On 2016/09/10 00:14:13, Pete Williamson wrote: > On 2016/09/09 23:58:21, dougarnett wrote: > > On 2016/09/09 17:16:19, Pete Williamson wrote: > > > This seems to my untrained eyes like a small amount of code, any reason not > to > > > do it now? (it being larger than it seems to me might be a good reason, for > > > instance). > > > > Really just have a design question here about should we just pass the event > > logger into the constructor. Should I go ahead and do that? > > Yes, please, if it isn't a lot of work. If it is, we can make a bug and do it > later. Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Totally good to have local logs for debugging. LGTM with couple of small nits: https://codereview.chromium.org/2324493005/diff/100001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2324493005/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:202: BackgroundSavePageResult::START_COUNT_EXCEEDED, request->request_id()); I'd pull the constant into a local const variable and then passed it in both places to avoid duplication and guarantee sameness. Same below. https://codereview.chromium.org/2324493005/diff/100001/components/offline_pag... File components/offline_pages/background/request_coordinator_event_logger.cc (right): https://codereview.chromium.org/2324493005/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator_event_logger.cc:58: DCHECK(false); NOTREACHED() is better to document an intent here. https://codereview.chromium.org/2324493005/diff/100001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2324493005/diff/100001/components/offline_pag... components/offline_pages/background/request_picker.cc:266: event_logger_->RecordDroppedSavePageRequest( I'd record the log before calling NotifyCompleted, since NotifyCompleted just synchronously calls into external observers and they can do anything. There is a good reason to do that right before exiting the operation, since even 'this' pointer is not guaranteed, strictly speaking.
The CQ bit was checked by dougarnett@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...
https://codereview.chromium.org/2324493005/diff/100001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2324493005/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:202: BackgroundSavePageResult::START_COUNT_EXCEEDED, request->request_id()); On 2016/09/12 19:43:13, Dmitry Titov wrote: > I'd pull the constant into a local const variable and then passed it in both > places to avoid duplication and guarantee sameness. Same below. Done. https://codereview.chromium.org/2324493005/diff/100001/components/offline_pag... File components/offline_pages/background/request_coordinator_event_logger.cc (right): https://codereview.chromium.org/2324493005/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator_event_logger.cc:58: DCHECK(false); On 2016/09/12 19:43:13, Dmitry Titov wrote: > NOTREACHED() is better to document an intent here. Done. https://codereview.chromium.org/2324493005/diff/100001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2324493005/diff/100001/components/offline_pag... components/offline_pages/background/request_picker.cc:266: event_logger_->RecordDroppedSavePageRequest( On 2016/09/12 19:43:13, Dmitry Titov wrote: > I'd record the log before calling NotifyCompleted, since NotifyCompleted just > synchronously calls into external observers and they can do anything. There is a > good reason to do that right before exiting the operation, since even 'this' > pointer is not guaranteed, strictly speaking. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, dimich@chromium.org Link to the patchset: https://codereview.chromium.org/2324493005/#ps120001 (title: "Addresses dimich feedback")
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.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Adds event logs for requests dropped due to number of start or complete attempts. BUG=645155 ========== to ========== [Offline Pages] Adds event logs for requests dropped due to number of start or complete attempts. BUG=645155 Committed: https://crrev.com/32febbf7cb75de2bcbd277cdbc994749ce9e6d48 Cr-Commit-Position: refs/heads/master@{#418064} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/32febbf7cb75de2bcbd277cdbc994749ce9e6d48 Cr-Commit-Position: refs/heads/master@{#418064} |
