refactor: config package #2439

atzoum · 2022-09-14T14:36:29Z

Description

Refactoring the config package and introducing the following features:

Support for multiple, independent config instances
Thread-safe initialization & reinitialization of config instances (including the default, singleton instance). No more nil pointer dereference panics.
Rely exclusively on viper's precedence order (see doc - explicit overrides have higher precedence than environment variables)
Support overriding config at runtime, without having to use environment variables, example (thread-safe, see config.Set)
Remove warehouse-specific code
Test coverage is now >80%

Notion Ticket

Link

Security

The code changed/added as part of this pull request won't create any security issues with how the software is being used.

codecov · 2022-09-14T14:51:49Z

Codecov Report

Base: 38.57% // Head: 39.48% // Increases project coverage by +0.90% 🎉

Coverage data is based on head (319c0b4) compared to base (da55a86).
Patch coverage: 60.95% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2439      +/-   ##
==========================================
+ Coverage   38.57%   39.48%   +0.90%     
==========================================
  Files         168      172       +4     
  Lines       36728    36619     -109     
==========================================
+ Hits        14168    14458     +290     
+ Misses      21649    21240     -409     
- Partials      911      921      +10

Impacted Files	Coverage Δ
enterprise/suppress-user/setup.go	`0.00% <0.00%> (ø)`
enterprise/suppress-user/suppressUser.go	`82.71% <0.00%> (ø)`
event-schema/setup.go	`0.00% <0.00%> (ø)`
...connection-tester/destination_connection_tester.go	`15.38% <0.00%> (ø)`
services/filemanager/filemanager.go	`36.70% <0.00%> (ø)`
services/streammanager/kafka/kafkamanager.go	`55.18% <0.00%> (ø)`
utils/misc/dbutils.go	`0.00% <0.00%> (ø)`
warehouse/api.go	`0.00% <0.00%> (ø)`
warehouse/archiver.go	`3.15% <0.00%> (ø)`
warehouse/deltalake/deltalake.go	`0.00% <0.00%> (ø)`
... and 33 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

fracasula

1st round 👍

config/config.go

fracasula · 2022-09-21T11:38:48Z

config/config.go

+	c.envLock.RLock()
+	if _, ok := c.envs[key]; !ok {
+		c.envLock.RUnlock()
+		c.envLock.Lock() // don't really care about race here, setting the same value
+		c.envs[strings.ToUpper(key)] = envVar
+		c.envLock.Unlock()
+	} else {
+		c.envLock.RUnlock()


[optional] Even though you don't care about the race it can make sense to simplify this bit and just Lock() and defer Unlock() instead of mixing.

For a strange reason I tend to prefer this kind of (biased) optimisation, considering that in most cases the key will be already in the map, so we don't really need to be pessimistic and block competing config readers.

Yes I understand, I'm suggesting mostly to simplify the logic. I don't see that much of a performance impact in this case so perhaps simplyfing can be good. Up to you of course 👍

config/config_test.go

config/env.go

config/config_test.go

config/env.go

fracasula · 2022-09-21T16:25:19Z

config/load.go

+	defer func() {
+		if r := recover(); r != nil {
+			err := fmt.Errorf("cannot update Config Variables: %v", r)
+			fmt.Println(err)


How are we going to catch this when it happens?

Used this piece in its original form, just moved it to another file.
I wouldn't worry that much about it though, since config files don't change randomly in our managed environments. We do changes purposefully and then monitor, expecting to verify that our changes have actually been applied. Failure to do so, might trigger someone to have a look at the logs :)

Makes sense at least for now it shouldn't cause major issues given that we apply and check 👍

I wonder what we could do to improve these bits, perhaps structured logging might be useful in these cases cc @lvrach

atzoum · 2022-09-22T12:16:24Z

config/load.go

+	_ = c.checkAndHotReloadConfig(c.hotReloadableConfig)
+	isChanged := c.checkAndHotReloadConfig(c.nonHotReloadableConfig)
+	if isChanged && getEnvAsBool("RESTART_ON_CONFIG_CHANGE", false) {
+		os.Exit(1)


@chandumlg @cisse21 do we really still need this functionality?

We can get rid of it... we never actually used this and should be easy to add again if needed

cisse21 · 2022-09-22T23:42:21Z

eventorder_test.go

@@ -98,48 +103,46 @@ func TestEventOrderGuarantee(t *testing.T) {
 		"workspaceId": workspaceID,
 	}
 	configJsonPath := workspaceConfig.CreateTempFile(t, "testdata/eventOrderTestTemplate.json", templateCtx)
-	t.Setenv("RSERVER_BACKEND_CONFIG_CONFIG_FROM_FILE", "true")
-	t.Setenv("RSERVER_BACKEND_CONFIG_CONFIG_JSONPATH", configJsonPath)
+	config.Set("BackendConfig.configFromFile", true)


what is the reason we are not using t.Setenv anymore ?

We cannot consider having tests running in parallel at any point in the future if we continue using environment variables for setting up our test environments. t.Setenv sets an environment variable and unsets it when the test is done, but this doesn't mean that the environment variable is specific for the test, it will affect all other tests running in parallel at the same time.

config/config.go

lvrach · 2022-09-19T07:36:39Z

services/validators/envValidator.go

@@ -258,7 +257,7 @@ func CheckAndValidateWorkspaceToken() {

 	pkgLogger.Warn("Previous workspace token is not same as the current workspace token. Parking current jobsdb aside and creating a new one")

-	dbName := config.GetEnv("JOBS_DB_DB_NAME", "ubuntu")
+	dbName := config.GetString("DB.name", "ubuntu")


Is this change backwards compatible ?

yes see here

lvrach · 2022-09-21T11:21:04Z

eventorder_test.go

@@ -98,48 +103,46 @@ func TestEventOrderGuarantee(t *testing.T) {
 		"workspaceId": workspaceID,
 	}
 	configJsonPath := workspaceConfig.CreateTempFile(t, "testdata/eventOrderTestTemplate.json", templateCtx)
-	t.Setenv("RSERVER_BACKEND_CONFIG_CONFIG_FROM_FILE", "true")
-	t.Setenv("RSERVER_BACKEND_CONFIG_CONFIG_JSONPATH", configJsonPath)
+	config.Set("BackendConfig.configFromFile", true)


I want to understand more behind the motivation of this change?

Given that this is a service integration test that includes the execution of main.go and thus the environment variables parsing are consider part of the application interface.

I assume that the problem was config left over from previous tests, but exposing config here is not the ideal approach. I would like to examine/discuss alternate solutions.

We could use @fracasula were the service executed on a separated process.

Starting a service as a separate process comes with disadvantages too. Please see this comment and let me know if you wish me to elaborate further

@atzoum it sure has disadvantages like harder to debug (setting breakpoints) and code verbosity (setting up the exec, piping output, etc...). have you come across more issues? I'd like to know because in some places it helped me with isolation.

Practically its only advantage is isolation, everything else could be considered as a disadvantage, e.g. a non-restrictive list:

debugging (practically impossible)

extensibility (unit testing components, i.e. non-programms)

intuitiveness (a test case executing go run causes some people to say WTF)

configuration (expecting that the version of the go binary executing the test is the same as the go binary available in the path)

Good point about the configuration. About extensibility one thing is to do an integration test with it, one thing a unit test. The latter definitely not recommended 🙂

github-actions bot added warehouse-team with tests labels Sep 14, 2022

atzoum force-pushed the refactor.config branch 6 times, most recently from 022b7b3 to 9d7e1ae Compare September 17, 2022 10:33

atzoum changed the title ~~[WIP] refactor: modularize config package~~ [WIP] refactor: config package Sep 18, 2022

atzoum changed the title ~~[WIP] refactor: config package~~ refactor: config package Sep 18, 2022

atzoum marked this pull request as ready for review September 19, 2022 06:04

atzoum force-pushed the refactor.config branch 3 times, most recently from 54681fe to e99a83a Compare September 20, 2022 11:08

atzoum requested review from lvrach, chandumlg, fracasula and achettyiitr September 21, 2022 06:03

$fracasula$

fracasula reviewed Sep 21, 2022

View reviewed changes

config/env.go Outdated Show resolved Hide resolved

atzoum force-pushed the refactor.config branch from e99a83a to 5f663d2 Compare September 21, 2022 15:36

atzoum requested a review from fracasula September 21, 2022 15:55

$fracasula$

fracasula reviewed Sep 21, 2022

View reviewed changes

config/config_test.go Outdated Show resolved Hide resolved

$fracasula$

fracasula reviewed Sep 21, 2022

View reviewed changes

config/env.go Show resolved Hide resolved

$fracasula$

fracasula reviewed Sep 21, 2022

View reviewed changes

atzoum force-pushed the refactor.config branch from 5f663d2 to f55e59f Compare September 22, 2022 06:21

atzoum commented Sep 22, 2022

View reviewed changes

atzoum force-pushed the refactor.config branch 2 times, most recently from 41219b7 to 055edf9 Compare September 22, 2022 13:58

cisse21 reviewed Sep 22, 2022

View reviewed changes

atzoum force-pushed the refactor.config branch from 055edf9 to fc5b2dd Compare September 23, 2022 07:39

atzoum requested a review from cisse21 September 23, 2022 07:39

$fracasula$

fracasula approved these changes Sep 23, 2022

View reviewed changes

atzoum requested review from abhimanyubabbar and Jayachand September 23, 2022 09:26

lvrach reviewed Sep 23, 2022

View reviewed changes

atzoum requested a review from lvrach September 23, 2022 13:54

atzoum added 8 commits September 23, 2022 17:02

refactor: config package

f93a8eb

fixup! refactor: config package

b50139a

fixup! refactor: config package

0c6ee08

fixup! refactor: config package

846afb8

fixup! refactor: config package

c6e4b0c

fixup! refactor: config package

9ca8b7f

fixup! refactor: config package

fc2f4ec

fixup! refactor: config package

319c0b4

atzoum force-pushed the refactor.config branch from a4f964e to 319c0b4 Compare September 23, 2022 14:02

cisse21 approved these changes Sep 24, 2022

View reviewed changes

achettyiitr approved these changes Sep 26, 2022

View reviewed changes

cisse21 merged commit 2ce9210 into master Sep 26, 2022

cisse21 deleted the refactor.config branch September 26, 2022 05:30

rudder-server-bot mentioned this pull request Sep 26, 2022

chore: release 1.2.0 #2427

Closed

rudder-server-bot mentioned this pull request Oct 6, 2022

chore: release 1.2.0 #2527

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: config package #2439

refactor: config package #2439

atzoum commented Sep 14, 2022 •

edited

codecov bot commented Sep 14, 2022 •

edited

$@fracasula$ fracasula left a comment

$@fracasula$ fracasula Sep 21, 2022

atzoum Sep 21, 2022

$@fracasula$ fracasula Sep 23, 2022

$@fracasula$ fracasula Sep 21, 2022

atzoum Sep 22, 2022 •

edited

$@fracasula$ fracasula Sep 23, 2022

atzoum Sep 22, 2022

cisse21 Sep 22, 2022

cisse21 Sep 22, 2022

atzoum Sep 23, 2022 •

edited

lvrach Sep 19, 2022

atzoum Sep 23, 2022

lvrach Sep 21, 2022

atzoum Sep 23, 2022

$@fracasula$ fracasula Sep 23, 2022

atzoum Sep 23, 2022

$@fracasula$ fracasula Sep 23, 2022

refactor: config package #2439

refactor: config package #2439

Conversation

atzoum commented Sep 14, 2022 • edited

Description

Notion Ticket

Security

codecov bot commented Sep 14, 2022 • edited

Codecov Report

fracasula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atzoum Sep 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atzoum Sep 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atzoum commented Sep 14, 2022 •

edited

codecov bot commented Sep 14, 2022 •

edited

$@fracasula$ fracasula left a comment

atzoum Sep 22, 2022 •

edited

atzoum Sep 23, 2022 •

edited