Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2104393002:
Adds UMA for PrerenderingOffliner request processing result status. (Closed)
|
Created:
4 years, 5 months ago by dougarnett Modified:
4 years, 5 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, asvitkine+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds UMA for PrerenderingOffliner request processing result status.
BUG=624452
Committed: https://crrev.com/33755c4215fbcb64f09bab1a90dc72914488dc16
Cr-Commit-Position: refs/heads/master@{#405208}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Revised histogram naming from OfflinerStatus to OfflinerRequestStatus #
Total comments: 5
Patch Set 3 : Updates per petewil feedback #Patch Set 4 : Added comment to RecordStatusUMA wrt to call only once per request. #Patch Set 5 : Renamed status codes more specifically and moved recording to the Coordinator #Patch Set 6 : Fix rename in unittest (from merge). #
Total comments: 2
Patch Set 7 : Clarifies and fixes behavior wrt Offliner::Cancel() not calling the callback #
Total comments: 2
Patch Set 8 : Adds note about interim offliner status codes and performs DCHECK that offliner does not return the… #Messages
Total messages: 36 (12 generated)
Description was changed from ========== Adds UMA for PrerenderingOffliner request processing result status. BUG=624452 ========== to ========== Adds UMA for PrerenderingOffliner request processing result status. BUG=624452 ==========
dougarnett@chromium.org changed reviewers: + fgorski@chromium.org, petewil@chromium.org
https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:22: UNKNOWN = 0, // No status determined/reported yet. Value assignments are not necessary here. https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:24: SAVED = 2, // Offline page snapshot saved. Is it possible that a single request will end up recording multiple states? e.g.: loaded -> saved loaded -> request_canceled ? loaded -> save_failed If so perhaps it makes sense to have a strategy for such cases and a good explanation https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:26: LOAD_CANCELED = 4, // Load was canceled vs. failed (from below). Is that a cancel for reasons different than user canceling request? https://codereview.chromium.org/2104393002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2104393002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:35562: + enum="OfflinePagesBackgroundOfflinerStatus"> suggestion: both enum and histogram name might be better with "Request" mentioned someplace.
https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:22: UNKNOWN = 0, // No status determined/reported yet. On 2016/06/29 18:47:03, fgorski wrote: > Value assignments are not necessary here. But the histogram enum values need to match, right? So seemed a bit better to be explicit so they are easier to check by inspection. https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:24: SAVED = 2, // Offline page snapshot saved. On 2016/06/29 18:47:02, fgorski wrote: > Is it possible that a single request will end up recording multiple states? > > e.g.: > loaded -> saved > loaded -> request_canceled ? > loaded -> save_failed > > > If so perhaps it makes sense to have a strategy for such cases and a good > explanation No, just one final status recorded per request is what I aiming for. In the histogram enum defn I am marking UNKNOWN and LOADED as not expected to be recorded. https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:26: LOAD_CANCELED = 4, // Load was canceled vs. failed (from below). On 2016/06/29 18:47:02, fgorski wrote: > Is that a cancel for reasons different than user canceling request? This "from below" is a cancel from the prerenderer. There are a few cases where the PrerenderManager will cancel a request (due to other requests or navigations or low memory) that are not necessarily reflection of whether the page's rendering would have succeeded. The previous "from above" one, REQUEST_CANCELED, is a new status that is for when the Coordinator cancels the request (the current use case for this is when time limit expires). I was debating whether to record there results here or in the Coordinator - if I did it in the Coordinator, I could characterize more accurately as a TIMEOUT. But I ended up thinking maybe good, clean to have these in the Offliner impl. https://codereview.chromium.org/2104393002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2104393002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:35562: + enum="OfflinePagesBackgroundOfflinerStatus"> On 2016/06/29 18:47:03, fgorski wrote: > suggestion: both enum and histogram name might be better with "Request" > mentioned someplace. Done.
drive-by comments https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:22: UNKNOWN = 0, // No status determined/reported yet. On 2016/06/29 19:42:52, dougarnett wrote: > On 2016/06/29 18:47:03, fgorski wrote: > > Value assignments are not necessary here. > > But the histogram enum values need to match, right? So seemed a bit better to be > explicit so they are easier to check by inspection. IMO, assigning value seems to often imply that it is ok to assign any random value for enum constant. https://codereview.chromium.org/2104393002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2104393002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:19: void RecordStatusUma(Offliner::RequestStatus request_status) { nit: RecordStatusUMA
https://codereview.chromium.org/2104393002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2104393002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:23: static_cast<int>(Offliner::RequestStatus::STATUS_COUNT)); Will this succeed without static_cast? https://codereview.chromium.org/2104393002/diff/20001/components/offline_page... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2104393002/diff/20001/components/offline_page... components/offline_pages/background/offliner.h:20: // OfflinePagesBackgroundOfflinerRequestStatus histogram enum. Can we make the wording of the comment a bit stronger? // WARNING: You must change histograms.xml to add the enum value there too if you // add a new enum value here.
https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:22: UNKNOWN = 0, // No status determined/reported yet. On 2016/06/29 20:47:34, jianli wrote: > On 2016/06/29 19:42:52, dougarnett wrote: > > On 2016/06/29 18:47:03, fgorski wrote: > > > Value assignments are not necessary here. > > > > But the histogram enum values need to match, right? So seemed a bit better to > be > > explicit so they are easier to check by inspection. > > IMO, assigning value seems to often imply that it is ok to assign any random > value for enum constant. I think that would be fairly easy to catch in code review though. I think it is harder to catch the case when no-longer-used code is deleted from this enum and thus changes the value of codes that follow it. [Please let me know if I'm going against some chrome convention that was settled such debate previously.] https://codereview.chromium.org/2104393002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2104393002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:23: static_cast<int>(Offliner::RequestStatus::STATUS_COUNT)); On 2016/06/30 17:20:47, Pete Williamson wrote: > Will this succeed without static_cast? Done. https://codereview.chromium.org/2104393002/diff/20001/components/offline_page... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2104393002/diff/20001/components/offline_page... components/offline_pages/background/offliner.h:20: // OfflinePagesBackgroundOfflinerRequestStatus histogram enum. On 2016/06/30 17:20:47, Pete Williamson wrote: > Can we make the wording of the comment a bit stronger? > // WARNING: You must change histograms.xml to add the enum value there too if > you > // add a new enum value here. Done.
lgtm
dougarnett@chromium.org changed reviewers: + holte@chromium.org
Steven, could you review changes to histogram.xml (or refer me to someone else to do that)?
https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:22: UNKNOWN = 0, // No status determined/reported yet. On 2016/06/30 18:09:15, dougarnett wrote: > On 2016/06/29 20:47:34, jianli wrote: > > On 2016/06/29 19:42:52, dougarnett wrote: > > > On 2016/06/29 18:47:03, fgorski wrote: > > > > Value assignments are not necessary here. > > > > > > But the histogram enum values need to match, right? So seemed a bit better > to > > be > > > explicit so they are easier to check by inspection. > > > > IMO, assigning value seems to often imply that it is ok to assign any random > > value for enum constant. > > I think that would be fairly easy to catch in code review though. I think it is > harder to catch the case when no-longer-used code is deleted from this enum and > thus changes the value of codes that follow it. > > [Please let me know if I'm going against some chrome convention that was settled > such debate previously.] OK. I think dimich@ got that suggestion from a UMA reviewer recently so this can stay. Can you please bring that up with UMA reviewer for confirmation? https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:24: SAVED = 2, // Offline page snapshot saved. On 2016/06/29 19:42:52, dougarnett wrote: > On 2016/06/29 18:47:02, fgorski wrote: > > Is it possible that a single request will end up recording multiple states? > > > > e.g.: > > loaded -> saved > > loaded -> request_canceled ? > > loaded -> save_failed > > > > > > If so perhaps it makes sense to have a strategy for such cases and a good > > explanation > > No, just one final status recorded per request is what I aiming for. In the > histogram enum defn I am marking UNKNOWN and LOADED as not expected to be > recorded. Please document that in the code. I will have the same problem reading this code a month from now. https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:26: LOAD_CANCELED = 4, // Load was canceled vs. failed (from below). On 2016/06/29 19:42:52, dougarnett wrote: > On 2016/06/29 18:47:02, fgorski wrote: > > Is that a cancel for reasons different than user canceling request? > > This "from below" is a cancel from the prerenderer. There are a few cases where > the PrerenderManager will cancel a request (due to other requests or navigations > or low memory) that are not necessarily reflection of whether the page's > rendering would have succeeded. > > The previous "from above" one, REQUEST_CANCELED, is a new status that is for > when the Coordinator cancels the request (the current use case for this is when > time limit expires). I was debating whether to record there results here or in > the Coordinator - if I did it in the Coordinator, I could characterize more > accurately as a TIMEOUT. But I ended up thinking maybe good, clean to have these > in the Offliner impl. Please document that in the code. I will have the same problem reading this code a month from now.
dimich@chromium.org changed reviewers: + dimich@chromium.org
https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:26: LOAD_CANCELED = 4, // Load was canceled vs. failed (from below). Maybe change name to say CANCELED_BY_PRERENDERER and CANCELED_BY_REQUEST_COORDINATOR to try to make it clearer?
https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:24: SAVED = 2, // Offline page snapshot saved. On 2016/06/30 18:50:54, fgorski wrote: > On 2016/06/29 19:42:52, dougarnett wrote: > > On 2016/06/29 18:47:02, fgorski wrote: > > > Is it possible that a single request will end up recording multiple states? > > > > > > e.g.: > > > loaded -> saved > > > loaded -> request_canceled ? > > > loaded -> save_failed > > > > > > > > > If so perhaps it makes sense to have a strategy for such cases and a good > > > explanation > > > > No, just one final status recorded per request is what I aiming for. In the > > histogram enum defn I am marking UNKNOWN and LOADED as not expected to be > > recorded. > > Please document that in the code. I will have the same problem reading this code > a month from now. Added comment to RecordStatusUMA method as it seemed the most applicable place to note this. https://codereview.chromium.org/2104393002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:26: LOAD_CANCELED = 4, // Load was canceled vs. failed (from below). On 2016/06/30 18:54:04, Dmitry Titov wrote: > Maybe change name to say CANCELED_BY_PRERENDERER and > CANCELED_BY_REQUEST_COORDINATOR to try to make it clearer? Yeah, I struggle with that a bit here as it is at odds with this being a pure interface. Should we pile in specific codes here from its implementations? Or should I define separate PrerenderingOffliner enum instead for the purpose of the UMA?
histograms lgtm
I'm going to put this on the shelf and reconsider after vacation. Starting to feel that a separate, more detailed enum that is dedicated for UMA would be better than using this API one.
On 2016/06/30 22:01:37, dougarnett wrote: > I'm going to put this on the shelf and reconsider after vacation. Starting to > feel that a separate, more detailed enum that is dedicated for UMA would be > better than using this API one. I do think this is the right direction. Having a specifically designed enums tied to UMA reporting exclusively is a good way to split responsibilities in code.
I tried playing with a separate enum for UMA in another branch and was not very happy with it - too much duplication and needing mapping functions. So I have come back to this one and decided to put more specific codes in the Offliner enum instead. Also, I revised the reporting to be in the Coordinator instead - which seems cleaner provided it is the api enum values we record. Please take another look. The only change to histograms.xml was updated label wording.
Still LGTM. One comment https://codereview.chromium.org/2104393002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2104393002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:84482: +<enum name="OfflinePagesBackgroundOfflinerRequestStatus" type="int"> Should we also have a comment here pointing back to offliner.h? (It's OK if the answer is "no", I just want to make sure the question has been though about).
https://codereview.chromium.org/2104393002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2104393002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:84482: +<enum name="OfflinePagesBackgroundOfflinerRequestStatus" type="int"> On 2016/07/11 20:12:08, Pete Williamson wrote: > Should we also have a comment here pointing back to offliner.h? (It's OK if the > answer is "no", I just want to make sure the question has been though about). Done - added the "Generated from ..." style comment that seems to be the most common in this file.
lgtm with comment. I see you added a note on UMA label, but perhaps a DCHECK would be helpful. https://codereview.chromium.org/2104393002/diff/120001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2104393002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:25: // only be called once per Offliner::LoadAndSave request. where is the comment about not reporting loaded or unknown status that we discussed in comments on patch 1 (next to enum definition)? Are they going to be reported in the end? Or was it missed? Perhaps it makes sense to DCHECK as well
https://codereview.chromium.org/2104393002/diff/120001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2104393002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:25: // only be called once per Offliner::LoadAndSave request. On 2016/07/12 22:17:06, fgorski wrote: > where is the comment about not reporting loaded or unknown status that we > discussed in comments on patch 1 (next to enum definition)? > > Are they going to be reported in the end? Or was it missed? > > Perhaps it makes sense to DCHECK as well Ok, added comment about the two interim status codes in enum defn and added DCHECKs in offliner->coordinator callback (as that is more central point for coordinator not expecting them and also then not reporting them.
lgtm
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: 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 holte@chromium.org, petewil@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2104393002/#ps140001 (title: "Adds note about interim offliner status codes and performs DCHECK that offliner does not return the…")
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 ========== Adds UMA for PrerenderingOffliner request processing result status. BUG=624452 ========== to ========== Adds UMA for PrerenderingOffliner request processing result status. BUG=624452 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Adds UMA for PrerenderingOffliner request processing result status. BUG=624452 ========== to ========== Adds UMA for PrerenderingOffliner request processing result status. BUG=624452 Committed: https://crrev.com/33755c4215fbcb64f09bab1a90dc72914488dc16 Cr-Commit-Position: refs/heads/master@{#405208} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/33755c4215fbcb64f09bab1a90dc72914488dc16 Cr-Commit-Position: refs/heads/master@{#405208} |
