Is it acceptable to extend user supplied instances of logger?

[ioquatix]

https://github.com/mperham/sidekiq/blob/c4dbc922305effd1db98527c293e0354456fc79a/lib/sidekiq/config.rb#L251

This line is causing a lot of issues when assigning instances which aren't instances of ::Logger or that fake the ::Logger interface.

Is there a better way to do this?

Simply doing instance_variable_set(:@logger, fake_logger_interface) works correctly.

Is per-job log level really that important?

[mperham]

I'm open to refactoring which improves those scenarios.

[ioquatix]

Well, at least this is the first thing I'd do: #5674 but even that is unsafe since you are depending on the internal implementation of ::Logger, but it's better than nothing.

Then, as I said above, I'd suggest you remove it. Your use case does not change since the default Sidekiq.logger is an instance of Sidekiq::Logger which supports the per-fiber log level.

This only impacts people assigning custom loggers, and you are assuming they will be compatible with your mixin (which is an incorrect assumption). And in that case, they probably aren't expecting that behaviour anyway.

https://github.com/mperham/sidekiq/blob/main/lib/sidekiq/config.rb#L234-L243

By the way, lazy initialisation of a logger using ||= is likely to be thread unsafe without a mutex.

[mperham]

By the way, lazy initialisation of a logger using ||= is likely to be thread unsafe without a mutex.

The logger is touched on the main thread, before any other threads are created.

[ioquatix]

So why bother creating it lazily if that's the case?

[mperham]

So I can log anywhere anytime during Sidekiq bootup without having to worry "Have I initialized the logger yet?"

[ioquatix]

I don't really understand. Logger is an instance variable of class Config. Surely adding @logger = logger || Logger.new... to Config#initialize would be before any usage, since before that, the instance of config wouldn't even exist.

[ioquatix]

It was fixed, details in #5674.