Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/compass-collection/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,14 @@ import {
dataServiceLocator,
type DataServiceLocator,
type DataService,
connectionInfoRefLocator,
} from '@mongodb-js/compass-connections/provider';
import { collectionModelLocator } from '@mongodb-js/compass-app-stores/provider';
import type { WorkspacePlugin } from '@mongodb-js/compass-workspaces';
import { workspacesServiceLocator } from '@mongodb-js/compass-workspaces/provider';
import { experimentationServiceLocator } from '@mongodb-js/compass-telemetry/provider';
import { createLoggerLocator } from '@mongodb-js/compass-logging/provider';
import { preferencesLocator } from 'compass-preferences-model/provider';
import {
CollectionWorkspaceTitle,
CollectionPluginTitleComponent,
Expand All @@ -29,6 +33,10 @@ export const WorkspaceTab: WorkspacePlugin<typeof CollectionWorkspaceTitle> = {
dataService: dataServiceLocator as DataServiceLocator<keyof DataService>,
collection: collectionModelLocator,
workspaces: workspacesServiceLocator,
experimentationServices: experimentationServiceLocator,
connectionInfoRef: connectionInfoRefLocator,
logger: createLoggerLocator('COMPASS-COLLECTION'),
preferences: preferencesLocator,
}
),
content: CollectionTab,
Expand Down
2 changes: 2 additions & 0 deletions packages/compass-collection/src/modules/collection-tab.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type AppRegistry from '@mongodb-js/compass-app-registry';
import type { workspacesServiceLocator } from '@mongodb-js/compass-workspaces/provider';
import type { CollectionSubtab } from '@mongodb-js/compass-workspaces';
import type { DataService } from '@mongodb-js/compass-connections/provider';
import type { experimentationServiceLocator } from '@mongodb-js/compass-telemetry';

function isAction<A extends AnyAction>(
action: AnyAction,
Expand All @@ -20,6 +21,7 @@ type CollectionThunkAction<R, A extends AnyAction = AnyAction> = ThunkAction<
localAppRegistry: AppRegistry;
dataService: DataService;
workspaces: ReturnType<typeof workspacesServiceLocator>;
experimentationServices: ReturnType<typeof experimentationServiceLocator>;
},
A
>;
Expand Down
168 changes: 166 additions & 2 deletions packages/compass-collection/src/stores/collection-tab.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ import Sinon from 'sinon';
import AppRegistry from '@mongodb-js/compass-app-registry';
import { expect } from 'chai';
import type { workspacesServiceLocator } from '@mongodb-js/compass-workspaces/provider';
import type { experimentationServiceLocator } from '@mongodb-js/compass-telemetry';
import type { connectionInfoRefLocator } from '@mongodb-js/compass-connections/provider';
import { createNoopLogger } from '@mongodb-js/compass-logging/provider';
import { ReadOnlyPreferenceAccess } from 'compass-preferences-model/provider';
import { TestName } from '../../../compass-telemetry/src/growth-experiments';

const defaultMetadata = {
namespace: 'test.foo',
Expand All @@ -32,6 +37,32 @@ const mockCollection = {
},
};

const mockAtlasConnectionInfo = {
current: {
id: 'test-connection',
title: 'Test Connection',
connectionOptions: {
connectionString: 'mongodb://localhost:27017',
},
atlasMetadata: {
clusterName: 'test-cluster',
projectId: 'test-project',
orgId: 'test-org',
clusterUniqueId: 'test-cluster-unique-id',
clusterType: 'REPLICASET' as const,
clusterState: 'IDLE' as const,
metricsId: 'test-metrics-id',
metricsType: 'replicaSet' as const,
regionalBaseUrl: null,
instanceSize: 'M10',
supports: {
globalWrites: false,
rollingIndexes: true,
},
},
},
};

describe('Collection Tab Content store', function () {
const sandbox = Sinon.createSandbox();

Expand All @@ -42,7 +73,19 @@ describe('Collection Tab Content store', function () {

const configureStore = async (
options: Partial<CollectionTabOptions> = {},
workspaces: Partial<ReturnType<typeof workspacesServiceLocator>> = {}
workspaces: Partial<ReturnType<typeof workspacesServiceLocator>> = {},
experimentationServices: Partial<
ReturnType<typeof experimentationServiceLocator>
> = {},
connectionInfoRef: Partial<
ReturnType<typeof connectionInfoRefLocator>
> = {},
logger = createNoopLogger('COMPASS-COLLECTION-TEST'),
preferences = new ReadOnlyPreferenceAccess({
enableGenAIFeatures: true,
enableGenAIFeaturesAtlasOrg: true,
cloudFeatureRolloutAccess: { GEN_AI_COMPASS: true },
})
) => {
({ store, deactivate } = activatePlugin(
{
Expand All @@ -54,6 +97,10 @@ describe('Collection Tab Content store', function () {
localAppRegistry,
collection: mockCollection as any,
workspaces: workspaces as any,
experimentationServices: experimentationServices as any,
connectionInfoRef: connectionInfoRef as any,
logger,
preferences,
},
{ on() {}, cleanup() {} } as any
));
Expand All @@ -76,11 +123,128 @@ describe('Collection Tab Content store', function () {
const store = await configureStore(undefined, {
openCollectionWorkspaceSubtab,
});
store.dispatch(selectTab('Documents'));
store.dispatch(selectTab('Documents') as any);
expect(openCollectionWorkspaceSubtab).to.have.been.calledWith(
'workspace-tab-id',
'Documents'
);
});
});

describe('experimentation integration', function () {
it('should assign experiment when Atlas metadata is available', async function () {
const assignExperiment = sandbox.spy(() => Promise.resolve(null));

await configureStore(
undefined,
{},
{ assignExperiment },
mockAtlasConnectionInfo
);

await waitFor(() => {
expect(assignExperiment).to.have.been.calledOnceWith(
TestName.mockDataGenerator,
{
team: 'Atlas Growth',
}
);
});
});

it('should not assign experiment when Atlas metadata is missing', async function () {
const assignExperiment = sandbox.spy(() => Promise.resolve(null));
const mockConnectionInfoRef = {
current: {
id: 'test-connection',
title: 'Test Connection',
connectionOptions: {
connectionString: 'mongodb://localhost:27017',
},
// No atlasMetadata
},
};

await configureStore(
undefined,
{},
{ assignExperiment },
mockConnectionInfoRef
);

// Wait a bit to ensure assignment would have happened if it was going to
await new Promise((resolve) => setTimeout(resolve, 50));
expect(assignExperiment).to.not.have.been.called;
});

it('should handle missing experimentationServices gracefully and initialize successfully', async function () {
const store = await configureStore(
undefined,
{},
undefined, // No experimentationServices provided
mockAtlasConnectionInfo
);

// Store should still be functional despite missing experimentationServices
await waitFor(() => {
expect(store.getState())
.to.have.property('metadata')
.deep.eq(defaultMetadata);
});
});

it('should not assign experiment when AI features are disabled at the org level', async function () {
const assignExperiment = sandbox.spy(() => Promise.resolve(null));

const mockPreferences = new ReadOnlyPreferenceAccess({
enableGenAIFeatures: true,
enableGenAIFeaturesAtlasOrg: false, // Disabled at org level
cloudFeatureRolloutAccess: { GEN_AI_COMPASS: true },
});

const store = await configureStore(
undefined,
{},
{ assignExperiment },
mockAtlasConnectionInfo,
undefined,
mockPreferences
);

// Wait a bit to ensure assignment would have happened if it was going to
await new Promise((resolve) => setTimeout(resolve, 50));
expect(assignExperiment).to.not.have.been.called;

// Store should still be functional
await waitFor(() => {
expect(store.getState())
.to.have.property('metadata')
.deep.eq(defaultMetadata);
});
});

it('should handle assignment errors gracefully', async function () {
const assignExperiment = sandbox.spy(() =>
Promise.reject(new Error('Assignment failed'))
);

await configureStore(
undefined,
{},
{ assignExperiment },
mockAtlasConnectionInfo
);

await waitFor(() => {
expect(assignExperiment).to.have.been.calledOnce;
});

// Store should still be functional despite assignment error
await waitFor(() => {
expect(store.getState())
.to.have.property('metadata')
.deep.eq(defaultMetadata);
});
});
});
});
44 changes: 43 additions & 1 deletion packages/compass-collection/src/stores/collection-tab.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@ import reducer, {
import type { Collection } from '@mongodb-js/compass-app-stores/provider';
import type { ActivateHelpers } from '@mongodb-js/compass-app-registry';
import type { workspacesServiceLocator } from '@mongodb-js/compass-workspaces/provider';
import type { experimentationServiceLocator } from '@mongodb-js/compass-telemetry';
import type { connectionInfoRefLocator } from '@mongodb-js/compass-connections/provider';
import type { Logger } from '@mongodb-js/compass-logging/provider';
import {
isAIFeatureEnabled,
type PreferencesAccess,
} from 'compass-preferences-model/provider';
import { TestName } from '../../../compass-telemetry/src/growth-experiments';
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't use relative imports for workspaces. You also shouldn't be importing from a file that is not explicitly exported. For this case you probably want to re-export this enum?

Suggested change
import { TestName } from '../../../compass-telemetry/src/growth-experiments';
import { ExperimentTestName } from '@mongodb-js/compass-telemetry/provider';


export type CollectionTabOptions = {
/**
Expand All @@ -31,18 +39,29 @@ export type CollectionTabServices = {
collection: Collection;
localAppRegistry: AppRegistry;
workspaces: ReturnType<typeof workspacesServiceLocator>;
experimentationServices: ReturnType<typeof experimentationServiceLocator>;
connectionInfoRef: ReturnType<typeof connectionInfoRefLocator>;
logger: Logger;
preferences: PreferencesAccess;
};

export function activatePlugin(
{ namespace, editViewName, tabId }: CollectionTabOptions,
services: CollectionTabServices,
{ on, cleanup }: ActivateHelpers
) {
): {
store: ReturnType<typeof createStore>;
deactivate: () => void;
} {
const {
dataService,
collection: collectionModel,
localAppRegistry,
workspaces,
experimentationServices,
connectionInfoRef,
logger,
preferences,
} = services;

if (!collectionModel) {
Expand All @@ -64,6 +83,7 @@ export function activatePlugin(
dataService,
workspaces,
localAppRegistry,
experimentationServices,
})
)
);
Expand All @@ -86,6 +106,28 @@ export function activatePlugin(

void collectionModel.fetchMetadata({ dataService }).then((metadata) => {
store.dispatch(collectionMetadataFetched(metadata));

// Assign experiment for Mock Data Generator:
// Only assign when experimentationServices.assignExperiment is initialized,
// we're connected to Atlas,
// and the org-level setting for AI features is enabled
if (
experimentationServices?.assignExperiment && // Ensures experimentation services are available
Copy link
Collaborator

Choose a reason for hiding this comment

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

As you are providing default context value, in what case are they not available?

connectionInfoRef.current?.atlasMetadata?.clusterName && // Ensures we only assign in Atlas
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might want to check the clusterState instead of the clusterName, like in this util function. We probably want to only assign if the cluster is in IDLE state or so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I looked at the connectable function, but I think we may actually want to assign regardless of cluster status. Once we're at the collections plugin, we should already be connected anyway.
We just want to make sure we're in Atlas.

Seems like other checks for Atlas in the codebase just check for atlasMetadata.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But maybe we drop the clusterName check to match other patterns?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it! Up to you then!

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can't connect to clusters and get to this screen if clusters are not in connectable states, there's no reason to check for specific states here

isAIFeatureEnabled(preferences.getPreferences()) // Ensures org-level AI features setting is enabled
) {
void experimentationServices
.assignExperiment(TestName.mockDataGenerator, {
team: 'Atlas Growth',
})
.catch((error) => {
logger.debug('Mock Data Generator experiment assignment failed', {
experiment: TestName.mockDataGenerator,
namespace: namespace,
error: error instanceof Error ? error.message : String(error),
});
});
}
});

return {
Expand Down
7 changes: 4 additions & 3 deletions packages/compass-telemetry/src/experimentation-provider.tsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
import React, { createContext, useContext, useRef } from 'react';
import type { types } from '@mongodb-js/mdb-experiment-js';
import type { typesReact } from '@mongodb-js/mdb-experiment-js/react';
import type { TestName } from './growth-experiments';

type UseAssignmentHook = (
experimentName: string,
experimentName: TestName,
trackIsInSample: boolean,
options?: typesReact.UseAssignmentOptions<types.TypeData>
) => typesReact.UseAssignmentResponse<types.TypeData>;

type AssignExperimentFn = (
experimentName: string,
experimentName: TestName,
options?: types.AssignOptions<string>
) => Promise<types.AsyncStatus | null>;

Expand All @@ -34,7 +35,7 @@ const initialContext: CompassExperimentationProviderContextValue = {
},
};

const ExperimentationContext =
export const ExperimentationContext =
createContext<CompassExperimentationProviderContextValue>(initialContext);

// Provider component that accepts MMS experiment utils as props
Expand Down
1 change: 1 addition & 0 deletions packages/compass-telemetry/src/growth-experiments.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export enum TestName {
earlyJourneyIndexesGuidance = 'EARLY_JOURNEY_INDEXES_GUIDANCE_20250328',
mockDataGenerator = 'MOCK_DATA_GENERATOR_20251001',
}
1 change: 1 addition & 0 deletions packages/compass-telemetry/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ export type {
} from './types';

export { CompassExperimentationProvider } from './experimentation-provider';
export { experimentationServiceLocator } from './provider';
Comment on lines 9 to +10
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be exported from the /provider named export, not from the index

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jcobis as I mentioned above, locators and hooks shouldn't be exported from the index export, only from provider, please remove this export

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I missed this somehow! Removed it here

Loading
Loading