Refactor FXIOS-12358 #26928 [Ohttp FxSuggest ping] fx suggest ping sent through ohttp #29166

lmarceau · 2025-09-04T19:58:07Z

📜 Tickets

💡 Description

We got a request to migrate the fx-suggest to OHTTP through Glean. This can be done in the yaml file definition directly, since we now have the ohttp capability.
Created a FxSuggestTelemetry class that handles the Firefox suggest telemetry (so it's not passed into the TelemetryWrapper anymore).
We're also adding the country as part of that ping because we currently use the IP address to infer the geo on the backend, so when the IP address goes away (by routing the request over OHTTP) we need to pass the geo explicitly. I am using the same Locale code to get the regionCode as used in the new search engine provider code (added in Add FXIOS-11351 [SEC] [WIP] Initial groundwork for Consolidated Search #25335). Created an extension to DRY the code as suggested in here.
Remove the feature flag .ohttpManagerGleanUploader as it won't be possible to have the ping sent through http or ohttp with a code toggle. Feature flag is still useful as it's merged in main and won't affect any release that is cut-off from main
Adding a Locale extension to DRY the code as suggest here.
Removed [email protected] from the yaml files since that person is not at mozilla anymore

📝 Checklist

I filled in the ticket numbers and a description of my work
I updated the PR name to follow our PR naming guidelines
I ensured unit tests pass and wrote tests for new code
If working on UI, I checked and implemented accessibility (Dynamic Text and VoiceOver)
If adding telemetry, I read the data stewardship requirements and will request a data review
If needed, I updated documentation and added comments to complex code
If needed, I added a backport comment (example @Mergifyio backport release/v150.0)

mobiletest-ci-bot · 2025-09-04T20:08:21Z

	Messages
📖	Project coverage: 38.67%
📖	Edited 14 files
📖	Created 4 files

Client.app: Coverage: 36.92

File	Coverage
BrowserViewController.swift	29.49%	⚠️
FeatureFlagsDebugViewController.swift	0.0%	⚠️
ASSearchEngineProvider.swift	0.0%	⚠️
NimbusFlaggableFeature.swift	95.38%	✅
SponsoredTileTelemetry.swift	90.48%	✅
FxSuggestTelemetry.swift	94.34%	✅
NimbusFeatureFlagLayer.swift	82.13%	✅
TelemetryWrapper.swift	68.44%	✅
GleanWrapper.swift	73.53%	✅
Locale+Extension.swift	63.64%	✅

Generated by 🚫 Danger Swift against df70419

lmarceau · 2025-09-04T20:31:09Z

Started a TestFlight build #60974

ih-codes

I quickly scanned the telemetry portion, I'm happy to see it pulled into its own type (yay!). Looks good to me at first glance. You mentioned you wanted to look at unit testing for this, so let's chat at our 1:1 next week. 👍

badboy · 2025-09-09T14:49:15Z

firefox-ios/Client/Glean/pings.yaml

@@ -50,13 +50,15 @@ fx-suggest:
    Distinguishable by its `ping_type`.
    Does not contain a `client_id`, preferring a `context_id` instead.
  include_client_id: false
+  uploader_capabilities:
+    - ohttp


Note that this ping contains client info data as it does NOT have metadata.include_info_sections: false.
That potentially allows it to be connected back to regular telemetry.

If we can't have that we will need a new ping name, as switching a ping to include_info_sections: false will not work in the pipeline.

I think @badboy is correct, removing the info sections as required for ohttp pings would fail schema validation on ingestion if we keep the same name, and removing fingerprintable data like the info sections is an important part of anonymizing the data we collect over OHTTP.

@badboy this ping has include_client_id: false so IIUC SDK won't submit client_id?
This is similar to https://github.com/mozilla-firefox/firefox/blob/4a1256a814d330833bb9ff484f069ec65f070969/mobile/android/fenix/app/pings.yaml#L116-L123 which does not have client_id populated in BQ.

I think the point was that with the info-sections the possibility of fingerprinting is fairly high, yet. But if this has been okayed with the info sections attached, then we will not block this landing.

Yup. If it's ok that the rest of client_info is included then this is fine.
If it's not we have to rename the ping.

lmarceau · 2025-09-10T13:57:42Z

I tested with the Debug Ping Viewer and all looks good to me. Here's the link. I declare this PR to be ready for review 👀

ih-codes

Just some thoughts for you, nothing jumped out at me overall! Great work! 😁

Looks like the telemetry eng folks had some good feedback for your consideration as well (passing client IDs).

Question:

When testing, is it expected behaviour to record searchResultImpression with extra "wikipedia-suggestion" for tapping on a suggested tab? (Not a wikipedia result). For example, switching to this standardebooks.org tab:

This also fires the fxSuggest ping as well for Wikipedia... 🤔

Anyway I tested in the sim with the Glean debugger and otherwise I think things look good. (I haven't been able to force a sponsored suggestion though to test the .amp cases yet though.)

P.S. For QA it might also be useful to note that the fx suggest ping should maintain exactly the same behaviour as before (besides the differences you've already noted on the ticket). Just so they can test before/after too. Maybe that's obvious from the context but I like to be explicit. 😆 Also, just FYI the ping examples you added for them will just live for about 30 days IIRC.

ih-codes · 2025-09-10T15:59:47Z

firefox-ios/Client/Extensions/Locale+Extension.swift

@@ -0,0 +1,15 @@
+// This Source Code Form is subject to the terms of the Mozilla Public


FYI I found that we already have this other localeRegionCode implemented on a separate Locale extension. 👀

Looks like it was added last December by @OrlaM for the DMA work. The one you grabbed from @mattreaganmozilla's SEC code seems to have more sophisticated logic for iOS version. I would like to suggest removing one but I also don't know why there's nuance here and don't want to break anything. 😅

I grabbed the one from SEC since it made sense to have the same region code in the different area of the app, particularly if it relates to search engine, and telemetry is in the search bar it just made sense to me to have the same region code. What do you think?

Yep I think that makes sense! 👍

I would like to remove the other method vs. having 2 methods that are supposed to do the same thing, but I don't have enough context around the DMA work to feel confident suggesting a change. (Feel free to ignore me as this is outside the scope of your PR haha)

ih-codes · 2025-09-10T16:53:31Z

firefox-ios/Client/Telemetry/FxSuggestTelemetry.swift

+        guard let contextIdString = TelemetryContextualIdentifier.contextId,
+              let contextId = UUID(uuidString: contextIdString) else {
+            return
+        }


Not sure the likelihood of error here, is this something we should log a warning for, or add an assertion? 🤔 Might be good to put an assertionFailure for these because the setupContextId code might get moved out of the TelemetryWrapper later...

Side note, TelemetryContextualIdentifier.setupContextId() is called in the TelemetryWrapper init, as well as one place under certain conditions in AppLaunchUtil. 🤔 I wonder if there's a more reliable place we can ensure this is set properly? (If we're relying on the TelemetryWrapper init specifically)

Unrelated: it's a bit sketchy to me because of the order of those 2 setupContextId() calls. Even if you pass false to setupContextId() later, GleanMetrics.TopSites.contextId is never unset.

I can put an assertion in case. As for the rest, there is some work for the context id that was started by Carson here but it got stale. This area needs improvement. The TelemetryContextualIdentifier.setupContextId() under AppLaunchUtil was added as a patch

Ohhh interesting. 👀 Thanks for the info! I hope the FXIOS-12301 ticket for this work can clean this up a bit.

firefox-ios/Client/Telemetry/SponsoredTileTelemetry.swift

firefox-ios/firefox-ios-tests/Tests/ClientTests/Telemetry/FxSuggestTelemetryTests.swift

lmarceau · 2025-09-10T19:53:15Z

Question:

When testing, is it expected behaviour to record searchResultImpression with extra "wikipedia-suggestion" for tapping on a suggested tab? (Not a wikipedia result).

I am using the same code as it was before, so this seems fine to the extent of my knowledge 😆 As long as I didn't make a copy paste error/mismatch I assume the previous code for those events makes sense and is triggered properly

ih-codes · 2025-09-10T20:14:09Z

Question:
When testing, is it expected behaviour to record searchResultImpression with extra "wikipedia-suggestion" for tapping on a suggested tab? (Not a wikipedia result).

I am using the same code as it was before, so this seems fine to the extent of my knowledge 😆 As long as I didn't make a copy paste error/mismatch I assume the previous code for those events makes sense and is triggered properly

Seems reasonable, I kinda figured this was the same code. I wonder if we need to file a bug ticket for this, as it seems a bit weird........ Any idea who we can ask about expected behaviour here? 🤔

ih-codes

🎉

…mebar.searchResultImpression are recorded

… the context ID

lmarceau requested review from travis79, ih-codes and nbhasin2 September 4, 2025 19:58

ih-codes reviewed Sep 4, 2025

View reviewed changes

travis79 approved these changes Sep 5, 2025

View reviewed changes

badboy reviewed Sep 9, 2025

View reviewed changes

lmarceau force-pushed the lm/FXIOS-12358-#26928-fx-suggest-ping branch from b4194bc to 920c807 Compare September 10, 2025 01:35

lmarceau changed the title ~~[WIP] Refactor FXIOS-12358 #26928 [Ohttp FxSuggest ping] fx suggest ping sent through ohttp~~ Refactor FXIOS-12358 #26928 [Ohttp FxSuggest ping] fx suggest ping sent through ohttp Sep 10, 2025

lmarceau marked this pull request as ready for review September 10, 2025 13:56

lmarceau requested a review from a team as a code owner September 10, 2025 13:56

lmarceau requested a review from ih-codes September 10, 2025 13:57

ih-codes reviewed Sep 10, 2025

View reviewed changes

lmarceau requested a review from ih-codes September 10, 2025 19:59

ih-codes approved these changes Sep 10, 2025

View reviewed changes

lmarceau added 11 commits September 10, 2025 16:35

Add requirements, then move telemetry code to new FxSuggestTelemetry

961c6c2

Add country system region to ping

bfab1dd

swiftlint

0a303de

Fixing tests

b490a75

Fix TODO

b396548

Adjust with latest changes in Search Engine

11fde77

Remove feature flag

2b9cb99

Add locale extension

9890da0

recordUUID added in GleanWrapper + fix recordURL

916fdd2

Added unit tests

9da8793

Forgot this

9af0a53

lmarceau added 3 commits September 10, 2025 16:35

Ensure GleanMetrics.Awesomebar.searchResultTap and GleanMetrics.Aweso…

0db266d

…mebar.searchResultImpression are recorded

Make changes following feedback

af610bf

Remove test with assertions, replace instead by tests ensuring we set…

df70419

… the context ID

lmarceau force-pushed the lm/FXIOS-12358-#26928-fx-suggest-ping branch from 36d3261 to df70419 Compare September 10, 2025 23:51

lmarceau merged commit 8dba04b into main Sep 11, 2025
11 checks passed

lmarceau deleted the lm/FXIOS-12358-#26928-fx-suggest-ping branch September 11, 2025 14:16

		@@ -0,0 +1,15 @@
		// This Source Code Form is subject to the terms of the Mozilla Public

Refactor FXIOS-12358 #26928 [Ohttp FxSuggest ping] fx suggest ping sent through ohttp #29166

Refactor FXIOS-12358 #26928 [Ohttp FxSuggest ping] fx suggest ping sent through ohttp #29166

Uh oh!

Conversation

lmarceau commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Tickets

💡 Description

📝 Checklist

Uh oh!

mobiletest-ci-bot commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Client.app: Coverage: 36.92

Uh oh!

lmarceau commented Sep 4, 2025

Uh oh!

ih-codes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmarceau commented Sep 10, 2025

Uh oh!

ih-codes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lmarceau commented Sep 10, 2025

Uh oh!

ih-codes commented Sep 10, 2025

Uh oh!

ih-codes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lmarceau commented Sep 4, 2025 •

edited

Loading

mobiletest-ci-bot commented Sep 4, 2025 •

edited

Loading