Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2351733002:
Upload MeminfoMemTotal to track installed RAM size for Chrome OS devices. (Closed)
DescriptionUpload MeminfoMemTotal to track installed RAM size for Chrome OS devices.
BUG=b:31295501
Committed: https://crrev.com/1d83b748cf38cdef67ce4692bd4ca95d3ea6056c
Cr-Commit-Position: refs/heads/master@{#419932}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Upload MeminfoMemTotal to track installed RAM size for Chrome OS devices. #Patch Set 3 : Upload MeminfoMemTotal to track installed RAM size for Chrome OS devices. #Messages
Total messages: 17 (3 generated)
bccheng@chromium.org changed reviewers: + jwd@chromium.org, kemp@chromium.org, semenzato@chromium.org
Thank you for adding this. See inline comment.
On 2016/09/19 15:06:51, Luigi Semenzato wrote: > Thank you for adding this. See inline comment. I don't see an inline comment.
https://codereview.chromium.org/2351733002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2351733002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:41903: +<histogram name="Platform.MeminfoMemTotal"> Instead of giving the units in the summary section, use the units specification (see other histograms). Also this may be a good time to reconsider the choice to re-send this number every 30s instead of just once. (Or file a bug for it.) Thanks!
https://codereview.chromium.org/2351733002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2351733002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:41903: +<histogram name="Platform.MeminfoMemTotal"> On 2016/09/19 17:03:09, Luigi Semenzato wrote: > Instead of giving the units in the summary section, use the units specification > (see other histograms). > > Also this may be a good time to reconsider the choice to re-send this number > every 30s instead of just once. (Or file a bug for it.) > > Thanks! I was going to question the 30s idea. How are these metrics analysed? What is "installed memory"? Is it the physical ram amount? If it's something that doesn't change within a chrome session, you could be better served by logging this once per report, you'd still be able to correlate it with other metrics in each report, but you'd much reduce the counts magnitude. You'd also be able to potentially normalize by UMA.RecordsCounts. (could introduce a bias if CrOS tends to have varying intervals for each UMA log record, which the every 30s somewhat mitigates)
On 2016/09/19 17:28:02, jwd wrote: > https://codereview.chromium.org/2351733002/diff/1/tools/metrics/histograms/hi... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2351733002/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:41903: +<histogram > name="Platform.MeminfoMemTotal"> > On 2016/09/19 17:03:09, Luigi Semenzato wrote: > > Instead of giving the units in the summary section, use the units > specification > > (see other histograms). > > > > Also this may be a good time to reconsider the choice to re-send this number > > every 30s instead of just once. (Or file a bug for it.) > > > > Thanks! > > I was going to question the 30s idea. How are these metrics analysed? > > What is "installed memory"? Is it the physical ram amount? If it's something > that doesn't change within a chrome session, you could be better served by > logging this once per report, you'd still be able to correlate it with other > metrics in each report, but you'd much reduce the counts magnitude. You'd also > be able to potentially normalize by UMA.RecordsCounts. (could introduce a bias > if CrOS tends to have varying intervals for each UMA log record, which the every > 30s somewhat mitigates) Ah, my bad, I approved the CL that added MeminfoMemTotal sent every 30s. Basically the histograms are cheap, and even if the samples are all the same, this helps correlate memory size and usage. So let's ignore my objection to sending this every 30s, and it would be great if you can address the units comment. Also it would be nice to fix the other Meminfo stats by adding units to those as well. For more information about UMA, you can look at uma.googleplex.com. Thanks!
I have uploaded a new CL that also cleans up all existing ones with histogram_suffixes.
On 2016/09/20 04:52:16, bccheng wrote: > I have uploaded a new CL that also cleans up all existing ones with > histogram_suffixes. Nice, I didn't know you could do this. LGTM. Actually, would you mind adding me to the owners too? I might as well since I added most of them initially and I keep getting review requests.
On 2016/09/20 05:01:22, Luigi Semenzato wrote: > On 2016/09/20 04:52:16, bccheng wrote: > > I have uploaded a new CL that also cleans up all existing ones with > > histogram_suffixes. > > Nice, I didn't know you could do this. LGTM. > > Actually, would you mind adding me to the owners too? I might as well since I > added most of them initially and I keep getting review requests. Done. jwd told me the trick when he reviewed my CL last time. :)
lgtm LGTM
LGTM (I'm fine with the continued use of 30s logging strategy) Thanks for the histograms cleanup!
The CQ bit was checked by bccheng@chromium.org
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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Upload MeminfoMemTotal to track installed RAM size for Chrome OS devices. BUG=b:31295501 ========== to ========== Upload MeminfoMemTotal to track installed RAM size for Chrome OS devices. BUG=b:31295501 Committed: https://crrev.com/1d83b748cf38cdef67ce4692bd4ca95d3ea6056c Cr-Commit-Position: refs/heads/master@{#419932} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1d83b748cf38cdef67ce4692bd4ca95d3ea6056c Cr-Commit-Position: refs/heads/master@{#419932} |
