Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2022283003:
Record offline histograms with suffix via Histogram::FactoryGet (Closed)
|
Created:
4 years, 6 months ago by jianli Modified:
4 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+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. |
DescriptionRecord offline histograms with suffix via Histogram::FactoryGet
Also move all offline client namespace constants into one central place.
BUG=616231
TBR=mmenke@chromium.org
Committed: https://crrev.com/8ccbcdc96179ab65038e17cdbddc149e28754830
Cr-Commit-Position: refs/heads/master@{#397233}
Committed: https://crrev.com/37ab199f7ba4c272d620813c458bc8f524c2c3ba
Cr-Commit-Position: refs/heads/master@{#397623}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Change to use Histogram::FactoryGet #
Total comments: 8
Patch Set 3 : Address more feedback #
Total comments: 6
Patch Set 4 : Rebase #Patch Set 5 : Address some more feedback #
Total comments: 6
Patch Set 6 : Trivial change to offline_page_model.cc to allow easy merge to M52 #Patch Set 7 : Rebase again #Messages
Total messages: 53 (27 generated)
Description was changed from ========== Pass static value to UMA_HISTOGRAM macros for offline pages BUG=616231 ========== to ========== Pass static value to UMA_HISTOGRAM macros for offline pages Also move all offline client namespace constants into one central place. BUG=616231 ==========
jianli@chromium.org changed reviewers: + fgorski@chromium.org
jianli@chromium.org changed reviewers: + dewittj@chromium.org - fgorski@chromium.org
Drive-by: The repetitive code with histogram macros look scary :) Since those macros are perf optimization (by caching a histogram in a static pointer), would it be possible to instead use a slow path here and create/find a histogram pointer every time using generated name? Basically, just calling base::Histogram::FactoryGet(...) every time. For relatively rare and long operations (like page creation) this can be just fine...
chili@chromium.org changed reviewers: + chili@chromium.org
https://codereview.chromium.org/2022283003/diff/1/components/offline_pages/cl... File components/offline_pages/client_policy_controller_unittest.cc (right): https://codereview.chromium.org/2022283003/diff/1/components/offline_pages/cl... components/offline_pages/client_policy_controller_unittest.cc:17: const char kUndefinedNamespace[] = "undefined"; should we also move undefined to the namespace constants too?
On 2016/05/31 21:46:18, Dmitry Titov wrote:
> Drive-by:
>
> The repetitive code with histogram macros look scary :)
>
> Since those macros are perf optimization (by caching a histogram in a static
> pointer), would it be possible to instead use a slow path here and create/find
a
> histogram pointer every time using generated name? Basically, just calling
> base::Histogram::FactoryGet(...) every time. For relatively rare and long
> operations (like page creation) this can be just fine...
This seems like a good idea, and easier to merge. For example,
//src/sql/connection.cc has this code:
std::string full_histogram_name = "Sqlite.SizeKB." + histogram_tag_;
base::HistogramBase* histogram =
base::Histogram::FactoryGet(
full_histogram_name, 1, 1000000, 50,
base::HistogramBase::kUmaTargetedHistogramFlag);
if (histogram)
histogram->Add(sample);
Description was changed from ========== Pass static value to UMA_HISTOGRAM macros for offline pages Also move all offline client namespace constants into one central place. BUG=616231 ========== to ========== Record offline histograms with suffix via Histogram::FactoryGet Also move all offline client namespace constants into one central place. BUG=616231 ==========
jianli@chromium.org changed reviewers: + dimich@google.com - dewittj@chromium.org
jianli@chromium.org changed reviewers: - chili@chromium.org
On 2016/05/31 21:46:18, Dmitry Titov wrote: > Drive-by: > > The repetitive code with histogram macros look scary :) > > Since those macros are perf optimization (by caching a histogram in a static > pointer), would it be possible to instead use a slow path here and create/find a > histogram pointer every time using generated name? Basically, just calling > base::Histogram::FactoryGet(...) every time. For relatively rare and long > operations (like page creation) this can be just fine... Great suggestion. Done.
dewittj@chromium.org changed reviewers: + dewittj@chromium.org
https://codereview.chromium.org/2022283003/diff/1/components/offline_pages/cl... File components/offline_pages/client_policy_controller_unittest.cc (right): https://codereview.chromium.org/2022283003/diff/1/components/offline_pages/cl... components/offline_pages/client_policy_controller_unittest.cc:17: const char kUndefinedNamespace[] = "undefined"; On 2016/05/31 21:51:33, chili wrote: > should we also move undefined to the namespace constants too? Don't think so, this is just for testing. https://codereview.chromium.org/2022283003/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/2022283003/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model.cc:117: UMA_HISTOGRAM_ENUMERATION("OfflinePages.SavePageResult.bookmark", Please open a bug to move this to the diagnostics service when it is available. https://codereview.chromium.org/2022283003/diff/20001/components/offline_page... File components/offline_pages/client_namespace_constants.h (right): https://codereview.chromium.org/2022283003/diff/20001/components/offline_page... components/offline_pages/client_namespace_constants.h:14: extern const char kBookmarkNamespace[]; can't this just be constexpr char kBookmarkNamespace[] = "foo";? https://codereview.chromium.org/2022283003/diff/20001/components/offline_page... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/2022283003/diff/20001/components/offline_page... components/offline_pages/offline_page_model.cc:678: UMA_HISTOGRAM_TIMES("OfflinePages.SavePageTime.bookmark", should port this to Histogram::FactoryGet as well.
https://codereview.chromium.org/2022283003/diff/1/components/offline_pages/cl... File components/offline_pages/client_policy_controller_unittest.cc (right): https://codereview.chromium.org/2022283003/diff/1/components/offline_pages/cl... components/offline_pages/client_policy_controller_unittest.cc:17: const char kUndefinedNamespace[] = "undefined"; On 2016/05/31 22:38:45, dewittj wrote: > On 2016/05/31 21:51:33, chili wrote: > > should we also move undefined to the namespace constants too? > > Don't think so, this is just for testing. actually I think you're right now that I look at it. https://codereview.chromium.org/2022283003/diff/20001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2022283003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:312: offline_pages::kBookmarkNamespace, this seems wrong now. https://codereview.chromium.org/2022283003/diff/20001/components/offline_page... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/2022283003/diff/20001/components/offline_page... components/offline_pages/offline_page_model.cc:22: #include "components/offline_pages/client_namespace_constants.h" This can be removed once you finish porting OfflinePageModel::OnAddOfflinePageDone
https://codereview.chromium.org/2022283003/diff/1/components/offline_pages/cl... File components/offline_pages/client_policy_controller_unittest.cc (right): https://codereview.chromium.org/2022283003/diff/1/components/offline_pages/cl... components/offline_pages/client_policy_controller_unittest.cc:17: const char kUndefinedNamespace[] = "undefined"; On 2016/05/31 23:12:01, dewittj wrote: > On 2016/05/31 22:38:45, dewittj wrote: > > On 2016/05/31 21:51:33, chili wrote: > > > should we also move undefined to the namespace constants too? > > > > Don't think so, this is just for testing. > > actually I think you're right now that I look at it. I think this is only used in testing code and it could be any random string value. https://codereview.chromium.org/2022283003/diff/20001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2022283003/diff/20001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:312: offline_pages::kBookmarkNamespace, On 2016/05/31 23:12:01, dewittj wrote: > this seems wrong now. This will be updated. For now, the offline related button is not shown. https://codereview.chromium.org/2022283003/diff/20001/components/offline_page... File components/offline_pages/client_namespace_constants.h (right): https://codereview.chromium.org/2022283003/diff/20001/components/offline_page... components/offline_pages/client_namespace_constants.h:14: extern const char kBookmarkNamespace[]; On 2016/05/31 22:38:45, dewittj wrote: > can't this just be constexpr char kBookmarkNamespace[] = "foo";? Yes, it would work. But we don't really needs their content in header file, right? If so, it is suggested to still put it in .cc file. https://codereview.chromium.org/2022283003/diff/20001/components/offline_page... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/2022283003/diff/20001/components/offline_page... components/offline_pages/offline_page_model.cc:22: #include "components/offline_pages/client_namespace_constants.h" On 2016/05/31 23:12:01, dewittj wrote: > This can be removed once you finish porting > OfflinePageModel::OnAddOfflinePageDone Done. https://codereview.chromium.org/2022283003/diff/20001/components/offline_page... components/offline_pages/offline_page_model.cc:678: UMA_HISTOGRAM_TIMES("OfflinePages.SavePageTime.bookmark", On 2016/05/31 22:38:45, dewittj wrote: > should port this to Histogram::FactoryGet as well. Done.
lgtm some comments about the metrics follow, not necessarily blocking though. https://codereview.chromium.org/2022283003/diff/1/components/offline_pages/cl... File components/offline_pages/client_policy_controller_unittest.cc (right): https://codereview.chromium.org/2022283003/diff/1/components/offline_pages/cl... components/offline_pages/client_policy_controller_unittest.cc:17: const char kUndefinedNamespace[] = "undefined"; On 2016/05/31 23:50:07, jianli wrote: > On 2016/05/31 23:12:01, dewittj wrote: > > On 2016/05/31 22:38:45, dewittj wrote: > > > On 2016/05/31 21:51:33, chili wrote: > > > > should we also move undefined to the namespace constants too? > > > > > > Don't think so, this is just for testing. > > > > actually I think you're right now that I look at it. > > I think this is only used in testing code and it could be any random string > value. got it... This is different from kDefaultNamespace at https://code.google.com/p/chromium/codesearch#chromium/src/components/offline... https://codereview.chromium.org/2022283003/diff/40001/components/offline_page... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/2022283003/diff/40001/components/offline_page... components/offline_pages/offline_page_model.cc:145: base::TimeDelta::FromMilliseconds(1), base::TimeDelta::FromSeconds(1), 50, Should we have a higher upper bound? https://codereview.chromium.org/2022283003/diff/40001/components/offline_page... components/offline_pages/offline_page_model.cc:154: 1, 10000, 50, base::HistogramBase::kUmaTargetedHistogramFlag); nit: this is KB, should be documented somewhere. https://codereview.chromium.org/2022283003/diff/40001/components/offline_page... components/offline_pages/offline_page_model.cc:225: (base::Time::Now() - offline_page_item.last_access_time).InMinutes()); This could be negative since time is not monotonic. Do we need to check that before passing into the histogram?
https://codereview.chromium.org/2022283003/diff/40001/components/offline_page... File components/offline_pages/offline_page_model.cc (right): https://codereview.chromium.org/2022283003/diff/40001/components/offline_page... components/offline_pages/offline_page_model.cc:145: base::TimeDelta::FromMilliseconds(1), base::TimeDelta::FromSeconds(1), 50, On 2016/06/01 00:04:13, dewittj wrote: > Should we have a higher upper bound? It should be 10 seconds for upper bound. Fixed. https://codereview.chromium.org/2022283003/diff/40001/components/offline_page... components/offline_pages/offline_page_model.cc:154: 1, 10000, 50, base::HistogramBase::kUmaTargetedHistogramFlag); On 2016/06/01 00:04:13, dewittj wrote: > nit: this is KB, should be documented somewhere. Done. https://codereview.chromium.org/2022283003/diff/40001/components/offline_page... components/offline_pages/offline_page_model.cc:225: (base::Time::Now() - offline_page_item.last_access_time).InMinutes()); On 2016/06/01 00:04:13, dewittj wrote: > This could be negative since time is not monotonic. Do we need to check that > before passing into the histogram? Yes, it could be possible but should be rare. In this case, histogram will simply treat it as 0. I think it'd simple to keep as it is.
Description was changed from ========== Record offline histograms with suffix via Histogram::FactoryGet Also move all offline client namespace constants into one central place. BUG=616231 ========== to ========== Record offline histograms with suffix via Histogram::FactoryGet Also move all offline client namespace constants into one central place. BUG=616231 TBR=mmenke@chromium.org ==========
jianli@chromium.org changed reviewers: + mmenke@chromium.org
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2022283003/#ps80001 (title: "Address some more feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022283003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2022283003/80001
Message was sent while issue was closed.
Description was changed from ========== Record offline histograms with suffix via Histogram::FactoryGet Also move all offline client namespace constants into one central place. BUG=616231 TBR=mmenke@chromium.org ========== to ========== Record offline histograms with suffix via Histogram::FactoryGet Also move all offline client namespace constants into one central place. BUG=616231 TBR=mmenke@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Record offline histograms with suffix via Histogram::FactoryGet Also move all offline client namespace constants into one central place. BUG=616231 TBR=mmenke@chromium.org ========== to ========== Record offline histograms with suffix via Histogram::FactoryGet Also move all offline client namespace constants into one central place. BUG=616231 TBR=mmenke@chromium.org Committed: https://crrev.com/8ccbcdc96179ab65038e17cdbddc149e28754830 Cr-Commit-Position: refs/heads/master@{#397233} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8ccbcdc96179ab65038e17cdbddc149e28754830 Cr-Commit-Position: refs/heads/master@{#397233}
Message was sent while issue was closed.
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2022283003/diff/80001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2022283003/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:145: 1, please add a comment to explain the value. I will not be able to fix this if it is broken. https://codereview.chromium.org/2022283003/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:147: static_cast<int>(SavePageResult::RESULT_COUNT) + 1, something feels wrong here: min/max/bucket count... https://codereview.chromium.org/2022283003/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:240: (base::Time::Now() - offline_page_item.last_access_time).InMinutes()); Can time be passed in so that the same value is reported and marked on the page? WDYT?
Message was sent while issue was closed.
"TBR" stands for "To Be Reviewed"...while in practice, TBRed issues often aren't signed off on by TBRed people, should make sure they're at least sent an email informing them of the existence of the CL, and telling them why you're TBRing them.
Message was sent while issue was closed.
On 2016/06/01 21:38:23, mmenke wrote: > "TBR" stands for "To Be Reviewed"...while in practice, TBRed issues often aren't > signed off on by TBRed people, should make sure they're at least sent an email > informing them of the existence of the CL, and telling them why you're TBRing > them. (And in this case, TBR is certainly reasonable, and I have no issue with you doing it)
Message was sent while issue was closed.
https://codereview.chromium.org/2022283003/diff/80001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2022283003/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:145: 1, On 2016/06/01 21:33:56, fgorski wrote: > please add a comment to explain the value. I will not be able to fix this if it > is broken. Per discussion offline, we're not going to add comment. https://codereview.chromium.org/2022283003/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:147: static_cast<int>(SavePageResult::RESULT_COUNT) + 1, On 2016/06/01 21:33:56, fgorski wrote: > something feels wrong here: min/max/bucket count... Per discussion offline, this is fine since UMA reserves an underflow slot. https://codereview.chromium.org/2022283003/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:240: (base::Time::Now() - offline_page_item.last_access_time).InMinutes()); On 2016/06/01 21:33:56, fgorski wrote: > Can time be passed in so that the same value is reported and marked on the page? > WDYT? We can do that but it will make code more complicated. I think we don't really care that much in this case.
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2028383002/ by jianli@chromium.org. The reason for reverting is: Need to revert this since the merge to M52 is very difficult without touching offline_page_model.cc. Will land this again with trivial change to offline_page_model.cc such that I can bring changes from offline_page_model_impl.cc to offline_page_model.cc in M52..
Message was sent while issue was closed.
Description was changed from ========== Record offline histograms with suffix via Histogram::FactoryGet Also move all offline client namespace constants into one central place. BUG=616231 TBR=mmenke@chromium.org Committed: https://crrev.com/8ccbcdc96179ab65038e17cdbddc149e28754830 Cr-Commit-Position: refs/heads/master@{#397233} ========== to ========== Record offline histograms with suffix via Histogram::FactoryGet Also move all offline client namespace constants into one central place. BUG=616231 TBR=mmenke@chromium.org Committed: https://crrev.com/8ccbcdc96179ab65038e17cdbddc149e28754830 Cr-Commit-Position: refs/heads/master@{#397233} ==========
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2022283003/#ps100001 (title: "Trivial change to offline_page_model.cc to allow easy merge to M52")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022283003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2022283003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jianli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022283003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2022283003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for components/offline_pages/offline_page_model.h:
While running git apply --index -3 -p1;
error: patch failed: components/offline_pages/offline_page_model.h:24
Falling back to three-way merge...
Applied patch to 'components/offline_pages/offline_page_model.h' with
conflicts.
U components/offline_pages/offline_page_model.h
Patch: components/offline_pages/offline_page_model.h
Index: components/offline_pages/offline_page_model.h
diff --git a/components/offline_pages/offline_page_model.h
b/components/offline_pages/offline_page_model.h
index
29107e0ac8135d1672cea86259277206246fe1a6..cfb7442478fc6c1a8fbd81ecf4f33ec5f7e36c0a
100644
--- a/components/offline_pages/offline_page_model.h
+++ b/components/offline_pages/offline_page_model.h
@@ -24,7 +24,6 @@ class Time;
namespace offline_pages {
-static const char* const kBookmarkNamespace = "bookmark";
static const int64_t kInvalidOfflineId = 0;
struct ClientId;
The CQ bit was unchecked by commit-bot@chromium.org
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2022283003/#ps140001 (title: "Rebase again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2022283003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2022283003/140001
Message was sent while issue was closed.
Description was changed from ========== Record offline histograms with suffix via Histogram::FactoryGet Also move all offline client namespace constants into one central place. BUG=616231 TBR=mmenke@chromium.org Committed: https://crrev.com/8ccbcdc96179ab65038e17cdbddc149e28754830 Cr-Commit-Position: refs/heads/master@{#397233} ========== to ========== Record offline histograms with suffix via Histogram::FactoryGet Also move all offline client namespace constants into one central place. BUG=616231 TBR=mmenke@chromium.org Committed: https://crrev.com/8ccbcdc96179ab65038e17cdbddc149e28754830 Cr-Commit-Position: refs/heads/master@{#397233} ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Record offline histograms with suffix via Histogram::FactoryGet Also move all offline client namespace constants into one central place. BUG=616231 TBR=mmenke@chromium.org Committed: https://crrev.com/8ccbcdc96179ab65038e17cdbddc149e28754830 Cr-Commit-Position: refs/heads/master@{#397233} ========== to ========== Record offline histograms with suffix via Histogram::FactoryGet Also move all offline client namespace constants into one central place. BUG=616231 TBR=mmenke@chromium.org Committed: https://crrev.com/8ccbcdc96179ab65038e17cdbddc149e28754830 Cr-Commit-Position: refs/heads/master@{#397233} Committed: https://crrev.com/37ab199f7ba4c272d620813c458bc8f524c2c3ba Cr-Commit-Position: refs/heads/master@{#397623} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/37ab199f7ba4c272d620813c458bc8f524c2c3ba Cr-Commit-Position: refs/heads/master@{#397623} |
