Azure / AppConfiguration-DotnetProvider

The .NET Standard configuration provider for Azure App Configuration
https://github.com/Azure/AppConfiguration
MIT License
80 stars 37 forks source link

How to force refresh feature flags without EventGrid? #457

Open gpetrou opened 1 year ago

gpetrou commented 1 year ago

Since I don't see a Discussions section in this repository, I am adding this as an issue.

There used to be a public SetDirty API that we could call to get the latest feature flag values in combination with TryRefreshAsync. Normally, we just use a CacheExpirationInterval of 1 hour in UseFeatureFlags and the flags get updated whenever the expiration happens, which is usually fine for our purpose. But as a fail-safe mechanism, or when we quickly want to check something, we would have an /api/feature-flags/refresh endpoint that would call SetDirty and after the next call to TryRefreshAsync we would immediately get the new values in a specific deployment.

Now that this is removed is there an alternative besides using EventGrid? The reason we don't want to use EventGrid is that we have many deployments of our app that use the same Azure App Configuration in various URLs that can change over time, so using WebHook or ServiceBus through EventGrid becomes impractical. I assume there is also a limit to how many URLs we can add to the webhook or how many subscriptions we can add to the service bus topic in Azure.

If instead of SetDirty I just use the following it seems to work, but it feels very hacky.

PushNotification pushNotification = new()
{
    SyncToken = "SyncToken",
    EventType = "Microsoft.AppConfiguration.KeyValueModified",
    ResourceUri = new Uri("THE_FEATURE_FLAGS_AZURE_ENDPOINT_VALUE")
};

foreach (var refresher in _configurationRefresherProvider.Refreshers)
{
    // Obsolete: refresher.SetDirty(TimeSpan.FromSeconds(1));
    refresher.ProcessPushNotification(pushNotification, TimeSpan.FromSeconds(1));
}

So, is there a better way to do this?

gpetrou commented 11 months ago

@amerjusupovic @zhenlan @avanigupta any thoughts?

amerjusupovic commented 11 months ago

@gpetrou sorry for the delayed response. We believe your use case sounds valid, so we're discussing the best option for you at the moment as well as how to solve this problem going forward. We don't have a definite answer yet, but I'll respond to this issue again once we have more to say. Thanks for your patience!

gpetrou commented 11 months ago

@amerjusupovic any news?

gpetrou commented 10 months ago

@amerjusupovic @zhenlan @avanigupta did you discuss this? It would be nice to get some feedback in order to know how to proceed with our implementation.

Eskibear commented 10 months ago

If it is a valid use case, how about something like TryRefreshAsync(boolean force = false)? When force is true, it forces to refresh regardless of CacheExpirationInterval.

amerjusupovic commented 10 months ago

@gpetrou @Eskibear I've discussed this with others on the team, and we're planning to add back SetDirty over adding a new force refresh so we can keep the random delay provided by this method. This helps with balancing out requests over time. If we confirm that plan or any other solution, we'd likely add it in the next stable release. Hope that helps!

gpetrou commented 10 months ago

@amerjusupovic thanks for the update. It would be great to bring this back in the next release.

zhiyuanliang-ms commented 1 month ago

Hi, @gpetrou Sorry for the late response. orz

We are not going to bring SetDirty back. The reason is that SetDirty isn't a solution as solid as we thought.

The problem of the SetDirty is that it doesn't guarantee the latest changes will be loaded during the next TryRefreshAysnc. It's a best-effort API.

The problem of the SetDirty is that it doesn't guarantee the latest changes will be loaded during the next TryRefreshAysnc.

We have some jitter in the original SetDirty implementation. So we can't promise that you will get the latest change immediately.

Besides, we support replicas now. The thing gets even more complex. You can have multiple relicas and the client library may get key value from anyone of them. If you update a keyvalue, the latest change will need some time to be propagated to all replicas.

Our current push model for dynamic refresh also faces the similar challenge. We have to do some extra work to deal with it. That's why we are coming to consensus not to invest more in the "push model", but make our "poll model" great. (BTW, I admit that dynamic refresh with EventGrid is not very easy to use. Sorry for that.)

Come back to your case:

You have a relatively long polling interval (1 hour) and you want to use the SetDirty for ad-hoc refresh. Since you have more than one instance, you would have to figure out a way to call SetDirty on every instance of your workload. This is not a trivial task. However, would using a shorter polling interval be a better solution? For example, will refreshing every 30 seconds be enough for your need?

We want to argue that "making a change and knowing it will be picked up in a few seconds" is a better user experience than "making a change and calling another API (and maybe try again if it doesn't work)"

gpetrou commented 1 month ago

@zhiyuanliang-ms using such a low polling interval is not an option. Due to the amount of instances we have, we will simply pass the 30,000 requests per hour limit as described in https://learn.microsoft.com/en-us/azure/azure-app-configuration/faq in a few minutes.

zhiyuanliang-ms commented 1 month ago

Hi, @gpetrou I understand that you could meet throttling issue. We are working towards resolving the throttling issue and making our poll model better.

We may offer a premium tier in the future that allows you to send an unlimited number of requests.