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

New mongo package options to optimize Oplog tailing performance to include/exclude certain collections #13009

Merged
merged 12 commits into from Apr 24, 2024

Conversation

Twisterking
Copy link

@Twisterking Twisterking commented Feb 16, 2024

Hello all,

Following several conversations on the drawbacks of Meteor's oplog implementation and its limitations when it comes to scaling (see: my forums post, a slightly older blog post and a 2 year old github discussion), I investigated in the core a bit and propose an easy solution which, I hope, should work for many users who have issues with this.

Please consider our situation at Orderlion: We have several collections, where huge amounts of data are being updated on a daily basis. On all of these collections we have not a single publication, for these we don't care about any reactivity.

Due to Meteor's Oplog implementation, even in these cases a big batch insert/update of data results often in very big sustained CPU spikes on the meteor app server.

Internal tests I did with my suggested implementation have shown, that the CPU load due to Oplog tailing collections with big batch updates, can be reduced about 100x with just completely ignoring those collections.
During an import of about 600k documents in a test collection, I could reduce the CPU load from an average 25-30% down to approx. 0,2 - 0,3%.

The usage is actually very straight forward: Consider a settings.json file like this:

{
  // ... all other settings,
  "public": {
    // all your public settings
  },
  "packages": {
    "mongo": {
      "oplogExcludeCollections": ["products", "prices"] // This would exclude both collections "products" and "prices" from any oplog tailing.
      // OR:
      "oplogIncludeCollections": ["messages"] // ONLY watch the "messages" collection for oplog updates (and all "admin.$cmd" oplogs)
    }
  }
}

Please feel free to comment but I really hope this can be merged soon. It would help us at Orderlion a lot and solve many unwanted server restarts (server is auto-restarting because it becomes unresponsive due to the sustained high CPU load).

Cheers, Patrick

@CLAassistant
Copy link

CLAassistant commented Feb 16, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@radekmie radekmie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the most important part will be to add tests. At least:

  1. Configure both and see if it fails.
  2. Configure inclusion and check if non-included ones are not processed.
  3. Configure exclusion and check if non-excluded ones are processed.

packages/mongo/oplog_tailing.js Outdated Show resolved Hide resolved
packages/mongo/oplog_tailing.js Show resolved Hide resolved
@harryadel
Copy link
Contributor

harryadel commented Feb 17, 2024

Why can the configuration be set from settings.json and not the code?

// This is pseudo code so don't judge
import { mongo } from 'meteor/mongo'

mongo.oplogExcludeCollections = [];
mongo.oplogWatchCollections = [];

I've noticed this trend in Meteor lately and I believe it's getting out of hand. The settings file is getting bloated with configurations; it's supposed to hold ENV variables/secrets. 🤷‍♂️

cc @zodern

@Twisterking
Copy link
Author

@harryadel
In my opinion the settings file is the right place for this. The oplog tailing is setup very very early during the startup of your app. Finding the right place to put your suggested code so that your settings are defined "on time" is not straight forward in my opinion.

The settings file is kinda "always loaded".

Also I tried to follow the logic that already exists for the mongo package - it already uses the Meteor.settings for other package options.

@harryadel
Copy link
Contributor

@Twisterking I understand why you went this path. I'm merely discussing the idea not trying to put you down :)

Finding the right place to put your suggested code so that your settings are defined "on time" is not straight forward in my opinion.

But isn't that what Meteor.startup is for?

Also I tried to follow the logic that already exists for the mongo package - it already uses the Meteor.settings for other package options.

Yeah, this trend has been going on for a while so naturally you continued down this trail. I just don't think this path is the best IMO.

@Twisterking
Copy link
Author

Twisterking commented Feb 17, 2024

@harryadel understood, no worries! No harm done!
I am aware of Meteor.startup of course, but I was not sure if this works in this case because the mongo package indeed is very much core.
Anyways ...

I will write some tests as suggested and will also create a PR for the meteor docs so this new setting is documented.

Any other feedback? As I said I would love to get this PR ready for merging asap! 👍

@radekmie
Copy link
Collaborator

@Twisterking I'm for leaving it in the settings. Doing it in the code would be problematic, as it'd need to execute before the connection is established and that's deep.

@harryadel The Meteor.startup is for executing code after the server starts, so it's too late. Even top-level in the app is too late (you can execute database operations there already).

@Twisterking
Copy link
Author

I think the most important part will be to add tests.

@radekmie I am very happy to do that! The issue is, I don't really have any experience with tinytest yet.
I will make that work somehow, but: What I do not know (and where I will need your support) is, how can I define a Meteor.settings entry when running the Oplog tests? I tried to just define it somehow like this:

process.env.MONGO_OPLOG_URL && Tinytest.addAsync(
  'mongo-livedata - oplog - options.oplogExcludeCollections',
  async test => {
    if (!Meteor.settings.packages) Meteor.settings.packages = {};
    if (!Meteor.settings.packages.mongo) Meteor.settings.packages.mongo = {};
    Meteor.settings.packages.mongo.oplogExcludeCollections = ['foobar'];
    console.log('oplogExcludeCollections', Meteor.settings.packages.mongo);
    test.equal(true, true);
  }
);

This of course works, but it is "too late" - the oplog file itself is already loaded and there, Meteor.settings.packages.mongo is undefined! 😢

--> How do I define a Meteor setting in a test "early enough"?

@radekmie
Copy link
Collaborator

The only easy option I see would be to change the Meteor.settings, create a new instance of OplogHandle, test it, and revert the Meteor.settings.

@Twisterking
Copy link
Author

I am sorry, but I am not familiar enough with the meteor core to do this based on this comment! :/ I will need some more detailed help please! Maybe @zodern can help?

@zodern
Copy link
Member

zodern commented Feb 19, 2024

This will be very helpful for some apps. I'm not very familiar with this code, but I have a couple questions.

  1. Meteor tracks the last oplog entry it's seen to know where to resume tailing the oplog. If there's been a large number of newer oplog entries for databases or collections that are excluded, could resume tailing cause performance issues if mongo has to filter out the unneeded docs? This happens every time Meteor does not receive an oplog doc for 30 seconds, there is a failover, or in some other situations.

To give an example in case my description isn't clear. Let's say the database that is being observed is updated every 10 minutes on average, and there is another database for metrics that has hundreds of updates per second. Whenever Meteor restarts tailing the oplog (which would happen almost every 30 seconds since there are rarely oplog entries for the observed database), it would query for all of the relevant docs newer than the last oplog entry it received. Since the last oplog entry Meteor received might have been up to 10 minutes ago, there could be a large number of other oplog entries mongo would have to filter.

  1. Would it be possible to make this automatic, at least for the databases? Meteor could track which databases have had observers created for them, and update the query whenever a new database is observed. It seems like as long as Meteor tracks the timestamp of the last oplog entry it processed, there shouldn't be any downside to restarting tailing with a new query. Meteor already does it frequently with the same query.

This would probably involve OplogHandle tracking the last ts it's seen:

self._entryQueue.push(doc);

and then stopping and recreating the oplog tail handle as needed. I'm not sure if there would be a performance issue with listing too many databases or collections.

  1. What happens if someone creates an observer on an excluded database or collection? Will the observer not provide updates after the initial find, or will it fallback to polling?

@Twisterking
Copy link
Author

Twisterking commented Feb 19, 2024

@zodern
Thanks for your reply! I indeed also think that this change is actually quite simple, but can have a huge impact in some cases like ours. It really changes the performance/scaling of Meteor for us at Orderlion significantly!

ad 1) good point indeed, to be honest I don't know.
ad 2) same here. Maybe I can ask you of any other maintainer to help here a bit? :)
ad 3) I thought about this too. Also, don't know yet. I am happy to test it! In my opinion both is fine, as long as these 2 new options are properly documented. It is really meant for collections where there are no subscriptions whatsoever. But it makes sense that maybe polling of course is better than nothing.

Another idea ad 3): I could also at least implement a simple check whenever a Collection is observe()d that is actually excluded and throw a server warning that the subscription will not work.

Is there anyone else who is willing to contribute here? I might be biased of course but I honestly believe this change can help with some Meteor scaling issues a lot for some projects out there! Really any help is very much appreciated! ❤️

@Twisterking
Copy link
Author

Okay, so I finally managed to make the tests work! 🎉
Also, I implemented a console.warn whenever a subscription on an excluded or not included collection is created.

Please have a look!

@zodern
Copy link
Member

zodern commented Feb 20, 2024

ad 1) good point indeed, to be honest I don't know.
ad 2) same here. Maybe I can ask you of any other maintainer to help here a bit? :)
ad 3) I thought about this too. Also, don't know yet. I am happy to test it! In my opinion both is fine, as long as these 2 new options are properly documented. It is really meant for collections where there are no subscriptions whatsoever. But it makes sense that maybe polling of course is better than nothing.

For 1), it might be worth mentioning in the docs that it would be inefficient if the watched collections are rarely updated, and other collections have a very large number of updates.

  1. I might create a package to experiment with this and some other ideas

  2. There are quite a few scenarios where Meteor is unable to use the oplog for an observer, and Meteor always falls back to polling in those situations. For consistency, it would be good if Meteor did the same thing here. Throwing an error or logging a warning might also work, though it should be done when creating the observer instead of in _publishCursor to handle all of the ways an observer can be created. The relevant function for either solution (falling back to polling, or throwing an error) would probably be

    MongoConnection.prototype._observeChanges = function (

@rj-david
Copy link
Contributor

rj-david commented Feb 21, 2024

On the discussion on where to place the configuration, in terms of developer experience, is it possible to do this in the definition of the collection (also where you normally add the schema). So when a developer creates a new collection, the developer can configure that collection for oplog tailing or not on the same file. Moving it outside the creation of the collection will result for most developers to forget to disable/enable oplog tailing. This has the benefit of adding it as a snippet/template for Collection files.

Just an idea and of course if it is even possible

[addendum]
Similar to redis-oplog, we can configure reactivity for the collection or reactivity for specific mutations or queries in the Collection file together with the schema. Everything about a collection is therefore maintained in one file.

@Twisterking
Copy link
Author

Twisterking commented Feb 21, 2024

@zodern: Thank you, I will have a look into _observeChanges today!
@rj-david: to be honest I think this is too late in the lifecycle! I thought about this approach too, but whenever you create collections, the Oplog handle/cursor has already been created. So this approach won't work afaik. That's why, again, I went for the Meteor.settings approach in the end.

EDIT: @zodern: That was really helpful, thank you! I now have implemented, that in these cases Meteor indeed now falls back to longpolling! (see here)

…d by the Oplog. We now also fall back to long pooling in these cases too.
@Twisterking
Copy link
Author

@zodern @radekmie
Does anyone of the Meteor team have infos, when this PR might be merged and released? 😃
We are really desperately waiting for its release! ✌️

@harryadel
Copy link
Contributor

While I'm not part of the core team but if @zodern @radekmie can give the green light since they're responsible for reviewing, @denihs @Grubba27 can move in.

docs/source/api/collections.md Outdated Show resolved Hide resolved
docs/source/api/collections.md Outdated Show resolved Hide resolved
packages/mongo/oplog_tests.js Outdated Show resolved Hide resolved
packages/mongo/oplog_tests.js Outdated Show resolved Hide resolved
packages/mongo/oplog_tests.js Outdated Show resolved Hide resolved
packages/mongo/oplog_tests.js Outdated Show resolved Hide resolved
packages/mongo/package.js Outdated Show resolved Hide resolved
@radekmie
Copy link
Collaborator

@zodern

  1. Meteor tracks the last oplog entry it's seen to know where to resume tailing the oplog. If there's been a large number of newer oplog entries for databases or collections that are excluded, could resume tailing cause performance issues if mongo has to filter out the unneeded docs? This happens every time Meteor does not receive an oplog doc for 30 seconds, there is a failover, or in some other situations.

It may be a visibly long operation, but it should not be an issue - oplog collection is usually already in memory, so it'll be really fast to grep through.

  1. Would it be possible to make this automatic, at least for the databases? Meteor could track which databases have had observers created for them, and update the query whenever a new database is observed. It seems like as long as Meteor tracks the timestamp of the last oplog entry it processed, there shouldn't be any downside to restarting tailing with a new query. Meteor already does it frequently with the same query.

It could, sure. I'd rather make it in a separate step to get this PR out sooner.

@Twisterking
Copy link
Author

@radekmie thanks for your feedback! PR updated!

- fixed some tests in collection_tests.js which were not able to be re-run, because of already existing Mongo collections (collection names were missing a "+ test.id")
@Twisterking
Copy link
Author

@radekmie one more update done! Thanks again for your feedback!

packages/mongo/oplog_tests.js Outdated Show resolved Hide resolved
packages/mongo/collection_tests.js Show resolved Hide resolved
@Twisterking
Copy link
Author

Hello everyone! Thanks for approving the PR @radekmie !
Any news on a possible release date? As stated, this is really blocking us more and more! :(

@StorytellerCZ
Copy link
Collaborator

I would be for Meteor 2.16 with #12436 asap.

@rj-david
Copy link
Contributor

rj-david commented Mar 1, 2024

Hello everyone! Thanks for approving the PR @radekmie ! Any news on a possible release date? As stated, this is really blocking us more and more! :(

You can already use this without waiting for a new release since the changes are in the package and not in the core

@Twisterking
Copy link
Author

Hello everyone! Thanks for approving the PR @radekmie ! Any news on a possible release date? As stated, this is really blocking us more and more! :(

You can already use this without waiting for a new release since the changes are in the package and not in the core

How? The version of the package was not released yet, no? Sorry, don't fully understand how this works! Also, I forget to increase the version number of the mongo package! my bad!

What exactly do I need to do to use this version of the mongo core package?

@rj-david
Copy link
Contributor

rj-david commented Mar 1, 2024

What exactly do I need to do to use this version of the mongo core package?

https://guide.meteor.com/writing-atmosphere-packages#local-packages

@Twisterking
Copy link
Author

What exactly do I need to do to use this version of the mongo core package?

guide.meteor.com/writing-atmosphere-packages#local-packages

That won't work here! I tried to install my package as a local package! The issue is, that all the other packages still used the "default mongo package" behind the scenes, and not my "custom" local package. Couldn't get it to work when I tried it.

@rj-david
Copy link
Contributor

rj-david commented Mar 1, 2024

Short of releasing this package, the things that can be tested are:

  1. Incrementing the version
  2. Also forking those packages that used mongo locally

@StorytellerCZ
Copy link
Collaborator

If you use meteor from local checkout with this branch that should do the trick if local package override is failing.

@Twisterking
Copy link
Author

Any updates on when the release of 2.16 will happen? Hopefully incl. this PR please! ✌️

@Grubba27 Grubba27 added this to the Release 2.16 milestone Mar 7, 2024
@StorytellerCZ StorytellerCZ changed the base branch from devel to release-2.16 March 25, 2024 15:12
@nachocodoner nachocodoner mentioned this pull request Mar 28, 2024
7 tasks
@Twisterking
Copy link
Author

Updates? Is there a planned release date for 2.16 yet? We really need this change asap ...

@nachocodoner
Copy link
Member

Updates? Is there a planned release date for 2.16 yet? We really need this change asap ...

No estimated date yet. First, we're focused on the 3.x RC. Meanwhile, we aim to merge candidates and make quick gains into the 2.16 branch. The PR has been updated to provide more details about the plan. We'll announce a date as soon as we have one.

@nachocodoner nachocodoner merged commit 849cf00 into meteor:release-2.16 Apr 24, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants