-
Notifications
You must be signed in to change notification settings - Fork 375
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
do not restrict startup logs to emit only once #3441
Conversation
f9a090e
to
13a2c2f
Compare
13a2c2f
to
055ed8f
Compare
055ed8f
to
d56d820
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
context 'with multiple invocations' do | ||
it 'executes only once' do | ||
it 'executes multiple times' do | ||
env_logger.collect_and_log! | ||
env_logger.collect_and_log! | ||
|
||
expect(logger).to have_received(:info).once | ||
expect(logger).to have_received(:info).twice | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Since this behavior was inherited from the parent module, I think we could probably simplify the spec and remove this test case instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I debated just removing these tests myself, but left them in to see if anyone else thought they were superfluous.
54c29e8
to
ee9e83e
Compare
2.0 Upgrade Guide notes
In 1.x startup logs would emit only once during initial configuration of the library. In 2.0 these logs will now emit whenever the library reconfigures itself, which may occur multiple times during startup. The increased verbosity in logging better represents the configuration state of the library at the time the logs are generated.
The default setting has not changed for startup logs. It remains
nil
, which generates startup logs for non-dev environments. To turn off startup logs completely you can set:What does this PR do?
Remove the
log_once
restriction on startup logs.Motivation:
Additional Notes:
How to test the change?
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!