Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TaggedLogging functionality when broadcasting to another logger: #44695

Merged
merged 1 commit into from Mar 16, 2022

Commits on Mar 15, 2022

  1. Fix TaggedLogging functionality when broadcasting:

    - This PR fixes two issues with the Tagged Logging feature in
      conjunction with broadcasting logs.
    
      For the sake of clarity I'll define the "main logger" and
      the "broadcasted logger" in this snippet:
    
      ```ruby
        main_logger = ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(io))
        broadcaster_logger = ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(io))
    
        main_logger.extend(Activesupport::Logger.broadcast(broadcasted_logger))
      ```
    
      1) The first issue that this PR fixes, is that the tags on the "main logger"
         don't propagate to the "broadcasted logger" when you pass in a block.
    
         ```ruby
         main_logger.tagged("FOO") { |logger| logger.info("Hello") }
    
         # Outputs:
         # [Foo] Hello <- By the main logger
         # Hello       <- By the broadcasted logger
         ```
    
         A fix was made in rails@70af536
         but that only works for the non block version
    
      2) It's quite common for the "broadcasted logger" to have a diffent
         log formatter that the "main logger". In example you'd want to
         output JSON logs in one and raw text in the other.
    
         That wasn't possible before. All loggers had to have the same
         instance of the formatter. The formatter was set on all loggers
         thanks to [this](https://github.com/rails/rails/blob/3fc9d12875dd434e454f74472da02014f6fa5d72/activesupport/lib/active_support/logger.rb#L45-L48) and it's [associated test](https://github.com/rails/rails/blob/3fc9d12875dd434e454f74472da02014f6fa5d72/activesupport/test/broadcast_logger_test.rb#L58-L64)
         This requirement was needed to make the Tagged Logging feature
         work; the tags being set in a thread variable whose name
         uses the `object_id` https://github.com/rails/rails/blob/3fc9d12875dd434e454f74472da02014f6fa5d72/activesupport/lib/active_support/tagged_logging.rb#L59
         (different formatter instance -> different object_id -> different variables)
    
         In this PR, I have removed the code that sets the same formatter
         instance on all logger. The "broadcaster logger" just need to
         have the `current_tags` point to the `current_tags` of the
         "main logger", I'm doing that by redefing the `current_tags`
         method each time the "main logger" uses a different formatter.
    
         The advantages by doing so is that custom made formatter
         can now call this `current_tags` method, which will return
         all the tags and process them the way they want.
    
         ```ruby
           class JSONLogFormatter
             def call(_, _, _, msg)
    	   tags = current_tags # Can now retrieve the tags
    
    	   { message: msg, tags: tags }.to_json
    	 end
           end
    
           broadcasted_logger = Logger.new(io)
           broadcaster_logger.formatter = JSONLogFormatter.new
           main_logger = Logger.new(io)
           main_logger.extend(ActiveSupport::Logger.broadcast(broadcasted_logger))
         ```
    
         The behavior remains the same as before if a logger uses the
         Rails vanilla formatter or the Tagged Logging formatter.
    Edouard-chin committed Mar 15, 2022
    Configuration menu
    Copy the full SHA
    75aa3a0 View commit details
    Browse the repository at this point in the history