Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add config to allow surpressing notification on launch (flag cache load) #2534

Merged
merged 20 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
44a87b2
Change the existing `UpdateCache` method to `UpdateCacheAndNotify`
8ma10s Oct 17, 2024
35e4274
Add config to control whether to notify on application init
8ma10s Oct 17, 2024
60c158c
Add a method to the cacheManager interface to update cache without no…
8ma10s Oct 17, 2024
ec669de
Don't send notification on cache initialization if DisableNotificatio…
8ma10s Oct 17, 2024
f790787
Update docs to refer to the newly available configuration
8ma10s Oct 17, 2024
ea65873
Fix golangci-lint errors
8ma10s Oct 17, 2024
fa5dda7
Make the wait longer to ensure that async call has completed
8ma10s Oct 17, 2024
ad3bf62
Add mutext to prevent race condition
8ma10s Oct 17, 2024
9b46412
Merge branch 'main' into skip-notification-on-launch
thomaspoignant Oct 21, 2024
f4f62dc
use wasNotifyCalled() instead of directly calling notifyCalled
8ma10s Oct 21, 2024
09852d0
DisableNotificationOnInit -> DisableNotifierOnInit
8ma10s Oct 22, 2024
b282acc
Reduce number of interface methods by exposing the internal parameter…
8ma10s Oct 22, 2024
eac35b7
Introduce an additional parameter to `retrieveFlagsAndUpdateCache` to…
8ma10s Oct 22, 2024
e39b7d1
Add comment to `retrieveFlags` to indicate its functionality
8ma10s Oct 22, 2024
85d7cb4
Use mocks from testutils/mock instead of creating mocks within the te…
8ma10s Oct 24, 2024
86d93e3
Remove unnecessary references to / declarations of UpdateCacheAndNotify
8ma10s Oct 24, 2024
b064acb
Fix README description to be more accurate about DisableNotifierOnInit
8ma10s Oct 24, 2024
2783fa1
Reduce length of the line to comply with linter
8ma10s Oct 24, 2024
d267719
Merge branch 'main' into skip-notification-on-launch
thomaspoignant Oct 24, 2024
ce61272
Merge branch 'main' into skip-notification-on-launch
thomaspoignant Oct 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions cmd/relayproxy/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,14 @@ type Config struct {
// Default: false
EnablePollingJitter bool `mapstructure:"enablePollingJitter" koanf:"enablepollingjitter"`

// DisableNotifierOnInit (optional) set to true if you do not want to call any notifier
// when the flags are loaded.
// This is useful if you do not want a Slack/Webhook notification saying that
// the flags have been added every time you start the application.
// Default is set to false for backward compatibility.
// Default: false
DisableNotifierOnInit bool `mapstructure:"DisableNotifierOnInit" koanf:"DisableNotifierOnInit"`

// FileFormat (optional) is the format of the file to retrieve (available YAML, TOML and JSON)
// Default: YAML
FileFormat string `mapstructure:"fileFormat" koanf:"fileformat"`
Expand Down
1 change: 1 addition & 0 deletions cmd/relayproxy/service/gofeatureflag.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@
DataExporter: exp,
StartWithRetrieverError: proxyConf.StartWithRetrieverError,
EnablePollingJitter: proxyConf.EnablePollingJitter,
DisableNotifierOnInit: proxyConf.DisableNotifierOnInit,

Check warning on line 95 in cmd/relayproxy/service/gofeatureflag.go

View check run for this annotation

Codecov / codecov/patch

cmd/relayproxy/service/gofeatureflag.go#L95

Added line #L95 was not covered by tests
EvaluationContextEnrichment: proxyConf.EvaluationContextEnrichment,
PersistentFlagConfigurationFile: proxyConf.PersistentFlagConfigurationFile,
}
Expand Down
8 changes: 8 additions & 0 deletions config.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@ type Config struct {
// Default: false
EnablePollingJitter bool

// DisableNotifierOnInit (optional) set to true if you do not want to call any notifier
// when the flags are loaded.
// This is useful if you do not want a Slack/Webhook notification saying that
// the flags have been added every time you start the application.
// Default is set to false for backward compatibility.
// Default: false
DisableNotifierOnInit bool

// Deprecated: Use LeveledLogger instead
// Logger (optional) logger use by the library
// Default: No log
Expand Down
32 changes: 24 additions & 8 deletions feature_flag.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func New(config Config) (*GoFeatureFlag, error) {
return nil, fmt.Errorf("impossible to initialize the retrievers, please check your configuration: %v", err)
}

err = retrieveFlagsAndUpdateCache(goFF.config, goFF.cache, goFF.retrieverManager)
err = retrieveFlagsAndUpdateCache(goFF.config, goFF.cache, goFF.retrieverManager, true)
if err != nil {
switch {
case config.PersistentFlagConfigurationFile != "":
Expand Down Expand Up @@ -154,7 +154,7 @@ func retrievePersistentLocalDisk(ctx context.Context, config Config, goFF *GoFea
return err
}
defer func() { _ = fallBackRetrieverManager.Shutdown(ctx) }()
err = retrieveFlagsAndUpdateCache(goFF.config, goFF.cache, fallBackRetrieverManager)
err = retrieveFlagsAndUpdateCache(goFF.config, goFF.cache, fallBackRetrieverManager, true)
if err != nil {
return err
}
Expand Down Expand Up @@ -192,7 +192,7 @@ func (g *GoFeatureFlag) startFlagUpdaterDaemon() {
select {
case <-g.bgUpdater.ticker.C:
if !g.IsOffline() {
err := retrieveFlagsAndUpdateCache(g.config, g.cache, g.retrieverManager)
err := retrieveFlagsAndUpdateCache(g.config, g.cache, g.retrieverManager, false)
if err != nil {
g.config.internalLogger.Error("Error while updating the cache.", slog.Any("error", err))
}
Expand All @@ -203,8 +203,13 @@ func (g *GoFeatureFlag) startFlagUpdaterDaemon() {
}
}

// retrieveFlagsAndUpdateCache is called every X seconds to refresh the cache flag.
func retrieveFlagsAndUpdateCache(config Config, cache cache.Manager, retrieverManager *retriever.Manager) error {
// retreiveFlags is a function that will retrieve the flags from the retrievers,
// merge them and convert them to the flag struct.
func retreiveFlags(
thomaspoignant marked this conversation as resolved.
Show resolved Hide resolved
config Config,
cache cache.Manager,
retrieverManager *retriever.Manager,
) (map[string]dto.DTO, error) {
retrievers := retrieverManager.GetRetrievers()
// Results is the type that will receive the results when calling
// all the retrievers.
Expand Down Expand Up @@ -250,7 +255,7 @@ func retrieveFlagsAndUpdateCache(config Config, cache cache.Manager, retrieverMa
retrieversResults := make([]map[string]dto.DTO, len(retrievers))
for v := range resultsChan {
if v.Error != nil {
return v.Error
return nil, v.Error
}
retrieversResults[v.Index] = v.Value
}
Expand All @@ -262,8 +267,19 @@ func retrieveFlagsAndUpdateCache(config Config, cache cache.Manager, retrieverMa
newFlags[flagName] = value
}
}
return newFlags, nil
}

// retrieveFlagsAndUpdateCache is a function that retrieves the flags from the retrievers,
// and update the cache with the new flags.
func retrieveFlagsAndUpdateCache(config Config, cache cache.Manager,
retrieverManager *retriever.Manager, isInit bool) error {
newFlags, err := retreiveFlags(config, cache, retrieverManager)
if err != nil {
return err
}

err := cache.UpdateCache(newFlags, config.internalLogger)
err = cache.UpdateCache(newFlags, config.internalLogger, isInit && !config.DisableNotifierOnInit)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't want to introduce breaking changes, we have to make false to be the original behavior, so that the default behavior would be same as the behavior before this PR. This is why we're making the config value name to be disableNotiferOnInit, and it's making this boolean logic slightly more difficult to read by introducing double negation.

Hopefully at some point, this repo makes the decision to make disabling the notifier the default behavior.
At that point, we can make the config value name to be enableNotifierOnInit (without negation, and that should make this code much easier to read:

err = cache.UpdateCache(newFlags, config.internalLogger, isInit && config.EnableNotifierOnInit)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inverting the logic is something I am open with but we will need to explicitly guide GOFF users about this change in the future.

I'll keep it in mind in the potential breaking changes for the future.

if err != nil {
log.Printf("error: impossible to update the cache of the flags: %v", err)
return err
Expand All @@ -286,7 +302,7 @@ func (g *GoFeatureFlag) ForceRefresh() bool {
if g.IsOffline() {
return false
}
err := retrieveFlagsAndUpdateCache(g.config, g.cache, g.retrieverManager)
err := retrieveFlagsAndUpdateCache(g.config, g.cache, g.retrieverManager, false)
if err != nil {
g.config.internalLogger.Error("Error while force updating the cache.", slog.Any("error", err))
return false
Expand Down
51 changes: 51 additions & 0 deletions feature_flag_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/thomaspoignant/go-feature-flag/ffcontext"
"github.com/thomaspoignant/go-feature-flag/internal/flag"
"github.com/thomaspoignant/go-feature-flag/model"
"github.com/thomaspoignant/go-feature-flag/notifier"
"github.com/thomaspoignant/go-feature-flag/retriever"
"github.com/thomaspoignant/go-feature-flag/retriever/fileretriever"
"github.com/thomaspoignant/go-feature-flag/retriever/s3retriever"
Expand Down Expand Up @@ -706,3 +707,53 @@ func Test_UseCustomBucketingKey(t *testing.T) {
assert.Equal(t, want, got)
}
}

func Test_DisableNotifierOnInit(t *testing.T) {
tests := []struct {
name string
config *ffclient.Config
disableNotification bool
expectedNotifyCalled bool
}{
{
name: "DisableNotifierOnInit is true",
config: &ffclient.Config{
PollingInterval: 60 * time.Second,
Retriever: &fileretriever.Retriever{Path: "testdata/flag-config.yaml"},
DisableNotifierOnInit: true,
},
expectedNotifyCalled: false,
},
{
name: "DisableNotifierOnInit is false",
config: &ffclient.Config{
PollingInterval: 60 * time.Second,
Retriever: &fileretriever.Retriever{Path: "testdata/flag-config.yaml"},
DisableNotifierOnInit: false,
},
expectedNotifyCalled: true,
},
{
name: "DisableNotifierOnInit is not set",
config: &ffclient.Config{
PollingInterval: 60 * time.Second,
Retriever: &fileretriever.Retriever{Path: "testdata/flag-config.yaml"},
},
expectedNotifyCalled: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
mockNotifier := &mock.Notifier{}
tt.config.Notifiers = []notifier.Notifier{mockNotifier}

gffClient, err := ffclient.New(*tt.config)
assert.NoError(t, err)
defer gffClient.Close()

time.Sleep(2 * time.Second) // wait for the goroutine to call Notify()
assert.Equal(t, tt.expectedNotifyCalled, mockNotifier.GetNotifyCalls() > 0)
})
}
}
10 changes: 6 additions & 4 deletions internal/cache/cache_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (

type Manager interface {
ConvertToFlagStruct(loadedFlags []byte, fileFormat string) (map[string]dto.DTO, error)
UpdateCache(newFlags map[string]dto.DTO, log *fflog.FFLogger) error
UpdateCache(newFlags map[string]dto.DTO, log *fflog.FFLogger, notifyChanges bool) error
Close()
GetFlag(key string) (flag.Flag, error)
AllFlags() (map[string]flag.Flag, error)
Expand Down Expand Up @@ -60,7 +60,7 @@ func (c *cacheManagerImpl) ConvertToFlagStruct(loadedFlags []byte, fileFormat st
return newFlags, err
}

func (c *cacheManagerImpl) UpdateCache(newFlags map[string]dto.DTO, log *fflog.FFLogger) error {
func (c *cacheManagerImpl) UpdateCache(newFlags map[string]dto.DTO, log *fflog.FFLogger, notifyChanges bool) error {
newCache := NewInMemoryCache(c.logger)
newCache.Init(newFlags)
newCacheFlags := newCache.All()
Expand All @@ -75,8 +75,10 @@ func (c *cacheManagerImpl) UpdateCache(newFlags map[string]dto.DTO, log *fflog.F
c.latestUpdate = time.Now()
c.mutex.Unlock()

// notify the changes
c.notificationService.Notify(oldCacheFlags, newCacheFlags, log)
if notifyChanges {
// notify the changes
c.notificationService.Notify(oldCacheFlags, newCacheFlags, log)
}
// persist the cache on disk
if c.persistentFlagConfigurationFile != "" {
c.PersistCache(oldCacheFlags, newCacheFlags)
Expand Down
Loading
Loading