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

Conversation

colesnodgrass
Copy link
Member

What

  • part of Performance - background json schema validation #21492
  • update feature-flag client capabilities
    • updated ConfigFileClient to support passing in a null config file path
      • if a null config is provided, the requested feature-flag default values will always be returned
      • this was changed to make instantiating the FeatureFlagClient easier
    • enable the Multi contexts to work with LDv5
      • convert the context-key to LDv5 user attributes so feature-flags can be correctly targeted
    • pass LDv6 contexts as LDv5 user attributes
      • as our LaunchDarkly account does not yet have access to contexts, this ensures that our Context attributes are set correctly on the LaunchDarkly user
  • add performance.backgroundJsonSchemaValidation feature flag
    • to both the flags.yml and Flags.kt files

Recommended reading order

  1. Flags.kt
  2. flags.yml
  3. Context.kt
  4. Client.kt
  5. tests

@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 18, 2023 18:26 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 18, 2023 18:26 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 18, 2023 18:54 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 18, 2023 18:55 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 18, 2023 19:05 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 18, 2023 19:05 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 18, 2023 20:00 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 18, 2023 20:00 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 18, 2023 20:17 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 18, 2023 20:17 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Jan 18, 2023

Airbyte Code Coverage

There is no coverage information present for the Files changed

Total Project Coverage 23.97%

@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 19, 2023 00:15 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 19, 2023 00:15 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 19, 2023 04:57 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 19, 2023 04:57 — with GitHub Actions Inactive
@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.

Copy link
Contributor

@jdpgrailsdev jdpgrailsdev left a comment

Choose a reason for hiding this comment

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

:shipit:

@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 19, 2023 18:37 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 19, 2023 18:39 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 24, 2023 03:23 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 24, 2023 03:23 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 26, 2023 00:35 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 26, 2023 00:36 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 26, 2023 00:43 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 26, 2023 00:43 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 26, 2023 03:46 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 26, 2023 03:46 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 26, 2023 05:39 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets January 26, 2023 05:39 — with GitHub Actions Inactive
@colesnodgrass colesnodgrass merged commit b7e3894 into master Jan 26, 2023
@colesnodgrass colesnodgrass deleted the cole/ff-config branch January 26, 2023 16:59
davinchia added a commit that referenced this pull request Jan 31, 2023
builds on add perf feature-flag; ensure compatability with LaunchDarkly (LDv5) users #21534
add support for checking the PerfBackgroundJsonValidation feature-flag within the RecordSchemaValidator class

I spent a lot of time with trying to get the Kotlin beans injectable into the java code. What should just work doesn't and it well documented on the Micronaut side as to what exactly is required (or at least I never found the documentation). After many attempts I and looking at the auto-generated kotlin Micronaut projects I landed on kapt being necessary in kotlin projects. Even this isn't entirely true.

If the Kotlin project is defined with the following gradle setup, the beans will not be visible to java projects (and possibly other projects as well):

plugins {
    kotlin("jvm") version "1.8.0"
}

dependencies {
    annotationProcessor(platform(libs.micronaut.bom))
    annotationProcessor(libs.bundles.micronaut.annotation.processor)
}
But they will be visible with the following setup:

plugins {
    kotlin("jvm") version "1.8.0"
    id("io.micronaut.minimal.application") version "3.7.0"
}

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

micronaut {
  version("3.8.2")
}
and also visible with the following setup:

plugins {
    kotlin("jvm") version "1.8.0"
    kotlin("kapt") version "1.8.0"
}

dependencies {
    kapt(platform(libs.micronaut.bom))
    kapt(libs.bundles.micronaut.annotation.processor)
}
I went with the kapt solution.

It's worth noting that kapt is deprecated and is being replaced with kps in Micronaut 4 (this is a good thing).

---------

Co-authored-by: Davin Chia <davinchia@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants