🪟 🔧 Handle LD feature flags besides featureService.overwrites

[ambirdsall]

Background

Feature flags in LaunchDarkly serve multiple uses in airbyte cloud at various points in the lifecycle of a feature release. Sometimes they are used to opt specific people into the feature for testing; sometimes they are used to "soft launch" a feature in a way that can be rolled out and rolled back without the lag of a code update and deployment; sometimes they are used to grandfather certain users into the ability to continue using otherwise deprecated code.

The original implementation of our LaunchDarkly frontend integration put all feature toggles into a single LD "feature flag", with various experiences defined as variants on that single flag. Because all of a given flag's variants are mutually exclusive to a given user, this means we could not independently toggle different features for the whole userbase without auditing and possibly editing every other more targeted rule and worrying about rule precedence in LD. This added complexity and hassle scales up exponentially with the number of distinct feature toggles in use at a given moment.

External context

A chunk of the work for this feature happened in LaunchDarkly itself: for each feature toogle that has been managed to date with the shared featureService.overwrites feature flag needed to be ported to a new feature flag using an { enabled?: boolean } payload, along with all targeting rules used to apply that variant. More on this below.

What this changes

This adds logic to the LaunchDarkly integration that populates the FeatureService's overwrites to accept single-toggle feature flags. It hardcodes the assumption that all such flags will use a key that begins with the same featureService. prefix as the shared feature flag: this (otherwise arbitrary) criterion lets us quickly and reliably filter the list of feature flags in LD to the set that is specifically relevant to the frontend codebase for visibility and auditing. Closes #19726.

Here's a recording of a manual test of the new code, toggling the presence of two different sub-nav link via new, independent feature flags. At the time of recording this screencap, #20105 had not yet been merged into the feature branch, which is kind of ideal for demonstration purposes: explicitly using the "enable feature" and "disable feature" variants work identically for each feature, while using the "use application default" variants enables the "Data residency" link and disables the "dbt Cloud integration" one, using the cloud build's default features as of commit 7b19f59:

🚨 User Impact 🚨

Existing feature flags are handled exactly as before, so there should be zero user impact. There is one cohort of users who have been grandfathered into continued use of sub-one-hour sync scheduling, so some care should be taken to ensure that deploying this change causes no interruption to that targeting logic.

Manual testing

Unfortunately, there's not a great way to add automated tests for this feature: writing jest tests would require lots of time consuming effort to mock out large swaths of the application context before any assertions can be written; cypress tests would be ideal, but we only use LDExperimentService in cloud, and we don't yet have a test suite for that. So manual testing it is.

Tests done:

• In the feature branch, each existing feature can be toggled on, off, or to the application default in isolation via the new feature flags (tested via the LD development environment and a local frontend)
• In the feature branch, toggling a feature via the shared featureService.overwrites feature flag does not affect the UI (same testing setup)
• In the existing prod app, creating and toggling the new feature flags does not affect the UI (tested via a targeting rule matching my personal email in the LD production environment and cloud.airbyte.io)

Documentation

This seems worth documenting for the Airbyte team! That is not, strictly speaking, a concern for this PR, since it happens outside of the codebase; just calling it out to keep myself honest.

[timroes]

I like the idea of having a better mechanism to treat those as individual flags. Two notes before you go too deep into finalizing this yet:

• I think we should remove the existing logic and just convert rules we might have had for that into the new flags instead. Transfering the existing rules is not that much work that it would imho justify keeping the old logic around and needing to maintain it.
• The way this is currently implemented is having a flaw: As soon as a feature flag exists in LaunchDarkly it will deliver it's value to the user, even if targeting is off (it will just pick the value from the bottom of the page, besides "if targeting is off"). Since those are atm implemented at boolean values, that means as soon as we'd create an individual feature flag in launchDarkly it would start overwriting the feature in code (whether default or set from the workspace) always - either by disbaling it or enabling it. This is not desired behavior, since it basically would disable all our in product features and just have LaunchDarkly features at that point. We'll need a tristate per flag, to handle that, so that we can separate between "disabled"/"enabled"/"not overwritten".

[ambirdsall]

That's a pair of very good points, @timroes, thank you.

• I hadn't thought through the option of swapping the implementation of existing feature flags to something nicer, but there's nothing blocking that: the alternate, independent feature flag definitions will be straightforward to create in advance and will be silently ignored until we deploy updated code that can use them. They will also make manual e2e testing much easier, since we already have conditional UI toggled by those flags (in particular, the multi-cloud and dbt Cloud integration flags both control the cloud build's settings page content).
• For representing all three possible states of an override flag (set on, set off, no override), we can define our LD flags to return a JSON object with an optional boolean property overwrite instead of a top-level boolean value (cf. the connector.orderOverwrite flag); then we can represent the three valid states like so:

// enable feature from LD
{ overwrite: true }

// disable feature from LD
{ overwrite: false }

// no override; default variation
{}

[ambirdsall]

The most important external context from a business perspective: the new targeting rule to grandfather certain users into sub-one-hour sync schedules matches the old one, so those users won't see any disruption of their current usage.

[josephkmh]

Very nice work @ambirdsall! This is a solid improvement to working with feature service overwrites 👏🏻 I tested locally with the data geography & cron sync mode flags, and everything worked as expected. I also checked the LD flags on production, and the targeting for ThriveCap on featureService.ALLOW_SYNC_SUB_ONE_HOUR_CRON_EXPRESSIONS looks good.

The only thought I had while reading through - and this is definitely out of scope for this PR - is why do we have different semantics for "experiments" and the "featureService" flags? Experiments uses keys of an interface while FeatureItem uses an enum, experiments uses an rxjs observable while featureService overwrites use a callback function, experiments are camelCase while featureService is ALL_CAPS. Anyway, not pertinent to the core changes here, just strikes me as strangely inconsistent.

[timroes]

Just a thought, not a necessary blocker for this PR: Would it maybe be nicer if we'd use a string for the tristate, that can either be empty, "true" or "false"? That would be easier to type in LD and would have the benefit, that the default value for string experiments already is the empty string, while using the JSON flag here, we'll require to type {} for each of the new features to not break the code below that destructures the object?

[ambirdsall]

While it would save us a bit of typing, I found the slightly more verbose JSON definitions to be a bit more readable in the devtools network tab. To minimize the hassle of creating new flags, I made sure they can be copy-pasted verbatim from the new documentation in Notion.

That said, "enabled" | "disabled" | "" would have been a very clean type signature ¯\_(ツ)_/¯