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 #features_for_actor to return all enabled features for a given actor #783

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MagoMathew
Copy link

@MagoMathew MagoMathew commented Dec 7, 2023

Adds #features_for_actor

With this change, you can get all features for the given actor as a param.

I've found we need that feature on my team, where we just load all the features once.

Even tho, the name could be just features_for but wasn't sure about it.

Please feel free to share your comments and thoughts,
Probably there's a better way to do it.

Thanks for the great work.

@MagoMathew
Copy link
Author

@jnunemaker
My approach could not be the most performant way to do what the PR does, so please feel free to suggest/apply any changes to make it work better.

I'm sure there's a way to get all the enabled features for a given actor with a single call to the DB, instead of calling one by one the features with the enabled? method.

@jnunemaker jnunemaker changed the title Add #features_for_actor to return all enabeld features for a given actor Add #features_for_actor to return all enabled features for a given actor Jan 3, 2024
@jnunemaker
Copy link
Collaborator

@MagoMathew thanks! This looks good. I've been mostly off for the holidays. Sorry for slow response.

Only thing I'm trying to decide on is if we should roll with what you have or maybe something like Flipper.features.enabled?(...). I'd have to make a proxy for that, so probably not worth it.

The other thought is maybe something that does Flipper.features_for_actor(...) but returns a Hash with each feature key as the key and true/false as the value.

@bkeepers any feelings on this method name or other ideas? If not, let me know and we can just merge as is.

@MagoMathew
Copy link
Author

The other thought is maybe something that does Flipper.features_for_actor(...) but returns a Hash with each feature key as the key and true/false as the value

Hi @jnunemaker

What you propose above is actually what I needed on my project.
But I solved it in a different way,

On my actor I have this method

 def features
    sql = "SELECT feature_key  FROM flipper_gates WHERE value = '#{self.flipper_id}'"
    res = ActiveRecord::Base.connection_pool.with_connection { |con| con.exec_query(sql) }.rows.flatten.map(&:to_sym)
    ::Feature.all.map { |f| { key: f.key, active: res.include?(f.key) } }
  end  

Do you think is a better/cleaner way to do it?
Regards

@skatkov
Copy link

skatkov commented Sep 13, 2024

How would this implementation work with active_record adapter? Would it fire off multiple queries to fetch all supported features? or will be done in one request?

@jnunemaker
Copy link
Collaborator

@skatkov if preloaded it would do minimum network calls. If not preloaded then it would be n+1.

I think we could move this to adapter level as well and allow each adapter to do this more efficiently (when necessary).

@skatkov
Copy link

skatkov commented Sep 13, 2024

@jnunemaker would be wonderful if you can merge this PR, this can potentially solve performance issues in our app (with changes in adapter). Any chance you're planning to approve/merge this anytime soon?

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.

3 participants