-
Notifications
You must be signed in to change notification settings - Fork 228
feat(compass-collection): Collection Plugin Experimentation Assignment Integration – CLOUDP-333845 #7165
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Integrates experimentation assignment into the Compass collection plugin to support the Mock Data Generator feature. When users navigate to a collection in Atlas with AI features enabled at the org level, they are automatically assigned to the 'mock-data-generator' experiment.
- Added experimentation services integration to collection plugin
- Implemented experiment assignment during collection metadata fetch for Atlas connections only
- Added org-level AI feature gating and comprehensive test coverage
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/compass-telemetry/src/provider.tsx | Adds service locator for experimentation services and exports experimentation context |
packages/compass-telemetry/src/index.ts | Exports the new experimentation service locator |
packages/compass-telemetry/src/experimentation-provider.tsx | Makes ExperimentationContext exportable for service locator usage |
packages/compass-collection/src/stores/collection-tab.ts | Implements experiment assignment logic during collection metadata fetch |
packages/compass-collection/src/stores/collection-tab.spec.ts | Adds comprehensive test coverage for experiment assignment scenarios |
packages/compass-collection/src/modules/collection-tab.ts | Updates type definitions to include experimentation services |
packages/compass-collection/src/index.ts | Adds service locator dependencies for experimentation integration |
experimentationServices && | ||
experimentationServices.assignExperiment && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The condition checks experimentationServices.assignExperiment
twice - once for its existence and once for being truthy. Consider combining these checks or using optional chaining: experimentationServices?.assignExperiment
for cleaner code.
experimentationServices && | |
experimentationServices.assignExperiment && | |
experimentationServices?.assignExperiment && |
Copilot uses AI. Check for mistakes.
import { createServiceLocator } from '@mongodb-js/compass-app-registry'; | ||
import { createTrack, type TelemetryServiceOptions } from './generic-track'; | ||
import { useLogger } from '@mongodb-js/compass-logging/provider'; | ||
import type { TrackFunction } from './types'; | ||
import { TestName } from './growth-experiments'; | ||
import { ExperimentationContext } from './experimentation-provider'; | ||
import type { types } from '@mongodb-js/mdb-experiment-js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really just a small note and not a request for changes in this PR, but when I've seen packages which have exports with highly generic names, they are typically being accessed through a generic import; e.g.
import type * as exp from '@mongodb-js/mdb-experiment-js';
// ...
Promise<exp.types.AsyncStatus | null>
not a big deal but in larger files this can keep one from jiggling around many unrelated concepts in the single "global namespace" of a given file, obviously for this one this is totally fine
// we're connected to Atlas, | ||
// and the org-level setting for AI features is enabled | ||
if ( | ||
experimentationServices?.assignExperiment && // Ensures experimentation services are available |
There was a problem hiding this comment.
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?
isAIFeatureEnabled, | ||
type PreferencesAccess, | ||
} from 'compass-preferences-model/provider'; | ||
import { TestName } from '../../../compass-telemetry/src/growth-experiments'; |
There was a problem hiding this comment.
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?
import { TestName } from '../../../compass-telemetry/src/growth-experiments'; | |
import { ExperimentTestName } from '@mongodb-js/compass-telemetry/provider'; |
export { CompassExperimentationProvider } from './experimentation-provider'; | ||
export { experimentationServiceLocator } from './provider'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Edit: Resolved by switching the export of the service locator |
// Assign experiment for Mock Data Generator | ||
// Only assign when we're connected to Atlas and the org-level setting for AI features is enabled | ||
if ( | ||
connectionInfoRef.current?.atlasMetadata?.clusterName && // Ensures we only assign in Atlas |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Our main CI is green now and there is too many failures here for them to be unrelated |
There are pretty clear error messages in the CI log and it's very easy to reproduce by following the same steps that CI is taking (running
Please adjust the code to resolve these typescript compilation issues |
isAIFeatureEnabled, | ||
type PreferencesAccess, | ||
} from 'compass-preferences-model/provider'; | ||
import { TestName } from '@mongodb-js/compass-telemetry/provider'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestName is a very generic name for this type, can we add experimentation to it to make it clearer what this refers to?
Description
Integrates experimentation assignment into the collection plugin to support the Mock Data Generator feature. When users navigate to a collection in Atlas with AI features enabled at the org level, they are automatically assigned to the 'mock-data-generator' experiment.
Changes:
isAIFeatureEnabled()
Implementation Details:
Checklist
Motivation and Context
The Mock Data Generator feature requires users to be assigned to the experiment when they navigate to collections in Atlas. This ticket implements the experimentation assignment integration as specified in the engineering goals:
"When a user in the experiment selects a collection from the Data Explorer and the org-level setting for Generative AI features is enabled"
The implementation ensures experiment assignment only occurs when:
atlasMetadata
)enableGenAIFeaturesAtlasOrg: true
)Types of changes