opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

Warn on unknown environment variable; add feature gate to error out

Open mx-psi opened this issue 1 year ago • 6 comments

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

mx-psi avatar Jul 22 '22 22:07 mx-psi

Codecov Report

Merging #5734 (eddd847) into main (3e14ee9) will increase coverage by 0.03%. The diff coverage is 100.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.

codecov[bot] avatar Jul 22 '22 22:07 codecov[bot]

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

mx-psi avatar Jul 22 '22 23:07 mx-psi

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?

mx-psi avatar Jul 23 '22 11:07 mx-psi

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.

dyladan avatar Aug 04 '22 16:08 dyladan

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.

dyladan avatar Aug 04 '22 16:08 dyladan

Asked on #5615 for more people to participate in the discussion.

mx-psi avatar Aug 05 '22 08:08 mx-psi

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

github-actions[bot] avatar Aug 20 '22 03:08 github-actions[bot]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

github-actions[bot] avatar Sep 03 '22 03:09 github-actions[bot]