Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1266993002:
Added histograms descriptions for History DB. (Closed)
|
Created:
5 years, 4 months ago by Alexander Yashkin Modified:
5 years, 4 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded histograms descriptions for History DB.
BUG=
R=shess@chromium.org,rkaplow@chromium.org
Committed: https://crrev.com/d00b04448dc7a412f8264e238fa2bee49fe3c723
Cr-Commit-Position: refs/heads/master@{#342335}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fixes after review, round 1 #
Total comments: 13
Patch Set 3 : Fixes after review, round 2 #Patch Set 4 : Change histograms owner from sky to shess #Messages
Total messages: 34 (7 generated)
a-v-y@yandex-team.ru changed reviewers: + rkaplow@chromium.org
BTW why were these never here before? Was the data never looked at? https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13283: + <owner>Please list the metric's owners. Add more owner tags as needed.</owner> please fill in owners files for each https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13301: + Size of History DB in MB. Seems to be duplicate of Profile.HistorySize. if this is a duplicate is this needed?
Seems some of them exist in the internal xml file? On Tue, Aug 4, 2015 at 11:03 AM, <rkaplow@chromium.org> wrote: > BTW why were these never here before? Was the data never looked at? > > > > https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... > File tools/metrics/histograms/histograms.xml (right): > > > https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:13283: + <owner>Please list the > metric's owners. Add more owner tags as needed.</owner> > please fill in owners files for each > > > https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:13301: + Size of History DB > in MB. Seems to be duplicate of Profile.HistorySize. > if this is a duplicate is this needed? > > https://codereview.chromium.org/1266993002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/08/04 15:10:14, chromium-reviews wrote: > Seems some of them exist in the internal xml file? > > On Tue, Aug 4, 2015 at 11:03 AM, <mailto:rkaplow@chromium.org> wrote: > > > BTW why were these never here before? Was the data never looked at? > > > > > > > > > https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... > > tools/metrics/histograms/histograms.xml:13283: + <owner>Please list the > > metric's owners. Add more owner tags as needed.</owner> > > please fill in owners files for each > > > > > > > https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... > > tools/metrics/histograms/histograms.xml:13301: + Size of History DB > > in MB. Seems to be duplicate of Profile.HistorySize. > > if this is a duplicate is this needed? > > > > https://codereview.chromium.org/1266993002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Ah I missed that, thanks for pointing this out. This CL is still correct since these histograms should be in the public xml. It looks like the internal ones have different comments and already have owners so they should possibly be looked at. Once this is submitted the internal ones should be cleaned up - should only exist in one.
rkaplow@chromium.org changed reviewers: + zea@chromium.org
Nicholas, would you be interested in owning these histograms?
zea@chromium.org changed reviewers: + sky@chromium.org
I can be, although I wasn't involved with any of these histograms from what I can recall (possibly some refactor a long time ago). sky@ is probably a better owner (and is an OWNER of the history component anyways, which I am not)
LGTM, though you might have meant sky@ originally :-). https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13301: + Size of History DB in MB. Seems to be duplicate of Profile.HistorySize. On 2015/08/04 15:03:09, rkaplow wrote: > if this is a duplicate is this needed? Also duplicates Sqlite.SizeKB.History .
https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13283: + <owner>Please list the metric's owners. Add more owner tags as needed.</owner> On 2015/08/04 15:03:09, rkaplow wrote: > please fill in owners files for each Added sky@chromium.org as owner. https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13301: + Size of History DB in MB. Seems to be duplicate of Profile.HistorySize. On 2015/08/04 15:03:09, rkaplow wrote: > if this is a duplicate is this needed? History.DatabaseFileMB is send on 1% of browser runs and Profile.HistorySize is send on every profile load. So it makes sense to me to remove sending of History.DatabaseFileMB histogram from browser code. Yet, if i understand correctly, this file must contain descriptions for all histograms that were ever send, even obsolete and deprecated.
Also added "Sqlite.Migration.History histogram description in last commit.
On 2015/08/04 15:15:30, rkaplow wrote: > On 2015/08/04 15:10:14, chromium-reviews wrote: > > Seems some of them exist in the internal xml file? > > > > On Tue, Aug 4, 2015 at 11:03 AM, <mailto:rkaplow@chromium.org> wrote: > > > > > BTW why were these never here before? Was the data never looked at? > > > > > > > > > > > > > > > https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > > > > https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... > > > tools/metrics/histograms/histograms.xml:13283: + <owner>Please list the > > > metric's owners. Add more owner tags as needed.</owner> > > > please fill in owners files for each > > > > > > > > > > > > https://codereview.chromium.org/1266993002/diff/1/tools/metrics/histograms/hi... > > > tools/metrics/histograms/histograms.xml:13301: + Size of History DB > > > in MB. Seems to be duplicate of Profile.HistorySize. > > > if this is a duplicate is this needed? > > > > > > https://codereview.chromium.org/1266993002/ > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Ah I missed that, thanks for pointing this out. > > This CL is still correct since these histograms should be in the public xml. It > looks like the internal ones have different comments and already have owners so > they should possibly be looked at. Once this is submitted the internal ones > should be cleaned up - should only exist in one. I suppose that internal xml file for histograms is not in chromium repo, so i can not compare and verify correctness of my description. Should i revert this commit? Or can you check my description agains internal xml?
https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13282: +<histogram name="History.DatabaseAdvancedMetricsTime"> units="milliseconds" https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13290: +<histogram name="History.DatabaseBasicMetricsTime"> units="milliseconds" https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13298: +<histogram name="History.DatabaseFileMB"> units="MB" https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13399: + Number of unique hosts visited within last month. Metrics are logged on Unique hostnames in History database urls table with last-visit times in the last 30 days, collected at startup. Can keep the last part about when logged. https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13407: + Number of urls visited within last month. Metrics are logged on Unique URLs in History database urls table with last-visit times in the last 30 days, collected at startup. https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42652: +<histogram name="Sqlite.Migration.History" units="ms"> milliseconds is more common
https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42652: +<histogram name="Sqlite.Migration.History" units="ms"> Sqlite.* is generally stuff coming from sql/ implementation. I think this should go into History.* somewhere.
I found a copy of the internal file (I think). I'm not going to include owner
lines, because I doubt the previous owner wants to be involved. I'm posting
these for informational purposes only, not to suggest that they _should_ be this
way.
<histogram name="History.DatabaseAdvancedMetricsTime" units="milliseconds">
<summary>
Time to collect advanced history database metrics (weekly and monthly URL,
host, and category counts).
</summary>
</histogram>
<histogram name="History.DatabaseBasicMetricsTime" units="milliseconds">
<summary>
Time to collect basic history database metrics (file size and table counts).
</summary>
</histogram>
<histogram name="History.DatabaseFileMB" units="MB">
<summary>Size of the main History file.</summary>
</histogram>
<histogram name="History.MonthlyHostCount">
<summary>
Unique hostnames in History database urls table with last-visit times in the
last 30 days, collected at startup.
</summary>
</histogram>
<histogram name="History.MonthlyURLCount">
<summary>
Unique URLs in History database urls table with last-visit times in the last
30 days, collected at startup.
</summary>
</histogram>
<histogram name="History.MonthlyVisitCount">
<summary>
Rows in History database visits table on startup with visit times in the
last 30 days.
</summary>
</histogram>
<histogram name="History.URLTableCount">
<summary>Rows in History database urls table.</summary>
</histogram>
<histogram name="History.VisitTableCount">
<summary>Rows in History database visits table.</summary>
</histogram>
<histogram name="History.WeeklyHostCount">
<summary>
Unique hostnames in History database urls table with last-visit times in the
last 7 days, collected at startup.
</summary>
</histogram>
<histogram name="History.WeeklyURLCount">
<summary>
Unique URLs in History database urls table with last-visit times in the last
7 days, collected at startup.
</summary>
</histogram>
<histogram name="History.WeeklyVisitCount">
<summary>
Rows in History database visits table on startup with visit times in the
last 7 days.
</summary>
</histogram>
Fixed review comments. Also updated descriptions to include text from internal histogram xml. It was more detailed and technical. https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13282: +<histogram name="History.DatabaseAdvancedMetricsTime"> On 2015/08/05 15:24:18, rkaplow wrote: > units="milliseconds" Done. https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13290: +<histogram name="History.DatabaseBasicMetricsTime"> On 2015/08/05 15:24:18, rkaplow wrote: > units="milliseconds" Done. https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13298: +<histogram name="History.DatabaseFileMB"> On 2015/08/05 15:24:18, rkaplow wrote: > units="MB" Done. https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13399: + Number of unique hosts visited within last month. Metrics are logged on On 2015/08/05 15:24:18, rkaplow wrote: > Unique hostnames in History database urls table with last-visit times in the > last 30 days, collected at startup. > > Can keep the last part about when logged. Done. https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13407: + Number of urls visited within last month. Metrics are logged on On 2015/08/05 15:24:18, rkaplow wrote: > Unique URLs in History database urls table with last-visit times in the last > 30 days, collected at startup. Done. https://codereview.chromium.org/1266993002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42652: +<histogram name="Sqlite.Migration.History" units="ms"> On 2015/08/05 15:24:18, rkaplow wrote: > milliseconds is more common Done.
lgtm lgtm but if you can wait for sky@ to approve as well to make sure he's ok with owning these.
I don't want to be the owner for these. Can the folks that added them be the owners?
On 2015/08/06 18:04:54, sky wrote: > I don't want to be the owner for these. Can the folks that added them be the > owners? If we go there ... some of those in the original file were unowned, others were pamg. I don't think she's super involved these days where she's going to want to be owner. That said, owner for histograms isn't really a high bar.
shess it is then:)
LGTM
On 2015/08/06 18:32:48, sky wrote: > LGTM LGTM assuming owner is switched to shess :)
On 2015/08/06 18:35:07, rkaplow wrote: > On 2015/08/06 18:32:48, sky wrote: > > LGTM > > LGTM assuming owner is switched to shess :) LGTM. I don't really want them, but I have a bunch of others, and I don't think I've ever had someone contact me about them except in the context of arguing about who should be owner of histograms, so ...
On 2015/08/06 19:03:45, Scott Hess wrote: > On 2015/08/06 18:35:07, rkaplow wrote: > > On 2015/08/06 18:32:48, sky wrote: > > > LGTM > > > > LGTM assuming owner is switched to shess :) > > LGTM. I don't really want them, but I have a bunch of others, and I don't think > I've ever had someone contact me about them except in the context of arguing > about who should be owner of histograms, so ... Changed owner to shess. All hail to the new owner :)
The CQ bit was checked by a-v-y@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, sky@chromium.org, shess@chromium.org Link to the patchset: https://codereview.chromium.org/1266993002/#ps60001 (title: "Change histograms owner from sky to shess")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266993002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266993002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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 a-v-y@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1266993002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1266993002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d00b04448dc7a412f8264e238fa2bee49fe3c723 Cr-Commit-Position: refs/heads/master@{#342335} |
