Provide warning when $ is used in config for env var expansion
#9162
Comments
|
+1 on the warning message. It should also include the recommended syntax for expansion so users know exactly how to fix it as soon as they're aware the current method is being deprecated. |
|
I can make a pr on this |
|
@tomasmota assigned to you :) |
|
thanks @mx-psi , maybe you can give me a hint here. It seems like the only point at which we "know" we are dealing with a $ENV is inside this function we pass to os.Expand (https://github.com/open-telemetry/opentelemetry-collector/blob/main/confmap/converter/expandconverter/expand.go#L52). The thing is that it can be ran several times and we probably only want to print the warning once, so I suppose the solution would be setting some sort of flag from inside it? Not really sure how to approach it. |
|
Hey @tomasmota, sorry for the delay.
I would do this in the body of
If you want to log once per environment variable, I think your best option is to keep a set of the environment variables for which you have already logged. You can add a new field to the One more thing here is that you don't really have a logger today. You can start with using a noop logger while I and other work on passing a logger to the expandconverter |
…nd providers (#9443) **Description:** For both #5615 and #9162 we need to be able to log during the confmap resolution. My proposed solution is to pass a `*zap.Logger` to converters and providers during initialization. These components can then use this to log any warnings they need. This PR does the first step: being able to pass anything to converters and providers during initialization. The obvious alternative to this is to change the interface of `confmap.Provider` and `confmap.Converter` to pass any warnings in an explicit struct. I think the `*zap.Logger` alternative is more natural for developers of providers and converters: you just use a logger like everywhere else in the Collector. One problem for the Collector usage of `confmap` is: How does one pass a `*zap.Logger` before knowing how a `*zap.Logger` should be configured? I think we can work around this by: 1. Passing a special 'deferred' Logger that just stores the warnings without actually logging them (we can use something like `zaptest/observer` for this) 2. Resolving configuration 3. Building a `*zap.Logger` with said configuration 4. Logging the entries stored in (1) with the logger from (3) (using `zaptest/observer` we can do that by taking the `zapcore.Core` out of the logger and manually writing) **We don't actually need ProviderSettings today, just ConverterSettings, but I think it can still be useful.** **Link to tracking Issue:** Relates to #5615 and #9162 --------- Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
**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 --------- Co-authored-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
|
I just merged #9547 that adds the logging, we now need to actually pass a logger to close this! |
|
As far as I can see, so far no loggers are created before the service actually starts, any errors seem to just use fmt.Errorf, so I'm wondering what the best place is to create this logger. I was considering taking the LoggingOptions from CollectorSettings and creating a logger here, but afaik we don't have access to a LogsConfig, which is inside the service configuration itself which we can't access from there.. Could definitely use some pointers here :) |
|
@tomasmota There is a draft PR (#9908) by @ankitpatel96 on this, we were discussing how to do this also with @evan-bradley. If you have an opinion on how to approach this, could you leave a review on #9908 or create an alternative PR? |
**Description:** Adds an RFC about how environment variable resolution should work **Link to tracking Issue:** Fixes #9515, relates to: - #8215 - #8565 - #9162 - #9531 - #9532 --------- Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com> Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
#### Description This is an RFC to help us decide how we want `otelcol` to provide a logger before the primary logger is created. As we discuss I will update the doc. Before this is merged we should have decided on a solution and the Accepted Solution section must be updated. Related to #10056 #### Link to tracking issue This unblocks: - #9162 - #5615 --------- Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com> Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Provides a logger to confmap that buffers logs in memory until the primary logger can be used. Once the primary logger exists, places that used the original logger are given the updated Core. If an error occurs that would shut down the collector before the primary logger could be created, the logs are written to stdout/err using a fallback logger. Alternative to #10008 I've pushed the testing I did to show how the logger successfully updates. Before config resolution the debug log in confmap is not printed, but afterwards it is. test config: ```yaml receivers: nop: exporters: otlphttp: endpoint: http://0.0.0.0:4317 headers: # Not set x-test: ${env:TEMP3} debug: # set to "detailed" verbosity: $TEMP service: telemetry: logs: level: debug pipelines: traces: receivers: - nop exporters: - debug ```  <!-- Issue number if applicable --> #### Link to tracking issue Related to #9162 Related to #5615 <!--Describe what testing was performed and which tests were added.--> #### Testing If we like this approach I'll add tests <!--Describe the documentation added.--> #### Documentation --------- Co-authored-by: Dan Jaglowski <jaglows3@gmail.com> Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
#### Description Adds a Logger to ConverterSettings to enable logging from within converters. Also update the expand converter to log a warning if the env var is empty or missing. #### Link to tracking issue Closes #9162 Closes #5615 #### Testing Unit tests and local testing  ```yaml receivers: nop: exporters: otlphttp: endpoint: http://0.0.0.0:4317 headers: # Not set x-test: $TEMP3 debug: # set to "detailed" verbosity: $TEMP service: telemetry: logs: level: info pipelines: traces: receivers: - nop exporters: - otlphttp - debug ``` #### Documentation Added godoc comments
From Collector SIG Discussion 12/20/2023
Use of the
$in collector configurations for environment variable substitution will be deprecated in the future in favor of${}or${env:foo}. It would be nice if we started giving users warnings during collector startup if$is used for env variable expansion to ease the transition pain.The text was updated successfully, but these errors were encountered: