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

Remove explicit Logger class verification #1181

Merged
merged 2 commits into from Sep 23, 2020
Merged

Remove explicit Logger class verification #1181

merged 2 commits into from Sep 23, 2020

Conversation

bartekbsh
Copy link
Contributor

This PR addresses problematic check in setter configuration that disallows using loggers that are not ruby ::Logger descendants. Current behavior not only doesn't allow using certain loggers (e.g. semantic_logger or logging) without dirty hacks, but also silently ignores setting value that does not match this validation.

I tried finding common behavior of more popular logging libraries to introduce idiomatic duck-typing instead of current class comparison, but there doesn't seem to be anything simple enough (apart from validating presence of all methods that dd-trace uses) -- commonly found methods #add and #log are not always present, and they are not always public.

I didn't find any test that would validate that only Logger descendants can be used, so I didn't introduce any changes to existing specs. Please let me know if you think, that's something worth verifying.

@bartekbsh bartekbsh requested a review from a team September 21, 2020 11:41
@marcotc
Copy link
Member

marcotc commented Sep 21, 2020

👋 @bartekbsh, thank you very much for you contribution!
I think these changes do make sense: is_a?(::Logger) is too strict for sure.

We have some test around this logic here:

describe '#logger' do
describe '#instance' do
subject(:instance) { settings.logger.instance }
it { is_expected.to be nil }
end
describe '#instance=' do
let(:logger) { Datadog::Logger.new(STDOUT) }
it 'updates the #instance setting' do
expect { settings.logger.instance = logger }
.to change { settings.logger.instance }
.from(nil)
.to(logger)
end
end
describe '#level' do
subject(:level) { settings.logger.level }
it { is_expected.to be ::Logger::INFO }
end
describe 'level=' do
let(:level) { ::Logger::DEBUG }
it 'changes the #statsd setting' do
expect { settings.logger.level = level }
.to change { settings.logger.level }
.from(::Logger::INFO)
.to(level)
end
end
end

Would you mind adding tests that covers the ability to set any arbitrary object as a logger?

@marcotc marcotc added community Was opened by a community member core Involves Datadog core libraries labels Sep 21, 2020
@bartekbsh
Copy link
Contributor Author

Hey @marcotc! I'll add the test later today.

@bartekbsh
Copy link
Contributor Author

I have decided to use double with logger-like interface in spec.

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Thank you for you contribution @bartekbsh! 🎉

@marcotc marcotc added this to the 0.41.0 milestone Sep 22, 2020
@marcotc marcotc merged commit aab3a75 into DataDog:master Sep 23, 2020
@ericmustin
Copy link
Contributor

👋 @bartekbsh This has been released in 0.41.0 https://github.com/DataDog/dd-trace-rb/releases/tag/v0.41.0, thanks again for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member core Involves Datadog core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants