Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2857333002:
[TTS] Write initial Tap-features to Ranker. (Closed)
|
Created:
3 years, 7 months ago by Donn Denman Modified:
3 years, 7 months ago CC:
chromium-reviews, twellington+watch_chromium.org, Roger McFarlane (Chromium), mdjones+watch_chromium.org, donnd+watch_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org, hamelphi, msramek Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[TTS] Write initial Tap-features to Ranker.
This is an initial write-to-Ranker change that just writes features
that we already have on-hand to validate the feature-write mechanism.
This introduces a new native class that does the actual feature writing
to Ranker. This class is connected to an existing Java feature collection
stub (implemented a few months ago).
All new code is behind a new flag, so nothing should change by default.
Still TODO: Tests.
BUG=676108
Review-Url: https://codereview.chromium.org/2857333002
Cr-Commit-Position: refs/heads/master@{#473234}
Committed: https://chromium.googlesource.com/chromium/src/+/1c0f254c93df392634578b2520fad79b6cf1003f
Patch Set 1 #Patch Set 2 : Updated code that writes to UKM to include a URL and reset the builder between records. #Patch Set 3 : Reverted changes to translate_ranker_impl.cc and changed DVLOG(0) to DVLOG(3). #
Total comments: 12
Patch Set 4 : Removed the Flush() call, and some other nits from Robert's review. Rebased, which restricts what … #Patch Set 5 : Added logging of the CS-model URL and updated console-logging output. #
Total comments: 15
Patch Set 6 : Updated based on reviews by Roger and Theresa. Some API and doc changes. Handle null base page UR… #
Total comments: 1
Patch Set 7 : Fix a FindBugs issue in ContextualSearchRankerLoggerImpl.java and rebase. #Patch Set 8 : Remove the metric that specifies what model to use -- we'll use Chrome version to determine that. #Patch Set 9 : Reworked ContextualSearchRankerLoggerImpl.java somewhat: assert that calling sequence is valid even… #
Total comments: 2
Patch Set 10 : Removed DVLOGs and rebased. #
Total comments: 2
Patch Set 11 : Added an entry for CS and metrics written to ukm.xml. #
Total comments: 1
Messages
Total messages: 69 (42 generated)
The CQ bit was checked by donnd@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...
donnd@chromium.org changed reviewers: + twellington@chromium.org
Theresa, Here's a first cut at a first implementation of writing code to Ranker via UKM. See the Issue description for some major TODOs. Just take a look at whatever level you feel inspired to look (no rush). I decided to send it out with the overall philosophy of "sooner and smaller is better". :-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
Description was changed from
==========
[TTS] Write initial Tap-features to Ranker.
This is an initial write-to-Ranker change that just writes features
that we already have on-hand to validate the feature-write mechanism.
This introduces a new native class that does the actual feature writing
to Ranker. This class is connected to an existing Java feature collection
stub (implemented a few months ago).
All new code is behind a new flag, so nothing should change by default.
Still TODO: pipe the base-page URL down to the UKM logging, and
make sure the details of that logging are correct.
Also TODO: Tests.
Also deprecate the following histograms and remove logging code:
- Search.ContextualSearchAllCapsResultsSeen
- Search.ContextualSearchResultsSeenSelectionWasUrl
BUG=676108
==========
to
==========
[TTS] Write initial Tap-features to Ranker.
This is an initial write-to-Ranker change that just writes features
that we already have on-hand to validate the feature-write mechanism.
This introduces a new native class that does the actual feature writing
to Ranker. This class is connected to an existing Java feature collection
stub (implemented a few months ago).
All new code is behind a new flag, so nothing should change by default.
Still TODO: make sure the details of this logging are correct.
Also TODO: Tests.
BUG=676108
==========
The CQ bit was checked by donnd@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 donnd@chromium.org
donnd@chromium.org changed reviewers: + rkaplow@chromium.org, rogerm@chromium.org
Robert, PTAL at contextual_search_ranker_logger_impl to see if the way we're logging to UKM seems reasonable. Roger, PTAL at contextual_search_ranker_logger_impl, and anything else that seems interesting. This is just a first-cut at logging to UKM for Ranker. If things look good I'll ask Theresa to do a review of the whole CL. Thanks!
The CQ bit was checked by donnd@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/05/10 02:58:42, Donn Denman wrote: > Robert, PTAL at contextual_search_ranker_logger_impl to see if the way we're > logging to UKM seems reasonable. > > Roger, PTAL at contextual_search_ranker_logger_impl, and anything else that > seems interesting. > > This is just a first-cut at logging to UKM for Ranker. If things look good I'll > ask Theresa to do a review of the whole CL. > > Thanks! rkaplow@ and rogerm@ friendly ping to PTAL if not too busy with i/o. I'd like to try to land this and start sending data through Dev asap.
https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/... File chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc (right): https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:16: ContextualSearchRankerLoggerImpl::ContextualSearchRankerLoggerImpl( to verify - when void destroy() is called from the Java, that will destruct this object instance, correct? https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:35: ukm::UkmService* ukm_service = g_browser_process->ukm_service(); where is g_browser_process from? https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:37: // TODO(donnd): what to do with the model URL? I'm actually not sure the difference between the model URL and the base URL - can you document this somewhere https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:46: int32_t source_id_ = ukm_service_->GetNewSourceID(); can remove type https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:74: ukm_service_->Flush(); I wouldn't use Flush - i think we actually want to make this private to UKM actually. https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:76: builder_ = ukm_service_->GetEntryBuilder(source_id_, "ContextualSearch"); this is fine to use when the interaction with CS is complete and you want to start a new Entry. Note we want the abstraction that an Entry should coorespond to a collection of related information - in your case, probably a single CS interaction. In this case, this is a new set of information, but it is against the same source_. WHen do we want this to be triggered, when CS is re-opened for example?
Description was changed from
==========
[TTS] Write initial Tap-features to Ranker.
This is an initial write-to-Ranker change that just writes features
that we already have on-hand to validate the feature-write mechanism.
This introduces a new native class that does the actual feature writing
to Ranker. This class is connected to an existing Java feature collection
stub (implemented a few months ago).
All new code is behind a new flag, so nothing should change by default.
Still TODO: make sure the details of this logging are correct.
Also TODO: Tests.
BUG=676108
==========
to
==========
[TTS] Write initial Tap-features to Ranker.
This is an initial write-to-Ranker change that just writes features
that we already have on-hand to validate the feature-write mechanism.
This introduces a new native class that does the actual feature writing
to Ranker. This class is connected to an existing Java feature collection
stub (implemented a few months ago).
All new code is behind a new flag, so nothing should change by default.
Still TODO: Tests.
BUG=676108
==========
Robert, thanks for the review!!!! https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/... File chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc (right): https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:16: ContextualSearchRankerLoggerImpl::ContextualSearchRankerLoggerImpl( On 2017/05/11 19:46:49, rkaplow wrote: > to verify - when void destroy() is called from the Java, that will destruct this > object instance, correct? That's correct. The java nativeDestroy method calls the native Destroy method in this class, which calls the destructor explicitly. https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:35: ukm::UkmService* ukm_service = g_browser_process->ukm_service(); On 2017/05/11 19:46:49, rkaplow wrote: > where is g_browser_process from? Evidently this is a Chrome global, included by browser_process.h (https://cs.chromium.org/search/?q=g_browser_process&sq=package:chromium&type=cs). I cribbed this from the translate UKM code. https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:37: // TODO(donnd): what to do with the model URL? On 2017/05/11 19:46:49, rkaplow wrote: > I'm actually not sure the difference between the model URL and the base URL - > can you document this somewhere Done by adding param descriptions to this header file for SetUkmServiceAndUrls. https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:46: int32_t source_id_ = ukm_service_->GetNewSourceID(); On 2017/05/11 19:46:49, rkaplow wrote: > can remove type Nice catch, thanks! Done. https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:74: ukm_service_->Flush(); On 2017/05/11 19:46:49, rkaplow wrote: > I wouldn't use Flush - i think we actually want to make this private to UKM > actually. OK, I'm not sure I understand the nuances of this, but removed the Flush() call. https://codereview.chromium.org/2857333002/diff/40001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:76: builder_ = ukm_service_->GetEntryBuilder(source_id_, "ContextualSearch"); On 2017/05/11 19:46:49, rkaplow wrote: > this is fine to use when the interaction with CS is complete and you want to > start a new Entry. Note we want the abstraction that an Entry should coorespond > to a collection of related information - in your case, probably a single CS > interaction. > > In this case, this is a new set of information, but it is against the same > source_. WHen do we want this to be triggered, when CS is re-opened for example? That's right, this method should be called each time the overlay panel goes away and we've collected a full record of that single user-interaction. I'm not clear if we should ever switch to a different source, except as we do now when we exit/restart Chrome.
donnd@chromium.org changed reviewers: - rogerm@chromium.org
Theresa, PTAL -- this should be ready for a full review now.
https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:147: mTapSuppressionRankerLogger.writeLogAndReset(); Why was just this line pulled out of the if (mRankerLogExperiments != null) block? https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java:29: if (mBasePageUrl != null) { Won't mBasePageUrl will always be null in the constructor? https://codereview.chromium.org/2857333002/diff/80001/chrome/browser/android/... File chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:39: base::HashMetricName(model_url)); Should we change model URL to model name or something similar to help distinguish it from the base_page_url? https://codereview.chromium.org/2857333002/diff/80001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:77: DVLOG(0) << "Creating a new record builder"; We should probably remove these before landing since they're just for debugging, right? https://codereview.chromium.org/2857333002/diff/80001/chrome/browser/android/... File chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.h (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.h:27: // Sets up the UKM service to log contextual Search features for the given nit: s/contextual/Contextual
rogerm@chromium.org changed reviewers: + rogerm@chromium.org
https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java:68: nativeLogString(mNativePointer, feature.toString(), value.toString()); Do we have go ahead to do string logging like this? Maybe hold off on this particular bit of functionality until we've identified a specific use case and gotten privacy's sign-off.
Roger and Theresa, PTAL, and THANKS for the reviews! https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:147: mTapSuppressionRankerLogger.writeLogAndReset(); On 2017/05/12 01:02:25, Theresa wrote: > Why was just this line pulled out of the if (mRankerLogExperiments != null) > block? I noticed that we log to Ranker in multiple places in this method, including line 126 above, and so it might be possible to not complete a record if there are no mRankerLoggerExperiments set but we did have mWasQuickActionClicked. Switched to a model where we must always call writeLogAndReset when ending search, and have the RankerLogger keep track internally of whether anything was written and and in this case start a new record. https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java:29: if (mBasePageUrl != null) { On 2017/05/12 01:02:25, Theresa wrote: > Won't mBasePageUrl will always be null in the constructor? You're right, we can remove this section. Done. https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java:53: if (mBasePageUrl != null) { I noticed that this is null for URIs that are not proper URLs, like chrome://flags. Updated this class to separately track whether the setup has been done, and whether we can actually log (which is false for these URIs that are not URLs). https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLoggerImpl.java:68: nativeLogString(mNativePointer, feature.toString(), value.toString()); On 2017/05/12 15:43:26, Roger McFarlane (Chromium) wrote: > Do we have go ahead to do string logging like this? > > Maybe hold off on this particular bit of functionality until we've identified a > specific use case and gotten privacy's sign-off. Good point! Removed all string logging from this class and its associated native class. https://codereview.chromium.org/2857333002/diff/80001/chrome/browser/android/... File chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:39: base::HashMetricName(model_url)); On 2017/05/12 01:02:25, Theresa wrote: > Should we change model URL to model name or something similar to help > distinguish it from the base_page_url? Good suggestion -- I think I like "model selector". Done. https://codereview.chromium.org/2857333002/diff/80001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:77: DVLOG(0) << "Creating a new record builder"; On 2017/05/12 01:02:25, Theresa wrote: > We should probably remove these before landing since they're just for debugging, > right? I think they're OK since they're debug-only, and the feature is off by default. But let me know if that doesn't sound fine to you! https://codereview.chromium.org/2857333002/diff/80001/chrome/browser/android/... File chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.h (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/browser/android/... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.h:27: // Sets up the UKM service to log contextual Search features for the given On 2017/05/12 01:02:25, Theresa wrote: > nit: s/contextual/Contextual Done. https://codereview.chromium.org/2857333002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLogger.java (right): https://codereview.chromium.org/2857333002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchRankerLogger.java:36: void setupLoggingForPage(URL basePageUrl); Updated this method name and description to reflect that setup must be done each time before logging and writeLogAndReset done each time after all logging for that user-interaction.
The CQ bit was checked by donnd@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by donnd@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...
donnd@chromium.org changed reviewers: + msramek@chromium.org
cc: Martin Šrámek for Chrome Privacy.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Reviewers, I plan to make some minor changes to this CL so don't bother reviewing until after I ask for another PTAL. Sorry for the thrash!
Martin, Please review for Chrome Privacy. Everything that is logged is listed in ContextualSearchRankerLogger.java and you should be able to match it to fields documented in go/cs-ranker-ukm go/ukm-cs. Note that all UKM writing is done in contextual_search_ranker_logger_imp.cc using AddMetric in ContextualSearchRankerLoggerImpl::LogLong -- the ability to write string data has been removed. No page-content is logged. Don't hesitate to ping me if you have questions!
Thanks. I'll leave the detailed review to the UKM owners. From privacy side, I looked at enum Feature and the way types are restricted in ContextualSearchRankerLoggerImpl and that LGTM.
The CQ bit was checked by donnd@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: This issue passed the CQ dry run.
The CQ bit was checked by donnd@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...
All, I think this is ready for a last round of reviews. PTAL in the next 24 hours if you can. I removed the ability to write string-features and dropped writing of the Ranker "model selector" since we can just use the Chrome version number and channel. I also reworked ContextualSearchRankerLoggerImpl.java to be more robust with asserts and not call native when nothing has actually been logged. Robert, PTAL at contextual_search_ranker_logger_impl.h/cc. I think your comments have been addressed. Roger, I made the changes you suggested (removed the ability to write strings and dropped writing of the model-selector). Take another look if you'd like. Theresa, Please take a look at ContextualSearchRankerLoggerImpl.java - everything else is pretty much the same as when you last looked except what's mentioned here. Thanks everyone!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
donnd@chromium.org changed reviewers: + tedchoc@chromium.org
Ted, PTAL at chrome_jni_registrar.cc. All Reviewers, I plan to try to land this tomorrow morning unless there are additional changes needed.
On 2017/05/17 22:24:26, Donn Denman wrote: > Ted, PTAL at chrome_jni_registrar.cc. > > All Reviewers, I plan to try to land this tomorrow morning unless there are > additional changes needed. chrome_jni_registrar.cc - lgtm, but you'll want to wait for Theresa's sign off for the remainder of the android code
lgtm https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:147: mTapSuppressionRankerLogger.writeLogAndReset(); On 2017/05/12 23:08:55, Donn Denman wrote: > On 2017/05/12 01:02:25, Theresa wrote: > > Why was just this line pulled out of the if (mRankerLogExperiments != null) > > block? > > I noticed that we log to Ranker in multiple places in this method, including > line 126 above, and so it might be possible to not complete a record if there > are no mRankerLoggerExperiments set but we did have mWasQuickActionClicked. > > Switched to a model where we must always call writeLogAndReset when ending > search, and have the RankerLogger keep track internally of whether anything was > written and and in this case start a new record. Will you please add a code comment with some of these details? https://codereview.chromium.org/2857333002/diff/160001/chrome/browser/android... File chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc (right): https://codereview.chromium.org/2857333002/diff/160001/chrome/browser/android... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:41: DVLOG(0) << "ctxs Setting up UKM for url: " << page_url.spec(); I think these DVLOG() statements should be pulled out before landing.
Thanks Theresa! https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java (right): https://codereview.chromium.org/2857333002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:147: mTapSuppressionRankerLogger.writeLogAndReset(); On 2017/05/18 15:35:11, Theresa wrote: > On 2017/05/12 23:08:55, Donn Denman wrote: > > On 2017/05/12 01:02:25, Theresa wrote: > > > Why was just this line pulled out of the if (mRankerLogExperiments != null) > > > block? > > > > I noticed that we log to Ranker in multiple places in this method, including > > line 126 above, and so it might be possible to not complete a record if there > > are no mRankerLoggerExperiments set but we did have mWasQuickActionClicked. > > > > Switched to a model where we must always call writeLogAndReset when ending > > search, and have the RankerLogger keep track internally of whether anything > was > > written and and in this case start a new record. > > Will you please add a code comment with some of these details? Done. https://codereview.chromium.org/2857333002/diff/160001/chrome/browser/android... File chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc (right): https://codereview.chromium.org/2857333002/diff/160001/chrome/browser/android... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:41: DVLOG(0) << "ctxs Setting up UKM for url: " << page_url.spec(); On 2017/05/18 15:35:11, Theresa wrote: > I think these DVLOG() statements should be pulled out before landing. Done.
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, twellington@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2857333002/#ps180001 (title: "Removed DVLOGs and rebased.")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Robert, PTAL. Approval needed for ukm_service.h. Thanks!
https://codereview.chromium.org/2857333002/diff/180001/chrome/browser/android... File chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc (right): https://codereview.chromium.org/2857333002/diff/180001/chrome/browser/android... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:43: builder_ = ukm_service_->GetEntryBuilder(source_id_, "ContextualSearch"); can you register the UKM entry name and features in the ukm.xml https://cs.chromium.org/chromium/src/tools/metrics/ukm/ukm.xml?q=ukm.xml+pack...
Robert, PTAL. https://codereview.chromium.org/2857333002/diff/180001/chrome/browser/android... File chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc (right): https://codereview.chromium.org/2857333002/diff/180001/chrome/browser/android... chrome/browser/android/contextualsearch/contextual_search_ranker_logger_impl.cc:43: builder_ = ukm_service_->GetEntryBuilder(source_id_, "ContextualSearch"); On 2017/05/18 18:33:13, rkaplow wrote: > can you register the UKM entry name and features in the ukm.xml > > https://cs.chromium.org/chromium/src/tools/metrics/ukm/ukm.xml?q=ukm.xml+pack... Sorry about totally missing this! Done.
rkaplow@, Hopefully you're OK with landing this first cut and doing some iteration. If you'd rather try to get it right the first time I can do another spin at this CL but my preference is to land as-is and iterate. https://codereview.chromium.org/2857333002/diff/200001/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2857333002/diff/200001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:175: <metric name="DURATION_BEFORE_SCROLL_MS"> One quick comment -- these upper-case names seem very ugly to me, but we can update them in a subsequent CL. I plan to make several additions and some changes so this can be done soon. In fact I plan on removing this specific feature since it's not actionable by Ranker.
lgtm
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, twellington@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2857333002/#ps200001 (title: "Added an entry for CS and metrics written to ukm.xml.")
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": 200001, "attempt_start_ts": 1495208677382470,
"parent_rev": "fbbd5ac1076958be90b1b127132552887be2f76b", "commit_rev":
"1c0f254c93df392634578b2520fad79b6cf1003f"}
Message was sent while issue was closed.
Description was changed from
==========
[TTS] Write initial Tap-features to Ranker.
This is an initial write-to-Ranker change that just writes features
that we already have on-hand to validate the feature-write mechanism.
This introduces a new native class that does the actual feature writing
to Ranker. This class is connected to an existing Java feature collection
stub (implemented a few months ago).
All new code is behind a new flag, so nothing should change by default.
Still TODO: Tests.
BUG=676108
==========
to
==========
[TTS] Write initial Tap-features to Ranker.
This is an initial write-to-Ranker change that just writes features
that we already have on-hand to validate the feature-write mechanism.
This introduces a new native class that does the actual feature writing
to Ranker. This class is connected to an existing Java feature collection
stub (implemented a few months ago).
All new code is behind a new flag, so nothing should change by default.
Still TODO: Tests.
BUG=676108
Review-Url: https://codereview.chromium.org/2857333002
Cr-Commit-Position: refs/heads/master@{#473234}
Committed:
https://chromium.googlesource.com/chromium/src/+/1c0f254c93df392634578b2520fa...
==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/1c0f254c93df392634578b2520fa... |
