Add ConfigUnmarshaler interface to allow mutations on the parsed Config

[bogdandrutu]

Fixes #3023

Signed-off-by: Bogdan Drutu [email protected]

[bogdandrutu]

/cc @jcchavezs @pavolloffay

[tigrannajaryan]

Build failed somewhere in config tests, not sure if related to the change.

[pjanotti]

When I first read the name ConfigLoader I thought that it was related to load the configuration file. The "loader" unmarshall a config parser into a service configuration. Perhaps the name should be something around config builder or unmarshall, load sounds to me more about "loading" the configuration into the parser.

[bogdandrutu]

@pjanotti good feedback, will update the PR.

[bogdandrutu]

@tigrannajaryan @jrcamp @dmitryax can one of you approve this?

[tigrannajaryan]

Approved. Looking forward to see docs in a future PR.

[jcchavezs]

My 2p here: I think this PR addresses #3023 in a unexpected way, if someone wants to apply some changes over config object then they need to implement the ConfigUnmarshaler interface (yet not caring about unmarshaling at all) by wrapping the default one or whatever user wants to use, this is somehow inconvenient because the interface isn't explicit and users wouldn't expect the config to change in this step and also it does not guarantee us this changes are the last processing for config which could lead us to unexpected behaviours in the future.

I wonder if there are actual use cases where ConfigUnmarshaler interface works a use case by itself and point out them in this PR.

For our use case we patched the collector with this approach (not saying this is the best but it looks very explicit: hypertrace@7c9197e).

[bogdandrutu]

@jcchavezs I think even though this may not seem as intuitive, this design choice gives us the best of both worlds. The reason is because it is generic enough to allow us to change the unmarshalling logic as well as being able to change the generated config.

I do understand your concern, but I would say that your proposal is very focused on your use-case, and may be too specific, so instead I believe that going with a more generic approach like this allows us to extend in the future everything that we need. There may be implementations that need to use other structure tags, or that may override some values, etc.

Also the chaining proposed has the same problem of ordering as you pointed if using this interface, because it is not guaranteed that the "config processors" will be configured in the expected order (similar issues).

This proposal also avoids adding new concepts like "config processors" so I think in some ways it is simpler.