Skip to content
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

add perf feature-flag; ensure compatability with LaunchDarkly (LDv5) users #21534

Merged
merged 18 commits into from
Jan 26, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 31 additions & 13 deletions airbyte-featureflag/src/main/kotlin/Client.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
}
}
}

Expand Down Expand Up @@ -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)
}
Expand Down
20 changes: 18 additions & 2 deletions airbyte-featureflag/src/main/kotlin/Context.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions airbyte-featureflag/src/main/kotlin/Flags.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
23 changes: 11 additions & 12 deletions airbyte-featureflag/src/main/kotlin/config/Factory.kt
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +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
Expand All @@ -20,22 +19,22 @@ internal const val CONFIG_OSS_KEY = "airbyte.feature-flag.path"

@Factory
class Factory {
@Requirements(
Requires(property = CONFIG_LD_KEY),
Requires(missingProperty = CONFIG_OSS_KEY),
)

@Requires(property = CONFIG_LD_KEY)
@Singleton
fun Cloud(@Property(name = CONFIG_LD_KEY) apiKey: String): FeatureFlagClient {
fun LaunchDarklyClient(@Property(name = CONFIG_LD_KEY) apiKey: String): FeatureFlagClient {
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)
@Requires(missingProperty = CONFIG_LD_KEY)
@Singleton
fun ConfigFileClient(@Property(name = CONFIG_OSS_KEY) configPath: String): FeatureFlagClient {
val path: Path? = if (configPath.isNotBlank()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, Kotlin doesn't have a more succinct way to represent this type of if/else? Or, are you just avoiding using a ternary or elvis operator for stylistic/readability reasons?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kotlin doesn't have a ternary operator, instead having an if/else and when expressions. The elvis operator only works when the left-hand side of the operator is null. As the configPath variables cannot be null (it's a String not a String?) I would have to convert it to a null in order to use a elvis operator.

Here I think it's a stylistic/readability choice.

Which one is easier to read?
if/else:

val path: Path? = if (configPath.isNotBlank()) {
  Path.of(configPath)
} else {
  null
}

when:

val path: Path? = when {
  configPath.isNotBlank() -> Path.of(configPath)
  else -> null
}

elvis (actually doesn't need an elvis as we want the null value)

val path: Path? = configPath.takeIf { it.isNotBlank() }?.let { Path.of(it) }

Having written these all out now, maybe I'll change this to use the when statement.

Path.of(configPath)
} else {
null
}
return ConfigFileClient(path)
}
}
35 changes: 15 additions & 20 deletions airbyte-featureflag/src/test/kotlin/ClientTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -25,13 +24,12 @@ import java.nio.file.Path
import java.util.concurrent.TimeUnit
import kotlin.io.path.createTempFile
import kotlin.io.path.writeText
import kotlin.test.assertFailsWith
import kotlin.test.assertFalse
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)

Expand All @@ -49,7 +47,20 @@ class ConfigFileClient {
}

@Test
fun `verify platform reload capabilities`() {
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
fun `verify config-file reload capabilities`() {
val contents0 = """flags:
| - name: reload-test-true
| enabled: true
Expand Down Expand Up @@ -151,22 +162,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()
Expand Down
63 changes: 60 additions & 3 deletions airbyte-featureflag/src/test/kotlin/ContextTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
3 changes: 3 additions & 0 deletions configs/flags.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
flags:
- name: performance.backgroundJsonSchemaValidation
enabled: true