Use the Rails application logger #427
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since the parent of this branch is v1.4.0, we only need to merge main ignoring its changes to this branch (the `ours` main strategy) to effectively revert all of main's changes: $ git merge main -s ours
Setting the logger to be the Active Job logger changes the behavior compared to using logger directly on the job. The reason is that previously we ended up logging to Rails.logger, because when ActiveJob initializes itself in the context of a Rails application, it sets its logger to the Rails one: https://github.com/rails/rails/blob/v7.0.7.2/activejob/lib/active_job/railtie.rb#L13-L15 Whereas saving the value of ActiveJob::Base.logger early will use the default Active Job logger which logs to stdout: https://github.com/rails/rails/blob/v7.0.7.2/activejob/lib/active_job/logging.rb#L11 This prevents early binding and keeps the pre-1.4.0 behavior of using the current value of ActiveJob::Base.logger during jobs.
etiennebarrie
force-pushed
the
use-rails-logger
branch
from
11a7b4e
to
604f43e
Compare
|
I added a commit first in the branch (which starts on v1.4.0) which reverts all the changes from v1.4.0 to main by merging main but ignoring all its changes ( |
etiennebarrie
force-pushed
the
use-rails-logger
branch
from
c2c5636
to
a758bc3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Setting the logger to be the Active Job logger changes the behavior compared to using logger directly on the job: https://github.com/Shopify/job-iteration/pull/338/files#r1315002833
The reason is that previously we ended up logging to Rails.logger, because when ActiveJob initializes itself in the context of a Rails application, it sets its logger to the Rails one:
https://github.com/rails/rails/blob/v7.0.7.2/activejob/lib/active_job/railtie.rb#L13-L15
Whereas saving the value of ActiveJob::Base.logger early will use the default Active Job logger which logs to stdout:
https://github.com/rails/rails/blob/v7.0.7.2/activejob/lib/active_job/logging.rb#L11
This prevents early binding and keeps the pre-1.4.0 behavior of using the current value of ActiveJob::Base.logger during jobs.
I also directly bumped to 1.4.1 because I think this is a regression.
I investigated using a Railties to add an initializer to copy the Rails application logger later in the boot process, but ultimately found that this is simpler. It involved depending on railties, or simply defining the Railtie if the
Rails::Railtieconstant was defined, but it felt a bit too much (also depending on railties pulls in actionpack). Just pushing the binding later (i.e. not calling the method early and saving the result, but callingActiveJob::Base.loggereach time we need the logger) work well enough IMO.