1601344 - Fix fennec.page_options.total_sites_pinned_to_topsites and fennec.page_options.total_added_search_engines core pings

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Andrei Lazar]

Fix fennec.page_options.total_sites_pinned_to_topsites and fennec.page_options.total_added_search_engines core pings.

Comment 1

[Andrei Lazar]

Attachment: Bug 1601344 - Fix fennec.page_options.total_sites_pinned_to_topsites and fennec.page_options.total_added_search_engines core pings r=petru

Fixed logic flaw for retrieving the number of added search engines, flaw which lead to negative number count because of the wrong number of default engines.
Fixed implementation error for retrieving the number of pinned topsites.

Comment 2

[Andrei Lazar]

Comment on attachment 9114900 [details]
Bug 1601344 - Fix fennec.page_options.total_sites_pinned_to_topsites and fennec.page_options.total_added_search_engines core pings r=petru

Beta/Release Uplift Approval Request

• User impact if declined: No user impact. This data gathering is required by the data science team.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small changes with no feature behavior changes.
• String changes made/needed:

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9114900 [details]
Bug 1601344 - Fix fennec.page_options.total_sites_pinned_to_topsites and fennec.page_options.total_added_search_engines core pings r=petru

Fixes a bug in the new Telemetry probes shipped in Fennec 68.3. Approved for Fennec 68.4b2.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/8fc36c7ccc1d

Comment 5

[Diana Rus]

Hi, tested on Firefox Beta 68.4b2 with One Plus 6T/6013 (Android 9) and Google Pixel 3a (Android 9)

• fennec.page_options.total_sites_pinned_to_topsites:
  • the value for the ping is incremented
  • there is no reset for the counter when the next ping is sent
• fennec.page_options.total_added_search_engines:
  • the value for the ping is incremented

Note:
@Frank, can you please clarify the below questions?

• In case of update like eliminating a search engine or a pinned top site, should the value for the pings be updated as per application or should it decrement?
• For "fennec.page_options.total_added_search_engines" in case of adding and removing a search engine in the same session than when the new core ping the value is set to 0.
  How should it be?
• As of now the value, based on an update of eliminating a pinned top site for "fennec.page_options.total_sites_pinned_to_topsites" is decrementing and the value for "fennec.page_options.total_added_search_engines" remains the same as based on the last incrementation.

Thank you

Comment 6

[Frank Bertsch [:frank]]

Sorry for the delay - total_added_search_engines sounds like it's wrong. That should always be the total count of added search engines, so if they add one during that session it increments, and if they remove one it decrements; but if they start with e.g. 4 engines, it also has 4.

Comment 7

[Diana Rus]

Hi, based on the last comment and the results from Comment 5 for total_added_search_engines, I will mark the issue as affected.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9114900 [details]
Bug 1601344 - Fix fennec.page_options.total_sites_pinned_to_topsites and fennec.page_options.total_added_search_engines core pings r=petru

Clearing the approval flag to get this off the needs-uplift radar.

Comment 9

[Petru-Mugurel Lingurar [:petru]]

Tested this now and regarding total_added_search_engines I saw that:

• documentation says that The "total_added_search_engines" is an absolute value that contains the number of search engines the user has added manually.
  So we ignore the number of default search engines and try to only log the number of search engines the user has manually added.
• we do log the absolute number of user added search engines at the time the ping is sent (may include cached pings).
  In my tests I saw the value for total_added_search_engines being properly incremented and decremented.
• documentation says that The value is determined at the time of sending the core ping and never reset to zero.
  I don't understand why it should never reset to 0 if the user has the option to remove all the search engines he added manually.
  The current implementation do allow for 0 to be sent as the value for this probe.
• there is a bug in how we differentiate between default and user added search engines - we use a hardcoded value for the number of default search engines - 6.
  This would mean that it might be possible that for a particular application - locale/release channel the actual number might differ.
  More than this, the user can remove all but one default search engines. If she then adds custom ones but the total number will be fewer than 6 (the presumed number of default search engines) we'll still log 0 user added search engines.

The easiest fix for the above, and based on that never reset to zero I think what was desired from the start is to log the absolute number of search engines that appear in Settings -> Search -> Installed search engines.
Otherwise it currently it is a bit tricky to differentiate between default and user added search engines.
Asking your opinion on the above Frank.

Comment 10

[Frank Bertsch [:frank]]

The easiest fix for the above, and based on that never reset to zero I think what was desired from the start is to log the absolute number of search engines that appear in Settings -> Search -> Installed search engines.
Otherwise it currently it is a bit tricky to differentiate between default and user added search engines.
Asking your opinion on the above Frank.

I think this is exactly what we are looking for, thank you for clarifying that Petru.

Comment 11

[Petru-Mugurel Lingurar [:petru]]

Thank you for confirming this Frank!
Because the work for this ticket has already been merged I've created a new one - bug 1609720 in which we'll the fennec.page_options.total_added_search_engines probe log the number of all search engines appearing in Settings -> Search -> Installed search engines.

Comment 12

[Diana Rus]

Hi, verified as fixed for fennec.page_options.total_sites_pinned_to_topsites on Firefox RC with Samsung Galaxy S8+ (Android 8.0.0) and Google Pixel 3 XL (Android 9).
For ping fennec.page_options.total_added_search_engines it has been treated in bug 1609720.