search

Unleash / unleash-client-go

Unleash client SDK for Go
https://docs.getunleash.io
Apache License 2.0
138 stars 55 forks source link

Return variant and feature enabled from a single method #159

Closed jameshartig closed 10 months ago

jameshartig commented 10 months ago

Describe the feature request

Currently there's no way with a single call to differentiate between a feature being disabled and no variants on the feature. Both of those cases return the default variant. Ideally there would be a call that returns (variant, enabled).

Background

We would like to make a single call and understand if a feature is enabled and what variant is applicable, if any. Instead we have to make 2 calls to understand the distinction. I'm surprised that this hasn't been brought up already and I'm curious how others are using this library in production with variants.

Solution suggestions

There are 2 potential solutions I see:

  1. Fixing the fallback options. Currently the descriptions of WithVariantFallback and WithVariantFallbackFunc are wrong. Those are actually used when a feature is not found. I filed this as https://github.com/Unleash/unleash-client-go/issues/160. A new option could be added that is used to fallback when a feature is not found and the existing fallbacks could be correctly used when there are no variants on the feature. This would be a breaking change though, but I'm not sure how many people realize the existing options are not doing what they say they do.
  2. Add a new method that returns a variant and if it's enabled or not. This could also return a struct similar to the StrategyResult struct so it could be added to in the future if more things need to be returned.

Personally we're fine with either but the options being broken does seem like it should be fixed and might lend itself to just adding a new option.

jameshartig commented 10 months ago

If we can decide on a path I'm fine doing the change and submitting a PR.

jameshartig commented 10 months ago

I'm actually leaning more towards the second path because we also need the underlying api.Feature to know what type of feature it is (see #162). We will have some special logic for kill-switch-type features. In #161 I mentioned it might be useful to return the api.Feature in the api.StrategyResult method. If we added a new method that returned api.StrategyResult with a Feature then that would solve both of those.

FredrikOseberg commented 10 months ago

Describe the feature request

Currently there's no way with a single call to differentiate between a feature being disabled and no variants on the feature. Both of those cases return the default variant. Ideally there would be a call that returns (variant, enabled).

Background

We would like to make a single call and understand if a feature is enabled and what variant is applicable, if any. Instead we have to make 2 calls to understand the distinction. I'm surprised that this hasn't been brought up already and I'm curious how others are using this library in production with variants.

Solution suggestions

There are 2 potential solutions I see:

  1. Fixing the fallback options. Currently the descriptions of WithVariantFallback and WithVariantFallbackFunc are wrong. Those are actually used when a feature is not found. I filed this as WithVariantFallback and WithVariantFallbackFunc are called when feature isn't found #160. A new option could be added that is used to fallback when a feature is not found and the existing fallbacks could be correctly used when there are no variants on the feature. This would be a breaking change though, but I'm not sure how many people realize the existing options are not doing what they say they do.
  2. Add a new method that returns a variant and if it's enabled or not. This could also return a struct similar to the StrategyResult struct so it could be added to in the future if more things need to be returned.

Personally we're fine with either but the options being broken does seem like it should be fixed and might lend itself to just adding a new option.

@jameshartig

Thanks for pointing this out, it's definitely a gap we're looking to close. When this SDK was first created it was based on our node-client, which had this interface already. As we have SDKs in multiple different languages, we do strive to keep the API surface as similar as possible, both to avoid SDK bloat but also to keep the mental model clean across language boundaries.

Expanding the API for one SDK is really something we want to try to avoid. Especially since we have started this journey already in our python SDK here. Would it be acceptable for now to add a similar property to the GetVariant response?

Look forward to hearing from you

jameshartig commented 10 months ago

Would it be acceptable for now to add a similar property to the GetVariant response?

You're talking about adding IsFeatureEnabled to api.Variant? Yes, that seems fine. I appreciate the quick reply!

FredrikOseberg commented 10 months ago

Perfect! Do you want to cook something up? I will make sure we follow up next week. By the way I think the property was renamed FeatureEnabled after discussions in the PR. Would be nice to keep the same naming conventions here.

fre. 8. des. 2023 kl. 16:39 skrev James Hartig @.***>:

Would it be acceptable for now to add a similar property to the GetVariant response?

You're talking about adding IsFeatureEnabled to api.Variant? Yes, that seems fine. I appreciate the quick reply!

— Reply to this email directly, view it on GitHub https://github.com/Unleash/unleash-client-go/issues/159#issuecomment-1847400173, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD2WIPQTETSEPRLSPBEXC6LYIMYBNAVCNFSM6AAAAABALHRSZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBXGQYDAMJXGM . You are receiving this because you commented.Message ID: @.***>