chore(data-service, connections): make sure that instance info is only fetched once on initial connection COMPASS-9549 COMPASS-7675 #7089

gribnoysup · 2025-07-03T15:11:20Z

Something that came up as a low hanging fruit in terms of potentially speeding up the initial compass connection. We are running all these dataService.instance requests in parallel and so if your connection pool is big enough and can be poulated quickly it's not really a big issue, but in compass-web where initial connection times are slow and pool is pretty limited, removing redundant calls might have a positive effect on connection times

Definitely better to review with ws changes off, diff is over-reporting here

…y fetched once on initial connection

gribnoysup · 2025-07-03T15:13:26Z

packages/compass-connections/src/stores/connections-store-redux.ts

+        // pass it down to telemetry and instance model. This is a relatively
+        // expensive dataService operation so we're trying to keep the usage
+        // very limited
+        const instanceInfo = await dataService.instance();


This is somewhat a breaking change in Compass behavior as described in COMPASS-7675 but I think it's probably okay to do so as we were already discussing doing this. Basically the main change in the behavior here is that some connection related errors that would show up in this weird state where you connected, but we didn't even manage to get to the listing database part of the logic before now will start showing up as part of the connection flow, this doesn't sound too bad to me, but I also asked Betsy to confirm in the ticket

gribnoysup · 2025-07-03T15:16:21Z

packages/data-service/src/data-service.ts

  /**
   * Get the current instance details.
+   *
+   * @deprecated avoid using `instance` directly and use `InstanceModel` instead


Literally everywhere except for those initial / inside models fetches we should be using models instead, so I'm adding this marking to make IDEs highlight the method usage

nbbeeken · 2025-07-03T21:30:01Z

packages/compass-app-stores/src/stores/instance-store.ts

+        connections.getDataServiceForConnection(instanceConnectionId);
+      const connectionString = dataService.getConnectionString();
+      const firstHost = connectionString.hosts[0] || '';
+      const [hostname, port] = firstHost.split(':');


Not new code, but isn't this problematic for IPv6 addresses

Yeah, good catch (and we are accounting for it in other places), I'll adjust while I'm moving stuff around here

Updated in a8f551c

nbbeeken · 2025-07-03T21:33:16Z

packages/compass-connections/src/stores/connections-store-redux.ts

+        // We're trying to optimise the initial Compass loading times here: to
+        // make sure that the driver connection pool doesn't immediately get
+        // overwhelmed with requests, we fetch instance info only once and then


Yea, in fact it doesn't really matter what we set our pool size to, browsers do not permit more than 6 parallel connections to be creating at the same time.

It obviously matters for our total count in the long run, but at least at boot that's our bottleneck.

We still have multiplexing in the plans, right? Just wondering

nbbeeken · 2025-07-08T17:48:39Z

I had a ticket that was somewhat covering the change you're proposing here, just repurposing it to track this: https://jira.mongodb.org/browse/COMPASS-9549

I assigned the ticket to you, but happy to pick up this if needs further iteration (seems like it is pretty good though!)

gribnoysup · 2025-07-14T09:02:07Z

Got confirmation from Betsy that this is an okay change in behavior from her perspective, so marking as ready for review

lerouxb · 2025-07-14T10:22:49Z

packages/compass-app-stores/src/stores/instance-store.ts

+    connections,
+    'connected',
+    function (
+      instanceConnectionId: string,


We're now tacking on more positional arguments. Wouldn't one object where you can just pick what you want from it start to make more sense? Or do we have too many things using this and that would involve lots of changes? Or is there some other reason I can't immediately spot why we can't change it?

I'll take a closer look at the usage, adding a positional was just the easiest for me, but generally speaking yes, we can probably pack it all into one object (also instanceConnectionId is just connectionInfo.id, so I'll just drop it I think)

I went back and forth on this and decided not to change this, I started moving them to an object of properties, but then disconnect event is different and wanted to keep them consistent, but then connectionId is like the only used argument in disconnect and having it inside an object was weird, so I was trying different combinations, like maybe keeping connectionId as an argument, but moving everything else into an object in a second argument, and this also didn't look right, then I thought I'm overthinking it and it was cleaner to just keep it as is. Hope that's okay

lerouxb · 2025-07-14T10:24:47Z

packages/compass-app-stores/src/stores/instance-store.ts

+      instance.set({
+        status: 'ready',
+        statusError: null,
+        ...(instanceInfo as Partial<MongoDBInstanceProps>),


Maybe I'm reading this wrong, but don't you want status and statusError to override what's already on instanceInfo rather than the other way around?

Actually, I can just pass it all one line higher to the constructor. Generally speaking you're right about this, on practice we won't get those as part of instanceInfo, but absolutely doesn't hurt to be more precise about this

Updated in a8f551c

lerouxb · 2025-07-14T10:29:06Z

packages/compass-connections/src/stores/connections-store-redux.ts

-                err
-              );
-            });
+          void dispatch(


So showEndOfLifeMongoDBWarningModal() now may or may not show the modal because it looks up latestEndOfLifeServerVersion and decides for itself?

Wondering if it should be renamed to something like maybeShowEndOfLifeMongoDBWarningModal() to reflect that. (yes I realise that that's a wildly long and specific name as I type it..)

Yeah, I don't mind renaming it. Also maybe I should've kept it under an if statement here to be honest, I'll take another look and adjust either way

Decided to split into two methods, updated in 7ecdc82

lerouxb

Looks good to me. Just some nitpicky comments, feel free to ignore.

…model; pass instance info to constructor instead of extra .set call

chore(data-service, connections): make sure that instance info is onl…

626c999

…y fetched once on initial connection

gribnoysup requested a review from a team as a code owner July 3, 2025 15:11

gribnoysup marked this pull request as draft July 3, 2025 15:11

gribnoysup commented Jul 3, 2025

View reviewed changes

nbbeeken self-requested a review July 3, 2025 15:18

nbbeeken reviewed Jul 3, 2025

View reviewed changes

nbbeeken changed the title ~~chore(data-service, connections): make sure that instance info is only fetched once on initial connection~~ chore(data-service, connections): make sure that instance info is only fetched once on initial connection COMPASS-9549 Jul 8, 2025

gribnoysup marked this pull request as ready for review July 14, 2025 09:02

Merge branch 'main' into do-not-overfetch-instance-info

d00eb2d

gribnoysup added the no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion) label Jul 14, 2025

lerouxb reviewed Jul 14, 2025

View reviewed changes

lerouxb approved these changes Jul 14, 2025

View reviewed changes

gribnoysup added 3 commits July 14, 2025 18:43

chore(connections): separate should show and show methods

7ecdc82

chore(app-stores): handle ipv6 hosts when building data for instance …

a8f551c

…model; pass instance info to constructor instead of extra .set call

chore(connections): no need for promise.all

980508f

gribnoysup merged commit 01e92b0 into main Jul 15, 2025
59 checks passed

gribnoysup deleted the do-not-overfetch-instance-info branch July 15, 2025 08:15

gribnoysup changed the title ~~chore(data-service, connections): make sure that instance info is only fetched once on initial connection COMPASS-9549~~ chore(data-service, connections): make sure that instance info is only fetched once on initial connection COMPASS-9549 COMPASS-7675 Aug 6, 2025

chore(data-service, connections): make sure that instance info is only fetched once on initial connection COMPASS-9549 COMPASS-7675 #7089

chore(data-service, connections): make sure that instance info is only fetched once on initial connection COMPASS-9549 COMPASS-7675 #7089

Uh oh!

Conversation

gribnoysup commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gribnoysup Jul 3, 2025 • 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!

gribnoysup Jul 3, 2025 • 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!

gribnoysup Jul 4, 2025 • 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!

nbbeeken commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gribnoysup commented Jul 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup Jul 14, 2025 • 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!

lerouxb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gribnoysup commented Jul 3, 2025 •

edited

Loading

gribnoysup Jul 3, 2025 •

edited

Loading

gribnoysup Jul 3, 2025 •

edited

Loading

gribnoysup Jul 4, 2025 •

edited

Loading

nbbeeken commented Jul 8, 2025 •

edited

Loading

gribnoysup Jul 14, 2025 •

edited

Loading