bkeepers / dotenv

A Ruby gem to load environment variables from `.env`.
MIT License
6.61k stars 505 forks source link

Use existing TaggedLogger instead of re-wrapping it with `TaggedLogging` #488

Closed kriansa closed 9 months ago

kriansa commented 9 months ago

https://github.com/bkeepers/dotenv/blob/6dd03851298c4bd250c36ef14ba253892db04063/lib/dotenv/rails.rb#L91-L96

This initializer wraps the existing Rails logger into ActiveSupport::TaggedLogging, which for the default Rails config is not bad, but when custom loggers are in place, it could be troublesome.

I, for instance, currently have a custom logger that mimics TaggedLogging and supports its interface. I think we should just test if the existing Rails logger responds to .tagged and then call it instead of re-wrapping it. Something like that (haven't tested yet):

 initializer "dotenv", after: :initialize_logger do |app| 
   # Set up a new logger once Rails has initialized the logger and replay logs 
   new_logger = ::Rails.logger&.tagged("dotenv") || ::Rails.logger
   logger.replay new_logger if logger.respond_to?(:replay) 
   self.logger = new_logger 
 end 

What do you think? By the way, awesome work on the new release! :+1:

bkeepers commented 9 months ago

@kriansa Yep, that sounds reasonable. Do you want to work up a pull request with tests?

kriansa commented 9 months ago

Sure, will send a patch shortly

kriansa commented 9 months ago

@bkeepers I opened a PR, (#489). Lmk if it looks ok

dtbaker commented 9 months ago

interesting, I just bumped activesupport from 7.0.8 to 7.0.8.1 and hit this error