Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2257533005:
Change logging of about:flags from actions to a histogram. (Closed)
|
Created:
4 years, 4 months ago by Alexei Svitkine (slow) Modified:
3 years, 6 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. |
DescriptionChange logging of about:flags from actions to a histogram.
This CL deprecates all the "AboutFlags_*" actions and instead
replaces their logging with a AboutFlags.Seen histogram, that
has all the actions as enumerations.
This new histogram is logged using the same code that logs
Login.CustomFlags histogram on ChromeOS. Following this
change, via separate CLs I plan to deprecate Login.CustomFlags
in favor of AboutFlags.Seen and also make the new histogram
get logged on Android, where I think it's currently not logged.
BUG=639304
Committed: https://crrev.com/ab33c92ba610714412e7a572451bb58e5317c86f
Cr-Commit-Position: refs/heads/master@{#415410}
Patch Set 1 #
Total comments: 9
Patch Set 2 : . #
Total comments: 2
Patch Set 3 : Fix typo in header. #
Total comments: 2
Messages
Total messages: 35 (21 generated)
The CQ bit was checked by asvitkine@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Change logging of about:flags from actions to a histogram. BUG=... ========== to ========== Change logging of about:flags from actions to a histogram. BUG=639304 ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Description was changed from ========== Change logging of about:flags from actions to a histogram. BUG=639304 ========== to ========== Change logging of about:flags from actions to a histogram. This CL deprecates all the "AboutFlags_*" actions and instead replaces their logging with a AboutFlags.Seen histogram, that has all the actions as enumerations. This new histogram is logged using the same code that logs Login.CustomFlags histogram on ChromeOS. Following this change, via separate CLs I plan to deprecate Login.CustomFlags in favor of AboutFlags.Seen and also make the new histogram get logged on Android, where I think it's currently not logged. BUG=639304 ==========
asvitkine@chromium.org changed reviewers: + isherman@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for picking this up, Alexei! https://codereview.chromium.org/2257533005/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2257533005/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:653: // to "count users" view on the dashboard. nit: You repeat this below as well -- is that intended? https://codereview.chromium.org/2257533005/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:671: // to calculate and verify checksum. Hmm, the user actions measured how often users *switched* a given flag. The histogram is measuring the state at startup, which is a different thing. Are you sure that there aren't consumers of this data who are interested specifically in how often flags are switched? https://codereview.chromium.org/2257533005/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2226: } nit: Why not call ReportCustomFlags("AboutFlags.Seen", flags)? (And maybe rename that function?) https://codereview.chromium.org/2257533005/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2257533005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75: +<histogram name="AboutFlags.Seen" enum="LoginCustomFlags"> nit: WDYT of "FlagsAtStartup" rather than "Seen"? https://codereview.chromium.org/2257533005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75: +<histogram name="AboutFlags.Seen" enum="LoginCustomFlags"> It looks like you're adding a new top-level category. Any reason not to reuse the "Login.CustomFlags" histogram, or at least log both sets of data to the same histogram? If we do introduce a new category, I'm wondering whether we could find a slightly more general one -- e.g. "CommandLine.Flags.AtStartup". Or, maybe it makes sense to reuse the existing "Launch" or "Settings" categories?
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Thanks - addressed comments. PTAL! https://codereview.chromium.org/2257533005/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2257533005/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:653: // to "count users" view on the dashboard. On 2016/08/29 21:19:59, Ilya Sherman wrote: > nit: You repeat this below as well -- is that intended? Nope, removed! https://codereview.chromium.org/2257533005/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:671: // to calculate and verify checksum. On 2016/08/29 21:19:58, Ilya Sherman wrote: > Hmm, the user actions measured how often users *switched* a given flag. The > histogram is measuring the state at startup, which is a different thing. Are > you sure that there aren't consumers of this data who are interested > specifically in how often flags are switched? I thought so too originally, but turns out they were logged at startup, in fact. You can take a look at RecordUMAStatistics() which was logging them, whose implementation I'm replacing in this CL. https://codereview.chromium.org/2257533005/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2226: } On 2016/08/29 21:19:59, Ilya Sherman wrote: > nit: Why not call ReportCustomFlags("AboutFlags.Seen", flags)? (And maybe > rename that function?) Done. https://codereview.chromium.org/2257533005/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2257533005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75: +<histogram name="AboutFlags.Seen" enum="LoginCustomFlags"> On 2016/08/29 21:19:59, Ilya Sherman wrote: > It looks like you're adding a new top-level category. Any reason not to reuse > the "Login.CustomFlags" histogram, or at least log both sets of data to the same > histogram? > > If we do introduce a new category, I'm wondering whether we could find a > slightly more general one -- e.g. "CommandLine.Flags.AtStartup". Or, maybe it > makes sense to reuse the existing "Launch" or "Settings" categories? Good idea - renamed to Launch.FlagsAtStartup. I plan to deprecate Login.CustomFlags in favor of this new histogram in a later CL.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM, thanks! https://codereview.chromium.org/2257533005/diff/60001/chrome/browser/about_fl... File chrome/browser/about_flags.h (right): https://codereview.chromium.org/2257533005/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.h:93: void ReportAboutFlagsHistogram(const std::string& uma_histogram_hame, nit: s/hame/name here too :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2257533005/diff/60001/chrome/browser/about_fl... File chrome/browser/about_flags.h (right): https://codereview.chromium.org/2257533005/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.h:93: void ReportAboutFlagsHistogram(const std::string& uma_histogram_hame, On 2016/08/29 22:31:29, Ilya Sherman wrote: > nit: s/hame/name here too :) Done.
asvitkine@chromium.org changed reviewers: + thestig@chromium.org
+thestig for chrome/ OWNERS
lgtm
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2257533005/#ps80001 (title: "Fix typo in header.")
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.
Description was changed from ========== Change logging of about:flags from actions to a histogram. This CL deprecates all the "AboutFlags_*" actions and instead replaces their logging with a AboutFlags.Seen histogram, that has all the actions as enumerations. This new histogram is logged using the same code that logs Login.CustomFlags histogram on ChromeOS. Following this change, via separate CLs I plan to deprecate Login.CustomFlags in favor of AboutFlags.Seen and also make the new histogram get logged on Android, where I think it's currently not logged. BUG=639304 ========== to ========== Change logging of about:flags from actions to a histogram. This CL deprecates all the "AboutFlags_*" actions and instead replaces their logging with a AboutFlags.Seen histogram, that has all the actions as enumerations. This new histogram is logged using the same code that logs Login.CustomFlags histogram on ChromeOS. Following this change, via separate CLs I plan to deprecate Login.CustomFlags in favor of AboutFlags.Seen and also make the new histogram get logged on Android, where I think it's currently not logged. BUG=639304 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Change logging of about:flags from actions to a histogram. This CL deprecates all the "AboutFlags_*" actions and instead replaces their logging with a AboutFlags.Seen histogram, that has all the actions as enumerations. This new histogram is logged using the same code that logs Login.CustomFlags histogram on ChromeOS. Following this change, via separate CLs I plan to deprecate Login.CustomFlags in favor of AboutFlags.Seen and also make the new histogram get logged on Android, where I think it's currently not logged. BUG=639304 ========== to ========== Change logging of about:flags from actions to a histogram. This CL deprecates all the "AboutFlags_*" actions and instead replaces their logging with a AboutFlags.Seen histogram, that has all the actions as enumerations. This new histogram is logged using the same code that logs Login.CustomFlags histogram on ChromeOS. Following this change, via separate CLs I plan to deprecate Login.CustomFlags in favor of AboutFlags.Seen and also make the new histogram get logged on Android, where I think it's currently not logged. BUG=639304 Committed: https://crrev.com/ab33c92ba610714412e7a572451bb58e5317c86f Cr-Commit-Position: refs/heads/master@{#415410} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ab33c92ba610714412e7a572451bb58e5317c86f Cr-Commit-Position: refs/heads/master@{#415410}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2257533005/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2257533005/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:666: // to calculate and verify checksum. Hm, this makes adding flags harder than it used to be :-/ (ref https://codereview.chromium.org/2936273002/)
Message was sent while issue was closed.
https://codereview.chromium.org/2257533005/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2257533005/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:666: // to calculate and verify checksum. On 2017/06/14 21:27:58, Nico wrote: > Hm, this makes adding flags harder than it used to be :-/ (ref > https://codereview.chromium.org/2936273002/) Actually, this CL didn't change it - it was already the case before this CL for a while that we had a histogram for this - it just used to be CrOS only.
Message was sent while issue was closed.
On Wed, Jun 14, 2017 at 5:29 PM, <asvitkine@chromium.org> wrote: > > https://codereview.chromium.org/2257533005/diff/80001/ > chrome/browser/about_flags.cc > File chrome/browser/about_flags.cc (right): > > https://codereview.chromium.org/2257533005/diff/80001/ > chrome/browser/about_flags.cc#newcode666 > chrome/browser/about_flags.cc:666: // to calculate and verify checksum. > On 2017/06/14 21:27:58, Nico wrote: > > Hm, this makes adding flags harder than it used to be :-/ (ref > > https://codereview.chromium.org/2936273002/) > > Actually, this CL didn't change it - it was already the case before this > CL for a while that we had a histogram for this - it just used to be > CrOS only. > Which sounds like a change (?) > > https://codereview.chromium.org/2257533005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Sorry, I should have been more clear - all the entries still needed to be listed in the XML (regardless of the platform they applied to) even before the change, despite the histogram previously only getting logged on CrOS. Anyway, yes it makes it a bit more tedious, but the unit test failure message I think makes it pretty clear what needs to be added. On Wed, Jun 14, 2017 at 5:30 PM, Nico Weber <thakis@chromium.org> wrote: > On Wed, Jun 14, 2017 at 5:29 PM, <asvitkine@chromium.org> wrote: > >> >> https://codereview.chromium.org/2257533005/diff/80001/chrome >> /browser/about_flags.cc >> File chrome/browser/about_flags.cc (right): >> >> https://codereview.chromium.org/2257533005/diff/80001/chrome >> /browser/about_flags.cc#newcode666 >> chrome/browser/about_flags.cc:666: // to calculate and verify checksum. >> On 2017/06/14 21:27:58, Nico wrote: >> > Hm, this makes adding flags harder than it used to be :-/ (ref >> > https://codereview.chromium.org/2936273002/) >> >> Actually, this CL didn't change it - it was already the case before this >> CL for a while that we had a histogram for this - it just used to be >> CrOS only. >> > > Which sounds like a change (?) > > >> >> https://codereview.chromium.org/2257533005/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
