Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1814213004:
Record UMA for Physical Web state (Closed)
DescriptionRecord UMA for Physical Web state
The "Physical Web state" refers to:
* Data connection
* BT permission/enablement
* Location permission/provider
We record this state:
* At chrome startup
* When launching the ListUrlsActivity from the diagnostics page
* When launching the ListUrlsActivity from the preferences page
BUG=594194
Committed: https://crrev.com/10cbc3ad60388fc48ebfbbd16c8d3db9872c009b
Cr-Commit-Position: refs/heads/master@{#386215}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use enums correctly in histograms.xml #
Total comments: 5
Patch Set 3 : Break states into distinct metrics #
Total comments: 4
Patch Set 4 : Use histogram_suffix #Patch Set 5 : Included utils.java in gn build #
Messages
Total messages: 37 (11 generated)
cco3@chromium.org changed reviewers: + mattreynolds@chromium.org
lgtm
cco3@chromium.org changed reviewers: + newt@chromium.org, rkaplow@chromium.org
https://codereview.chromium.org/1814213004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1814213004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:34952: +<histogram name="PhysicalWeb.State.ChromeStart" units="PhysicalWebStateAndroid"> this should be enum=
cco3@chromium.org changed reviewers: + nyquist@chromium.org
lgtm This seems ok, but it's not great that you need to encode so many variables in one bit of information. Are you sure we can't treat these as independent variables that we track separately? Using it this way makes it more difficult to use our internal tools. Would be good to learn your expected analysis. https://codereview.chromium.org/1814213004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1814213004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:77484: +<enum name="PhysicalWebStateAndroid" type="int"> can you add a description here showing what each of the subfields mean.
lgtm with a few suggestions. Though I also share Robert's feeling that it would be easier to understand this metric via the UMA dashboard if it were broken into 5 separate metrics. https://codereview.chromium.org/1814213004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/1814213004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:120: public static void onChromeStart(ChromeApplication application) { suggestion: It's nice to keep the public interface together, above the protected and private methods. https://codereview.chromium.org/1814213004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java (right): https://codereview.chromium.org/1814213004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:214: * Calculate a PhysicalWeb state. Probably worth describing this a bit more. What does this state contain?
I was just following the pattern we had set a long time ago for our iOS startup status reporting. I'll break it down into distinct metrics, thanks. On 2016/04/05 06:24:34, newt (no new reviews. mostly) wrote: > lgtm with a few suggestions. Though I also share Robert's feeling that it would > be easier to understand this metric via the UMA dashboard if it were broken into > 5 separate metrics. > > https://codereview.chromium.org/1814213004/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java > (right): > > https://codereview.chromium.org/1814213004/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:120: > public static void onChromeStart(ChromeApplication application) { > suggestion: It's nice to keep the public interface together, above the protected > and private methods. > > https://codereview.chromium.org/1814213004/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java > (right): > > https://codereview.chromium.org/1814213004/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:214: > * Calculate a PhysicalWeb state. > Probably worth describing this a bit more. What does this state contain?
https://codereview.chromium.org/1814213004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/1814213004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:120: public static void onChromeStart(ChromeApplication application) { On 2016/04/05 06:24:34, newt (no new reviews. mostly) wrote: > suggestion: It's nice to keep the public interface together, above the protected > and private methods. Done. https://codereview.chromium.org/1814213004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java (right): https://codereview.chromium.org/1814213004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:214: * Calculate a PhysicalWeb state. On 2016/04/05 06:24:34, newt (no new reviews. mostly) wrote: > Probably worth describing this a bit more. What does this state contain? Acknowledged.
Any chance I could get this in before the branch cut?
lgtm oops, forgot to send this comment. Would be good if you could get this change done but I wouldn't let it block you. You can fix it later if need be. https://codereview.chromium.org/1814213004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1814213004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:35462: +<histogram name="PhysicalWeb.State.ChromeStart.Bluetooth" this is much better and cleaner! The tools will actually work well now with the data. I would suggest using the histogram_suffix structure to make this section shorter and less error prone. You can specify "ChromeStart", "LaunchFromDiagnostics" and "LaunchFromPreferences" as your suffixes. (not the affix example so you can keep the names the same). This will make this list much shorter and easier to add to.
lgtm % comment https://codereview.chromium.org/1814213004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java (right): https://codereview.chromium.org/1814213004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:65: private static final String PREFERENCE = "PREFERENCE"; Why all caps (unlike all the others)? Would this make more sense to call "Enabled"?
rkaplow@, is this is what you had in mind? https://codereview.chromium.org/1814213004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java (right): https://codereview.chromium.org/1814213004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:65: private static final String PREFERENCE = "PREFERENCE"; On 2016/04/08 18:00:11, newt (no new reviews. mostly) wrote: > Why all caps (unlike all the others)? Would this make more sense to call > "Enabled"? Done. https://codereview.chromium.org/1814213004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1814213004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:35462: +<histogram name="PhysicalWeb.State.ChromeStart.Bluetooth" On 2016/04/08 17:54:21, rkaplow wrote: > this is much better and cleaner! The tools will actually work well now with the > data. > > I would suggest using the histogram_suffix structure to make this section > shorter and less error prone. > > You can specify "ChromeStart", "LaunchFromDiagnostics" and > "LaunchFromPreferences" as your suffixes. > > (not the affix example so you can keep the names the same). > This will make this list much shorter and easier to add to. Done.
lgtm
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1814213004/#ps60001 (title: "Use histogram_suffix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814213004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814213004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
On 2016/04/08 20:06:23, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) What do I need to do to include Utils.java for gn builds?
On 2016/04/08 20:12:17, cco3 wrote: > On 2016/04/08 20:06:23, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, > > > https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) > > What do I need to do to include Utils.java for gn builds? git grep for "PhysicalWebUma.java" (or any other Java file in chrome/android/java) and you'll see which GN file needs to be updated :) (Hint: it's chrome/android/java_sources.gni)
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org, rkaplow@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1814213004/#ps80001 (title: "Included utils.java in gn build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814213004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814213004/80001
On 2016/04/08 20:36:13, newt (no new reviews. mostly) wrote: > On 2016/04/08 20:12:17, cco3 wrote: > > On 2016/04/08 20:06:23, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, > > > > > > https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) > > > > What do I need to do to include Utils.java for gn builds? > > git grep for "PhysicalWebUma.java" (or any other Java file in > chrome/android/java) and you'll see which GN file needs to be updated :) > > (Hint: it's chrome/android/java_sources.gni) Duh, sorry. I just didn't recall needing to do this previously, so I figured something else other than registration was needed.
On 2016/04/08 20:40:56, cco3 wrote: > On 2016/04/08 20:36:13, newt (no new reviews. mostly) wrote: > > On 2016/04/08 20:12:17, cco3 wrote: > > > On 2016/04/08 20:06:23, commit-bot: I haz the power wrote: > > > > Try jobs failed on following builders: > > > > android_chromium_gn_compile_dbg on tryserver.chromium.android > (JOB_FAILED, > > > > > > > > > > https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) > > > > > > What do I need to do to include Utils.java for gn builds? > > > > git grep for "PhysicalWebUma.java" (or any other Java file in > > chrome/android/java) and you'll see which GN file needs to be updated :) > > > > (Hint: it's chrome/android/java_sources.gni) > > Duh, sorry. I just didn't recall needing to do this previously, so I figured > something else other than registration was needed. Explicitly listing Java files in java_sources.gni is new as of 3/23. Before that, you didn't need to update any build files when adding Java files. That's why :)
On 2016/04/08 20:47:16, newt (no new reviews. mostly) wrote: > On 2016/04/08 20:40:56, cco3 wrote: > > On 2016/04/08 20:36:13, newt (no new reviews. mostly) wrote: > > > On 2016/04/08 20:12:17, cco3 wrote: > > > > On 2016/04/08 20:06:23, commit-bot: I haz the power wrote: > > > > > Try jobs failed on following builders: > > > > > android_chromium_gn_compile_dbg on tryserver.chromium.android > > (JOB_FAILED, > > > > > > > > > > > > > > > https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) > > > > > > > > What do I need to do to include Utils.java for gn builds? > > > > > > git grep for "PhysicalWebUma.java" (or any other Java file in > > > chrome/android/java) and you'll see which GN file needs to be updated :) > > > > > > (Hint: it's chrome/android/java_sources.gni) > > > > Duh, sorry. I just didn't recall needing to do this previously, so I figured > > something else other than registration was needed. > > Explicitly listing Java files in java_sources.gni is new as of 3/23. Before > that, you didn't need to update any build files when adding Java files. That's > why :) Makes sense. Thanks for all the help!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by cco3@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814213004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814213004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Record UMA for Physical Web state The "Physical Web state" refers to: * Data connection * BT permission/enablement * Location permission/provider We record this state: * At chrome startup * When launching the ListUrlsActivity from the diagnostics page * When launching the ListUrlsActivity from the preferences page BUG=594194 ========== to ========== Record UMA for Physical Web state The "Physical Web state" refers to: * Data connection * BT permission/enablement * Location permission/provider We record this state: * At chrome startup * When launching the ListUrlsActivity from the diagnostics page * When launching the ListUrlsActivity from the preferences page BUG=594194 Committed: https://crrev.com/10cbc3ad60388fc48ebfbbd16c8d3db9872c009b Cr-Commit-Position: refs/heads/master@{#386215} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/10cbc3ad60388fc48ebfbbd16c8d3db9872c009b Cr-Commit-Position: refs/heads/master@{#386215} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
