chore: provide methods to access config init errors

[lvrach]

Description

This PR is a diffrent approach on muting config init logs, which are needed for rudder-server, but no other dependant service. In services that don't use a config file or have no need for .env, the logs can being annoying.

Moreover, since we not using the logger library, the logs are not confronting with JSON output causing parsing errors in Loki.

In this approach the errors are being stored instead of logged. Library consumers have more control over how they handle the errors, deciding if and how they will log them or even take another action. Also, because logger is already configured (even with defaults), a proper logger configuration can be used to print them thus the output will be consistent.

This is the corresponding change on rudder-server, will need to be applied once rudder-go-kit version is updated.

Linear Ticket

https://linear.app/rudderstack/issue/PIPE-956/rudder-go-kit-config-init-error-logs

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.75%. Comparing base (83d2786) to head (c026fc7).
Report is 4 commits behind head on main.

❗ Current head c026fc7 differs from pull request most recent head a9e57d4. Consider uploading reports for the commit a9e57d4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #406      +/-   ##
==========================================
+ Coverage   79.25%   79.75%   +0.50%     
==========================================
  Files          93       91       -2     
  Lines        6937     6823     -114     
==========================================
- Hits         5498     5442      -56     
+ Misses       1154     1114      -40     
+ Partials      285      267      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fracasula]

I don't like this approach either. You have to remember to check the errors from those methods and if you don't then it silently goes on. It feels like another workaround.

The most natural thing in my head is to have config.Default not load anything from .env and just carry on with its defaults.

If you want it to load the env then you shouldn't use the default config but you should do:

conf, err := config.New(config.WithDotEnv())
if err != nil {
    // handle error
}

We know the default won't have that option so there is no risk to panic in the init:

func init() {
	Default, err = New()
	if err != nil {
		panic(err) // should never happen because New() is called without the option for loading from file
	}
}

It's a small breaking change but I think it's worth it and should be more final. Wdyt?

[lvrach]

You have to remember to check the errors from those methods and if you don't then it silently goes on. It feels like another workaround.

That is true, and it is indeed a workaround. The problem remains, global variables/package methods. Until we convert rudder-go-kit and rudder-server to use DI for config all the solutions would be ugly.

Also, remember that we already allow the process to move on if the config is not loaded.

The most natural thing in my head is to have config.Default not load anything from .env and just carry on with its defaults.

This is going to be a breaking change for rudder-server, for the people that depend on their .env files. Since rudder-server is mostly using the default, and .env won't be loaded by default.

The benefit of this change is that we don't need to set the env CONFIG_LOG_ERRORS=true, it keeps the change close to the rudder-server binary. We are keeping compatibility in all existing environments not only the ones that we go setup the env.

[fracasula]

This is going to be a breaking change for rudder-server, for the people that depend on their .env files. Since rudder-server is mostly using the default, and .env won't be loaded by default.

Yeah I know but I think we need to think of a way that makes the behaviour explicit and the most obvious is to use the constructor. We can of course think of alternatives to have an explicit behaviour.

Anyway, please consider that we control rudder-server, so if a newer version of rudder-server points to a new kit that has the behaviour I explained, then the important thing is to make sure we don't use config.Default in rudder-server, right?

I did a quick search and there aren't many places in our organization where we use config.Default AND we expect it to load a .env, but of course we should check everything to make sure first.

[atzoum]

I did a quick search and there aren't many places in our organization where we use config.Default

It is not only about config.Default, it is also about all other package-level methods other than New that use config.Default.

[atzoum]

@lvrach @fracasula how are we proceeding with muting log warnings from the config package? At this point I'm fine as long we have a way to do it :)

[lvrach]

@lvrach @fracasula how are we proceeding with muting log warnings from the config package? At this point I'm fine as long we have a way to do it :)

I want to move forward with a hacky solution asap.

In practise, what I prefer with this approach is that will work for people that don't have environment variable configure, local development and open source users.

On Q2 we will work on a rudder-go-kit and rudder-server refactoring that will eliminate global variables and inits. In a few months from now the need for a hack will be eliminated.

[fracasula]

On Q2 we will work on a rudder-go-kit and rudder-server refactoring that will eliminate global variables and inits. In a few months from now the need for a hack will be eliminated.

Given this I'm cool with either approach. Approving this one as well (Aris' is also approved by me) so you can merge the hack you like the most 😂