Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1870523002:
Streamline UMA uploading (Closed)
DescriptionStreamline UMA uploading
Currently, to add a new metric, we need to update PhysicalWebUma.java
in at least five different locations. This change stores more
information about each metric so that less needs to be manually
specified.
BUG=601251
Committed: https://crrev.com/a22fbdcbe8ea5ce5edba1d777a5a9a2fa53a8fb7
Cr-Commit-Position: refs/heads/master@{#385831}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Only create default records when needed #
Total comments: 2
Patch Set 3 : Use newt@'s suggestion #
Total comments: 4
Patch Set 4 : Reorganize some methods #Messages
Total messages: 16 (5 generated)
cco3@chromium.org changed reviewers: + mattreynolds@chromium.org
https://codereview.chromium.org/1870523002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java (right): https://codereview.chromium.org/1870523002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:272: JSONObject record = new JSONObject(prefs.getString(key, String.format("{" Let's use null instead of supplying a default so we can avoid constructing the JSONObject when the pref is already defined.
lgtm
cco3@chromium.org changed reviewers: + newt@chromium.org
This seems reasonable, though the JSON manipulation is unfortunately verbose.
Here's an alternative (just a suggestion) that's much simpler. Starting from the
original code, make these changes:
- In storeCount(), storeEnum(), etc, set a shared pref that indicates that
UmaUploader needs to be run.
- Remove member variables from UmaUploader
- Delete UmaUploader.isEmpty()
- Read shared prefs inside UmaUploader, rather than in uploadDeferredMetrics()
- uploadDeferredMetrics() becomes very simple: just set sUploadAllowed = true,
check whether UmaLoader needs to be run (by reading the new shared pref), and
start UmaUploader if needed.
This satisfies your goal of reducing the number of occurrences of each metric,
and avoids the messiness of extra JSON manipulation.
Something like
private static void storeAction(Context context, String key) {
// As before
...
// With this added:
prefsEditor.putBoolean(PREF_HAS_DEFERRED_PREFS, true);
}
public static void uploadDeferredMetrics(Context context) {
sUploadAllowed = true;
if (... PREF_HAS_DEFERRED_PREFS is true) {
AsyncTask.THREAD_POOL_EXECUTOR.execute(new UmaUploader(prefs));
}
}
private static class UmaUploader implements Runnable {
private SharedPreferences mPrefs;
public UmaUploader(SharedPreferences prefs) {
mPrefs = prefs;
}
public void run() {
// Similar to before, but no json strings:
...
uploadActions(PREFS_LOCATION_GRANTED_COUNT);
uploadTimes(PWS_BACKGROUND_RESOLVE_TIMES, TimeUnit.MILLISECONDS);
uploadTimes(PWS_FOREGROUND_RESOLVE_TIMES, TimeUnit.MILLISECONDS);
...
// Also, clear PREF_HAS_DEFERRED_PREFS at the end
}
private static void uploadTimes(String metricName, final TimeUnit tu) {
// New line to read value from SharedPrefs:
String jsonTimesStr = mPrefs.getString(metricName, "[]");
Long[] times = parseJsonLongArray(jsonTimesStr);
if (times == null) {
Log.e(TAG, "Error reporting " + key + " with values: " +
jsonTimesStr);
return;
}
for (Long time : times) {
RecordHistogram.recordTimesHistogram(metricName, time,
TimeUnit.MILLISECONDS);
}
}
...
}
https://codereview.chromium.org/1870523002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java
(right):
https://codereview.chromium.org/1870523002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:302:
record.put(TIME_UNIT_KEY, tu.toString());
this should be tu.name(), right?
That worked out really well. Thanks for the suggestion. https://codereview.chromium.org/1870523002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java (right): https://codereview.chromium.org/1870523002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:302: record.put(TIME_UNIT_KEY, tu.toString()); On 2016/04/07 05:26:50, newt (no new reviews. mostly) wrote: > this should be tu.name(), right? toString() does the same thing. I didn't realize there was a .name()...it wasn't in the API I was looking at, but apparently it works.
Great :) lgtm % nits https://codereview.chromium.org/1870523002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java (right): https://codereview.chromium.org/1870523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:294: private void uploadActions(String key) { how about putting all the upload*() methods together, either above or below the parse*() methods? https://codereview.chromium.org/1870523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:299: removePref(key); nit: for consistency with the other upload*() methods, I'd remove the pref right after reading it
https://codereview.chromium.org/1870523002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java (right): https://codereview.chromium.org/1870523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:294: private void uploadActions(String key) { On 2016/04/07 17:43:37, newt (no new reviews. mostly) wrote: > how about putting all the upload*() methods together, either above or below the > parse*() methods? Done. https://codereview.chromium.org/1870523002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebUma.java:299: removePref(key); On 2016/04/07 17:43:37, newt (no new reviews. mostly) wrote: > nit: for consistency with the other upload*() methods, I'd remove the pref right > after reading it Done.
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/1870523002/#ps60001 (title: "Reorganize some methods")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1870523002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1870523002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Streamline UMA uploading Currently, to add a new metric, we need to update PhysicalWebUma.java in at least five different locations. This change stores more information about each metric so that less needs to be manually specified. BUG=601251 ========== to ========== Streamline UMA uploading Currently, to add a new metric, we need to update PhysicalWebUma.java in at least five different locations. This change stores more information about each metric so that less needs to be manually specified. BUG=601251 Committed: https://crrev.com/a22fbdcbe8ea5ce5edba1d777a5a9a2fa53a8fb7 Cr-Commit-Position: refs/heads/master@{#385831} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a22fbdcbe8ea5ce5edba1d777a5a9a2fa53a8fb7 Cr-Commit-Position: refs/heads/master@{#385831} |
