[confmap] log a warning when using $VAR in config (WIP)

[tomasmota]

Description: As requested by @mx-psi , added a no-op log for when variables using the deprecated $VAR style are used. The logger should be replaced once it is clear how to pass it down (see #9443). Also, from my testing, the function passed to os.Expand is in fact only run when we have $VAR and not for ${env:VAR}, so I did not add additional checking.

Link to tracking Issue: #9162

Testing: I am not sure how to go about testing it, since we are not passing a logger in yet, there is no easy way to know what is being logged or what the map looks like. Some ideas on this would be appreciated

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.03%. Comparing base (7f13812) to head (169069f). Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9547      +/-   ##
==========================================
+ Coverage   90.99%   91.03%   +0.03%     
==========================================
  Files         353      353              
  Lines       18706    18709       +3     
==========================================
+ Hits        17022    17031       +9     
+ Misses       1356     1349       -7     
- Partials      328      329       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[tomasmota]

Thanks for opening the PR! I left some suggestions:

• String sets are typically represented in Go with the type `map[string]struct{}. I left some suggestions to do just that
• We need to warn only for $var, not for ${var}
• We need some new unit tests for this!

Thanks, regarding the unit tests, as stated in the PR, I would appreciate a suggestion with regards to how I could check for the warning. Not sure how do do it before the logger is being injected :/

[mx-psi]

For testing you can replace the logger with https://pkg.go.dev/go.uber.org/zap/zaptest/observer

[tomasmota]

For testing you can replace the logger with https://pkg.go.dev/go.uber.org/zap/zaptest/observer

That's what I wanted to do, but then I need to pass down the logger in ConverterSettings instead of hardcoding, so I can change it in the test, and from reading https://github.com/open-telemetry/opentelemetry-collector/pull/9443 I got the impression that it is not yet totally clear how to pass it down. I would intuitively just add

type ConverterSettings struct{
    Logger *zap.Logger
}

Is this what you were expecting I do?

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[mx-psi]

For testing you can replace the logger with pkg.go.dev/go.uber.org/zap/zaptest/observer

That's what I wanted to do, but then I need to pass down the logger in ConverterSettings instead of hardcoding, so I can change it in the test, and from reading #9443 I got the impression that it is not yet totally clear how to pass it down. I would intuitively just add

type ConverterSettings struct{
    Logger *zap.Logger
}

Is this what you were expecting I do?

@tomasmota Hey, apologies for the delay on replying, I went on vacation and had a huge backlog afterwards :) Yes, this is what I want to do, we can do it on this PR but IMO it's not necessary for testing right now

[mx-psi]

@tomasmota is this ready for another review?

[tomasmota]

@tomasmota is this ready for another review?

Yes, and sorry about the mess of commits, I messed it up and got too lazy to reset properly. guess you will just squash in the end anyways.

[tomasmota]

I'll take a stab at it when I get the time :) thanks for the indications!

[tomasmota]

Code looks great! Would you be up for writing some tests? You can write a function for testing like this:

func NewTestConverter() (confmap.Converter, observer.ObservedLogs) {
   core, logs := observer.New(zapcore.InfoLevel)
   conv := converter{ loggedDeprecations: make(map[string]struct{}), logger: zap.New(core)}
   return conv, logs
}

And then call it with some example confmap.Conf.

If this is too much, I am also happy to take over, please let me know :)

I have added some tests, feel free to review

[tomasmota]

@mx-psi i think you can review, I addressed your comments directly.

[tomasmota]

@mx-psi I added a test with what I understood you were asking for, not sure it was exactly that you meant 😅

[mx-psi]

Yeah, that's what I meant, thanks!

[tomasmota]

Great, really appreciate your help and patience!