Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log when Environment Provider tries to pull unset or empty env var #9837

Merged
merged 17 commits into from
Apr 4, 2024

Conversation

ankitpatel96
Copy link
Contributor

@ankitpatel96 ankitpatel96 commented Mar 25, 2024

Description:
Creates a logger in the confmap.ProviderSettings and uses it to log when there is a missing or blank environment variable referenced in config. For now the noop logger is used everywhere except tests.

Link to tracking Issue: 5615

Testing:
I wrote unit tests that ensured

  1. logging occurred when an environment variable was unset
  2. logging occcured when the env var was empty.
  3. there was no log when an env var was used correctly

I also started the otel collector with the sample config - and added an env var reference in the sample config. I then inserted a print statement next to each log call to see whether my code paths were hit in the live application. I then went through the 3 cases mentioned above and ensured that logging behavior was accurate.

Copy link

linux-foundation-easycla bot commented Mar 25, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@ankitpatel96 ankitpatel96 changed the title Log when Environment Provider tries to pull nonexistent env var Log when Environment Provider tries to pull unset or empty env var Mar 26, 2024
@ankitpatel96 ankitpatel96 marked this pull request as ready for review March 26, 2024 18:59
@ankitpatel96 ankitpatel96 requested a review from a team as a code owner March 26, 2024 18:59
@mx-psi mx-psi self-requested a review March 27, 2024 14:50
confmap/provider/envprovider/provider.go Outdated Show resolved Hide resolved
confmap/provider.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.32%. Comparing base (99b367c) to head (dd0842e).
Report is 29 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9837      +/-   ##
==========================================
+ Coverage   91.13%   91.32%   +0.19%     
==========================================
  Files         353      357       +4     
  Lines       18735    19179     +444     
==========================================
+ Hits        17074    17516     +442     
- Misses       1333     1335       +2     
  Partials      328      328              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after rewording the function name and moving to an appropriate place. We may undergo a bit of churn on the confmap code still, so I think it's okay to leave NewNopProviderSettings there until we figure out how the code structure will look like

.chloggen/log-env-empty.yaml Outdated Show resolved Hide resolved
confmap/provider.go Outdated Show resolved Hide resolved
confmap/provider.go Outdated Show resolved Hide resolved
@mx-psi mx-psi requested a review from evan-bradley April 2, 2024 10:02
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for adding this.

confmap/provider/envprovider/provider.go Outdated Show resolved Hide resolved
@mx-psi mx-psi merged commit 4f1a893 into open-telemetry:main Apr 4, 2024
49 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants