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

Selector provider #21

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

Selector provider #21

wants to merge 2 commits into from

Conversation

sebastiandev
Copy link
Collaborator

@sebastiandev sebastiandev commented May 4, 2022

Selector provider allows to instantiate services based on conditions like

  • feature flags
  • env vars
  • custom callables

Usually when using a selector provider you need to define

  • a selector to be used
  • a key to select by
  • a default service to use as fallback
  • a mapping of options to services

The selector will be called with the key and it should return an option. Different services should be mapped to each option.

A typical use case is instantiating services based on feature flags. The key will be the feature flag and the selector will return the option for that flag depending on the context (current user, request params, etc). We can then assign different services to the different values for that feature flag.

The trivial case is a on/off feature flag, implemented as a boolean selector here, meaning we can map the onand off state with different services.

Boolean Selector Example

service.v1:
 class: some.module.ServiceA

service.v2:
 class: some.module.ServiceB

service:
  selector: some.path.feature_flag.selector
  named_arguments:
    key: "new-service-enabled"
    is_bool: True
    default: '@service.v1'
    on: '@service.v2'

If the feature_flag new-service-enabled is True, service.v2 will be used, otherwise service.v1will be used.

Multivariate Selector Example

service.v1:
 class: some.module.ServiceA

service.v2:
 class: some.module.ServiceB

service.v3:
 class: some.module.ServiceC

service:
  selector: some.path.feature_flag.selector
  named_arguments:
    key: "new-use-case"
    default: '@service.v1'
    v1: '@service.v1'
    v2: '@service.v2'
    v3: '@service.v3'

The feature_flag new-use-case is a multivariate flag. We have defined 3 possible options v1, v2, v3 each assigned to different services. If the selector ends up returning a new or different option the default service will be used.

@sebastiandev sebastiandev requested review from skqr and stphivos May 4, 2022 14:14
@sebastiandev sebastiandev self-assigned this May 4, 2022
@Lacrymology
Copy link

I'm not a fan of the concept that a service instantiation should incur in a DB call. We use services extensively, they're not properly cached, and we usually call it from wherever we need them, rather than passing them around, sounds like a recipe for unnecessary and uncontrolled DB queries.

Also, if I understand correctly, the feature flag selector (and most of the usages I can think of, given that user/account is the only variable parameter I can think of in the context of instantiating a service) would work only if @current.user was set, which means, they won't work for jobs, etc., which in turn means that services that do a task that could be call from either graphql endpoints or job handlers cannot use this

@sebastiandev
Copy link
Collaborator Author

sebastiandev commented May 4, 2022

I'm not a fan of the concept that a service instantiation should incur in a DB call. We use services extensively, they're not properly cached, and we usually call it from wherever we need them, rather than passing them around, sounds like a recipe for unnecessary and uncontrolled DB queries.

Also, if I understand correctly, the feature flag selector (and most of the usages I can think of, given that user/account is the only variable parameter I can think of in the context of instantiating a service) would work only if @current.user was set, which means, they won't work for jobs, etc., which in turn means that services that do a task that could be call from either graphql endpoints or job handlers cannot use this

Wait. I'm not suggesting to make db queries (we could, but is not the goal). This it not meant to be used every time we evaluate a feature flag either. There are different ways of forking behavior depending on feature flags and it depends mostly on the type of service. A good reference for this is here, where it talks about toggles at the edge or in the core.

Toggles at the edge

This refers to use case services. You can decide to use one use case implementation or another depending on a feature flag, and in this case you will probably do it based on the current user. In this scenario the use case service definition can benefit from the selector service and we can avoid flow controls at the resolver/view/controller level.

Toggles in the core

This kind of toggle doesn't necessarily depend on a user. So applying the forking at the injection level is not the best approach. Here we can have a factory or builder to abstract the knowledge of the feature flag handler or simply ask for one or another depending on the feature flag value.

We do have to get better are differentiating use case services, and other domain services. This particular feature helps with the former.

@stphivos
Copy link

stphivos commented May 4, 2022

@Lacrymology I agree about avoiding "a service instantiation should incur in a DB call" and it's true that we "usually call it from wherever we need them". But this implementation is still quite generic, given a selector might just be any boolean or string value not necessarily retrieved from the db, e.g. a hardcoded value - nothing wrong with that. Obviously the real value will come from what you mentioned, using account or user or some other dynamic value which requires database calls so we need to make sure it will be properly used in such cases also. For that, @sebastiandev I can think of a few ways to maybe provide as alternative to this interface, just to make sure it doesn't lead to uncontrolled queries:

  1. Be able to pass the selector dynamically (you guessed it!)
require("service", selector=user)
  1. Or be able to specify decorated method parameter to use as the selector
@requires("service", alias="repo", selector-param="user"):
def foo(user, repo=None):
    pass

@sebastiandev
Copy link
Collaborator Author

@Lacrymology I agree about avoiding "a service instantiation should incur in a DB call" and it's true that we "usually call it from wherever we need them". But this implementation is still quite generic, given a selector might just be any boolean or string value not necessarily retrieved from the db, e.g. a hardcoded value - nothing wrong with that. Obviously the real value will come from what you mentioned, using account or user or some other dynamic value which requires database calls so we need to make sure it will be properly used in such cases also. For that, @sebastiandev I can think of a few ways to maybe provide as alternative to this interface, just to make sure it doesn't lead to uncontrolled queries:

1. Be able to pass the selector dynamically (you guessed it!)
require("service", selector=user)
2. Or be able to specify decorated method parameter to use as the selector
@requires("service", alias="repo", selector-param="user"):
def foo(user, repo=None):
    pass

I thought about putting this in requires but then the same service could be retrieved in multiple places with and without the selector, which might introduce regressions and other unexpected issues.

There's one thing that I haven't told you, which is the plan to start using feature flags as a service, thus getting rid of account_new_feature_flags and all its variations. This selector provider would use a callable to fetch the feature flag from the external service, so no db calls. For old flags we can still use our tables and have the selector check them, but is not terrible, its just doing current_user.account.check_feature_flag and we could even join load the flags with the account.

Still there's the ability to instantiate services based on other conditions, like env vars or whatever is needed. But in general you want the same service to be provided the same way always, so having this as a provider accomplishes that goal.

Copy link

@stphivos stphivos left a comment

Choose a reason for hiding this comment

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

@sebastiandev yeah i see your point about "the same service could be retrieved in multiple places with and without the selector" which is not ideal. I think eager loading feature flags, old and new would make a lot of sense. Old feature flags is just 1 row, and new ones can't be more than 100 right? Probably way less, so fetching all toggles when a user authenticates and updating current-user to have them set, would make these concerns go away.

@hugopellissari
Copy link

Question: I was wondering the case where we have the option to either run a service or not, rather than selecting a service among two or more options. Would this case be covered by this? Would we need to provide a no-operation service in the off (or better yet, the default) option for the boolean selector?

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.

4 participants