feat(atlas-service): ping if ai feature enabled COMPASS-7193 #4840
Conversation
Anemy
added
feature flagged
| // Default to what's already in Compass when we can't fetch the preference. | ||
| log.error( | ||
| mongoLogId(1_001_000_243), | ||
| 'AtlasService', | ||
| 'Failed to load if the AI feature is enabled', | ||
| { error: (err as Error).stack } | ||
| ); |
There was a problem hiding this comment.
This doesn't look right to me: is it okay that if the feature is disabled now and we somehow failed to fetch it, it will not be disabled if it was enabled before?
There was a problem hiding this comment.
@mcasimir and I were chatting on this. When someone is not connected to the internet when they first open Compass we still want them to be able to use the feature. If we set it to disabled then they can't use it, even if they've used it in the past.
It makes a case for having some polling or something here.
There was a problem hiding this comment.
Doesn't seem like it's a good way to work around a corner case. In combination with storing this value with preferences on disk this error handling creates a very easy way to bypass the whole check. Feel free to ignore it if that's what we already decided on
There was a problem hiding this comment.
I think we are assuming these requirements here:
- the ping should be completely transparent for the end user, failures should never affect the usage in any way
- the last value "read" should be used by Compass: the purpose of the flag is to allow for a progressive rollout to % of users and for a rollback of a % of the usage (in extreme cases).
- we don't need a security mechanism, is fine if someone finds ways to bypass it
- the feature should not be disabled intermittently (due to a failed ping), once is enabled for a user, it should stay like that unless is intentionally rolled back
Is there anything that is not matching here?
There was a problem hiding this comment.
the feature should not be disabled intermittently (due to a failed ping), once is enabled for a user, it should stay like that unless is intentionally rolled back
What is "rolled back" in this case? If backend returned feature as enabled initially, can it return it as disabled later? If the answer is "yes", then my concern still applies, but it seems like we are okay with all the implications of that. If the answer is "no", then we probably shouldn't try to fetch the flags again if the flags are not stored yet, feature is already enabled.
There was a problem hiding this comment.
Yes, the backend can return the feature as enabled initially, and then return it as disabled later.
My impression is that we're okay with the implications there. @mcasimir is that alright with you?
Ultimately we can manage access and request load through the backend.
There was a problem hiding this comment.
Yeah, that's to keep the possibility to scale down. Would be good to double check if we really want that though.
then my concern still applies
Do you refer to this? "is it okay that if the feature is disabled now and we somehow failed to fetch it, it will not be disabled if it was enabled before?"
I guess, wouldn't it be bad if for a sporadic failure the feature is going to get disabled?
There was a problem hiding this comment.
It wouldn't be great, but if we want this to be a reliable killswitch for the feature, we should probably not ignore errors and have another pass as designing how we want to handle that. If we see this as a simple and temporary rollout mechanism and just generally okay with people escaping it, then it's probably fine to just ignore this
There was a problem hiding this comment.
ahh now i see what you mean. no is only intended to control the roll-out, maybe it would be helpful to leave a comment, or just change the language from "featureEnablement" to something like "featureRolledOutToUser" 🤷♂️
There was a problem hiding this comment.
That would be great to make it clearer in the code what's the purpose of this then, yes 👍
| cloudFeatureRolloutAccess: { | ||
| ui: false, | ||
| cli: false, | ||
| global: false, | ||
| description: null, | ||
| validator: z | ||
| .object({ | ||
| GEN_AI_COMPASS: z.boolean().optional(), | ||
| }) | ||
| .optional(), | ||
| type: 'object', | ||
| }, |
There was a problem hiding this comment.
Is it right that we are storing this on disk with user preferences? If I block requests to the hello endpoint and modify the preferences I'd be able to enable anything I want it seems
There was a problem hiding this comment.
I think that's okay. We have rate limiting on the backend.
| const { args } = fetchStub.getCall(0); | ||
|
|
||
| expect(AtlasService['fetch']).to.have.been.calledOnce; | ||
| expect(args[0]).to.eq(`http://example.com/ai/api/v1/hello/test`); |
There was a problem hiding this comment.
Just FYI, you can do this shorter using calledOnceWith
| const { args } = fetchStub.getCall(0); | |
| expect(AtlasService['fetch']).to.have.been.calledOnce; | |
| expect(args[0]).to.eq(`http://example.com/ai/api/v1/hello/test`); | |
| expect(AtlasService['fetch']).to.have.been.calledOnceWith('http://example.com/ai/api/v1/hello/test') |
COMPASS-7193
This adds a request to the
helloendpoint (https://github.com/10gen/compass-mongodb-com/pull/98). This returns if the ai feature is enabled/disabled. This will be used to incrementally rollout the ai feature.We make a request to this endpoint every time Compass is started. I'm thinking we'll eventually remove this once we're feeling solid about the feature and have it released to everyone.
In COMPASS-7147 we will add another preference for command line disabling the ai functionality.