Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
303423003:
Added Login.UsersActiveWeekly UMA metric description to histograms.xml. (Closed)
|
Created:
6 years, 6 months ago by Alexander Alekseev Modified:
6 years, 6 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews, jar (doing other things), asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded Login.UsersActiveWeekly UMA metric description to histograms.xml.
BUG=365352
TEST=none
NOCHECK=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274075
Patch Set 1 #
Total comments: 5
Patch Set 2 : Description updated. #
Messages
Total messages: 12 (0 generated)
Please review. Metrics is sent from ChromeOS: https://chromium-review.googlesource.com/202280
Curious: Why does the CL description list me as TBR'ed? https://codereview.chromium.org/303423003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/303423003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:9227: + Chrome OS histogram that keeps track of number of users who has logged in in nit: "users who has" -> "users who have" https://codereview.chromium.org/303423003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:9228: + the last 7 days. Reported on every boot and once a day after that. Note that this will bias your metric toward users who reboot more frequently. Is that intentional/acceptable?
On 2014/05/30 23:03:55, Ilya Sherman wrote: > Curious: Why does the CL description list me as TBR'ed? Sorry, I just looked on who has reviewed my two previous changes to histogram.xml ;-) There is no designated owner for histograms.xml and you are one of tools/metrics/OWNERS .
https://codereview.chromium.org/303423003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/303423003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:9227: + Chrome OS histogram that keeps track of number of users who has logged in in On 2014/05/30 23:03:55, Ilya Sherman wrote: > nit: "users who has" -> "users who have" Done. https://codereview.chromium.org/303423003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:9228: + the last 7 days. Reported on every boot and once a day after that. On 2014/05/30 23:03:55, Ilya Sherman wrote: > Note that this will bias your metric toward users who reboot more frequently. > Is that intentional/acceptable? This metric actually counts number of user home directories with modification time less than 7 days. So the actual value doesn't depend on report time. What is more important, it is always reported with total number of user home directories. So these two metrics match.
On 2014/05/30 23:20:32, alemate wrote: > On 2014/05/30 23:03:55, Ilya Sherman wrote: > > Curious: Why does the CL description list me as TBR'ed? > > Sorry, I just looked on who has reviewed my two previous changes to > histogram.xml ;-) > There is no designated owner for histograms.xml and you are one of > tools/metrics/OWNERS . To clarify, my question was about the use of "TBR" rather than a regular review. I don't mind the choice of reviewer :) https://codereview.chromium.org/303423003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/303423003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:9228: + the last 7 days. Reported on every boot and once a day after that. On 2014/05/30 23:20:39, alemate wrote: > On 2014/05/30 23:03:55, Ilya Sherman wrote: > > Note that this will bias your metric toward users who reboot more frequently. > > Is that intentional/acceptable? > > This metric actually counts number of user home directories with modification > time less than 7 days. So the actual value doesn't depend on report time. > > What is more important, it is always reported with total number of user > home directories. So these two metrics match. My concern is that the metric is *reported* on every boot. Hence, users who reboot more frequently will contribute more samples than users who reboot less frequently. However, if your goal is to match an existing metric that already has this bias, then LGTM.
On 2014/05/30 23:24:35, Ilya Sherman wrote: > On 2014/05/30 23:20:32, alemate wrote: > > On 2014/05/30 23:03:55, Ilya Sherman wrote: > > > Curious: Why does the CL description list me as TBR'ed? > > > > Sorry, I just looked on who has reviewed my two previous changes to > > histogram.xml ;-) > > There is no designated owner for histograms.xml and you are one of > > tools/metrics/OWNERS . > > To clarify, my question was about the use of "TBR" rather than a regular review. > I don't mind the choice of reviewer :) I used to think that TBR is just a tag to add reviewers automatically. Is there a difference against adding reviewers to existing code-review? > My concern is that the metric is *reported* on every boot. Hence, users who > reboot more frequently will contribute more samples than users who reboot less > frequently. Ah, you mean "devices, which are rebooted more frequently", not "different users on a single device". > However, if your goal is to match an existing metric that already has this bias, > then LGTM. Yes, it's old way of reporting usage metrics. But it always used to be this way. So I've just copied this comment from another metric.
The CQ bit was checked by alemate@chromium.org
The CQ bit was unchecked by alemate@chromium.org
The CQ bit was checked by alemate@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alemate@chromium.org/303423003/20001
On 2014/05/30 23:43:30, alemate wrote: > On 2014/05/30 23:24:35, Ilya Sherman wrote: > > On 2014/05/30 23:20:32, alemate wrote: > > > On 2014/05/30 23:03:55, Ilya Sherman wrote: > > > > Curious: Why does the CL description list me as TBR'ed? > > > > > > Sorry, I just looked on who has reviewed my two previous changes to > > > histogram.xml ;-) > > > There is no designated owner for histograms.xml and you are one of > > > tools/metrics/OWNERS . > > > > To clarify, my question was about the use of "TBR" rather than a regular > review. > > I don't mind the choice of reviewer :) > > I used to think that TBR is just a tag to add reviewers automatically. > Is there a difference against adding reviewers to existing code-review? "TBR" stands for "To Be Reviewed", and is used to commit CLs without waiting for approval. It's generally used for reverting CLs or making trivial fixups. If you just want to add a reviewer, you can use "R=" rather than "TBR=" :)
Message was sent while issue was closed.
Change committed as 274075 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
