fix: dont call method on unitialized logger

[aglover-zendesk]

What does this PR do?

Fixes an issue where the error method is called on a nil @logger instance variable. See

fluent-plugin-datadog/lib/fluent/plugin/out_datadog.rb

Line 144 in 49f8e4e

@logger.error("Uncaught processing exception in datadog forwarder #{e.message}")

The @logger instance variable is only set in the DatadogClient class and its subclasses, but it is never initialized in the parent Fluent::DatadogOutput class. Instead, I believe the output plugin should be using the log method of the PluginLoggerMixin. At least this would match the example from the docs.

Note that I tried to write tests for this, but (a) was unable to mock a failed method call in test-unit and (b) was unable to access the instance of the Fluent log object to assert anything. Let me know if I'm missing something, happy to take another swing at a unit test if this is doable.

Motivation

Any exception coming from the write method is lost. Right now it is very difficult to debug failures from the plugin. The only error message I'm getting is undefined method 'error' for nil:NilClass. This has come up in previous issues like #41 as well

Additional Notes

Anything else we should know when reviewing?

I appreciate your work on this Gem ❤️

[remeh]

Hey @aglover-zendesk
Sorry for the very late reply and thanks a lot for the pull-request 🙇
I'll look into this, I'll also try to add a unit test.

About the raise you've added: while I understand why you want this exception to be raised after X retries, it makes sense, I'm afraid this could break the setup of some users. What do you think if we add it behind a configuration flag? I'm still weighting pros and cons on this.

[aglover-zendesk]

hey @remeh thanks for the reply. your point is valid, changing the behavior such that an exception can be raised feels like a breaking change. I will remove the raise from this PR, my main concern is fixing the logging call to print the exception that was raised.

if you think there's value in (optionally) raising an exception after retries are exhausted, let me know and i'd be happy to implement that as a separate PR.

[remeh]

@aglover-zendesk thanks for having adapted your PR, I'll merge it immediately in the main branch and will create a new release shipping it 👍
On raising the exception, I'll see what is done in other plugins and various clients that we have and see what's the best in our situation.