Add TestTrack.app_ab for application-wide feature flags

[effron]

/domain @Betterment/test_track_core
/no-platform

This adds the concept of an ApplicationIdentity, and allows apps to configure an app_name. This enables consumers to easily access app-wide feature flags, that are useful in cases that aren't necessarily tied to an end user (e.g., a background job, or toggling between different versions of an external API that your app is consuming). This allows you to decouple deployment from release without the added granularity of a gradual rollout in cases where you don't need it.

This sets up a singleton ApplicationIdentity, which is consumed through a module_method on TestTrack using TestTrack.app_ab. It also exposes a TestTrack attribute called app_name that can be configured on application initialization (and is only needed if you actually use the ApplicationIdentity).

One caveat here is that the test track server needs to have an identifier called 'app_id'. I will look into following this up with a change to the TestTrack server to provide that by default

[nanda-prbot]

@effron needs to request domain and platform reviewers, or explicitly opt out, e.g.:

/no-platform

[Jaco-Pretorius]

There was some discussion of dropping the context here, do we want to do that? Or just default it to 'global' or something?

[effron]

I opened a separate pr here #61, but I/we need to do some more research to see if anyone actually uses context, and then figure out if we should just remove it altogether. I didn't want this to be blocked on figuring that out

[jmileham]

What about auto-suffixing with _enabled or something? It’s gonna be weird to do feature_enabled?(:thing_enabled) and _enabled is now officially a meaningful suffix so it’s not crazy.

But on the other hand I’m not sure I like making the identity - app in this case - implicit. Is this sugar method even necessary or should you just use ab directly?

[jmileham]

What about TestTrack.app.feature_enabled?(:thing) where the split is called thing_enabled

[jmileham]

That’d presumably be a feature of identity, which I think is appropriate because we basically often use vary for aab split tests and ab for feature gates, and ab is a weird syntax to use for gating.

[effron]

i just wanted to build easy access to the one use case that we know about, which is a simple question of whether or not a feature is enabled (as opposed to exposing the entire TestTrack::Identity api)

I like the idea of auto-prefixing, but that also might make it slightly harder to find all usages of a feature flag (e.g., i usually search for all usages of my_new_feature_enabled when cleaning up an old split, if the name is different between the app identity and a normal identity, it might be harder to find).

Are you suggesting adding another method to TestTrack::Identity called test_track_feature_enabled? that is mostly an alias of ab but with some prefixing?

[jmileham]

Yeah I guess that’s pretty verbose too. And I hear you re greppability. Riffing - TestTrack.app_ab(:thing_enabled) or something like that reduces the number of new concepts right now?

[samandmoore]

I’d love to find a way to get comfortable with TestTrack.feature_enabled?(...)

But if that doesn’t work then I’d vote for TestTrack.ab(...) over app_ab because the app part feels implied at this scope.

[samandmoore]

Alternatively, I’d take

TestTrack.app.ab(:thing_enabled)

That feels good to me. It makes it clear that we’re not magically loading in a visitor somewhere but instead we're working with an "app visitor." Given that, I think we should make TestTrack.app act the same as a VisitorDSL

[samandmoore]

We spoke in-person. We're going to do TestTrack.app_ap(:thing_enabled) for now. We feel like that allows us to hide the parts of the ApplicationIdentity that don't make sense at the scope of an application, like vary which is only used for split-tests.

[effron]

i added this to further hide the rest of the TestTrack::Identity api that doesn't make sense in the ApplicationIdentity context, such as test_track_login! and test_track_vary. I'm not sure it's worth it, because TestTrack::ApplicationIdentity is already hidden behind the TestTrack module. Any opinions here?

[Jaco-Pretorius]

This feels kinda weird to me - I don't think I would have been able to figure out what's going on if you didn't leave this comment here. If we only want to have test_track_ab here can we just copy those 3 lines from TestTrack::Identity?

I think the duplication is easier to manage than a strange abstraction.

[effron]

spoke offline, i think copying the three methods from TestTrack::Identity does not achieve the api that we want (we'd have to expose two additional public methods), and i think it involves duplication of somewhat complex code. I prefer using an approach that exposes only the minimal api that we need, and makes use of our existing patterns for managing visitor session

[effron]

but i'd be more open to a refactor or another pattern of using that that may be more self explanatory? these names feel a bit off, but they are all hidden behind a simpler public api, so they don't seem super important either

[samandmoore]

I don't love this name and i don't have a good sense for a better pattern to use right now. Fortunately, this is all private API, with the exception of TestTrack::ApplicationIdentity which i think is a good name.

[effron]

nanda?

[nanda-prbot]

Needs somebody from @Betterment/test_track_core to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

[effron]

test track pr that adds "app_id" to seeds Betterment/test_track#82

[Jaco-Pretorius]

<< domainLGTM

[nanda-prbot]

Approved! ⚡ 🎯 💅