Testing the Removal of Artificial Latency behind a feature flag

[arafatkatze]

This PR aims to remove the artificial delay introduced for low-performance languages and specific AST nodes, as outlined in th issue. The artificial delay was originally implemented for low performance languages so that users are not spammed with unhelpful suggestions. , but it has become less relevant over time, and it may now impact the predictability of autocomplete latency and user experience (UX).

Changes:

Introduce a feature flag:

A new feature flag has been added to toggle the artificial delay.

• Objective: The main objective is to achieve predictable latency and a consistent autocomplete UX for all languages.

• After merging this PR, the feature flag will be enabled for controlled testing, and data will be collected to assess the impact on CAR and latency for all languages

• Depending on the results, the team will decide whether to remove the artificial delay logic entirely.

Test Plan:

I tested this PR locally by enabling and disabling the feature flag on my local Sourcegraph instance(I added the feature flag on it and then set the true/false values). I also used the debugger to run VS Code Extension on Desktop

Changelog

[dominiccooney]

Is getting feature flags really fast, when they're cached? I'm just wondering if making getFeatureFlag async means you're going to introduce another kind of random delay that depends on how busy the main thread is.

Could we drill down on the point mentioned in the test?

[valerybugakov]

We can use vi.spyOn here to spy on relevant featureFlagProvider methods and replace their return values with spy.mockReturnValue(). Or we can mock the whole import by the path. With that in place, vi.restoreAllMocks() would make featureFlagProvider.instance = originalFeatureFlagProviderInstance obsolete.

Also, we can spy on the graphql client, which resolves feature flag values. That would test the feature flag provider logic, too.

[arafatkatze]

@valerybugakov I got something better. I adopted the new logic to get the value of the feature flag by doing a subscription
and then changed the function to have the args

export function getArtificialDelay(params: {
    featureFlags: LatencyFeatureFlags
    uri: string
    languageId: string
    codyCompletionDisableLowPerfLangDelay: boolean
    completionIntent?: CompletionIntent
}): number {

This made the testing of the function much easier as I only have to tweak the params codyCompletionDisableLowPerfLangDelay.

NOTE:

1. The logic of subscription to a feature flag + GQL CLient is also tested here(but that's a separate thing). Since that logic is already tested I don't see the reason to make this an additional E2E test when those are two separate things.
2. NOTE: I also tested the logic locally for this as Quinn recently merged this PR make some services more reactive to config #5550 So whenever Client Config is changed the Feature flags are "reloaded" and I could verify that subscription logic. The nuance here is that everytime you change the feature flag on the server that doesn't mean its automatically uploaded on the client. You either reload the client or change the config settings of the client for the feature flags to be reloaded(Quinn explained that to me and I tried it).

[valerybugakov]

This is great!

[github-actions]

‼️ Hey @sourcegraph/cody-security, please review this PR carefully as it introduces the usage of an unsafe_ function or abuses PromptString.

[arafatkatze]

@valerybugakov Apologies I was still in the middle of fixing the PR when you saw the new changes. It wasn't in the right state(I will try to be mindful about communicating that the PR is not in the right state)

[arafatkatze]

@valerybugakov I updated the PR again with all the recommendations that you suggested and now its a lot lot cleaner.

With the latest changes from Quinn I have the subscription/update logic for feature flags also(Read this comment)

As for testing I have also incorporated the changes that you recommended so its easy to understand the tests too.

Thanks again for such a thorough review, I would like another one now.

[valerybugakov]

Do we need both of these? There's an example of using CodyAutocompleteTracing in the InlineCompletionItemProvider constructor, which relies only on one call sequence to initialize and keep the local value up to date.

[arafatkatze]

Both of these are technically NOT needed but I wanted to maintain the logical consistency with "initializing" something and then subscribing to its value but yeah since its redundant I don't see the reason to actually keep it for anything other than having a reasonably understandable logic.
So I removed it

[valerybugakov]

Thank you for addressing all the comments! 💜

[valerybugakov]

We need to update the variable name here because it doesn't match the underlying feature flag.

[arafatkatze]

Done