opentelemetry-collector
opentelemetry-collector copied to clipboard
Warn on unknown environment variable; add feature gate to error out
Description:
- Add warning when expanding unknown environment variable.
- Add
confmap.expandconverter.RaiseErrorOnUnknownEnvVar
feature gate to turn this warning into an error
Link to tracking Issue: Fixes #5615
Testing: Added unit tests
Codecov Report
Merging #5734 (eddd847) into main (3e14ee9) will increase coverage by
0.03%
. The diff coverage is100.00%
.
:exclamation: Current head eddd847 differs from pull request most recent head a95eeef. Consider uploading reports for the commit a95eeef to get more accurate results
@@ Coverage Diff @@
## main #5734 +/- ##
==========================================
+ Coverage 91.56% 91.60% +0.03%
==========================================
Files 192 192
Lines 11411 11456 +45
==========================================
+ Hits 10449 10494 +45
Misses 768 768
Partials 194 194
Impacted Files | Coverage Δ | |
---|---|---|
confmap/converter/expandconverter/expand.go | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 3e14ee9...a95eeef. Read the comment docs.
It looks like this needs a bigger refactor than what I originally thought :( We need to pass the logger to be able to log this warning, but for that we need to create the converter later in the process
This is the best I could come up with, although it comes with a lot of plumbing. The logger is not available when loading the configuration, so we need to have a way to bubble up the information that we want to be logged. To do that, I created a new nonfatalerror
package to signal that an error is not fatal.
@dyladan (or anyone else :)) can you think of a better solution for doing this?
Hmm I see the issue with the logger not being constructed yet. Honestly it might be better to just fail instead of gating that behavior. The only risk is that existing collector configs might start failing on new collector versions but the collector is still 0.x so I think it isn't out of bounds to do that. Might even help users catch issues they didn't know they had.
nonfatalerror is probably the same thing I would have come up with if you want the fail fast strategy to be gated but I might not have used the same name. The error might be fatal if there is a gate set, so it's not really the same. It's more of a sometimes fatal error than a nonfatal error. I'm trying to think of a word other than fatal which describes something that is normally not fatal but can be fatal if in a strict mode.
Asked on #5615 for more people to participate in the discussion.
This PR was marked stale due to lack of activity. It will be closed in 14 days.
Closed as inactive. Feel free to reopen if this PR is still being worked on.