reidmorrison / rails_semantic_logger

Rails Semantic Logger replaces the Rails default logger with Semantic Logger
https://logger.rocketjob.io/rails
Apache License 2.0
320 stars 114 forks source link

Sidekiq jobs aren't Loggable with config.eager_load=true #226

Closed jdelStrother closed 2 months ago

jdelStrother commented 2 months ago

Thanks for the recent sidekiq 7 support! I come bearing bugs:

Environment

Expected Behavior

SemanticLogger::Loggable is consistently included into Sidekiq::Job

Actual Behavior

SemanticLogger::Loggable is included into Sidekiq::Job once RailsSemanticLogger's after_initialize hook has fired. If Rails eager-loading is enabled, that means that all your sidekiq job classes get loaded (with the 'original' version of Sidekiq::Job), and then RailsSemanticLogger mixes in Loggable - which is too late.

I've added a sample repo here:

https://github.com/jdelStrother/semantic-sidekiq

It defines a SlowJob class which uses Sidekiq::Job.

You can try using that logger with something like this in console:

image

which behaves as-expected.

However, if you enable eager loading:

image

there's no SlowJob.logger class method. There's a logger instance method, but that's the regular Sidekiq::Job#logger method.

Possible fix?

Maybe RailsSemanticLogger's after_initialize hook should instead be initializer 'rails_semantic_logger', before: :eager_load! ? Or the sidekiq patches could possibly be moved to a separate earlier hook (before_initialize) ? I'm not super-familiar with the Railtie initialization process, so don't take any of those as gospel.

reidmorrison commented 2 months ago

Worth giving Rails Semantic Logger v4.17.0 a try (release today), it moves the logger installation here: https://github.com/reidmorrison/rails_semantic_logger/blob/6e1291f49c74f7241dbc9e92e28fbd60f8a1f37e/lib/rails_semantic_logger/engine.rb#L133

If not try pulling master and give your great options above a try to see if any resolve the class methods?

jdelStrother commented 2 months ago

Looks good! I've only tested locally, but it seems to fix this eager load issue.