Closed Bug 1306639 Opened 8 years ago Closed 8 years ago

Searching in locationbar by typing something and pressing enter is not accounted in telemetry.

Categories

(Firefox :: Address Bar, defect, P1)

51 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 52
Tracking Status
firefox49 --- unaffected
firefox50 - unaffected
firefox51 --- verified
firefox52 --- verified

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(2 files)

basically, typing something in the location bar and pressing Enter, doesn't increase search/urlbar nor SEARCH_COUNTS. This is because in that case selection is -1 and thus we don't have an action uri.

I think we really need a browser_urlbar_telemetry.js test that verifies the various actions properly count searches, or most of our search telemetry may be broken. Not sure how hard that may be, but we really need to not regress this.

The operations to test are:
1. type something that will execute a search ENTER
2. Enter on a search suggestion from the popup
3. click on a search suggestion in the popup
4. enter on a one-off button
5. click on a one-off button
6. one-off search settings button should not increase the counter

Also keywords (bookmarks and search engine aliases), we may double count them, so it's worth testing those too.

In the shied study I'll have to somehow workaround this bug.
Also, selecting a url that looks like a search, increases the searches count and also things like search/autocomplete-other, search/autocomplete-default, that are logged by BrowserUITelemetry _logAwesomeBarSearchResult
Since it's not an actual search but a revisit of an old search, it should not increase search/urlbar, but be counted apart, I think?
in comment 1 case, we don't increase SEARCH_COUNTS. That sounds correct. SEARCH_COUNTS should equal the sum of:
search/ [ "abouthome","contextmenu","newtab","searchbar","urlbar" ]
looks like it may be a recent regression, maybe due to oneoffs? I just tried FF 50 and it seems to be working fine.
It's plausible, even likely...  The one-offs patch touched the URL-loading paths.
hm, the current beta I'm testing doesn't include bug 1304027 yet.
So it's possible this will affect 50 too. That would be a problem for our shield study.
I must test the newer beta.
[Tracking Requested - why for this release]:

Ok confirmed this is also a bug in beta4.
We really need a fix for this since we want to run a Shield Study on this data.
Probably it's not strictly a regression from our changes, it could also happen before but intermittently. The problem is that when we set the preselected index, we don't respect completeSelectedIndex. When in enterMatch we try to complete to the finalValue, we don't know that index 0 is selected but not completed yet, so we don't see the moz-action url, we just see the typed text.
Blocks: 1308211
(In reply to Marco Bonardo [::mak] from comment #1)
> Also, selecting a url that looks like a search, increases the searches count
> and also things like search/autocomplete-other, search/autocomplete-default,
> that are logged by BrowserUITelemetry _logAwesomeBarSearchResult
> Since it's not an actual search but a revisit of an old search, it should
> not increase search/urlbar, but be counted apart, I think?

Is this for loading a search page from history or re-requesting a search page?
If the latter, then we may need to count this for matching partner data.
(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> (In reply to Marco Bonardo [::mak] from comment #1)
> > Also, selecting a url that looks like a search, increases the searches count
> > and also things like search/autocomplete-other, search/autocomplete-default,
> > that are logged by BrowserUITelemetry _logAwesomeBarSearchResult
> > Since it's not an actual search but a revisit of an old search, it should
> > not increase search/urlbar, but be counted apart, I think?
> 
> Is this for loading a search page from history or re-requesting a search
> page?
> If the latter, then we may need to count this for matching partner data.

Per IRC, yes, re-requesting search pages is not counted.
This is expected and the same behavior like re-visiting any other page from history.
as usual, I must look into browser_canonizeURL.js. I'm starting hating that test :)
ah, this time the test is right, the awesomebar doesn't know about shift+Enter canonization, it will keep thinking we are doing a search. The right thing to do would be to update the first result when shift is pressed to properly indicate the action... but that may be complex and hard to uplift. I wonder if I could use nsIAutocompletePopup::overrideValue instead and file a follow-up to fix the heuristic entry.
filed bug 1308716 for canonizeURL, but that's not a regression.
the fact now we respect the heuristic result, this starts exposing bugs in unifiedcomplete, that in general is a positive thing so we can fix them. For example typing "http://example" shows a "search" result but does a visit instead.

I'm slightly changing the behavior of things like "mozilla/something" currently we require whitelisting the "mozilla" domain to consider this a url, otherwise we make a search, but it doesn't seem to make a lot of sense. it makes sense if the user types "mozilla" and it's not whitelisted, but not for more complex strings that urifixup can fix to a uri.
OK, while we wait for a Try run, let me explain some of this.

I started from the fact even if we have completeSelectedIndex, the preselected entry never completes the value, cause we update the selectedIndex in the frontend and the controller doesn't know about that.
Due to that, the frontend load code doesn't get the expected moz-action uri and we can't collect proper telemetry (we don't know if that will search or visit).

I introduced an initiallySelectedIndex for that purpose, that sets the selectedIndex but also updates the internal controller tracking. By doing so, we now always respect the moz-action url (that means in the future we can simplify some code paths like getShortcutOrURI).

The problem is that doing so breaks canonizeURL (SHIFT+ENTER), cause it wants to override the input value, but now we have an action url and bail our earlier.
So I made canonizeURL use nsIAutocompletePopup.overrideValue, since in the end this is an override, I think this is a better approach than what we had.

The second problem is that respecting what unifiedcomplete wants to do, has uncovered cases where it was suggesting the wrong thing. For things like "http://mozilla" unifiedcomplete thought we wanted to search, but the docshell instead tries to load them. This was due to some "smart" code in unifiedcomplete, that is not needed at all, fixupURI does a good job by itself.

Finally, with the visiturl moz-action we pass the fixedURI, this would reach the docshell, that in case of dns failure would try an alternate uri. Unfortunately we already fixed it, so it would try to fix a fixed uri. While the result may be fine (debatable), it would be a behavior change we don't want without a discussion. So I modified the frontend code to pass over the original input instead. The docshell will end up having to fix the uri again, as it was before, but the behavior stays the same.
the only remaining failure on try makes completely sense, it's other 2 cases where we were hinting the wrong action, we thought it was a search, but the browser actually does a visit, and only later fallbacks to a search.
I don't feel the need to track this for Fx50 (as there is no clear end-user benefit here). I will take a low-risk uplift soon. We will mostly enter code freeze mode in a week, ~50.9b8.
Comment on attachment 8798885 [details]
Bug 1306639 - Searching in locationbar by typing something and pressing enter is not accounted in telemetry.

https://reviewboard.mozilla.org/r/84270/#review83956

LGTM.  I like the idea of codifying an initially selected index.
Attachment #8798885 - Flags: review?(adw) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/70d19ed29c98
Searching in locationbar by typing something and pressing enter is not accounted in telemetry. r=adw
https://hg.mozilla.org/mozilla-central/rev/70d19ed29c98
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
This change broke the Universal Search Test Pilot add-on:

https://github.com/mozilla/universal-search/issues/322
Thanks for the heads up, I hope it can be fixed easily, we need to uplift this change or all of the telemetry we use to take decisions about search and locationbar is messed up.
Comment on attachment 8798885 [details]
Bug 1306639 - Searching in locationbar by typing something and pressing enter is not accounted in telemetry.

Approval Request Comment
[Feature/regressing bug #]: awesomebar perf/behavior fixes (bug 1304027)
[User impact if declined]: The impact is indirect, since the telemetry we use to make decisions will be completely wrong, so we could rely on bogus data. We plan to run a Shield Study using this data in Firefox 50, with this regression it should be delayed to Late January and would miss its goal.
[Describe test coverage new/current, TreeHerder]: nightly, QE ongoing
[Risks and why]: the only major change is that the awesomebar has a bit more control over the kind of load that will happen, but I added a bunch of additional automated tests and I did a lot of manual testing. QE is also ongoing on Nightly (will likely complete on Monday)
[String/UUID change made/needed]: one backwards compatible change to nsIAutoCompleteController.
Attachment #8798885 - Flags: approval-mozilla-beta?
Attachment #8798885 - Flags: approval-mozilla-aurora?
Depends on: 1310737
Comment on attachment 8798885 [details]
Bug 1306639 - Searching in locationbar by typing something and pressing enter is not accounted in telemetry.

Let's stabilize this on Aurora51 before we uplift to 50.0b9 on Thursday (it is already too late for this kind of change :( )
Attachment #8798885 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Mak, have you verified that this fix works as expected based on Nightly telemetry data? Which probes should be used for verification? Just curious. Thanks!
Flags: needinfo?(mak77)
Cristian Comorasu is testing the patch in nightly and checking telemetry. He should have results tomorrow.
We use both UITelemetry "countables" and SEARCH_COUNTS keyed histogram, I verified both on my side, Cristin is also verifying both.

The only problem we found so far is bug 1310737, that Francesco filed today. Looks like this patch will break some kind of bookmark keywords, specifically the ones using an uppercase %s (that would be trivial and safe to fix even in a late beta, since it's just a string search missing a case insensitive comparison) and POST ones. POST keywords are more complex to fix and that fix could not be uplifted.
I don't know how much that may influence the decision here, since it's a minor feature and the most common case works. While I have some telemetry about keywords usage (very low) I don't have telemetry about how many keywords use POST in percentage, I assume it's a very tiny fraction. But it would still be a minor bug we'd uplift.
Flags: needinfo?(mak77)
What I will try to do is a beta specific patch that only touches the broken telemetry, so there's no risk of regressions. on 51 and 51 we'll instead fix the keywords regression so we have the "proper" code. I'll try to have it ready today.
I have filed bug 1310953 about a regression in bookmarklets with "%s". I am not completely sure whether it is related to this bug, but it seemed the most likely candidate from the pushlog. Sorry for the spam if it is not.
(In reply to Jan Moesen from comment #32)
> I have filed bug 1310953 about a regression in bookmarklets with "%s". I am
> not completely sure whether it is related to this bug, but it seemed the
> most likely candidate from the pushlog. Sorry for the spam if it is not.

Thanks, I've duped it to the already known keywords regression, since it's managed by the same code. A fix is in the workings.
Attachment #8798885 - Flags: approval-mozilla-beta?
Attached patch Beta patchSplinter Review
Drew, care to have a quick look at this?

Approval Request Comment
[Feature/regressing bug #]: awesomebar perf/behavior fixes (bug 1304027)
[User impact if declined]: The impact is indirect, since the telemetry we use to make decisions will be completely wrong, so we could rely on bogus data. We plan to run a Shield Study using this data in Firefox 50, with this regression it should be delayed to Late January and would miss its goal.
[Describe test coverage new/current, TreeHerder]: automated test
[Risks and why]: This patch is a workaround that only tries to add telemetry, it should have no impact on behavior.
[String/UUID change made/needed]: none
Attachment #8802104 - Flags: review?(adw)
Attachment #8802104 - Flags: approval-mozilla-beta?
I reproduced this bug using Fx 52.0a1, build ID: 20160930030315, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 52.0a1, build ID: 20161017030209 and Fx 51.0a2, build ID: 20161018004015, on Windows 10 x64, Mac OS X 10.12 and Ubuntu 14.04 LTS.
I also ran a test suite to verify if this fix caused any other issues to resurface, every test was passed. 

Cheers!
Attachment #8802104 - Flags: review?(adw) → review+
Comment on attachment 8802104 [details] [diff] [review]
Beta patch

Telemetry data was verified on Nightly52, seems safe to uplift to Beta50.
Attachment #8802104 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I'll look into it, I had run the test on Beta, some other uplifted changes may have touched this path. It shouldn't be a big deal, it's just the test that this same patch is expected to touch.
Depends on: 1312018
Depends on: 1312167
Good news, we don't need this fix in Beta.

From the beginning this has been caused by a confusing code path due to the old MOZ_TELEMETRY_REPORTING define (recently renamed but was already there before). That define was added lots of time ago when telemetry was initially introduced, and today doesn't make any sense. We have only 2 points in browser that use it, and one of those points is in nsBrowserGlue where the "keyword-search" observer is added only when it's defined.
http://searchfox.org/mozilla-central/rev/84075be5067b68dc6cb3b89f999645650e68c05b/browser/components/nsBrowserGlue.js#465
Now the problem is that such define is not defined by local developer builds, and that's why nor me, nor Alessio did notice the urlbar search telemetry updating, it was undefined by that dumb directive.
It doesn't make any sense that local builds have less code than what we ship, and all the other browser code points don't check it. I'll file a bug to remove this dangerous define from browser, at least, cause it can really hide or create bugs.

Now, this doesn't mean this fix is useless, it fixed a lot of obscure code points in the urlbar code, fixed various bugs where urlbar didn't agree with the final target and now thanks to this in bug 1310737 I can remove duplicated code paths, that were doing pointless work. The only unfortunate thing is that some hours were wasted trying to figure out a test failure caused by that dumb old leftover code.
Flags: needinfo?(mak77)
Depends on: 1312182
(In reply to Marco Bonardo [::mak] from comment #40)
> Good news, we don't need this fix in Beta.
> 
> From the beginning this has been caused by a confusing code path due to the
> old MOZ_TELEMETRY_REPORTING define (recently renamed but was already there
> before). That define was added lots of time ago when telemetry was initially
> introduced, and today doesn't make any sense. We have only 2 points in
> browser that use it, and one of those points is in nsBrowserGlue where the
> "keyword-search" observer is added only when it's defined.
> http://searchfox.org/mozilla-central/rev/
> 84075be5067b68dc6cb3b89f999645650e68c05b/browser/components/nsBrowserGlue.
> js#465
> Now the problem is that such define is not defined by local developer
> builds, and that's why nor me, nor Alessio did notice the urlbar search
> telemetry updating, it was undefined by that dumb directive.
> It doesn't make any sense that local builds have less code than what we
> ship, and all the other browser code points don't check it. I'll file a bug
> to remove this dangerous define from browser, at least, cause it can really
> hide or create bugs.
> 
> Now, this doesn't mean this fix is useless, it fixed a lot of obscure code
> points in the urlbar code, fixed various bugs where urlbar didn't agree with
> the final target and now thanks to this in bug 1310737 I can remove
> duplicated code paths, that were doing pointless work. The only unfortunate
> thing is that some hours were wasted trying to figure out a test failure
> caused by that dumb old leftover code.

Based on this comment I will close this issue as verified.

Cheers!
Status: RESOLVED → VERIFIED
Depends on: 1313620
Version: unspecified → 51 Branch
Depends on: 1326908
Depends on: 1334019
Depends on: 1334911
Depends on: 1336083
Depends on: 1334844
Depends on: 1358835
Depends on: 1365887
You need to log in before you can comment on or make changes to this bug.