search

Unleash / unleash-client-go

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

Avoid fetching feature again in GetVariant #161

Closed jameshartig closed 10 months ago

jameshartig commented 10 months ago

Describe the feature request

Currently GetVariant calls isEnabled and then uses a custom resolver or getToggle to fetch the feature immediately afterwards.

Background

We are starting to use the Unleash client in very performance-sensitive places and we notied this inefficiency in testing when we noticed our custom resolver being called twice.

Solution suggestions

Ideally StrategyResult could contain the Feature so that GetVariant doesn't have to fetch it again. I do realize this is probably a small perf improvement but it seems simple.

jameshartig commented 10 months ago

If you're generally okay with the proposed solution I'm fine submitting a PR myself.

thomasheartman commented 10 months ago

Hey, @jameshartig! Thanks for the issue. I know you've got a few of them open (this one, #162, and #160), so permit me to briefly talk to you here for some info. Are they all still relevant after your merging of #164?

Regarding this one specifically:

For clarity, do you mean that whenever you call GetVariant, the SDK makes an API call to Unleash to fetch the updated state about the flag? All of Unleash's SDKs poll Unleash at set intervals, so you can make as many checks to isEnabled you want without making any extra network calls. So the impact of doing this should at most be that you can skip some in-memory evaluation, I would think. However, I haven't looked that deeply into this SDK myself; are you saying that that's not how it works?

jameshartig commented 10 months ago

Thanks for the issue. I know you've got a few of them open (this one, https://github.com/Unleash/unleash-client-go/issues/162, and https://github.com/Unleash/unleash-client-go/issues/160), so permit me to briefly talk to you here for some info. Are they all still relevant after your merging of https://github.com/Unleash/unleash-client-go/pull/164?

Yes, #160 is a bug with either the documentation or in the code but I'm not sure which. #162 is a feature request.

For clarity, do you mean that whenever you call GetVariant, the SDK makes an API call to Unleash to fetch the updated state about the flag? All of Unleash's SDKs poll Unleash at set intervals, so you can make as many checks to isEnabled you want without making any extra network calls. So the impact of doing this should at most be that you can skip some in-memory evaluation, I would think. However, I haven't looked that deeply into this SDK myself; are you saying that that's not how it works?

Yes, I understand it doesn't do another network lookup, but it does call r.options.storage.Get again which happens to be Redis for us. Let me create a quick PR to show my proposed solution since I think will be simpler to get across.

thomasheartman commented 10 months ago

Great; thanks! 🙏🏼

It appears that #162 has been resolved and that you have a PR in the works for this one that @FredrikOseberg has said he'll have a look at, so that's sorted.

Regarding #160, I think it's the documentation that's unclear (but I'll double check against the implementations in the other SDKs). And while I think you're right in that it's at least misleading, it could be read as being correct: if the flag exists, then you'll always find a variant (even if it's the disabled variant), so the only time the sdk won't be able to find the variant is if the flag is missing. However, that is not at all obvious and the names can certainly be misleading. I'll add it as a thing to solve and see if we can tackle it shortly.