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

Only publish to Remote Settings recipes-capabilities collection #2047

Closed
leplatrem opened this issue Nov 18, 2019 · 7 comments · Fixed by #2141
Closed

Only publish to Remote Settings recipes-capabilities collection #2047

leplatrem opened this issue Nov 18, 2019 · 7 comments · Fixed by #2141

Comments

@leplatrem
Copy link
Contributor

With the current implementation #1960, recipes are published to two Remote Settings collections.

When a recipe does not leverage advanced client capabilities, it is published to two collections: one fetched by latest clients, and one for the legacy clients. This is done synchronously within the HTTP request lifecycle on the Normandy server, when the recipe is created/modified.

This means that, on the Remote Settings side, we receive two set of HTTP requests (to create and sign both recipes collections), within a few milliseconds. Recipes are published or changed quite often (several times a day), so we see these double hits on Remote Settings quite often. Each publication leads to two consecutive requests to Autograph and the Push service.

I don't really like this situation where two Push notifications are sent to clients with a few milliseconds of gap. I think it would be better if we could avoid it.

The solution I would suggest consists in publishing in the exhaustive collection (with capabilities) only. And then, have a scheduled job to backport records in the legacy collection every hour or so for Firefox < 70.

We already have some backport jobs for the onecrl blocklist etc., so this would be trivial to add.
This is would be a bit particular since it would filter based on the listed capabilities, so instead of having some Normandy specific code in the Remote Settings lambdas repo, we could have an extra field on recipes (eg. is_baseline: true/false) and just filter on this...

What do you think?

@mythmon
Copy link
Contributor

mythmon commented Nov 18, 2019

I don't really like this situation where two Push notifications are sent to clients with a few milliseconds of gap. I think it would be better if we could avoid it.

Clients only effectively get pushes for collections they are subscribed to, right? If so, no client should be subscribed to both of these collections, and so even though we are sending two notifications, each client would only receive one.

Or is the problem not what clients are receiving, but what we are requesting the push service to do?

This is would be a bit particular since it would filter based on the listed capabilities, so instead of having some Normandy specific code in the Remote Settings lambdas repo, we could have an extra field on recipes (eg. is_baseline: true/false) and just filter on this...

I like this idea. I've been thinking about adding information about whether a recipe matches baseline criteria for a while, as it will be helpful for debugging as well. I also like the idea of not adding this Normandy-specific logic to Remote Settings.

If we did this today, I'd want the backports to be more frequent than one hour, as we still have a significant population on Firefox 68 and 69 that we are working with. If we delay this until the next Firefox version I'd be happier with the slower sync.

Would it be possible/reasonable to be able to manually backport the changes as well? This could be important for some situations, but I don't think it's a blocker.

@leplatrem
Copy link
Contributor Author

Clients only effectively get pushes for collections they are subscribed to, right? If so, no client should be subscribed to both of these collections, and so even though we are sending two notifications, each client would only receive one.

That's not exactly how push notifications work. We only have one global notification for the whole set of collections (@glasserc do you remember if we ever considered having a topic per collection?).

When a notification is received, we pull the list of collections that changed since last successful sync, and start a synchronization for every collection that is subscribed, or that has local data, or that has JSON dump packaged.

If we did this today, I'd want the backports to be more frequent than one hour, as we still have a significant population on Firefox 68 and 69 that we are working with. If we delay this until the next Firefox version I'd be happier with the slower sync.

Could be 5min or whatever, most of the time the run would just consist in comparing the two ETags.

Would it be possible/reasonable to be able to manually backport the changes as well? This could be important for some situations, but I don't think it's a blocker.

You could ask ops to run the lambda manually yes.
(note, the normandy collections are only writable by a specific user)

@mythmon
Copy link
Contributor

mythmon commented Nov 18, 2019

Ahh, I assumed that we had a topic per collection. That's a very interesting detail, thanks for explaining it.

I think that being able to trigger the lambda manually is sufficient for my concern about going fast, and also if we have something as fast as 5 minutes, it isn't likely we'll need to trigger things manually anyways.

Given that, and a faster backport rate, I'm +1 on this plan.

@glasserc
Copy link
Contributor

@glasserc do you remember if we ever considered having a topic per collection?

Yes, I definitely considered it, and I ended up not going with that approach although I don't remember exactly why. I don't think there's a robust technical reason, just that it was easier this way. In particular, the browser has to register for every single topic that it cares about, which would mean knowing every Remote Setting that the browser knows/cares about. This may have seemed like an abstraction violation to me at the time and/or I may have thought pollChanges was the better "main entry point".

@leplatrem
Copy link
Contributor Author

leplatrem commented Nov 20, 2019

The plan would be:

  • Step 1: add the field to the API
  • Step 2: deploy a lambda that backports (uses_only_baseline_capabilities=true) recipes to the old collection (will be no-op since both in sync)
  • Step 3: modify Poucave checks
  • Step 4: remove the code that publishes to both collections

@leplatrem
Copy link
Contributor Author

Here we can see the push timestamps differ from a couple of ms :)
Screenshot from 2019-11-21 23-25-40

bors bot added a commit that referenced this issue Nov 25, 2019
2048: Add 'uses_only_baseline_capabilities' field to API v3 r=leplatrem a=leplatrem

Ref #2047 

Co-authored-by: Mathieu Leplatre <mathieu@mozilla.com>
bors bot added a commit that referenced this issue Dec 4, 2019
2053: Expose 'uses_only_baseline_capabilities' field in Remote Settings (ref #2047) r=mythmon a=leplatrem

ref #2047

In order to be able to backport records (using [this new lambda feature](mozilla-services/remote-settings-lambdas#686)), the RS records must have this field!

Note that recipes will have to be republished once released and deployed.

Co-authored-by: Mathieu Leplatre <mathieu@mozilla.com>
bors bot added a commit that referenced this issue Jan 10, 2020
2070: Fix exposure of 'uses_only_baseline_capabilities' field in Remote Setttings (ref #2047) r=mythmon a=leplatrem

Ref #2047

This should fix the signature issues that we had. If you have suggestions on how to concretely test signatures verification please let me know!

Co-authored-by: Mathieu Leplatre <mathieu@mozilla.com>
@glasserc
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants