Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
335913002:
Add bad block count histograms (Closed)
|
Created:
6 years, 6 months ago by dehrenberg Modified:
6 years, 6 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, ghines_google.com, wzhong1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd bad block count histograms
This corresponds to the Chromecast change at this URL:
https://eureka-internal-review.googlesource.com/#/c/19857/
R=gwendal@chromium.org, isherman@chromium.org, robertshield@chromium.org, semenzato@chromium.org, asvitkine@chromium.org
BUG=384533
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278002
Patch Set 1 #Patch Set 2 : Add bad block count histograms #
Total comments: 6
Patch Set 3 : Fixes from code review #Patch Set 4 : Fix typo #
Total comments: 3
Messages
Total messages: 13 (0 generated)
PTAL
You might want to also say how often these numbers are sampled.
On 2014/06/13 22:39:17, semenzato wrote: > You might want to also say how often these numbers are sampled. Done
https://codereview.chromium.org/335913002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/335913002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:18931: + relevant for devices with raw NAND flash, such as Chromecast. Sampled daily. How does daily sampling work? If I have a device that's only on for 30 minutes a day, and I use it five out of 7 days in a week, how many samples will I upload? Does it matter whether those five uses were on the same day or on different days? https://codereview.chromium.org/335913002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:19062: +</histogram> Can you use a <histogram_suffixes> element to reduce the amount of repetition in this patch?
https://codereview.chromium.org/335913002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/335913002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:18927: +<histogram name="Platform.BadBlocksBackupsys"> I would not name the variables in the Platform realm directly: use something like Platform.Storage.Flash.BadBlock... This way, all storage, all flash variables will be next to each other when listed in the UMA histograms pull down.
https://codereview.chromium.org/335913002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/335913002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:18927: +<histogram name="Platform.BadBlocksBackupsys"> On 2014/06/14 02:12:27, gwendal wrote: > I would not name the variables in the Platform realm directly: > > use something like Platform.Storage.Flash.BadBlock... > > This way, all storage, all flash variables will be next to each other when > listed in the UMA histograms pull down. Done. https://codereview.chromium.org/335913002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:18931: + relevant for devices with raw NAND flash, such as Chromecast. Sampled daily. On 2014/06/14 00:42:50, Ilya Sherman wrote: > How does daily sampling work? If I have a device that's only on for 30 minutes > a day, and I use it five out of 7 days in a week, how many samples will I > upload? Does it matter whether those five uses were on the same day or on > different days? Done. https://codereview.chromium.org/335913002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:19062: +</histogram> On 2014/06/14 00:42:50, Ilya Sherman wrote: > Can you use a <histogram_suffixes> element to reduce the amount of repetition in > this patch? Done.
This passes validate_format.py
LGTM https://codereview.chromium.org/335913002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/335913002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:19352: + Chromecast is on for any significant length of time in the day. So... is this being sampled during shutdown? If so, is there a chance that logs might get lost due to unclean shutdowns? Or is it assumed that the Chromecast is always on, just mostly idle? Is that actually a fair assumption? IMO it would be great to list the specific event that triggers sampling (assuming there's something more to say), and then to think about whether there are any potential sources of bias from this sampling approach. If so, it's wise to list the most relevant potential sources of bias as caveats, so that anyone viewing this data knows what to watch out for.
https://codereview.chromium.org/335913002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/335913002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:19352: + Chromecast is on for any significant length of time in the day. On 2014/06/17 21:07:05, Ilya Sherman wrote: > So... is this being sampled during shutdown? If so, is there a chance that logs > might get lost due to unclean shutdowns? Or is it assumed that the Chromecast > is always on, just mostly idle? Is that actually a fair assumption? > > IMO it would be great to list the specific event that triggers sampling > (assuming there's something more to say), and then to think about whether there > are any potential sources of bias from this sampling approach. If so, it's wise > to list the most relevant potential sources of bias as caveats, so that anyone > viewing this data knows what to watch out for. I'm not sure how much detail I should go into--it's pretty boring. There's a daemon which wakes up every 100 ms and checks whether it's a new day by checking the timestamp of a special file which it updates. If it sees that it's a new day, then it tries to upload the bad block counts. But the device may be shut down before it gets the chance to send it, so it's not so reliable if it's been the new day for a very short amount of time. No events trigger sampling beyond the device being on. Should I put that in the summary?
https://codereview.chromium.org/335913002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/335913002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:19352: + Chromecast is on for any significant length of time in the day. On 2014/06/17 21:15:36, dehrenberg wrote: > On 2014/06/17 21:07:05, Ilya Sherman wrote: > > So... is this being sampled during shutdown? If so, is there a chance that > logs > > might get lost due to unclean shutdowns? Or is it assumed that the Chromecast > > is always on, just mostly idle? Is that actually a fair assumption? > > > > IMO it would be great to list the specific event that triggers sampling > > (assuming there's something more to say), and then to think about whether > there > > are any potential sources of bias from this sampling approach. If so, it's > wise > > to list the most relevant potential sources of bias as caveats, so that anyone > > viewing this data knows what to watch out for. > > I'm not sure how much detail I should go into--it's pretty boring. > > There's a daemon which wakes up every 100 ms and checks whether it's a new day > by checking the timestamp of a special file which it updates. If it sees that > it's a new day, then it tries to upload the bad block counts. But the device may > be shut down before it gets the chance to send it, so it's not so reliable if > it's been the new day for a very short amount of time. No events trigger > sampling beyond the device being on. > > Should I put that in the summary? Thanks. I think the summary is probably fine as is, given this mechanism.
The CQ bit was checked by dehrenberg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dehrenberg@chromium.org/335913002/60001
Message was sent while issue was closed.
Change committed as 278002 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
