Bug 2062665 - Surface new suggestion_id field in suggest component by mashalifshin · Pull Request #7554 · mozilla/application-services

mashalifshin · 2026-08-19T00:22:49Z

https://bugzilla.mozilla.org/show_bug.cgi?id=2062665 / https://mozilla-hub.atlassian.net/browse/SPONS-177

amp suggestions now have suggestion_ids. This change surfaces the new suggestion_id in the suggest component for use by apps in their quick-suggest/fx-suggest interaction pings.

For more context/background on suggestion_ids, see the Jira ticket's parent epic, design doc linked there as well.

iOS: Add Bug-2062665 [Suggest] Add new suggestionId field on Suggestion.amp mozilla-mobile/firefox-ios#35272
Android: Here is the diff needed for Android tests to pass after the application-services version bump lands: https://github.com/mozilla-firefox/firefox/compare/main...mashalifshin:suggest-add-suggestion-id?expand=1 . I am working on set up with Phabricator so I can provide a proper revision
Desktop: https://github.com/mozilla-firefox/firefox/compare/main...mashalifshin:firefox:suggest-suggestion-id-desktop-tests?expand=1

Pull Request checklist

Breaking changes: This PR follows our breaking change policy
This PR follows the breaking change policy:
- There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

mashalifshin · 2026-08-19T01:09:05Z

Hi colleagues, this is my first commit in application-services and first time managing breaking changes as well. Please let me know if anything looks off here, and I may require a bit of handholding to make sure this lands properly

luc-lisi · 2026-08-19T21:55:32Z

    pub impression_url: String,
    #[serde(rename = "icon")]
    pub icon_id: String,
+    pub suggestion_id: String,


Are we already forwarding this to all users? If not this will probably need to be an Option to avoid a deserialization error if a client doesn't receive a suggestion_id

To add to this (and I'm not entirely sure how this interacts with remote settings so forgive me if this doesn't apply) but if a user had really old RS artifacts still loaded for some reason (or an old enough version of FF that they are using the legacy RS collection) would this break things if suggestion_id is required?

My rust is very basic so I got some questions about this to Claude.
The problem is serialization errors are swallowed entirely:

match serde_json::from_slice::<SuggestAttachment<T>>(&attachment_data) { Ok(attachment) => ingestion_handler(dao, &record.id, attachment.suggestions()), // If the attachment doesn't match our expected schema, just skip it. It's possible // that we're using an older version. If so, we'll get the data when we re-ingest // after updating the schema. Err(_) => Ok(()), }

An attachment is a JSON array of thousands of suggestions, so a single record missing suggestion_id causes the entire attachment to be dropped — no error, no log, no metric. The user just silently gets zero sponsored suggestions for that region/form-factor.

Instead of using Option is recommends using #[serde(default)].
The cost of an issue is a silent, total loss of sponsored suggestions with no telemetry to detect it. It also protects against a mars rollback and against stale non-prod collections. Importantly, #[serde(default)] keeps the Rust type as String and leaves the public API exactly as this PR has it — so unlike switching to Option

Thanks for flagging this! It's really good to think through. I think it is okay to have it required but you let me know if the following makes sense. Being required is an important property since this is load-bearing billing telemetry, if we don't do it this iteration, we'll have to come back in again and do it later. And I think leaving it required is low risk.

The field has been populated in production for about a month, and Claude can check that it hasn't been missing in any payload -- the deserialization risk happens if some records lack the field. In this case the client would only lack sponsored suggestions until the next resync, which Claude tells me happens about daily for active clients, and client that are dormant refresh when they start up again. And afaict the way the migrations work with the clear_database() call, it forces a fresh re-ingestion.

Please see if you can confirm, I'm new to rust, also relying on Claude. (Side question: Are our Claudes just talking to each other and are we vibe coding too close to the sun?)

Today I'll post the PR for review by disco team

I think using Claude on Claude is still useful. The questions we ask are different and that is where the real value comes from is as it triggers different paths within Claude. Its like having a Claude with multiple personalities (that never ends well in the movies though!)

This seems okay to me as-is assuming that a) serialization errors cause records to be skipped rather than errors to be thrown and b) The production RS data has had this field for about a month. In that case, there's not really a risk of "total loss of sponsored suggestions". I think the only effect is that users who have extremely stale data in the RS cache and who can't download new data won't see the stale suggestions.

I do agree that silently ignoring the errors feels wrong. You could consider adding an error_support::report_error! call to this PR or filing a issue.

Agreed with bendk, this should be fine, i.e., no need to make this an Option. IIRC we typically don't worry about outdated RS data being ingested by updated clients. It looks like the suggestions in the quicksuggest-amp collection already include suggestion_id and probably have for some time I imagine?

It looks like the suggestions in the quicksuggest-amp collection already include suggestion_id and probably have for some time I imagine?

Yes, the suggestion_ids have been included in the prod RS collection since MARS deploy on 7/22/2026, so almost a month now. From what you're both saying that sounds like a sufficient amount of time.

I think the only effect is that users who have extremely stale data in the RS cache and who can't download new data won't see the stale suggestions.

I think there's also a good chance we wouldn't have gotten paid for many of those suggestions even if we were able to show them anyway, since those ad campaigns have probably ended by now...so just piling on reasons that this is probably okay to ship as required.

I do agree that silently ignoring the errors feels wrong. You could consider adding an error_support::report_error! call to this PR or filing a issue.

Yes, agreed, the whole point of having it required is to fail loudly and early when it's missing, so that feels incomplete without an error. I'll take a swing at implementing this

luc-lisi · 2026-08-19T21:56:24Z

        raw_click_url: String,
        score: f64,
        fts_match_info: Option<FtsMatchInfo>,
+        suggestion_id: String,


Same comment as above as to whether we think this should be an Option or not

No need, we can/should expect suggestions to have IDs now.

bendk

This change looks pretty good to me. The next step is getting the changes for ios/android/desktop. https://github.com/mozilla/application-services/blob/main/docs/howtos/breaking-changes.md has instructions there, the TLDR is you want to create a firefox-ios and moz-central PR that resolves the breaking changes and get those PRs approved before merging this one.

bendk · 2026-08-20T20:51:15Z

+
+### Suggest
+
+- `Suggestion.Amp` gained a new `suggestionId` field: a unique identifier for the sponsored suggestion assigned by the ingestion pipeline, deserialized from the `suggestion_id` field of the remote settings AMP data. Adding a field to the `Amp` variant is source-breaking for consumers that destructure it positionally (Firefox iOS); consumers that access fields by name (Firefox Android, Firefox Desktop) are unaffected. ([#7554](https://github.com/mozilla/application-services/pull/7554))


Adding a field to the Amp variant is source-breaking for consumers that destructure it positionally (Firefox iOS); consumers that access fields by name (Firefox Android, Firefox Desktop) are unaffected.

This would be a breaking change for any application that constructs a Suggestion.Amp value, since they would now have to supply a new field. I think that might mean test breakage for Android and Desktop.

I would consider leaving out this part and just leaving it at "Suggestion.Amp gained a new suggestionId field (...)"

This would be a breaking change for any application that constructs a Suggestion.Amp value, since they would now have to supply a new field. I think that might mean test breakage for Android and Desktop.

Ahh you're absolutely right, great catch. I did end up with updates to tests for Android (see the diff linked in the PR description) to make it build and pass tests, but let me check about tests for Desktop as well and see if those need updates.

I would consider leaving out this part and just leaving it at "Suggestion.Amp gained a new suggestionId field (...)"

Good call thank you, I updated the changelog entry to remove the incorrect info and to be more concise.

bendk · 2026-08-20T20:56:27Z

    pub impression_url: String,
    #[serde(rename = "icon")]
    pub icon_id: String,
+    pub suggestion_id: String,


This seems okay to me as-is assuming that a) serialization errors cause records to be skipped rather than errors to be thrown and b) The production RS data has had this field for about a month. In that case, there's not really a risk of "total loss of sponsored suggestions". I think the only effect is that users who have extremely stale data in the RS cache and who can't download new data won't see the stale suggestions.

I do agree that silently ignoring the errors feels wrong. You could consider adding an error_support::report_error! call to this PR or filing a issue.

bendk · 2026-08-20T21:17:27Z

you want to create a (...) moz-central PR that resolves the breaking changes

That's assuming that this actually dos break things on Android and/or Desktop. If you do a local build and everything works then you can skip this step.

0c0w3

Thanks! moz_suggestion_id is a little clunky but I don't have a better idea except for like uuid or something, and using a name that's so dissimilar from the one used in RS is a drawback too. Anyway, it's not too big a deal since that name only needs to be used internally. Too bad we already use suggestion_id.

0c0w3 · 2026-08-20T22:12:57Z

    pub impression_url: String,
    #[serde(rename = "icon")]
    pub icon_id: String,
+    pub suggestion_id: String,


Agreed with bendk, this should be fine, i.e., no need to make this an Option. IIRC we typically don't worry about outdated RS data being ingested by updated clients. It looks like the suggestions in the quicksuggest-amp collection already include suggestion_id and probably have for some time I imagine?

0c0w3 · 2026-08-20T22:15:22Z

        raw_click_url: String,
        score: f64,
        fts_match_info: Option<FtsMatchInfo>,
+        suggestion_id: String,


No need, we can/should expect suggestions to have IDs now.

The pull request has been modified, dismissing previous reviews.

mashalifshin · 2026-08-20T23:39:47Z

This change looks pretty good to me. The next step is getting the changes for ios/android/desktop. https://github.com/mozilla/application-services/blob/main/docs/howtos/breaking-changes.md has instructions there, the TLDR is you want to create a firefox-ios and moz-central PR that resolves the breaking changes and get those PRs approved before merging this one.

That's assuming that this actually dos break things on Android and/or Desktop. If you do a local build and everything works then you can skip this step.

I think it has breaking changes, but please let me know if anything looks off here:

to the code in iOS: Add Bug-2062665 [Suggest] Add new suggestionId field on Suggestion.amp mozilla-mobile/firefox-ios#35272
to the tests in Android: https://github.com/mozilla-firefox/firefox/compare/main...mashalifshin:suggest-add-suggestion-id?expand=1

With those changes, I can locally build iOS and Android and pass the tests. I want to double check about Desktop since as you saw upthread I missed that.

The iOS PR is approved, for Android I think I can comment on updatebot's bugzilla bug for the version bump pointing at the diff and have the team fold it in. I'm not set up with phabricator yet, so hoping that will be sufficient, but lmk if I gotta make it an actual phab patch.

mashalifshin · 2026-08-20T23:42:18Z

Thanks! moz_suggestion_id is a little clunky but I don't have a better idea except for like uuid or something, and using a name that's so dissimilar from the one used in RS is a drawback too. Anyway, it's not too big a deal since that name only needs to be used internally. Too bad we already use suggestion_id.

I agree it's clunky and also couldn't think of anything better and wish suggestion_id was available.

I actually initially named it suggestion_uuid but I received feedback that uid can commonly mean "user id" in systems where they track that sort of thing, so it was important to rename so we don't create any confusion about what is being identified here (the ad, never the user)

mashalifshin · 2026-08-21T00:02:00Z

Oh yes lots of breaking tests in Desktop, thank you for the great catch @bendk. I'll work on:

making updates to Desktop tests
adding an error if suggestion_id is missing
getting set up and familiar with phabricator so I can turn my github fork diffs for Desktop and Android into proper revisions

and I'll follow up again once those are done.

0c0w3 · 2026-08-21T03:02:08Z

Please let me know if you need any help updating desktop, especially the tests!

bendk

Those iOS and Android patches look good to me, I'm going to approve now and let you merge once those Desktop tests are ready.

The pull request has been modified, dismissing previous reviews.

mashalifshin · 2026-08-22T00:28:35Z

Updating before I take off for the weekend:

I have

updated the Desktop tests (now linked in PR description)
added an error for the suggestion_id

I still need to get set up with Phabricator, and I still haven't gotten the Desktop tests to run successfully on my machine. Hopefully will be able to finish this up Monday

mashalifshin mentioned this pull request Aug 19, 2026

Add Bug-2062665 [Suggest] Add new suggestionId field on Suggestion.amp mozilla-mobile/firefox-ios#35272

Open

3 tasks

mashalifshin force-pushed the spons-177-surface-suggestion-ids-in-as-suggest-component branch from 3fee79b to 36cd6c5 Compare August 19, 2026 00:24

mashalifshin force-pushed the spons-177-surface-suggestion-ids-in-as-suggest-component branch from 36cd6c5 to cd5d2a3 Compare August 19, 2026 21:36

luc-lisi reviewed Aug 19, 2026

View reviewed changes

0c0w3 self-requested a review August 20, 2026 20:14

bendk reviewed Aug 20, 2026

View reviewed changes

0c0w3 previously approved these changes Aug 20, 2026

View reviewed changes

mashalifshin force-pushed the spons-177-surface-suggestion-ids-in-as-suggest-component branch from cd5d2a3 to f2b73f2 Compare August 20, 2026 23:11

mashalifshin force-pushed the spons-177-surface-suggestion-ids-in-as-suggest-component branch from f2b73f2 to 3362505 Compare August 20, 2026 23:15

bendk previously approved these changes Aug 21, 2026

View reviewed changes

mashalifshin added 4 commits August 21, 2026 16:37

Bug 2062665 - Surface new suggestion_id field in suggest component

36828a8

Update CHANGELOG.md with PR link

fb44ddb

Fix wrong CHANGELOG.md comment and make more concise

f54c709

Emit an error when invalid attachments are ignored

a34ae94

mashalifshin force-pushed the spons-177-surface-suggestion-ids-in-as-suggest-component branch from 2f507c2 to a34ae94 Compare August 21, 2026 23:37


		### Suggest

		- `Suggestion.Amp` gained a new `suggestionId` field: a unique identifier for the sponsored suggestion assigned by the ingestion pipeline, deserialized from the `suggestion_id` field of the remote settings AMP data. Adding a field to the `Amp` variant is source-breaking for consumers that destructure it positionally (Firefox iOS); consumers that access fields by name (Firefox Android, Firefox Desktop) are unaffected. ([#7554](https://github.com/mozilla/application-services/pull/7554))

Uh oh!

Conversation

mashalifshin commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request checklist

Uh oh!

mashalifshin commented Aug 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kalamazilla Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

bendk 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!

bendk commented Aug 20, 2026

Uh oh!

0c0w3 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!

mashalifshin commented Aug 20, 2026

Uh oh!

mashalifshin commented Aug 20, 2026

Uh oh!

mashalifshin commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0c0w3 commented Aug 21, 2026

Uh oh!

bendk left a comment

Choose a reason for hiding this comment

Uh oh!

mashalifshin commented Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mashalifshin commented Aug 19, 2026 •

edited

Loading

kalamazilla Aug 19, 2026 •

edited

Loading

mashalifshin commented Aug 21, 2026 •

edited

Loading

mashalifshin commented Aug 22, 2026 •

edited

Loading