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

LaunchDarkly modules #1212

Merged
merged 2 commits into from Oct 2, 2019

Conversation

@mightyguava
Copy link
Collaborator

commented Oct 1, 2019

Porting this over from skim. Adds the following modules:

misk-feature -- feature service interfaces
misk-feature-testing -- testing impl of said interfaces
misk-launchdarkly-core -- LaunchDarkly SDK wrapper that doesn't depend on misk, so we can use it in our monorepo
misk-launchdarkly -- Misk guice bindings for launchdarkly

@mightyguava mightyguava requested review from wesleyk, jjestrel and AlexSzlavik Oct 1, 2019
* In-memory test implementation of [FeatureFlags] that allows flags to be overridden.
*/
@Singleton
class TestFeatureFlags @Inject constructor() : AbstractIdleService(),

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 1, 2019

Collaborator

Can you rename this to FakeFeatureFlags ? Or perhaps SimpleFeatureFlags ?

import javax.inject.Singleton

/**
* In-memory test implementation of [FeatureFlags] that allows flags to be overridden.

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 1, 2019

Collaborator

should this be thread-safe?

This comment has been minimized.

Copy link
@mightyguava

mightyguava Oct 1, 2019

Author Collaborator

Yeah. Should be fine to just make the underlying map a ConcurrentHashMap right?

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 1, 2019

Collaborator

Yep!


private val overrides = mutableMapOf<Feature, Any>()

override fun getBool(feature: Feature, token: String, attrs: Attrs?): Boolean {

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 1, 2019

Collaborator

Bool –> Boolean

This comment has been minimized.

Copy link
@mightyguava

mightyguava Oct 1, 2019

Author Collaborator

Should I make GetInt GetInteger then?

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 1, 2019

Collaborator

No, a good rule is to never invent synonyms. Kotlin gives us Int and Boolean and those are the words we should use.

package misk.feature.testing

import com.google.common.util.concurrent.AbstractIdleService
import misk.feature.Attrs

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 1, 2019

Collaborator

Please rename this

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 1, 2019

Collaborator

Convention in Java land is to not abbreviate things. Motivation is that abbreviations are hard to guess and hard to grep

This comment has been minimized.

Copy link
@mightyguava

mightyguava Oct 1, 2019

Author Collaborator

Standard abbreviations are fine no? I don't think anyone would be confused what Attr means. We use HTTP and RPC all over the place. RPC is more alien than Attr imo

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 2, 2019

Collaborator

I'm recalling Effective Java Item 68. This is how we’ve named most of the things in Misk and I think it’s also good for us to be consistent!

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 2, 2019

Collaborator

(Abbreviations and Acronyms are different things, and the rules of acronyms is that they’re written like words: XmlHttpRequest instead of XMLHTTPRequest.)

class TestFeatureFlagsModule(
private val qualifier: KClass<out Annotation>? = null
) : KAbstractModule() {
private val testFeatureFlags = TestFeatureFlags()

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 1, 2019

Collaborator

This class has an injectable constructor, you shouldn’t instantiate it manually

This comment has been minimized.

Copy link
@mightyguava

mightyguava Oct 1, 2019

Author Collaborator

I instantiate manually to enable the withOverrides thing.


override fun configure() {
val key = TestFeatureFlags::class.toKey(qualifier)
bind(key).toInstance(testFeatureFlags)

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 1, 2019

Collaborator

you can say bind(key); and that’ll bind it to the injectable constructor

* })
* ```
*/
fun withOverrides(lambda: (TestFeatureFlags) -> Unit): TestFeatureFlagsModule {

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 1, 2019

Collaborator

If you want a fancier API:

   install(TestFeatureFlagsModule().withOverrides {
     overrideBool(Feature("foo"), true)
   })

You do this:

  fun withOverrides(lambda: TestFeatureFlags.() -> Unit): TestFeatureFlagsModule {

This comment has been minimized.

Copy link
@mightyguava

mightyguava Oct 1, 2019

Author Collaborator

Cool!


// Can be overridden
subject.override(FEATURE, Dinosaur.TYRANNOSAURUS)
assertThat(subject.getEnum<Dinosaur>(FEATURE, TOKEN))

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 1, 2019

Collaborator

Nice API!

/**
* Extra attributes to be used for evaluating features.
*/
data class Attrs(

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 1, 2019

Collaborator

Could you make an instance of Attrs that’s emtpy, and make that the default? As-is I can specify both empty attrs and null.

* Extra attributes to be used for evaluating features.
*/
data class Attrs(
val stringAttrs: Map<String, String> = mapOf(),

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 1, 2019

Collaborator

Do we want to pass this through exactly as-is? Do we have a use case? This seems like unnecessary complexity

This comment has been minimized.

Copy link
@mightyguava

mightyguava Oct 1, 2019

Author Collaborator

This is how you'd pass user properties through for segmenting them, like by country, platform (ios/android)

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 1, 2019

Collaborator

Gotcha. Can you add that example? Should we use types for this?

This comment has been minimized.

Copy link
@mightyguava

mightyguava Oct 2, 2019

Author Collaborator

I'm not attached to this. I think almost all usages will be passing in a string map.

Not sure what you mean by using types. Can you expand on that?

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 2, 2019

Collaborator

In my experience if you give developers a Map<String, String> then some of them will use state/prov: CALIFORNIA and others will use STATE: ca. Giving them a type instead makes it easier to be consistent, which increases the value of the data gathered.

Here’s a hypothetical sketch:

/** Subtypes are data classes whose fields are captured with feature flag lookups. */
interface Attributes {
}

data class PlasmaAttributes {
  val state: UspsState,
  val statusLevel: StatusLevel,
  val userAgent: UserAgent
} : Attributes

used like this:

  override fun getBoolean(feature: Feature, token: String, attributes: Attributes) Boolean {
    ...
  }

That way every developer says “what is an Attributes?” and they find their project’s implementation and that makes things typesafe.

Lets land this as-is before adding that though.

This comment has been minimized.

Copy link
@mightyguava

mightyguava Oct 2, 2019

Author Collaborator

We want to have consistency across services. I'd like to explore deeper how to do that in a type safe manner. My initial thought is that we may get most of what we want by exposing constants for common keys rather than adding types. We could also potentially add a superclass?

override fun getInt(feature: Feature, token: String, attributes: Attributes): Int {
checkNotNull(
overrides[feature]) { "Int flag $feature must be overridden with override() before use" }
return overrides[feature] as Int

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 2, 2019

Collaborator

if you want, you can combine the cast with the nullcheck AND get a free typecheck

return overrides[feature] as? Int
  ?: throw IllegalArgumentException("Int flag $feature must be overridden with override() before use")
when (k) {
// LaunchDarkly has some built-in keys that have to be initialized with their named
// methods.
"secondary" -> builder.secondary(k)

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 2, 2019

Collaborator

with respect to types, it’d be better if this was part of the API instead of part of the implementation.

// methods.
"secondary" -> builder.secondary(k)
"ip" -> builder.ip(k)
"email" -> builder.email(k)

This comment has been minimized.

Copy link
@swankjesse

swankjesse Oct 2, 2019

Collaborator

We probably never want to send them email addresses right?

This comment has been minimized.

Copy link
@mightyguava

mightyguava Oct 2, 2019

Author Collaborator

Probably not, I'm just transcribing the set of "special" keys

@mightyguava mightyguava force-pushed the yunchi/ld branch from bf56de6 to caac352 Oct 2, 2019
@mightyguava mightyguava merged commit e73ebdb into master Oct 2, 2019
3 checks passed
3 checks passed
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: java Your tests passed on CircleCI!
Details
ci/circleci: node Your tests passed on CircleCI!
Details
@mightyguava mightyguava deleted the yunchi/ld branch Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.