thomaspoignant / go-feature-flag

GO Feature Flag is a simple, complete and lightweight self-hosted feature flag solution 100% Open Source. ๐ŸŽ›๏ธ
https://gofeatureflag.org/
MIT License
1.49k stars 148 forks source link

(change) Don't send "new flag added" notification upon `ffClient` initialization #2532

Closed 8ma10s closed 4 weeks ago

8ma10s commented 1 month ago

Motivation

Problem

The current code calls retrieveFlagsAndUpdateCache to "initialize" the flag cache upon ffClient initialization, then starts the FlagUpdaterDaemon to continuously monitor the changes. ref: https://github.com/thomaspoignant/go-feature-flag/blob/9ce4b9354baa944924671cfb2dd6cc22fddda044/feature_flag.go#L105

Unfortunately, any calls to retrieveFlagsAndUpdateCache will unconditionally call notifier.Notify if there is a difference between the new cache and the old cache. Upon initialization, "old cache" is always an empty hash, thus always having a diff and and ALWAYS sending "new flag added" notification on every ffClient initialization.

This behavior can be quite annoying (or at least unnecessary) for most applications, as the "new flag added" notification always gets sent every time in any kind of rolling deploy (the container with the old code gets replaced by another container with the new code).

How (I think) It should be

I think it's better to NOT call the notifier(s) on the first cache load, because at this moment of initialization, the 'diff in cache' being detected are almost always due to previous cache being empty.

At least, I think there should be an option to skip the initial notification upon cache initialization.

Note

I can prepare a PR if okay with the proposal.

Requirements

thomaspoignant commented 1 month ago

@8ma10s thanks for raising this issue. This actually a good point, but since it has been the behavior since day 1 of GOFF I would avoid to change that by default.

But I am not opposed to offer a configuration option to not do this when starting GO Feature Flag. Would it be somehow acceptable for you to have something like that?

8ma10s commented 1 month ago

@thomaspoignant

but since it has been the behavior since day 1 of GOFF I would avoid to change that by default.

I totally understand your concern, and I'm okay with just having a config that enables such behavior ๐Ÿ‘ Maybe something like PreloadCache or IgnoreDiffOnInit or something, then set them to false by default so that it doesn't change the current behavior. If you'd like, you can change the default behavior at some point in the future, but I think that's up to you.

Let me see if I can create a concept PR.

8ma10s commented 1 month ago

/assign-me

github-actions[bot] commented 1 month ago

๐Ÿ‘‹ Hey @8ma10s, this issue is already assigned to @thomaspoignant.

โš ๏ธ It will become unassigned if it isn't closed within 10 days.

๐Ÿ”ง A maintainer can also add you to the list of assignees or swap you with the current assignee.