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 the LoggerSilence to work as described: #34055

Merged
merged 1 commit into from Oct 3, 2018

Conversation

Edouard-chin
Copy link
Member

@Edouard-chin Edouard-chin commented Oct 2, 2018

  • Following the Rails guide which state that a logger needs to include
    the ActiveSupport::LoggerSilence as well as
    ActiveSupport::LoggerThreadSafe modules isn't enough and won't
    work.

    Here is a test cases with 3 tests that all fails
    https://gist.github.com/Edouard-chin/4a72930c2b1eafbbd72a80c66f102010

    The problems are the following:

    1. The logger needs to call after_initialize in order to setup
      some instance variables.
    2. The silence doesn't actually work because the bare ruby Logger
      add method checks for the instance variable @logger. We need to
      override the add (like we used to in the ActiveSupport::Logger
      class).
    3. Calling debug? info? etc... doesn't work as the bare ruby
      methods will check for the instance variable. Again we need to
      override this methods (like we used to in the ActiveSupport::Logger
      class)

    The LoggerSilence won't work without LoggerThreadSafe, but the later
    is not public API, the user shouldn't have to include it so I
    modified to include it automatically.
    Same for the after_initialize method. I find unuintitive to have
    to call it directly. I modified to instance the variables when the
    module get included.

cc/ @rafaelfranca

@rails-bot
Copy link

r? @pixeltrix

(@rails-bot has picked a reviewer for you, use r? to override)

- Following the Rails guide which state that a logger needs to include
  the `ActiveSupport::LoggerSilence` as well as
  `ActiveSupport::LoggerThreadSafe` modules isn't enough and won't
  work.

  Here is a test cases with 3 tests that all fails
  https://gist.github.com/Edouard-chin/4a72930c2b1eafbbd72a80c66f102010

  The problems are the following:

  1) The logger needs to call `after_initialize` in order to setup
  some instance variables.
  2) The silence doesn't actually work because the bare ruby Logger
  `add` method checks for the instance variable `@logger`. We need to
  override the `add` (like we used to in the ActiveSupport::Logger
  class).
  3) Calling `debug?` `info?` etc... doesn't work as the bare ruby
  methods will check for the instance variable. Again we need to
  override this methods (like we used to in the ActiveSupport::Logger
  class)

  The LoggerSilence won't work without LoggerThreadSafe, but the later
  is not public API, the user shouldn't have to include it so I
  modified to include it automatically.
  Same for the `after_initialize` method. I find unuintitive to have
  to call it directly. I modified to instance the variables when the
  module get included.
@sham-sheer

This comment has been minimized.

1 similar comment
@sham-sheer

This comment has been minimized.

@sham-sheer

This comment has been minimized.

@rafaelfranca rafaelfranca merged commit ebf98df into rails:master Oct 3, 2018
@Edouard-chin Edouard-chin deleted the ec-logger-fix branch October 3, 2018 20:51
casperisfine pushed a commit to Shopify/rails that referenced this pull request Nov 24, 2022
Fix: rails#46559

That's how it initially worked, but this was broken in
rails#34055

That PR replaced an instance variable by a class variable, causing
the level to be per thread, but to apply to all logger instances.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants