Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1155423008:
hosted: Add two histograms tracking "mayLaunchUrl" usage. (Closed)
Descriptionhosted: Add two histograms tracking "mayLaunchUrl" usage.
- "Hit rate": Whether the application predicted correctly.
- "Lead time": How much time between the prediction and the actual launch.
BUG=493170
Committed: https://crrev.com/c8ebbc48e976733f5981c66a795a0d3b924d232b
Cr-Commit-Position: refs/heads/master@{#333027}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address comments (replace several maps with one). #Patch Set 3 : More consistent naming. #Patch Set 4 : Rebase on https://codereview.chromium.org/1153363004 #
Total comments: 7
Patch Set 5 : Rebase and address comments. #
Total comments: 11
Patch Set 6 : Address comments. #Patch Set 7 : Rebase + Findbugs fix. #
Messages
Total messages: 23 (7 generated)
lizeb@chromium.org changed reviewers: + pasko@chromium.org, yusufo@chromium.org
Hello, A first "metrics" CL coming you way :-) PTAL.
https://codereview.chromium.org/1155423008/diff/1/chrome/android/java_staging... File chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java (right): https://codereview.chromium.org/1155423008/diff/1/chrome/android/java_staging... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:61: private final LongSparseArray<String> mSessionIdToPredictedUrl; The state logic is getting so complex,proper unit test coverage will become a must have pretty soon :). Just saying... https://codereview.chromium.org/1155423008/diff/1/chrome/android/java_staging... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:72: mSessionIdToPredictedUrl = new LongSparseArray<String>(); Looks like we are adding a few too many maps here. Since these are all per session Id, I think it is time we defined something like sessionIdParams and abstract all this inside that class. Then we can have a map from session id to that only.
Thank you for the review, I have collapsed the many maps to one. https://codereview.chromium.org/1155423008/diff/1/chrome/android/java_staging... File chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java (right): https://codereview.chromium.org/1155423008/diff/1/chrome/android/java_staging... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:61: private final LongSparseArray<String> mSessionIdToPredictedUrl; On 2015/06/01 14:33:08, Yusuf wrote: > The state logic is getting so complex,proper unit test coverage will become a > must have pretty soon :). Just saying... Acknowledged. https://codereview.chromium.org/1155423008/diff/1/chrome/android/java_staging... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:72: mSessionIdToPredictedUrl = new LongSparseArray<String>(); On 2015/06/01 14:33:08, Yusuf wrote: > Looks like we are adding a few too many maps here. > > Since these are all per session Id, I think it is time we defined something like > sessionIdParams and abstract all this inside that class. Then we can have a map > from session id to that only. Done.
https://codereview.chromium.org/1155423008/diff/60001/chrome/android/java_sta... File chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java (right): https://codereview.chromium.org/1155423008/diff/60001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:59: private static class SessionParams { I think we need a bit more structure here for accessing these. Lets make mUid final and have that be the only public field. The others should have getter and setter and be private since they can't be initialized in construction. We can have something like setPredictionMetrics for both mPredictedUrl and the timestamp. https://codereview.chromium.org/1155423008/diff/60001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:63: public long mLastMayLaunchUrl; mLastMayLaunchUrlTimeStamp https://codereview.chromium.org/1155423008/diff/60001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:72: mApplication = application; I know it doesn't have much to do with this CL, but I think we should just get the application context from ApplicationStatus and cast it to ChromiumApplication here to avoid getting the application in all getInstance calls. This will cleanup the ChromeBrowserConnection instances we have to keep here and there. Although it is a bit hacky, I feel like it will result in cleaner structure.
All done, thank you. https://codereview.chromium.org/1155423008/diff/60001/chrome/android/java_sta... File chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java (right): https://codereview.chromium.org/1155423008/diff/60001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:59: private static class SessionParams { On 2015/06/02 17:03:24, Yusuf wrote: > I think we need a bit more structure here for accessing these. Lets make mUid > final and have that be the only public field. The others should have getter and > setter and be private since they can't be initialized in construction. We can > have something like setPredictionMetrics for both mPredictedUrl and the > timestamp. Done. https://codereview.chromium.org/1155423008/diff/60001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:63: public long mLastMayLaunchUrl; On 2015/06/02 17:03:24, Yusuf wrote: > mLastMayLaunchUrlTimeStamp Done. https://codereview.chromium.org/1155423008/diff/60001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:72: mApplication = application; On 2015/06/02 17:03:24, Yusuf wrote: > I know it doesn't have much to do with this CL, but I think we should just get > the application context from ApplicationStatus and cast it to > ChromiumApplication here to avoid getting the application in all getInstance > calls. This will cleanup the ChromeBrowserConnection instances we have to keep > here and there. Although it is a bit hacky, I feel like it will result in > cleaner structure. I prefer to avoid casting Context into Application, as it is in no way guaranteed by the framework that this is possible, even though it does work in practice. Another way could be to add "getApplication" to ApplicationStatus, but this would probably be another CL. wdyt?
lgtm https://codereview.chromium.org/1155423008/diff/60001/chrome/android/java_sta... File chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java (right): https://codereview.chromium.org/1155423008/diff/60001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:72: mApplication = application; On 2015/06/03 09:29:01, lizeb wrote: > On 2015/06/02 17:03:24, Yusuf wrote: > > I know it doesn't have much to do with this CL, but I think we should just get > > the application context from ApplicationStatus and cast it to > > ChromiumApplication here to avoid getting the application in all getInstance > > calls. This will cleanup the ChromeBrowserConnection instances we have to keep > > here and there. Although it is a bit hacky, I feel like it will result in > > cleaner structure. > > I prefer to avoid casting Context into Application, as it is in no way > guaranteed by the framework that this is possible, even though it does work in > practice. > > Another way could be to add "getApplication" to ApplicationStatus, but this > would probably be another CL. > wdyt? OK. Let's leave this to another CL. Like I mentioned the main thing I am hoping to avoid is having a parameter in getInstance.
lizeb@chromium.org changed reviewers: + isherman@chromium.org
Hello isherman, Please review the histograms.xml change in this CL. Thank you.
https://codereview.chromium.org/1155423008/diff/80001/chrome/android/java_sta... File chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java (right): https://codereview.chromium.org/1155423008/diff/80001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:235: "CustomTabs.PredictionStatus", outcome, BAD_PREDICTION + 1); Using "BAD_PREDICTION + 1" as the bound is fragile. It would be easy for someone to add another possible outcome later on, without realizing that they need to update this particular line. Instead, I'd recommend defining an enum specifically for the values that can be emitted to this histogram, structured as "enum Foo {A, B, C, FOO_COUNT}", and passing FOO_COUNT as the boundary value. Also, please add documentation to the enum noting that it is used to back a histogram, and thus that it should be treated as append-only. https://codereview.chromium.org/1155423008/diff/80001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:238: elapsedTimeMs, 0, 3 * 60 * 1000, TimeUnit.MILLISECONDS, 100); nit: Technically, the minimum value for a time histogram is 1, rather than 0 -- bucket 0 is the underflow bucket. https://codereview.chromium.org/1155423008/diff/80001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:238: elapsedTimeMs, 0, 3 * 60 * 1000, TimeUnit.MILLISECONDS, 100); nit: Could you please spell "3 * 60 * 1000" as TimeUnit.SECONDS.toMillis(3)? That's much clearer for the reader. https://codereview.chromium.org/1155423008/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1155423008/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4410: + whether the call was later matched by a URL launch. When exactly is this recorded? I'm imagining that there might be some bias, wherein "No prediction" is recorded immediately, but the other two are only recorded after some -- potentially indefinite -- delay. If so, that would bias towards more "No prediction" records than others. https://codereview.chromium.org/1155423008/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4414: +<histogram name="CustomTabs.PredictionToLaunch" units="ms"> It looks like you're introducing a new group name, "CustomTabs", in the top-level namespace. This might be appropriate, but please consider whether there is an existing group name that makes sense to use. If there isn't, please consider whether the new group name strikes the right balance between being too general and too specific, so that it carves off a reasonably sized chunk of the namespace.
Thank you for the quick review! All done. https://codereview.chromium.org/1155423008/diff/80001/chrome/android/java_sta... File chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java (right): https://codereview.chromium.org/1155423008/diff/80001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:235: "CustomTabs.PredictionStatus", outcome, BAD_PREDICTION + 1); On 2015/06/03 22:07:32, Ilya Sherman wrote: > Using "BAD_PREDICTION + 1" as the bound is fragile. It would be easy for > someone to add another possible outcome later on, without realizing that they > need to update this particular line. Instead, I'd recommend defining an enum > specifically for the values that can be emitted to this histogram, structured as > "enum Foo {A, B, C, FOO_COUNT}", and passing FOO_COUNT as the boundary value. > Also, please add documentation to the enum noting that it is used to back a > histogram, and thus that it should be treated as append-only. You're right, added the boundary value and the comment. I'm not using a Java enum as it's against the recommended practices on Android (see http://developer.android.com/training/articles/memory.html#Overhead ). Done. https://codereview.chromium.org/1155423008/diff/80001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:238: elapsedTimeMs, 0, 3 * 60 * 1000, TimeUnit.MILLISECONDS, 100); On 2015/06/03 22:07:31, Ilya Sherman wrote: > nit: Could you please spell "3 * 60 * 1000" as TimeUnit.SECONDS.toMillis(3)? > That's much clearer for the reader. Done. https://codereview.chromium.org/1155423008/diff/80001/chrome/android/java_sta... chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java:238: elapsedTimeMs, 0, 3 * 60 * 1000, TimeUnit.MILLISECONDS, 100); On 2015/06/03 22:07:31, Ilya Sherman wrote: > nit: Technically, the minimum value for a time histogram is 1, rather than 0 -- > bucket 0 is the underflow bucket. Done. https://codereview.chromium.org/1155423008/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1155423008/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4410: + whether the call was later matched by a URL launch. On 2015/06/03 22:07:32, Ilya Sherman wrote: > When exactly is this recorded? I'm imagining that there might be some bias, > wherein "No prediction" is recorded immediately, but the other two are only > recorded after some -- potentially indefinite -- delay. If so, that would bias > towards more "No prediction" records than others. No, there is only one point in the code where this is recorded, and the recording happens at the same point in time regardless of the value being recorded. Thank you for the comment, it is useful to keep it in mind for other metrics. https://codereview.chromium.org/1155423008/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4414: +<histogram name="CustomTabs.PredictionToLaunch" units="ms"> On 2015/06/03 22:07:32, Ilya Sherman wrote: > It looks like you're introducing a new group name, "CustomTabs", in the > top-level namespace. This might be appropriate, but please consider whether > there is an existing group name that makes sense to use. If there isn't, please > consider whether the new group name strikes the right balance between being too > general and too specific, so that it carves off a reasonably sized chunk of the > namespace. Thank you for the comment. I looked at the top-level names in histogram, and none seem appropriate to me. "Custom Tabs" is a new feature (formerly "ChromePlate") that will require more histograms under it, and we want them separate in a group. It is Android-specific though, but Android is not a top-level group (nor Mobile). Does this work for you?
metrics lgtm https://codereview.chromium.org/1155423008/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1155423008/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4414: +<histogram name="CustomTabs.PredictionToLaunch" units="ms"> On 2015/06/04 11:16:29, lizeb wrote: > On 2015/06/03 22:07:32, Ilya Sherman wrote: > > It looks like you're introducing a new group name, "CustomTabs", in the > > top-level namespace. This might be appropriate, but please consider whether > > there is an existing group name that makes sense to use. If there isn't, > please > > consider whether the new group name strikes the right balance between being > too > > general and too specific, so that it carves off a reasonably sized chunk of > the > > namespace. > > Thank you for the comment. > I looked at the top-level names in histogram, and none seem appropriate to me. > "Custom Tabs" is a new feature (formerly "ChromePlate") that will require more > histograms under it, and we want them separate in a group. It is > Android-specific though, but Android is not a top-level group (nor Mobile). > > Does this work for you? Okay, fair enough. Having a top-level group for a new feature seems reasonable. Thanks.
The CQ bit was checked by lizeb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/1155423008/#ps100001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1155423008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by lizeb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1155423008/#ps120001 (title: "Rebase + Findbugs fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1155423008/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/c8ebbc48e976733f5981c66a795a0d3b924d232b Cr-Commit-Position: refs/heads/master@{#333027}
Message was sent while issue was closed.
lgtm! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
