-
Notifications
You must be signed in to change notification settings - Fork 24
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 TestTrack.app_ab
for application-wide feature flags
#60
Conversation
…ates, add configuration option to the TestTrack module for app_name
lib/test_track.rb
Outdated
@@ -57,4 +61,8 @@ def private_url | |||
def enabled? | |||
enabled_override.nil? ? !Rails.env.test? : enabled_override | |||
end | |||
|
|||
def feature_enabled?(split_name, context:) | |||
app.test_track_ab(split_name, context: context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was some discussion of dropping the context here, do we want to do that? Or just default it to 'global' or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about TestTrack.app.feature_enabled?(:thing)
where the split is called thing_enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
… of TestTrack::Identity api from consumers
PrivateIdentity.new(app_name) | ||
end | ||
|
||
class PrivateIdentity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
nanda? |
Needs somebody from @Betterment/test_track_core to claim domain review Use the shovel operator to claim, e.g.:
|
test track pr that adds "app_id" to seeds Betterment/test_track#82 |
TestTrack.feature_enabled?
for application-wide feature flagsTestTrack.app_ab
for application-wide feature flags
<< domainLGTM |
Approved! ⚡ 🎯 💅 |
/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 calledapp_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