Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902853002:
Add UMA metrics for the linux distro. (Closed)
DescriptionAdd UMA metrics for the linux distro.
At this point, only included the distros that are currently supported
both by us and the distro maker. For example, Ubuntu 15.10 is not
included as it is no longer supported by Ubuntu.
If there is a large UNKNOWN usage, then we can try to add other popular
distros, such as Mint.
BUG=724244
R=rkaplow@chromium.org,thomasanderson@chromium.org
Review-Url: https://codereview.chromium.org/2902853002
Cr-Commit-Position: refs/heads/master@{#475653}
Committed: https://chromium.googlesource.com/chromium/src/+/fcd78795faa1db43a347464fa235ead2a186636b
Patch Set 1 #
Total comments: 12
Patch Set 2 : Make review comment changes. #
Total comments: 8
Patch Set 3 : Use base::Version and other small changes. #
Total comments: 4
Patch Set 4 : Switch to enumeration histogram. #Patch Set 5 : Fix unit tests. #
Messages
Total messages: 38 (23 generated)
The CQ bit was checked by timbrown@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2902853002/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2902853002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:12: #include "base/linux_util.h" Guard this include with #if defined(OS_LINUX) && !defined(OS_CHROMEOS) since the only code that uses it has those guards https://codereview.chromium.org/2902853002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:64: enum UMALinuxDistro { Please follow the practices here: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... https://codereview.chromium.org/2902853002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:194: const std::string distro = base::GetLinuxDistro(); Does this call block? (GetLinuxDistro caches its result. Has it been called before this call?) https://codereview.chromium.org/2902853002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:198: const char ubuntu[] = "Ubuntu "; constants should have a k prefix and should be named CamelCase: eg: kDistroUbuntu https://codereview.chromium.org/2902853002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:202: if (distro.compare(0, strlen(ubuntu), ubuntu) == 0) { I think it's better to use base::SplitString() on the whitespaces once before the conditionals. Then you can avoid substr, strlen, etc. And you can do direct string comparison using std::string::operator== https://codereview.chromium.org/2902853002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:240: UMA_HISTOGRAM_SPARSE_SLOWLY("Linux.Distro", distro_result); UMA_HISTOGRAM_SPARSE_SLOWLY => UMA_HISTOGRAM_ENUMERATION
Please take a look. Thanks. https://codereview.chromium.org/2902853002/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2902853002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:12: #include "base/linux_util.h" On 2017/05/23 22:12:13, Tom Anderson wrote: > Guard this include with #if defined(OS_LINUX) && !defined(OS_CHROMEOS) since the > only code that uses it has those guards Ah, I missed those sections. Done. https://codereview.chromium.org/2902853002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:64: enum UMALinuxDistro { On 2017/05/23 22:12:13, Tom Anderson wrote: > Please follow the practices here: > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... Done. https://codereview.chromium.org/2902853002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:194: const std::string distro = base::GetLinuxDistro(); On 2017/05/23 22:12:13, Tom Anderson wrote: > Does this call block? (GetLinuxDistro caches its result. Has it been called > before this call?) Good catch. GetLinuxDistro() is called previously, but only as part of a task. So I guess there is no guarantee that it has completed (and cached) by this point. I have therefore made the call to RecordLinuxDistro() a task too. Done. https://codereview.chromium.org/2902853002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:198: const char ubuntu[] = "Ubuntu "; On 2017/05/23 22:12:13, Tom Anderson wrote: > constants should have a k prefix and should be named CamelCase: > eg: kDistroUbuntu Acknowledged. https://codereview.chromium.org/2902853002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:202: if (distro.compare(0, strlen(ubuntu), ubuntu) == 0) { On 2017/05/23 22:12:13, Tom Anderson wrote: > I think it's better to use base::SplitString() on the whitespaces once before > the conditionals. Then you can avoid substr, strlen, etc. And you can do > direct string comparison using std::string::operator== Good idea. It's a little longer now with guards on the vector size etc, but I think it's more readable. Done. https://codereview.chromium.org/2902853002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:240: UMA_HISTOGRAM_SPARSE_SLOWLY("Linux.Distro", distro_result); On 2017/05/23 22:12:13, Tom Anderson wrote: > UMA_HISTOGRAM_SPARSE_SLOWLY => UMA_HISTOGRAM_ENUMERATION rkaplow: Do you agree? From the comments here https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?&l=243 it seemed like a reasonable choice, but the benefits and drawbacks are still not really clear to me.
The CQ bit was checked by timbrown@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2902853002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2902853002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:66: #if defined(OS_LINUX) && !defined(OS_CHROMEOS) nit: I don't think the include guards here are necessary https://codereview.chromium.org/2902853002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:203: base::SplitString(base::GetLinuxDistro(), " .", base::TRIM_WHITESPACE, Instead of using " ." as the separators, use base::kWhitespaceASCII. This means versions will be kept as one token https://codereview.chromium.org/2902853002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:210: if (distro_tokens.size() >= 3) { Use base::Version for distro versions. You can compare against eg. Ubuntu trusty like: version.CompareToWildcardString("14.04.*") https://codereview.chromium.org/2902853002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:464: base::BindOnce(base::IgnoreResult(&RecordLinuxDistro))); RecordLinuxDistro doesn't return anything, so you can omit base::IgnoreResult
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL https://codereview.chromium.org/2902853002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2902853002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:66: #if defined(OS_LINUX) && !defined(OS_CHROMEOS) On 2017/05/24 01:19:13, Tom Anderson wrote: > nit: I don't think the include guards here are necessary Done. https://codereview.chromium.org/2902853002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:203: base::SplitString(base::GetLinuxDistro(), " .", base::TRIM_WHITESPACE, On 2017/05/24 01:19:13, Tom Anderson wrote: > Instead of using " ." as the separators, use base::kWhitespaceASCII. This means > versions will be kept as one token Done. https://codereview.chromium.org/2902853002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:210: if (distro_tokens.size() >= 3) { On 2017/05/24 01:19:13, Tom Anderson wrote: > Use base::Version for distro versions. > > You can compare against eg. Ubuntu trusty like: > version.CompareToWildcardString("14.04.*") Done. I also changed the debian check, since it uses patch releases too. I didn't bother changing openSuse and Fedora as they look straightforward enough to me. Let me know if you think they should be changed too though. https://codereview.chromium.org/2902853002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:464: base::BindOnce(base::IgnoreResult(&RecordLinuxDistro))); On 2017/05/24 01:19:13, Tom Anderson wrote: > RecordLinuxDistro doesn't return anything, so you can omit base::IgnoreResult Done.
lgtm % usage of UMA_HISTOGRAM_SPARSE_SLOWLY, but I defer to rkaplow@ on that matter
The CQ bit was checked by timbrown@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2902853002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2902853002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:253: UMA_HISTOGRAM_SPARSE_SLOWLY("Linux.Distro", distro_result); I agree that this can just be a regular UMA_HISTOGRAM_ENUMERATION https://codereview.chromium.org/2902853002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2902853002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26019: +<histogram name="Linux.Distro" enum="LinuxDistro"> what kind of analysis are you planning on doing? It may make sense to log this on every UMA record and not just startup so you can split other metrics by distro. Slightly more complicated to do that, so if not needed yet we can wait on that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2902853002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2902853002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:253: UMA_HISTOGRAM_SPARSE_SLOWLY("Linux.Distro", distro_result); On 2017/05/24 17:53:06, rkaplow wrote: > I agree that this can just be a regular UMA_HISTOGRAM_ENUMERATION Done. https://codereview.chromium.org/2902853002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2902853002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26019: +<histogram name="Linux.Distro" enum="LinuxDistro"> On 2017/05/24 17:53:06, rkaplow wrote: > what kind of analysis are you planning on doing? It may make sense to log this > on every UMA record and not just startup so you can split other metrics by > distro. Slightly more complicated to do that, so if not needed yet we can wait > on that. Thanks for your help in explaining this offline. Our biggest priority at the moment is just being able to see the breakdown of our users by distro, so I'll continue with the current cl.
rkaplow@google.com changed reviewers: + rkaplow@google.com
lgtm
The CQ bit was checked by timbrown@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/24 21:31:37, rkaplow1 wrote: > lgtm Thanks, but can I get this from your @chromium.org account please? I need your OWNERS power :).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm ah apologizes... wrong tab
The CQ bit was checked by timbrown@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thomasanderson@chromium.org, rkaplow@chromium.org, rkaplow@google.com Link to the patchset: https://codereview.chromium.org/2902853002/#ps80001 (title: "Fix unit tests.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by timbrown@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496168405428510,
"parent_rev": "e989b524800d27da66f4ad872dbc998472d5bae7", "commit_rev":
"fcd78795faa1db43a347464fa235ead2a186636b"}
Message was sent while issue was closed.
Description was changed from ========== Add UMA metrics for the linux distro. At this point, only included the distros that are currently supported both by us and the distro maker. For example, Ubuntu 15.10 is not included as it is no longer supported by Ubuntu. If there is a large UNKNOWN usage, then we can try to add other popular distros, such as Mint. BUG=724244 R=rkaplow@chromium.org,thomasanderson@chromium.org ========== to ========== Add UMA metrics for the linux distro. At this point, only included the distros that are currently supported both by us and the distro maker. For example, Ubuntu 15.10 is not included as it is no longer supported by Ubuntu. If there is a large UNKNOWN usage, then we can try to add other popular distros, such as Mint. BUG=724244 R=rkaplow@chromium.org,thomasanderson@chromium.org Review-Url: https://codereview.chromium.org/2902853002 Cr-Commit-Position: refs/heads/master@{#475653} Committed: https://chromium.googlesource.com/chromium/src/+/fcd78795faa1db43a347464fa235... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/fcd78795faa1db43a347464fa235... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
