ActiveJob extension for preserving logs #733
Conversation
This draft allows me to have an idea of what the logging module will look like. It consists of the execution id, the logger info and the logger debug. Those three data, could be added into a model and would have a belong_to relationship with the Execution model on one end; and on the other end, the Excution would have a has_one relationship with this model. And then, at each execution, a record of this model would be saved in the database.
…tiveSupport::Logger.broadcast
bensheldon
changed the title
|
@NeimadTL this looks great! I pushed up some changes so that the logs are split and stored in a Concurrent::Array. I think this hits Checkpoint 1 👍🏻 Do you want to try for Checkpoint 2? |
Thanks @bensheldon. I'm definitely up for checkpoint 2. Let's discuss tomorrow. |
There was a problem hiding this comment.
@bensheldon, just left some questions. Could you clarify them to me please?
| included do | ||
| around_perform do |job, block| | ||
| original_logger = job.logger | ||
| job.logger = ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(LogDevice.new(job)).extend(ActiveSupport::Logger.broadcast(original_logger))) |
There was a problem hiding this comment.
@bensheldon , the around_perform hook above makes sense to me but what do L10 and L11 do exactly please ?
There was a problem hiding this comment.
This is creating a new logger that outputs to the custom LogDevice, and then teeing the output via broadcast so that the output still goes to the old logger too.
I could probably break this up into multiple lines. e.g.
custom_logger = ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(LogDevice.new(job))
custom.logger.extend(ActiveSupport::Logger.broadcast(original_logger))
ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(LogDevice.new(job)).extend(ActiveSupport::Logger.broadcast(original_logger)))| end | ||
|
|
||
| class LogDevice | ||
| cattr_accessor :logs, default: Concurrent::Array.new |
There was a problem hiding this comment.
@bensheldon is the c in cattr_accessor to indicate that this is a class attribute
There was a problem hiding this comment.
Yes, it's a class attribute. By living on the class it becomes a singleton that can be access globally.
| self.class.logs << [@job.provider_job_id, message.strip] | ||
| end | ||
|
|
||
| def close |
There was a problem hiding this comment.
I don't think #close and #reopen are overrode method but I might be wrong here. Are those methods mandatory or you think we'll be using them in the future ?
There was a problem hiding this comment.
I'm trying to match the interface of Ruby's LogDevice:
https://github.com/ruby/ruby/blob/9001e53e68d282493f513ed67824e4014fd01d57/lib/logger/log_device.rb
That said, I wonder if it would be better to inherit from LogDevice to be clear that I'm trying to match/override the interface, rather than just be an implicit duck-type.
|
@NeimadTL thank you for the awesome Code Review! You raised some great questions about how this could be clearer. I can make those changes 👍 |
|
I'm going to close this for now and revisit in the future. |
This draft allows me to have an idea of what the logging module will look like. It consists of the execution id, the logger info and the logger debug. Those three data, could be added into a model and would have a
belong_torelationship with the Execution model on one end; and on the other end, the Excecution would have ahas_onerelationship with this model. And then, at each execution, a record of this model would be saved in the database.Connects to #710