(Feature Request) Enable passing api.Feature instead of a string to an alternative of IsEnabled

[unleash]

(Feature Request) Enable passing api.Feature instead of a string to an alternative of IsEnabled

[jrbarron]

I just thought I would chime in here 🙂 I'm not very active in the project anymore so my opinion matters less and less, but as someone who appreciates good API design, I think the current PR which adds a new top-level "overloaded" function is suboptimal and pollutes the public API.

I'm just brainstorming here so by all means feel free to disagree, but it seems like the only thing that needs to be abstracted here is how you get an instance of the feature toggle (which you already have). Ideally this would be solved by having the repository as an interface which could be overridden, but that is a much larger task unfortunately. It is one that should absolutely be considered in the next major refactor though 😉

In the absence of that abstraction though, the default implementation always goes through the repository and the storage API using a read lock so I understand there is a performance penalty here that you want to avoid. Perhaps a cleaner solution would be to extend only the behavior you want to override instead of duplicating the public APIs. In Go, function "overloading" is typically achieved through variadic arguments of functions so maybe we could do something similar using the FeatureOption we already have.

My suggestion would be to implement a new feature option that looks like this:

type FeatureResolver func(feature string) api.Feature

// WithResolver allows you to bypass the repository when resolving a feature name to its actual  instance.
func WithResolver(resolver FeatureResolver) FeatureOption {
	return func(opts *featureOption) {
		opts.resolver = resolver
	}
}

The isEnabled function would then check if there is a resolver present (and if so, use it) before using the default implementation which is getToggle.

Then, instead of doing this:

	isFeatureEnabled := client.IsFeatureEnabled(
		&features[0],
		WithContext(context.Context{
			Properties: map[string]string{"custom-id": "custom-ctx"},
		}),
	)

You would do this:

	isFeatureEnabled := client.IsEnabled(
		"",
		WithContext(context.Context{
			Properties: map[string]string{"custom-id": "custom-ctx"},
		}),
		WithResolver(func(_ string) {
			return features[0]
		}),
	)

I feel like extending the FeatureOption stuff is less intrusive than adding a new entry point to the library. A similar approach could be taken for the variant API as well I believe. Anyway, I'll leave this up to the Unleash guys to make the final call on what is the best way forward. I just thought I would add an alternative suggestion which (I think 😆 ) solves the same issue.

[sighphyre]

We're also super excited for our plans, we feel it's going to be a great change that improves a lot of things!

I've asked one of our engineers who's wiser in the ways of Golang than myself to take a more thorough look over your PR so there may be some feedback, but we're happy to accept it in principle. We'll close this issue off when it gets merged. Feel free to reach out to us on Slack if you have any more questions (or just open another issue!)

[sighphyre]

@jrbarron That's a huge improvement, thanks for the suggestion here, I think that pretty much covers my original concerns!

@courupteddata If you're willing to patch the PR that would be amazing, thank you!

[FredrikOseberg]

@jrbarron Thanks for chiming in, we value your input immensely. I definitely would prefer this way over the current implementation.

[nunogois]

Hi @courupteddata ! Thanks for raising this issue and submitting a PR.
Before we proceed, we would like to see some data regarding the performance improvements with your approach, if possible. From our understanding, retrieving the feature from the repository should not take the amount of time you're describing, and if that's the case we might have some underlying issues we need to tackle.

Some clarity about your first point on flexibility and control would also be welcome - Please feel free to expand on this and any other advantages you may identify with this change.

Thank you!

[courupteddata]

Thank you @nunogois for the quick reply.

Hopefully this will give some better background on the specific scenario. All of the features are stored in a Redis repository. We retrieve a value from the Redis store that has all of the features available and compare it against a list of provided features to check (Using the List features function). At the moment of needing to call IsEnabled we already have all of the feature instances and the request in IsEnabled to retrieve the specific feature by name is just repeating the process of pulling all of the features picking out just one feature from the map, hence the milliseconds of latency.

Regarding performance improvements, using Redis, each call to retrieve the value is costing us a round trip to Redis which is between 1-2 milliseconds and if multiple features are needed that is when that cost grows to unacceptable levels. Instead of eating the round trip cost once for the specific check for a list of features, we are subject to it N times. (N being the number of features being checked)

Leveraging the Redis repository we don't need to poll the Unleash host from each system and we can limit the number of calls to the underlying repository implementation.

By being able to provide a specific feature instance to a function like IsEnabled you are enabling the user to use the client in more creative ways without needing the expose underlying strategy logic.

Hopefully this has been enlightening and please if you have any more questions or if anything I've stated seems confusing please let me know and I'll happily elaborate.

[sighphyre]

Hi @courupteddata

Thanks for the details here, this now makes a lot of sense why you're seeing the performance hits here.

Just to give you a sense of why we're hesitating to merge the related PR here - we have 8 official SDKs (and a whole bunch of unofficial ones) that do identical things here so when we change the API surface of an SDK, it either generates confusion for users when they see they don't have similar methods or it requires a fair amount of maintenance work to keep them in sync. In cases where the changes don't add anything to the related SDKs, we try to be a bit careful around adding new things to the public interface of the SDKs (we're not a no, just trying to understand more and potentially resolve the issue behind your issue without introducing more pain).

The primary concern that we have on this one is that it looks like you're using the SDK in a way we didn't expect or prepare for (this is our bad, we put the footgun in). Generally the the repository layer in the SDKs are intended to be a in process memory store, which is why we were confused by the conversation around latency.

Usually we would recommend that if you're doing custom implementations of the repository layer that you keep a hashmap of the toggles and lazily sync that back to Redis but right now I'm not sure if that's even a sane ask without knowing more about why you're using this setup. What problem does Redis solve here for you?

[courupteddata]

Hi @sighphyre

Ideally this function would just be a function overload of IsEnabled and be transparent to the user, but unfortunately Go doesn't support that and it's my understanding Go has no plans to.

If necessary to get the change introduced, I'll happily work through and add it to the other eight official SDKs. 😄
(But then again still on the fence about the function name.)

I do like the idea of using the hashmap and lazily syncing with Redis, but then we still have inconsistency between different systems.
Redis is solving the inconsistency of having different systems polling the Unleash host at different times. A single system can update Redis and the changes are seen by all systems without them needing to update their local cache after some interval.

[sighphyre]

@courupteddata Right that makes a ton of sense. So we've chatted a fair amount about this one internally, it doesn't seem like a reasonable thing to introduce the same change in the other SDKs, we're just going accept that we may have to explain this away as a Golang specific feature with some of your reasoning.

We have some plans that may massively simplify your use case but they're still too experimental to propose right now so I think we're going to merge the related PR and possibly deprecate that method at a later stage if we believe we can find a simpler way to support your needs here

Thanks for the great explanations and the work to implement this!

[courupteddata]

This is wonderful news!
Thank you all for hearing me out with this change.
If there is anything else I can do just let me know, and I'm excited for the future plans.

[courupteddata]

@jrbarron I like your approach, it is much more elegant than my approach. If @sighphyre or @nunogois prefer your strategy I'll happily change the PR.

[courupteddata]

@sighphyre I've updated the PR.

Thank you so much @jrbarron for the suggestion/idea and letting me know how Go handles overloaded functions in an elegant way. 😄