add perf feature-flag; ensure compatability with LaunchDarkly (LDv5) …

…users (#21534)

* init commit of flags.yml

* add PerfBackgroundJsonValidation flag

* update Multi contexts to support LDv5

* add test for no context behavior

* allow ConfigFileClient to support a null Path

* add comment

* remove test that no longer applies

* change factory to always return one of the implementations

* add flags.yml config; quote ports

* replace @requires annotations with hand-checking

* make the code easier to read

airbyte-featureflag/src/main/kotlin/Client.kt

@@ -33,26 +33,30 @@ sealed interface FeatureFlagClient {
 }
 
 /**
- * Config file based feature-flag client. Feature-flag are derived from a yaml config file.
- * Also supports flags defined via environment-variables via the [EnvVar] class.
+ * Config file based feature-flag client.
+ *
+ * If no [config] is provided, will return the default state for each [Flag] requested.
+ * Supports [EnvVar] flags as well.
  *
- * @param [config] the location of the yaml config file that contains the feature-flag definitions.
- * The [config] will be watched for changes and the internal representation of the [config] will be updated to match.
+ * @param [config] optional location of the yaml config file that contains the feature-flag definitions.
+ * If the [config] is provided, it will be watched for changes and the internal representation of the [config] will be updated to match.
  */
-class ConfigFileClient(config: Path) : FeatureFlagClient {
+class ConfigFileClient(config: Path?) : FeatureFlagClient {
     /** [flags] holds the mappings of the flag-name to the flag properties */
-    private var flags: Map<String, ConfigFileFlag> = readConfig(config)
+    private var flags: Map<String, ConfigFileFlag> = config?.let { readConfig(it) } ?: mapOf()
 
     /** lock is used for ensuring access to the flags map is handled correctly when the map is being updated. */
     private val lock = ReentrantReadWriteLock()
 
     init {
-        if (!config.isRegularFile()) {
-            throw IllegalArgumentException("config must reference a file")
-        }
+        config?.also {
+            if (!it.isRegularFile()) {
+                throw IllegalArgumentException("config must reference a file")
+            }
 
-        config.onChange {
-            lock.write { flags = readConfig(config) }
+            it.onChange {
+                lock.write { flags = readConfig(config) }
+            }
         }
     }
 
@@ -175,9 +179,23 @@ private fun Path.onChange(block: () -> Unit) {
  * Once v6 is GA, this method would be removed and replaced with toLDContext.
  */
 private fun Context.toLDUser(): LDUser = when (this) {
-    is Multi -> throw IllegalArgumentException("LDv5 does not support multiple contexts")
-    else -> {
+    is Multi -> {
         val builder = LDUser.Builder(key)
+        with(contexts) {
+            // Add each individual context's value as an attribute on the LDUser.
+            // This allows for more granular targeting of feature-flag rules that target LDUser types.
+            forEach { builder.custom(it.kind, it.key) }
+
+            if (all { it.key == ANONYMOUS.toString() }) {
+                builder.anonymous(true)
+            }
+        }
+        builder.build()
+    }
+
+    else -> {
+        // for LDv5 Users, add the context type and valid as a custom attribute
+        val builder = LDUser.Builder(key).apply { custom(kind, key) }
         if (this.key == ANONYMOUS.toString()) {
             builder.anonymous(true)
         }

airbyte-featureflag/src/main/kotlin/Context.kt

@@ -39,8 +39,24 @@ data class Multi(val contexts: List<Context>) : Context {
     /** This value MUST be "multi" to properly sync with the LaunchDarkly client. */
     override val kind = "multi"
 
-    /** Multi contexts don't have a key */
-    override val key = ""
+    /**
+     * Multi contexts don't have a key, however for LDv5 reasons, a key must exist.
+     *
+     * Determine what the key should be based on the priority order of:
+     * workspace -> connection -> source -> destination -> user
+     * taking the first value
+     *
+     * When LDv5 support is dropped replace this with: override vale key = ""
+     */
+    override val key = when {
+        /** Intellij is going to recommend replacing .sortedBy with .minByOrNull, ignore this recommendation. */
+        fetchContexts<Workspace>().isNotEmpty() -> fetchContexts<Workspace>().sortedBy { it.key }.first().key
+        fetchContexts<Connection>().isNotEmpty() -> fetchContexts<Connection>().sortedBy { it.key }.first().key
+        fetchContexts<Source>().isNotEmpty() -> fetchContexts<Source>().sortedBy { it.key }.first().key
+        fetchContexts<Destination>().isNotEmpty() -> fetchContexts<Destination>().sortedBy { it.key }.first().key
+        fetchContexts<User>().isNotEmpty() -> fetchContexts<User>().sortedBy { it.key }.first().key
+        else -> throw IllegalArgumentException("unsupported context: ${contexts.joinToString { it.kind }}")
+    }
 
     init {
         // ensure there are no nested contexts (i.e. this Multi does not contain another Multi)

airbyte-featureflag/src/main/kotlin/Flags.kt

@@ -16,6 +16,8 @@ object AutoDetectSchema : EnvVar(envVar = "AUTO_DETECT_SCHEMA")
 object NeedStateValidation : EnvVar(envVar = "NEED_STATE_VALIDATION")
 object ApplyFieldSelection : EnvVar(envVar = "APPLY_FIELD_SELECTION")
 
+object PerfBackgroundJsonValidation : Temporary(key = "performance.backgroundJsonSchemaValidation")
+
 object FieldSelectionWorkspaces : EnvVar(envVar = "FIELD_SELECTION_WORKSPACES") {
     override fun enabled(ctx: Context): Boolean {
         val enabledWorkspaceIds: List<String> = fetcher(key)

airbyte-featureflag/src/main/kotlin/config/Factory.kt

@@ -10,8 +10,6 @@ import io.airbyte.featureflag.FeatureFlagClient
 import io.airbyte.featureflag.LaunchDarklyClient
 import io.micronaut.context.annotation.Factory
 import io.micronaut.context.annotation.Property
-import io.micronaut.context.annotation.Requirements
-import io.micronaut.context.annotation.Requires
 import jakarta.inject.Singleton
 import java.nio.file.Path
 
@@ -20,22 +18,23 @@ internal const val CONFIG_OSS_KEY = "airbyte.feature-flag.path"
 
 @Factory
 class Factory {
-    @Requirements(
-        Requires(property = CONFIG_LD_KEY),
-        Requires(missingProperty = CONFIG_OSS_KEY),
-    )
     @Singleton
-    fun Cloud(@Property(name = CONFIG_LD_KEY) apiKey: String): FeatureFlagClient {
-        val client = LDClient(apiKey)
-        return LaunchDarklyClient(client)
-    }
+    fun featureFlagClient(
+        @Property(name = CONFIG_LD_KEY) apiKey: String,
+        @Property(name = CONFIG_OSS_KEY) configPath: String,
+    ): FeatureFlagClient {
+        // I cannot get the @Requires annotation to work to load one instance if a property is set and another instance if unset.
+        // Combined both cases together here instead resulting to manually doing the is-set check via the isNotBlank function.
+        if (apiKey.isNotBlank()) {
+            val client = LDClient(apiKey)
+            return LaunchDarklyClient(client)
+        }
 
-    @Requirements(
-        Requires(property = CONFIG_OSS_KEY),
-        Requires(missingProperty = CONFIG_LD_KEY),
-    )
-    fun Platform(@Property(name = CONFIG_OSS_KEY) configPath: String): FeatureFlagClient {
-        val path = Path.of(configPath)
+        val path: Path? = if (configPath.isNotBlank()) {
+            Path.of(configPath)
+        } else {
+            null
+        }
         return ConfigFileClient(path)
     }
 }

airbyte-featureflag/src/test/kotlin/ClientTest.kt

@@ -10,7 +10,6 @@ import io.airbyte.featureflag.EnvVar
 import io.airbyte.featureflag.FeatureFlagClient
 import io.airbyte.featureflag.Flag
 import io.airbyte.featureflag.LaunchDarklyClient
-import io.airbyte.featureflag.Multi
 import io.airbyte.featureflag.Temporary
 import io.airbyte.featureflag.TestClient
 import io.airbyte.featureflag.User
@@ -32,7 +31,7 @@ import kotlin.test.assertTrue
 
 class ConfigFileClient {
     @Test
-    fun `verify platform functionality`() {
+    fun `verify config-file functionality`() {
         val cfg = Path.of("src", "test", "resources", "feature-flags.yml")
         val client: FeatureFlagClient = ConfigFileClient(cfg)
 
@@ -57,6 +56,19 @@ class ConfigFileClient {
      *
      * TODO: move this to a different test suite
      */
+    @Test
+    fun `verify no-config file returns default flag state`() {
+        val client: FeatureFlagClient = ConfigFileClient(null)
+        val defaultFalse = Temporary(key = "default-false")
+        val defaultTrue = Temporary(key = "default-true", default = true)
+
+        val ctx = Workspace("workspace")
+        with(client) {
+            assertTrue { enabled(defaultTrue, ctx) }
+            assertFalse { enabled(defaultFalse, ctx) }
+        }
+    }
+
     @Test
     @Ignore
     fun `verify platform reload capabilities`() {
@@ -161,22 +173,6 @@ class LaunchDarklyClient {
         }
     }
 
-    @Test
-    fun `verify multi-context is not supported`() {
-        /**
-         * TODO replace this test once LDv6 is being used and Context.toLDUser no longer exists, to verify Multi support
-         */
-        val ldClient: LDClient = mockk()
-        every { ldClient.boolVariation(any(), any<LDUser>(), any()) } returns false
-
-        val client: FeatureFlagClient = LaunchDarklyClient(ldClient)
-        val multiCtx = Multi(listOf(User("test")))
-
-        assertFailsWith<IllegalArgumentException> {
-            client.enabled(Temporary(key = "test"), multiCtx)
-        }
-    }
-
     @Test
     fun `verify env-var flag support`() {
         val ldClient: LDClient = mockk()

airbyte-featureflag/src/test/kotlin/ContextTest.kt

@@ -9,18 +9,75 @@ import io.airbyte.featureflag.Source
 import io.airbyte.featureflag.User
 import io.airbyte.featureflag.Workspace
 import org.junit.jupiter.api.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertFailsWith
 
 class MultiTest {
     @Test
     fun `verify data`() {
-        val user = User("user-id")
+        val source = Source("source")
         val workspace = Workspace("workspace")
 
-        Multi(listOf(user, workspace)).also {
+        Multi(listOf(source, workspace)).also {
             assert(it.kind == "multi")
-            assert(it.key == "")
         }
     }
+
+    @Test
+    fun `Multi cannot contain another Multi`() {
+        val source = Source("source")
+        val workspace = Workspace("workspace")
+        val multi = Multi(listOf(source, workspace))
+
+        assertFailsWith<IllegalArgumentException> {
+            Multi(listOf(source, workspace, multi))
+        }
+    }
+
+    @Test
+    fun `fetchContexts returns correct results`() {
+        val source1 = Source("source1")
+        val source2 = Source("source2")
+        val workspace = Workspace("workspace")
+        val multi = Multi(listOf(source1, source2, workspace))
+
+        assertEquals(listOf(source1, source2), multi.fetchContexts(), "two source contexts")
+        assertEquals(listOf(workspace), multi.fetchContexts(), "one workspace context")
+        assertEquals(listOf(), multi.fetchContexts<Connection>(), "should be empty as no connection context was provided")
+    }
+
+    @Test
+    fun `key set correctly based on contexts`() {
+        val workspace = Workspace("workspace")
+        val connection = Connection("connection")
+        val source = Source("source")
+        val destination = Destination("destination")
+        val user = User("user")
+
+        Multi(listOf(user, destination, source, connection, workspace)).also {
+            assert(it.key == workspace.key)
+        }
+        Multi(listOf(user, destination, source, connection)).also {
+            assert(it.key == connection.key)
+        }
+        Multi(listOf(user, destination, source)).also {
+            assert(it.key == source.key)
+        }
+        Multi(listOf(user, destination)).also {
+            assert(it.key == destination.key)
+        }
+        Multi(listOf(user)).also {
+            assert(it.key == user.key)
+        }
+    }
+
+    @Test
+    fun `no contexts is an exception`() {
+        assertFailsWith<IllegalArgumentException> {
+            Multi(listOf())
+        }
+    }
+
 }
 
 class WorkspaceTest {

configs/flags.yml

@@ -0,0 +1,3 @@
+flags:
+  - name: performance.backgroundJsonSchemaValidation
+    enabled: true

docker-compose.yaml

@@ -109,12 +109,14 @@ services:
       - MICRONAUT_ENVIRONMENTS=${WORKERS_MICRONAUT_ENVIRONMENTS}
       - APPLY_FIELD_SELECTION=${APPLY_FIELD_SELECTION}
       - FIELD_SELECTION_WORKSPACES=${FIELD_SELECTION_WORKSPACES}
+    configs:
+      - flags
     volumes:
       - /var/run/docker.sock:/var/run/docker.sock
       - workspace:${WORKSPACE_ROOT}
       - ${LOCAL_ROOT}:${LOCAL_ROOT}
     ports:
-      - 9000
+      - "9000"
     networks:
       - airbyte_internal
     depends_on:
@@ -155,7 +157,9 @@ services:
       - MICRONAUT_ENVIRONMENTS=${WORKERS_MICRONAUT_ENVIRONMENTS}
       - AUTO_DETECT_SCHEMA=${AUTO_DETECT_SCHEMA}
     ports:
-      - 8001
+      - "8001"
+    configs:
+      - flags
     volumes:
       - workspace:${WORKSPACE_ROOT}
       - data:${CONFIG_ROOT}
@@ -171,7 +175,7 @@ services:
     container_name: airbyte-webapp
     restart: unless-stopped
     ports:
-      - 80
+      - "80"
     environment:
       - AIRBYTE_ROLE=${AIRBYTE_ROLE:-}
       - AIRBYTE_VERSION=${VERSION}
@@ -222,6 +226,8 @@ services:
       - UPDATE_DEFINITIONS_CRON_ENABLED=${UPDATE_DEFINITIONS_CRON_ENABLED}
       - WORKSPACE_ROOT=${WORKSPACE_ROOT}
       - MICRONAUT_ENVIRONMENTS=${CRON_MICRONAUT_ENVIRONMENTS}
+    configs:
+      - flags
     volumes:
       - workspace:${WORKSPACE_ROOT}
     networks:
@@ -248,9 +254,9 @@ services:
     container_name: airbyte-proxy
     restart: unless-stopped
     ports:
-      - 8000:8000
-      - 8001:8001
-      - 8003:8003
+      - "8000:8000"
+      - "8001:8001"
+      - "8003:8003"
     environment:
       - BASIC_AUTH_USERNAME=${BASIC_AUTH_USERNAME}
       - BASIC_AUTH_PASSWORD=${BASIC_AUTH_PASSWORD}
@@ -271,6 +277,9 @@ volumes:
     name: ${DATA_DOCKER_MOUNT}
   db:
     name: ${DB_DOCKER_MOUNT}
+configs:
+  flags:
+    file: ./configs/flags.yml
 networks:
   airbyte_public:
   airbyte_internal: