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 feature-flag client #21091

Merged
merged 64 commits into from
Jan 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
64 commits
Select commit Hold shift + click to select a range
d0e976c
feature-flag client; wip
colesnodgrass Dec 14, 2022
2be5cea
feature-flag; wip
colesnodgrass Dec 15, 2022
a8999d1
updated wip/poc
colesnodgrass Dec 15, 2022
95072f6
make account truly optional
colesnodgrass Dec 15, 2022
e49360d
add temp flag example
colesnodgrass Dec 19, 2022
4c3f0aa
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Dec 19, 2022
4c40f81
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Dec 21, 2022
d6bdf72
wip of feature-flag client; upgrade to kotlin 1.8
colesnodgrass Dec 30, 2022
6af5d69
inject LDClient
colesnodgrass Dec 31, 2022
71cfcbe
feature-flag client wip; add tests; finalize classes
colesnodgrass Jan 5, 2023
985e60a
add cloud test with mockk
colesnodgrass Jan 5, 2023
037eb27
add EnvVar tests
colesnodgrass Jan 5, 2023
fc68bb8
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 5, 2023
9957dd2
remove account from workspace, add user
colesnodgrass Jan 5, 2023
10b07a2
comment out test
colesnodgrass Jan 5, 2023
e564fa4
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 5, 2023
45721f0
doc typo
colesnodgrass Jan 5, 2023
2f9d46f
move flags to top of file
colesnodgrass Jan 5, 2023
1a4484a
change LogConnectorMessages to EnvVar
colesnodgrass Jan 5, 2023
b7d1757
add @JvmOverloads to Workspace context
colesnodgrass Jan 5, 2023
2e50819
rename clients; add TestClient (with tests)
colesnodgrass Jan 5, 2023
05b449f
rename teams
colesnodgrass Jan 5, 2023
56a08da
rename Client -> FeatureFlagClient
colesnodgrass Jan 5, 2023
a4565f8
doc typo
colesnodgrass Jan 5, 2023
b766505
change parameter ordering in flags
colesnodgrass Jan 5, 2023
3fec8e3
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 5, 2023
0838a3f
rename files
colesnodgrass Jan 5, 2023
9f930a0
typo
colesnodgrass Jan 5, 2023
f2cec18
update docs to match re-org'd params
colesnodgrass Jan 5, 2023
9cb32d3
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 6, 2023
d5e0984
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 6, 2023
5575f36
rename PlatformClient -> ConfigFileClient; CloudClient -> LaunchDarkl…
colesnodgrass Jan 6, 2023
d13bc65
pr feedback; comment updates
colesnodgrass Jan 6, 2023
1fe86b0
pr feedback - remove star imports
colesnodgrass Jan 6, 2023
12484a4
pr feedback; update to use version catalog
colesnodgrass Jan 6, 2023
29ec8b4
pr feedback; add Connection, Source, Destination contexts
colesnodgrass Jan 6, 2023
95bc21d
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 6, 2023
b1ee714
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 6, 2023
03109ee
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 9, 2023
a8ee4c2
pr feedback - add multicontext support
colesnodgrass Jan 9, 2023
ea3e3c1
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 9, 2023
552a493
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 10, 2023
bae07fa
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 10, 2023
67b2fcc
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 10, 2023
924176b
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 10, 2023
53e9da6
replace Team idea with attributes map
colesnodgrass Jan 10, 2023
4c20074
pr feedback
colesnodgrass Jan 10, 2023
a880efa
Merge remote-tracking branch 'origin/master' into cole/ffclient-usage
colesnodgrass Jan 10, 2023
541eec8
explicit types
colesnodgrass Jan 10, 2023
1185ce3
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 10, 2023
904c662
rename data classes to match client
colesnodgrass Jan 10, 2023
f402103
pass context to envvar flags; make enabled open
colesnodgrass Jan 11, 2023
57858dc
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 11, 2023
6806ac6
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 11, 2023
caf2416
change fetcher to an internal property
colesnodgrass Jan 11, 2023
0d9abb5
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 11, 2023
81e8223
add UUID secondary constructors to context classes
colesnodgrass Jan 11, 2023
41e90ee
make values optional for TestClient
colesnodgrass Jan 11, 2023
57f36f5
add anonymous support
colesnodgrass Jan 12, 2023
ea560f3
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 12, 2023
3a104b3
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 12, 2023
365cad8
pr-feedback; add newlines to each file
colesnodgrass Jan 13, 2023
a7fb304
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 13, 2023
31a6906
Merge remote-tracking branch 'origin/master' into cole/ffclient
colesnodgrass Jan 17, 2023
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
39 changes: 39 additions & 0 deletions airbyte-featureflag/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import org.jetbrains.kotlin.gradle.dsl.JvmTarget
import org.jetbrains.kotlin.gradle.tasks.KotlinCompile

plugins {
`java-library`
kotlin("jvm") version "1.8.0"
}

dependencies {
annotationProcessor(platform(libs.micronaut.bom))
annotationProcessor(libs.bundles.micronaut.annotation.processor)

implementation(platform(libs.micronaut.bom))
implementation(libs.micronaut.inject)
implementation(libs.launchdarkly)
implementation(libs.jackson.databind)
implementation(libs.jackson.dataformat)
implementation(libs.jackson.kotlin)

testAnnotationProcessor(platform(libs.micronaut.bom))
testAnnotationProcessor(libs.micronaut.inject)
testAnnotationProcessor(libs.bundles.micronaut.test.annotation.processor)

testImplementation(kotlin("test"))
testImplementation(kotlin("test-junit5"))
testImplementation(libs.bundles.micronaut.test)
testImplementation(libs.mockk)
testImplementation(libs.bundles.junit)
}

tasks.withType<KotlinCompile> {
compilerOptions {
jvmTarget.set(JvmTarget.JVM_17)
}
}

tasks.test {
useJUnitPlatform()
}
211 changes: 211 additions & 0 deletions airbyte-featureflag/src/main/kotlin/Client.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
/*
* Copyright (c) 2022 Airbyte, Inc., all rights reserved.
*/

package io.airbyte.featureflag

import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory
import com.fasterxml.jackson.module.kotlin.readValue
import com.fasterxml.jackson.module.kotlin.registerKotlinModule
import com.launchdarkly.sdk.ContextKind
import com.launchdarkly.sdk.LDContext
import com.launchdarkly.sdk.LDUser
import com.launchdarkly.sdk.server.LDClient
import java.lang.Thread.MIN_PRIORITY
import java.nio.file.Path
import java.nio.file.StandardWatchEventKinds
import java.nio.file.WatchService
import java.util.concurrent.locks.ReentrantReadWriteLock
import kotlin.concurrent.read
import kotlin.concurrent.thread
import kotlin.concurrent.write
import kotlin.io.path.isRegularFile

/**
* Feature-Flag Client interface.
*/
sealed interface FeatureFlagClient {
/**
* Returns true if the flag with the provided context should be enabled. Returns false otherwise.
*/
fun enabled(flag: Flag, ctx: Context): Boolean
}

/**
* 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.
*
* @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.
*/
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)

/** 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.onChange {
lock.write { flags = readConfig(config) }
}
}

override fun enabled(flag: Flag, ctx: Context): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add another function to return a string or another value type. We currently have a string feature flag: https://github.com/airbytehq/airbyte/blob/master/airbyte-commons/src/main/java/io/airbyte/commons/features/FeatureFlags.java#L38 Maybe it should have been move the the EnvConfig though

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I missed this during my research. I think it makes sense to add in a string version as well, given that we already appear to be using it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@benmoriceau I dug into this a little further and I don't think we need to support string feature-flags yet. The FIELD_SELECTION_WORKSPACES contains a comma-delimited list of workspace identifiers that should have field selection enabled. This maps to the Workspace Context concept that this PR introduces.

Currently the env-var is defined similar to FIELD_SELECTION_WORKSPACES: "uuid-0000, uuid-0001"
which is used to determine which workspaces should have this functionality enabled. Instead of having the workspace-ids defined in this configuration you would grab the current workspace-id, wrap it in the Workspace context and then pass it to the feature-flag client.

The hard-coded workspace-ids would now reside on the LD side (and would be evaluated against the Workspace context provided) instead of being defined within the configuration.

Basically, in LD, if the provided workspace (from the context) matches one of the workspaces listed in LD, then that feature-flag would evaluate to true, otherwise false.

return when (flag) {
is EnvVar -> flag.enabled(ctx)
else -> lock.read { flags[flag.key]?.enabled ?: flag.default }
Copy link
Contributor

Choose a reason for hiding this comment

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

After your presentation the ?/?: is clear but I think that it would worth doing a presentation to the team to introduce this to the entire engineer team

}
}
}

/**
* LaunchDarkly based feature-flag client. Feature-flags are derived from an external source (the LDClient).
* Also supports flags defined via environment-variables via the [EnvVar] class.
*
* @param [client] the Launch-Darkly client for interfacing with Launch-Darkly.
*/
class LaunchDarklyClient(private val client: LDClient) : FeatureFlagClient {
override fun enabled(flag: Flag, ctx: Context): Boolean {
return when (flag) {
is EnvVar -> flag.enabled(ctx)
else -> client.boolVariation(flag.key, ctx.toLDUser(), flag.default)
}
}
}

/**
* Test feature-flag client. Intended only for usage in testing scenarios.
*
* All [Flag] instances will use the provided [values] map as their source of truth, including [EnvVar] flags.
*
* @param [values] is a map of [Flag.key] to enabled/disabled status.
*/
class TestClient @JvmOverloads constructor(val values: Map<String, Boolean> = mapOf()) : FeatureFlagClient {
override fun enabled(flag: Flag, ctx: Context): Boolean {
return when (flag) {
is EnvVar -> {
// convert to a EnvVar flag with a custom fetcher that uses the [values] of this Test class
// instead of fetching from the environment variables
EnvVar(envVar = flag.key, default = flag.default, attrs = flag.attrs).apply {
fetcher = { values[flag.key]?.toString() ?: flag.default.toString() }
}.enabled(ctx)
}

else -> values[flag.key] ?: flag.default
}
}
}


/**
* Data wrapper around OSS feature-flag configuration file.
*
* The file has the format of:
* flags:
* - name: feature-one
* enabled: true
* - name: feature-two
* enabled: false
*/
private data class ConfigFileFlags(val flags: List<ConfigFileFlag>)

/**
* Data wrapper around an individual flag read from the configuration file.
*/
private data class ConfigFileFlag(val name: String, val enabled: Boolean)


/** The yaml mapper is used for reading the feature-flag configuration file. */
private val yamlMapper = ObjectMapper(YAMLFactory()).registerKotlinModule()

/**
* Reads a yaml configuration file, converting it into a map of flag name to flag configuration.
*
* @param [path] to yaml config file
* @return map of feature-flag name to feature-flag config
*/
private fun readConfig(path: Path): Map<String, ConfigFileFlag> = yamlMapper.readValue<ConfigFileFlags>(path.toFile()).flags
.associateBy { it.name }

/**
* Monitors a [Path] for changes, calling [block] when a change is detected.
*
* @receiver Path
* @param [block] function called anytime a change is detected on this [Path]
*/
private fun Path.onChange(block: () -> Unit) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought it was some fancy kotlin builtin when I was reading the usage above ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

If i understand right, this overrides behavior on the Path object. What is the scope of that override? Is it scoped just to this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't override existing behavior on the Path object, it adds behavior to it. Really it's just syntactical-sugar that Kotlin provides which roughly equates to the following java code:

private static void onChange(final Path path, final Supplier<Void> block) {
  // ...
}

As this added behavior (aka extension function) is defined as private, it can only be called from within this file. If it was defined as internal then it could be called from anywhere within the airbyte-featureflag module, and if defined as public (which is the default visibility if not defined) then it could be called from anywhere that this module was included as a dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Makes sense.

val watcher: WatchService = fileSystem.newWatchService()
// The watcher service requires a directory to be registered and not an individual file. This Path is an individual file,
// hence the `parent` reference to register the parent of this file (which is the directory that contains this file).
// As all files within this directory could send events, any file that doesn't match this Path will need to be filtered out.
parent.register(watcher, StandardWatchEventKinds.ENTRY_MODIFY, StandardWatchEventKinds.ENTRY_CREATE)

thread(isDaemon = true, name = "feature-flag-watcher", priority = MIN_PRIORITY) {
val key = watcher.take()
// The context on the poll-events for ENTRY_MODIFY and ENTRY_CREATE events should return a Path,
// however officially `Returns: the event context; may be null`, so there is a null check here
key.pollEvents().mapNotNull { it.context() as? Path }
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit/QQ about kotlin, is the ? needed in the as? since we know that the map only map the non null value.
Also do we need to filter out null values or is it included in the mapNonNull function?

Copy link
Member Author

Choose a reason for hiding this comment

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

is the ? needed in the as?

As Kotlin requires null values to be explicitly marked as nullable, this also applies when casting one class to another.
If context() returned a null (which it can according to the javadocs) then the following code would result in an exception when context() was null

context() as Path

Kotlin as a "Safe" (nullable) cast operator (rolls right off the tonguq) for handling this scenario. If context() returns null, the result will be null instead of an exception. If context() is not-null then it will be cast to a Path (which could also throw an exception if the type isn't a Path, but according to the javadocs, it should be a Path).

do we need to filter out null values or is it included in the mapNonNull function

The [mapNotNull(https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/map-not-null.html) filters out null values. I had originally written this as two separate methods (one for the map and one for filtering out nulls) and intellij told me to collapse them into this one method call.

// As events are generated at the directory level and not the file level, any files that do not match the specific file
// this Path represents must be filtered out.
// E.g.
// If this path is "/tmp/dir/flags.yml",
// the directory registered with the WatchService was "/tmp/dir",
// and the event's path would be "flags.yml".
//
// This filter verifies that "/tmp/dir/flags.yml" ends with "flags.yml" before calling the block method.
.filter { this.endsWith(it) }
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this filter doing, I don't understand the mix of it and this. Also in the same spirit of making the type explicit, should we avoid using it when we can have a more explicit name?

Copy link
Member Author

@colesnodgrass colesnodgrass Jan 6, 2023

Choose a reason for hiding this comment

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

I'll add a comment to this code to make it more explicit what is going on here, as I agree you can't look at this code and know what is happening.

In Kotlin, lambdas have an implicit lambda parameter it that is scoped to the lambda body. You can provide a name to the parameter (name ->) but in my experience it just adds more bloat to the code with questionable benefit. Most Kotlin code uses the provided it parameter.

There are situations where a named lambda parameter makes sense, such as nested lambdas, or a really long lambda block (which should be avoided).


The this in this section of code is referring to the Path object itself as this function is an extension function.

private fun Path.onChange(block: () -> Unit) { /* ... */ }

This adds an onChange function to the Path object, but if this function needs to reference the Path object itself, it does so via this. The this.endsWith is calling Path.endsWith as this is a Path type.

Extension functions are Kotlin syntactical-sugar that essentially equate to the following java code

private static void onChange(final Path this, final Supplier<Void> block) { /* ... */ }

.forEach { _ -> block() }

key.reset()
}
}

/**
* LaunchDarkly v5 version
*
* LaunchDarkly v6 introduces the LDContext class which replaces the LDUser class,
* however this is only available to early-access users accounts currently.
*
* 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 -> {
val builder = LDUser.Builder(key)
if (this.key == ANONYMOUS.toString()) {
builder.anonymous(true)
}
builder.build()
}
}

/**
* LaunchDarkly v6 version
*
* Replaces toLDUser once LaunchDarkly v6 is GA.
*/
private fun Context.toLDContext(): LDContext {
if (this is Multi) {
val builder = LDContext.multiBuilder()
contexts.forEach { builder.add(it.toLDContext()) }
return builder.build()
}

val builder = LDContext.builder(ContextKind.of(kind), key)
if (key == ANONYMOUS.toString()) {
builder.anonymous(true)
}

when (this) {
is Workspace -> user?.let { builder.set("user", it) }
else -> Unit
}

return builder.build()
}
147 changes: 147 additions & 0 deletions airbyte-featureflag/src/main/kotlin/Context.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
/*
* Copyright (c) 2023 Airbyte, Inc., all rights reserved.
*/

package io.airbyte.featureflag

import java.util.*

/**
* Anonymous UUID to be used with anonymous contexts.
*
* Annotated with @JvmField for java interop.
*/
@JvmField
val ANONYMOUS = UUID(0, 0)

/**
* Context abstraction around LaunchDarkly v6 context idea
*
* I'm still playing around with this. Basically the idea is to define our own custom context types
* (by implementing this sealed interface) to ensure that we are consistently using the same identifiers
* throughout the code.
*
* @property [kind] determines the kind of context the implementation is,
* must be consistent for each type and should not be set by the caller of a context
* @property [key] is the unique identifier for the specific context, e.g. a user-id or workspace-id
*/
sealed interface Context {
gosusnp marked this conversation as resolved.
Show resolved Hide resolved
val kind: String
gosusnp marked this conversation as resolved.
Show resolved Hide resolved
cgardens marked this conversation as resolved.
Show resolved Hide resolved
val key: String
}

/**
* Context for representing multiple contexts concurrently. Only supported for LaunchDarkly v6!
*
* @param [contexts] list of contexts, must not contain another Multi context
*/
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 = ""

init {
// ensure there are no nested contexts (i.e. this Multi does not contain another Multi)
if (fetchContexts<Multi>().isNotEmpty()) {
throw IllegalArgumentException("Multi contexts cannot be nested")
}
}

/**
* Returns all the [Context] types contained within this [Multi] matching type [T].
*
* @param [T] the [Context] type to fetch.
* @return all [Context] of [T] within this [Multi], or an empty list if none match.
*/
internal inline fun <reified T> fetchContexts(): List<T> {
return contexts.filterIsInstance<T>()
}
}

/**
* Context for representing a workspace.
*
* @param [key] the unique identifying value of this workspace
* @param [user] an optional user identifier
*/
data class Workspace @JvmOverloads constructor(
override val key: String,
val user: String? = null
) : Context {
override val kind = "workspace"

/**
* Secondary constructor
*
* @param [key] workspace UUID
* @param [user] an optional user identifier
*/
@JvmOverloads
constructor(key: UUID, user: String? = null) : this(key = key.toString(), user = user)
}

/**
* Context for representing a user.
*
* @param [key] the unique identifying value of this user
*/
data class User(override val key: String) : Context {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a context for connection as well

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so. Jimmy called out some additional Contexts that should also be added.

override val kind = "user"

/**
* Secondary constructor
*
* @param [key] user UUID
*/
constructor(key: UUID) : this(key = key.toString())
}

/**
* Context for representing a connection.
*
* @param [key] the unique identifying value of this connection
*/
data class Connection(override val key: String) : Context {
override val kind = "connection"

/**
* Secondary constructor
*
* @param [key] connection UUID
*/
constructor(key: UUID) : this(key = key.toString())
}

/**
* Context for representing a source.
*
* @param [key] the unique identifying value of this source
*/
data class Source(override val key: String) : Context {
override val kind = "source"

/**
* Secondary constructor
*
* @param [key] Source UUID
*/
constructor(key: UUID) : this(key = key.toString())
}

/**
* Context for representing a destination.
*
* @param [key] the unique identifying value of this destination
*/
data class Destination(override val key: String) : Context {
override val kind = "destination"

/**
* Secondary constructor
*
* @param [key] Destination UUID
*/
constructor(key: UUID) : this(key = key.toString())
}
Loading