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

Proposed feature: Check if enabled only for some actors #481

Closed
wants to merge 4 commits into from

Conversation

iamvery
Copy link

@iamvery iamvery commented Aug 27, 2020

Context

I realize that this may not be a feature you want to support, but it came up in a use case that we have for Flipper, so I wanted to at least open the conversation here in case there is an opportunity to pay it forward. I will also say that the PR as it is being opened is more a conversation starter than anything, so please know that I'm not deeply committed to the implementation. I'm happy to iterate on the solution.

Problem

We recently started using Flipper as a way of empowering some testing-in-production features in our infrastructure. In particular, we wanted a way to flag certain domain object flowing through our system to behave in a certain way. For example:

Any time user:1234 attempts to buy something, behave as if their card is declined.

To do this, we use Flipper, but as a safety mechanism we added an additional check to make certain that the features aren't enabled globally, because we wouldn't want to decline every purchase for all users by accident 😎

Solution

Our solution involved a small wrapper around Flipper, so we thought I might be something to add to the library. It essentially amounts to checking for a feature which is globally enabled and faulting on that condition.

Please let me know your thoughts! I would love to understand your perspective on this problem and whether it is something you could like to include in Flipper.

@@ -34,6 +34,14 @@ def enabled?(name, *args)
feature(name).enabled?(*args)
end

def enabled_for_some?(name, actor)
if enabled?(name)
raise Flipper::Error, "The feature #{name.inspect} is enabled for all!"
Copy link
Author

Choose a reason for hiding this comment

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

It probably doesn't make sense to raise an error. I did so on a whim here, because I think it would be desirable to provide some logging in the case that this condition happens. I think a better solution might be an optional block or argument that can be leveraged by the caller to respond as needed.

Copy link
Author

Choose a reason for hiding this comment

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

I went ahead and converted this to a block in b962c0b.

@@ -34,6 +34,15 @@ def enabled?(name, *args)
feature(name).enabled?(*args)
end

def enabled_for_some?(name, actor)
Copy link
Author

Choose a reason for hiding this comment

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

My colleague made a good point, that this is mostly about "enabled for actors" or actor-only feature flags that aren't enabled by percentage or globally. That's a much better way to put it. There may be a better way to model this deeper in the abstractions, but my hot take would be name this method strict_enabled?.

@@ -34,6 +34,15 @@ def enabled?(name, *args)
feature(name).enabled?(*args)
end

def strict_enabled?(name, actor)
if enabled?(name)
Copy link
Author

Choose a reason for hiding this comment

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

This still leaves open the possibility of enabled by percentage which could also be problematic 🤔

@jnunemaker
Copy link
Collaborator

Could you show a couple examples of how this is used in the code? I can see and understand the code/specs, but I kind of want to see it in real life so I better understand the use case and if there is a different way we could maybe solve it using what already exists. I think a few real life code examples would help me understand. Thanks!

@iamvery
Copy link
Author

iamvery commented Aug 27, 2020

Yes, of course! Here is an example.

Give some small testing-in-production abstraction like:

module TestingInProduction
  def self.simulate_decline?(user)
    Flipper.strict_enabled?(:simulate_decline, user, on_reject: ->(feature){
      Rails.logger.warn <<~MSG
        The feature #{feature.inspect} is enabled too liberally to be used safely for testing-in-production.
      MSG
    })
  end
end

Note: In this example, I went with an on_reject: argument rather than a block as presented in the current PR code changes. Again just illustrating options. I don't have a strong opinion.

And later on in application code:

class PurchaseController < ApplicationController
  def create
    # Note: We only want this to return `true` when the user is an enabled actor. Not when the feature is enabled
    # by percentage or globally as that would affect other users in an unpredictable or unintended way.
    if TestingInProduction.simulate_decline?(user)
      render json: { error: "declined" }, status: 400
    else
      # ... (do things to actually complete the purchase)
      render json: { success: true }
    end
  end
end

I'm going to noodle more on another use case. We've repeated a pattern similar to this a few times, but it's all in the name of testing-in-production, so I don't have another use case handy in mind right now.

@iamvery
Copy link
Author

iamvery commented Aug 27, 2020

In discussion with a colleague, another interesting perspective came up. Potentially this feature could be limited to flipper-ui in that the primary risk that we're addressing is the potential for mistakenly toggling a feature on in a more "global" sense, so perhaps there would be room to add some level of restriction to the UI that is configurable. For example, disable the "Fully Enable" button for particular features. 🤔

Clearing: There's still a part of the UI-only idea that makes me uneasy. Given the "in production" aspect of my use case, I would worry that there is still some way to get into a pickle which could result in production issues. For that reason, I like the idea of a more "provably safe" method.

@jnunemaker
Copy link
Collaborator

Interesting. Seems like what you really want is a way to remove certain gates from a feature and disallow using them. I actually definitely want to support that. I haven't built it because it will involve adapter changes -- either some sort of KV or config storage per feature or switching to v2 #163 adapters that are more KV based and dumber, but with the ability to store more.

@jnunemaker
Copy link
Collaborator

@iamvery Did you end up working around this or is it still hanging open? Just curious. I'm probably going to close it due to age, but I'm very happy to keep working through this (and will do a better job of responding promptly, sorry about that).

@jnunemaker jnunemaker closed this Apr 20, 2021
@jnunemaker
Copy link
Collaborator

Oh, and here is the issue about customizing which gates are used per feature.

@iamvery iamvery deleted the iamvery/enabled-for-some branch April 20, 2021 19:00
@iamvery
Copy link
Author

iamvery commented Apr 20, 2021

I'm not actively working on this or thinking about it, so thank you for closing it out. For the time being on our side we're not actively making changes. If something changes, I'll come back to this. Thanks for responding! And no problem at all on the timing 🙏

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.

2 participants