Add metrics for omnibox Google searches that has no RLZ token for windows.

Add metrics for omnibox Google searches that has no RLZ token for windows.

This metric will give us idea whether we need to take further measures to
close the short gap between the time the RLZ token is first cached and
later used to perform searches.

BUG=71548
TEST=Manual.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=74430

[pastarmovj, 2011-02-09 10:57:17]

@Roger: After our discussion I tried to debug as much as possible into the
timing of RLZ fetch and query sendouts and now I am almost 100% convinced that
the chances we send out queries without RLZ are practically 0, Therefore I
propose that we first only introduce a metric for that and if we see that we
have too much missed queries to try to implement some other solution that will
slow down the browser.

Can you please review this change which introduces the aforementioned metric.

@Fumitoshi: I saw that you have introduced the code that calls the
GoogleURLTracker. So I though you would be the right person to ask if the place
I decided to hook my metrics is going to be called whenever the user issues a
search through the omnibox with Google? See autocomplete_edit.cc:378-382.

@Jochen: You can have a look at it too if you wish.

Thanks to all of you :)

[ukai, 2011-02-09 11:03:31]

On 2011/02/09 10:57:17, pastarmovj wrote:
> @Roger: After our discussion I tried to debug as much as possible into the
> timing of RLZ fetch and query sendouts and now I am almost 100% convinced that
> the chances we send out queries without RLZ are practically 0, Therefore I
> propose that we first only introduce a metric for that and if we see that we
> have too much missed queries to try to implement some other solution that will
> slow down the browser.
> 
> Can you please review this change which introduces the aforementioned metric.
> 
> @Fumitoshi: I saw that you have introduced the code that calls the
> GoogleURLTracker. So I though you would be the right person to ask if the
place
> I decided to hook my metrics is going to be called whenever the user issues a
> search through the omnibox with Google? See autocomplete_edit.cc:378-382.

LGTM.

> 
> @Jochen: You can have a look at it too if you wish.
> 
> Thanks to all of you :)

[(NOT FOR CODE REVIEWS), 2011-02-09 14:15:16]

Hi Julian,

Good to know that the chance of this happening is really really small.
 Adding a metric only for now to track this seems OK to me, as long as
we will have time to fix this if it turns out it is a problem.  If I
understand correctly:

- your initial change is already committed to trunk, but it has not
been pushed out to the beta or stable channel
- your initial change will be pushed out at the end of the current milestone

Is that correct?  If we commit this change today, will there be enough
time to analyze the metrics data to know that all is ok or not for the
current milestone?

Specifically about your code change here, would it be simpler to just
add a call to user metrics at line 304 in browser\rlz\rlz.cc like
this:

UserMetrics::RecordAction(UserMetricsAction("SearchWithoutRLZ"));

and then others call at 285 and 317 like this:

UserMetrics::RecordAction(UserMetricsAction("SearchWithRLZ"));

Thanks,
Roger

-



On Wed, Feb 9, 2011 at 05:57,  <pastarmovj@chromium.org> wrote:
> Reviewers: jochen, Roger Tawa, ukai,
>
> Message:
> @Roger: After our discussion I tried to debug as much as possible into the
> timing of RLZ fetch and query sendouts and now I am almost 100% convinced
> that
> the chances we send out queries without RLZ are practically 0, Therefore I
> propose that we first only introduce a metric for that and if we see that we
> have too much missed queries to try to implement some other solution that
> will
> slow down the browser.
>
> Can you please review this change which introduces the aforementioned
> metric.
>
> @Fumitoshi: I saw that you have introduced the code that calls the
> GoogleURLTracker. So I though you would be the right person to ask if the
> place
> I decided to hook my metrics is going to be called whenever the user issues
> a
> search through the omnibox with Google? See autocomplete_edit.cc:378-382.
>
> @Jochen: You can have a look at it too if you wish.
>
> Thanks to all of you :)
>
> Description:
> Add metrics for omnibox Google searches that has no RLZ token for windows.
>
> This metric will give us idea whether we need to take further measures to
> close the short gap between the time the RLZ token is first cached and
> later used to perform searches.
>
> BUG=71548
> TEST=Manual.
>
> Please review this at http://codereview.chromium.org/6465020/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/browser/autocomplete/autocomplete_edit.cc
>  M chrome/browser/search_engines/template_url.h
>  M chrome/browser/search_engines/template_url.cc
>  M chrome/tools/chromeactions.txt
>
>
> Index: chrome/browser/autocomplete/autocomplete_edit.cc
> diff --git a/chrome/browser/autocomplete/autocomplete_edit.cc
> b/chrome/browser/autocomplete/autocomplete_edit.cc
> index
>
f822d5e620460ab90de234de5718d95bbd9ed15d..d8681d77b8909ae71ab7c416ba726ce263cb4723
> 100644
> --- a/chrome/browser/autocomplete/autocomplete_edit.cc
> +++ b/chrome/browser/autocomplete/autocomplete_edit.cc
> @@ -373,8 +373,14 @@ void
> AutocompleteEditModel::AcceptInput(WindowOpenDisposition disposition,
>     const TemplateURL* default_provider =
>         profile_->GetTemplateURLModel()->GetDefaultSearchProvider();
>     if (default_provider && default_provider->url() &&
> -        default_provider->url()->HasGoogleBaseURLs())
> +        default_provider->url()->HasGoogleBaseURLs()) {
>       GoogleURLTracker::GoogleURLSearchCommitted();
> +#if defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)
> +      // TODO(pastarmovj): Remove these metrics once we have proven that
> (close
> +      // to) none searches that should have RLZ are sent out without one.
> +      default_provider->url()->CollectRLZMetrics();
> +#endif
> +    }
>   }
>   view_->OpenURL(match.destination_url, disposition, match.transition,
>                  alternate_nav_url, AutocompletePopupModel::kNoMatch,
> Index: chrome/browser/search_engines/template_url.cc
> diff --git a/chrome/browser/search_engines/template_url.cc
> b/chrome/browser/search_engines/template_url.cc
> index
>
f3d8682bde790751870eadb58f5945cfc6241683..24c2f6b5250458b6124982dc4d4218287dfe2fca
> 100644
> --- a/chrome/browser/search_engines/template_url.cc
> +++ b/chrome/browser/search_engines/template_url.cc
> @@ -9,13 +9,17 @@
>  #include "base/logging.h"
>  #include "base/string_number_conversions.h"
>  #include "base/utf_string_conversions.h"
> +#include "chrome/browser/metrics/user_metrics.h"
>  #include "chrome/browser/search_engines/search_engine_type.h"
>  #include "chrome/browser/search_engines/search_terms_data.h"
>  #include "chrome/browser/search_engines/template_url_model.h"
>  #include "chrome/common/url_constants.h"
> +#include "chrome/installer/util/google_update_settings.h"
>  #include "net/base/escape.h"
>  #include "ui/base/l10n/l10n_util.h"
>  #include "ui/gfx/favicon_size.h"
> +// TODO(pastarmovj): Remove google_update_settings and user_metrics when
> the
> +// CollectRLZMetrics function is not needed anymore.
>
>  // The TemplateURLRef has any number of terms that need to be replaced.
> Each of
>  // the terms is enclosed in braces. If the character preceeding the final
> @@ -518,6 +522,28 @@ bool TemplateURLRef::SameUrlRefs(const TemplateURLRef*
> ref1,
>   return ref1 == ref2 || (ref1 && ref2 && ref1->url() == ref2->url());
>  }
>
> +void TemplateURLRef::CollectRLZMetrics() const {
> +#if defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)
> +  ParseIfNecessary();
> +  for (size_t i = 0; i < replacements_.size(); ++i) {
> +    // We are interesed in searches that were supposed to send the RLZ
> token.
> +    if (replacements_[i].type == GOOGLE_RLZ) {
> +      string16 brand;
> +      // We only have RLZ tocken on a branded browser version.
> +      if (GoogleUpdateSettings::GetBrand(&brand) && !brand.empty() &&
> +           !GoogleUpdateSettings::IsOrganic(brand)) {
> +        // Now we know we should have had RLZ token check if there was one.
> +        if (url().find("rlz=") != std::string::npos)
> +          UserMetrics::RecordAction(UserMetricsAction("SearchWithRLZ"));
> +        else
> +          UserMetrics::RecordAction(UserMetricsAction("SearchWithoutRLZ"));
> +      }
> +      return;
> +    }
> +  }
> +#endif
> +}
> +
>  void TemplateURLRef::InvalidateCachedValues() const {
>   supports_replacements_ = valid_ = parsed_ = false;
>   host_.clear();
> Index: chrome/browser/search_engines/template_url.h
> diff --git a/chrome/browser/search_engines/template_url.h
> b/chrome/browser/search_engines/template_url.h
> index
>
44cdeec971fe0732914d602c32f03b28889658b0..0947d3f92496714e7e49277ef5e1f324a3bbbdc9
> 100644
> --- a/chrome/browser/search_engines/template_url.h
> +++ b/chrome/browser/search_engines/template_url.h
> @@ -132,6 +132,9 @@ class TemplateURLRef {
>   static bool SameUrlRefs(const TemplateURLRef* ref1,
>                           const TemplateURLRef* ref2);
>
> +  // Collects metrics whether searches through Google are sent with RLZ
> string.
> +  void CollectRLZMetrics() const;
> +
>  private:
>   friend class SearchHostToURLsMapTest;
>   friend class TemplateURL;
> Index: chrome/tools/chromeactions.txt
> diff --git a/chrome/tools/chromeactions.txt b/chrome/tools/chromeactions.txt
> index
>
63ee8d0d4b0cac1a7558401c02c242de3416385e..0845445b2449dff9df4a5bd2e5a4022d0250eb88
> 100644
> --- a/chrome/tools/chromeactions.txt
> +++ b/chrome/tools/chromeactions.txt
> @@ -1012,6 +1012,8 @@
>  0x0b8efe82e35086c2     ScreenLocker_Show
>  0xe643aadfeec1d6fc     ScreenLocker_Signout
>  0x61ce545b97b3f1cd     ScreenLocker_StartScreenSaver
> +0x7a0c1d1c5392ec8e     SearchWithRLZ
> +0x2e17e73bd11af537     SearchWithoutRLZ
>  0x702bbdc91c8e399d     SelectAll
>  0xd016dafd5f0eca55     SelectLastTab
>  0x64658593002b74ad     SelectLine
>
>
>

[pastarmovj, 2011-02-09 14:52:21]

Hi Roger,
Yes my first CL is on trunk, but I don't think my change will be out in
beta/stable on this milestone. At least I haven't made explicit request about
that and they are not really critical. The only problem is that you can't get
these metrics in the dev builds because the whole RLZ code only gets active in
chrome branded builds. What would you suggest to do about that?

As for your question regarding why I put it there - the reason is that
GetAccessPointRlz is being called a lot (at every change of the omnibar at
least) and if we have the metrics directly in rlz.cc we'll only know how many of
the RLZ calls failed until we got the token but not if they were used in a real
search query or were only used to update the possible query that the user will
send when he hits enter. The place I chose is the one that gets called only when
the user do start a search from the omnibar.

Julian

On 2011/02/09 14:15:16, rogerta1 wrote:
> Hi Julian,
> 
> Good to know that the chance of this happening is really really small.
>  Adding a metric only for now to track this seems OK to me, as long as
> we will have time to fix this if it turns out it is a problem.  If I
> understand correctly:
> 
> - your initial change is already committed to trunk, but it has not
> been pushed out to the beta or stable channel
> - your initial change will be pushed out at the end of the current milestone
> 
> Is that correct?  If we commit this change today, will there be enough
> time to analyze the metrics data to know that all is ok or not for the
> current milestone?
> 
> Specifically about your code change here, would it be simpler to just
> add a call to user metrics at line 304 in browser\rlz\rlz.cc like
> this:
> 
> UserMetrics::RecordAction(UserMetricsAction("SearchWithoutRLZ"));
> 
> and then others call at 285 and 317 like this:
> 
> UserMetrics::RecordAction(UserMetricsAction("SearchWithRLZ"));
> 
> Thanks,
> Roger

[Roger Tawa OOO till Jul 10th, 2011-02-09 16:51:39]

lgtm

Thanks for explanation Julian.  As for testing, please talk to Ananta, I think
she'll be able to point you to official builds that can be used.

On 2011/02/09 14:52:21, pastarmovj wrote:
> Hi Roger,
> Yes my first CL is on trunk, but I don't think my change will be out in
> beta/stable on this milestone. At least I haven't made explicit request about
> that and they are not really critical. The only problem is that you can't get
> these metrics in the dev builds because the whole RLZ code only gets active in
> chrome branded builds. What would you s
uggest to do about that?
> 
> As for your question regarding why I put it there - the reason is that
> GetAccessPointRlz is being called a lot (at every change of the omnibar at
> least) and if we have the metrics directly in rlz.cc we'll only know how many
of
> the RLZ calls failed until we got the token but not if they were used in a
real
> search query or were only used to update the possible query that the user will
> send when he hits enter. The place I chose is the one that gets called only
when
> the user do start a search from the omnibar.
> 
> Julian
> 
> On 2011/02/09 14:15:16, rogerta1 wrote:
> > Hi Julian,
> > 
> > Good to know that the chance of this happening is really really small.
> >  Adding a metric only for now to track this seems OK to me, as long as
> > we will have time to fix this if it turns out it is a problem.  If I
> > understand correctly:
> > 
> > - your initial change is already committed to trunk, but it has not
> > been pushed out to the beta or stable channel
> > - your initial change will be pushed out at the end of the current milestone
> > 
> > Is that correct?  If we commit this change today, will there be enough
> > time to analyze the metrics data to know that all is ok or not for the
> > current milestone?
> > 
> > Specifically about your code change here, would it be simpler to just
> > add a call to user metrics at line 304 in browser\rlz\rlz.cc like
> > this:
> > 
> > UserMetrics::RecordAction(UserMetricsAction("SearchWithoutRLZ"));
> > 
> > and then others call at 285 and 317 like this:
> > 
> > UserMetrics::RecordAction(UserMetricsAction("SearchWithRLZ"));
> > 
> > Thanks,
> > Roger

[gameovermylord.org, 2016-09-07 10:27:35]

On 2011/02/09 16:51:39, Roger Tawa wrote:
> lgtm
> 
> Thanks for explanation Julian.  As for testing, please talk to Ananta, I think
> she'll be able to point you to official builds that can be used.
> 
> On 2011/02/09 14:52:21, pastarmovj wrote:
> > Hi Roger,
> > Yes my first CL is on trunk, but I don't think my change will be out in
> > beta/stable on this milestone. At least I haven't made explicit request
about
> > that and they are not really critical. The only problem is that you can't
get
> > these metrics in the dev builds because the whole RLZ code only gets active
in
> > chrome branded builds. What would you s
> uggest to do about that?
> > 
> > As for your question regarding why I put it there - the reason is that
> > GetAccessPointRlz is being called a lot (at every change of the omnibar at
> > least) and if we have the metrics directly in rlz.cc we'll only know how
many
> of
> > the RLZ calls failed until we got the token but not if they were used in a
> real
> > search query or were only used to update the possible query that the user
will
> > send when he hits enter. The place I chose is the one that gets called only
> when
> > the user do start a search from the omnibar.
> > 
> > Julian
> > 
> > On 2011/02/09 14:15:16, rogerta1 wrote:
> > > Hi Julian,
> > > 
> > > Good to know that the chance of this happening is really really small.
> > >  Adding a metric only for now to track this seems OK to me, as long as
> > > we will have time to fix this if it turns out it is a problem.  If I
> > > understand correctly:
> > > 
> > > - your initial change is already committed to trunk, but it has not
> > > been pushed out to the beta or stable channel
> > > - your initial change will be pushed out at the end of the current
milestone
> > > 
> > > Is that correct?  If we commit this change today, will there be enough
> > > time to analyze the metrics data to know that all is ok or not for the
> > > current milestone?
> > > 
> > > Specifically about your code change here, would it be simpler to just
> > > add a call to user metrics at line 304 in browser\rlz\rlz.cc like
> > > this:
> > > 
> > > UserMetrics::RecordAction(UserMetricsAction("SearchWithoutRLZ"));
> > > 
> > > and then others call at 285 and 317 like this:
> > > 
> > > UserMetrics::RecordAction(UserMetricsAction("SearchWithRLZ"));
> > > 
> > > Thanks,
> > > Roger

[gameovermylord, 2016-09-07 10:29:35]

On 2016/09/07 10:27:35, gameovermylord.org wrote:
> On 2011/02/09 16:51:39, Roger Tawa wrote:
> > lgtm
> > 
> > Thanks for explanation Julian.  As for testing, please talk to Ananta, I
think
> > she'll be able to point you to official builds that can be used.
> > 
> > On 2011/02/09 14:52:21, pastarmovj wrote:
> > > Hi Roger,
> > > Yes my first CL is on trunk, but I don't think my change will be out in
> > > beta/stable on this milestone. At least I haven't made explicit request
> about
> > > that and they are not really critical. The only problem is that you can't
> get
> > > these metrics in the dev builds because the whole RLZ code only gets
active
> in
> > > chrome branded builds. What would you s
> > uggest to do about that?
> > > 
> > > As for your question regarding why I put it there - the reason is that
> > > GetAccessPointRlz is being called a lot (at every change of the omnibar at
> > > least) and if we have the metrics directly in rlz.cc we'll only know how
> many
> > of
> > > the RLZ calls failed until we got the token but not if they were used in a
> > real
> > > search query or were only used to update the possible query that the user
> will
> > > send when he hits enter. The place I chose is the one that gets called
only
> > when
> > > the user do start a search from the omnibar.
> > > 
> > > Julian
> > > 
> > > On 2011/02/09 14:15:16, rogerta1 wrote:
> > > > Hi Julian,
> > > > 
> > > > Good to know that the chance of this happening is really really small.
> > > >  Adding a metric only for now to track this seems OK to me, as long as
> > > > we will have time to fix this if it turns out it is a problem.  If I
> > > > understand correctly:
> > > > 
> > > > - your initial change is already committed to trunk, but it has not
> > > > been pushed out to the beta or stable channel
> > > > - your initial change will be pushed out at the end of the current
> milestone
> > > > 
> > > > Is that correct?  If we commit this change today, will there be enough
> > > > time to analyze the metrics data to know that all is ok or not for the
> > > > current milestone?
> > > > 
> > > > Specifically about your code change here, would it be simpler to just
> > > > add a call to user metrics at line 304 in browser\rlz\rlz.cc like
> > > > this:
> > > > 
> > > > UserMetrics::RecordAction(UserMetricsAction("SearchWithoutRLZ"));
> > > > 
> > > > and then others call at 285 and 317 like this:
> > > > 
> > > > UserMetrics::RecordAction(UserMetricsAction("SearchWithRLZ"));
> > > > 
> > > > Thanks,
> > > > Roger

[gameovermylord, 2016-09-07 10:37:16]

On 2016/09/07 10:29:35, gameovermylord wrote:
> On 2016/09/07 10:27:35, http://gameovermylord.org wrote:
> > On 2011/02/09 16:51:39, Roger Tawa wrote:
> > > lgtm
> > > 
> > > Thanks for explanation Julian.  As for testing, please talk to Ananta, I
> think
> > > she'll be able to point you to official builds that can be used.
> > > 
> > > On 2011/02/09 14:52:21, pastarmovj wrote:
> > > > Hi Roger,
> > > > Yes my first CL is on trunk, but I don't think my change will be out in
> > > > beta/stable on this milestone. At least I haven't made explicit request
> > about
> > > > that and they are not really critical. The only problem is that you
can't
> > get
> > > > these metrics in the dev builds because the whole RLZ code only gets
> active
> > in
> > > > chrome branded builds. What would you s
> > > uggest to do about that?
> > > > 
> > > > As for your question regarding why I put it there - the reason is that
> > > > GetAccessPointRlz is being called a lot (at every change of the omnibar
at
> > > > least) and if we have the metrics directly in rlz.cc we'll only know how
> > many
> > > of
> > > > the RLZ calls failed until we got the token but not if they were used in
a
> > > real
> > > > search query or were only used to update the possible query that the
user
> > will
> > > > send when he hits enter. The place I chose is the one that gets called
> only
> > > when
> > > > the user do start a search from the omnibar.
> > > > 
> > > > Julian
> > > > 
> > > > On 2011/02/09 14:15:16, rogerta1 wrote:
> > > > > Hi Julian,
> > > > > 
> > > > > Good to know that the chance of this happening is really really small.
> > > > >  Adding a metric only for now to track this seems OK to me, as long as
> > > > > we will have time to fix this if it turns out it is a problem.  If I
> > > > > understand correctly:
> > > > > 
> > > > > - your initial change is already committed to trunk, but it has not
> > > > > been pushed out to the beta or stable channel
> > > > > - your initial change will be pushed out at the end of the current
> > milestone
> > > > > 
> > > > > Is that correct?  If we commit this change today, will there be enough
> > > > > time to analyze the metrics data to know that all is ok or not for the
> > > > > current milestone?
> > > > > 
> > > > > Specifically about your code change here, would it be simpler to just
> > > > > add a call to user metrics at line 304 in browser\rlz\rlz.cc like
> > > > > this:
> > > > > 
> > > > > UserMetrics::RecordAction(UserMetricsAction("SearchWithoutRLZ"));
> > > > > 
> > > > > and then others call at 285 and 317 like this:
> > > > > 
> > > > > UserMetrics::RecordAction(UserMetricsAction("SearchWithRLZ"));
> > > > > 
> > > > > Thanks,
> > > > > Roger