Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Telemetry-22: Properly record in-content searches in the German locale #23

Merged
merged 2 commits into from Dec 20, 2016

Conversation

past
Copy link

@past past commented Dec 16, 2016

@mak77 does this look OK to you? I verified that Yahoo is properly accounted for in both host and results buckets, and Google is counted properly in host only. Getting it counted in results as well would require a more substantial change as the search plugin that ships with the German locale has no locale-specific information. It all happens server-side in Google's servers.

This is also why I had to hardcode the "de" string, it's not the localized build that causes this, but the geolocation of the client.

@luciancor if you are using a localized build, can you verify that this works for you?

@past
Copy link
Author

past commented Dec 16, 2016

Fixes #22.

@luciancor
Copy link
Contributor

@past I tested using an older german Firefox version (46) and it didn't work but not because of this fix but because of the code here - https://github.com/cliqz-oss/browser-core/blob/test-pilot/specific/firefox/cliqz@cliqz.com/modules/FirefoxTelemetry.jsm#L95 - In Firefox 46 for default yahoo I got the following url https://de.search.yahoo.com/search?p=hello+from+urlbar&ei=UTF-8&fr=moz35

Is there any description regarding the parameters Firefox adds to the search actions?

@past
Copy link
Author

past commented Dec 19, 2016

@luciancor What do you mean it didn't work? I get the same URL, but the visit is counted in telemetry. Don't you see a testpilottest event in about:telemetry (archived ping data) with "event": "userVisitedEngineResult" and "contentSearch": "yahoo"?

@mak77
Copy link

mak77 commented Dec 19, 2016

yahoo-de is defined here:
http://searchfox.org/mozilla-central/source/browser/locales/searchplugins/yahoo-de.xml
as pointed out by @luciancor it uses <Param name="fr" value="moz35" /> instead of <Param name="hspart" value="mozilla"/> (en-US), so you need to change "hspart=mozilla" with "fr=moz35", or just add both, it doesn't matter.
Google and Bind just redirect, but they seem to retain the arguments (at least here in Italy).

This is one of the reasons content searches counting is hard, every engine is special as well as every locale.

@mak77
Copy link

mak77 commented Dec 19, 2016

The other changes look fine to me.

@past
Copy link
Author

past commented Dec 20, 2016

Thanks for the comments, I updated the check to account for the different parameter in the German locale.

@past past merged commit fa4d559 into cliqz-oss:test-pilot Dec 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants