Add thread-local log level #85

mperham · 2023-01-15T16:32:14Z

This adds Logger#with_level which provides a thread-local log level so Threads can temporarily enable logging for a given block.

log = Logger.new
log.level = :info

Thread.new do
  log.with_level(:debug) do
    log.debug("Will log")
  end
end.join

log.debug("Won't log")

Fixes #84

mperham · 2023-02-06T22:48:39Z

lib/logger.rb

+  #   logger.with_level(:debug) do
+  #     logger.debug { "Hello" }
+  #   end
+  def with_level(severity, &block)


Alternative name: at?

log.at(:debug) do log.debug("You'll see this") end

I think with_level is much more clear what is going on.

&block is not needed, since you are only yielding and not capturing.

ioquatix · 2023-02-07T00:44:29Z

Sorry, I did not see this. Please mention me or add me as a reviewer in the future, otherwise I don't know about it :)

ioquatix

LGTM.

jeremyevans

I'm OK with the idea, but I'm against pollute Thread.current with this. Instead, add a instance variable storing a hash inside the logger keyed by Thread.current. This approach also makes it simple to change from a thread-local implementation to a fiber-local or global implementation by changing the key.

lib/logger/severity.rb

jeremyevans · 2023-02-09T02:57:31Z

lib/logger.rb

+  #   logger.with_level(:debug) do
+  #     logger.debug { "Hello" }
+  #   end
+  def with_level(severity, &block)


&block is not needed, since you are only yielding and not capturing.

ioquatix · 2023-02-09T03:33:02Z

Thanks @jeremyevans, great review. What do you think about using Fiber[] instead of strictly fiber/thread local? I'm not sure anyone want's to use Thread.current as the key. The current implementation would be equivalent to Fiber.current.

It might make sense just to leave this as implementation dependent so that other loggers (e.g. the ones I maintain) can be fiber-specific or use fiber-storage instead.

mperham · 2023-02-09T17:06:15Z

Ok, refactored based on feedback.

jeremyevans

Looks pretty good. One more implementation change requested.

lib/logger.rb

lib/logger/severity.rb

lib/logger.rb

Co-authored-by: Samuel Williams <samuel.williams@oriontransfer.co.nz>

ioquatix · 2023-02-10T00:54:41Z

Sorry, one more change is required, we will need to add require 'fiber' for Fiber.current to work.

jeremyevans

Couple of small things that could be fixed, but I don't think need to block merging.

CHANGELOG.md

jeremyevans · 2023-02-10T00:56:56Z

test/logger/test_severity.rb

+      logger.with_level(DEBUG) do # verify reentrancy
+        assert_equal(logger.level, DEBUG)
+
+        Thread.new do


This tests for Thread-local, not explicitly for Fiber-local, but works since a new Thread is also a new Fiber. Not sure if we want to explicitly test for Fiber-localness. Could be done later.

mperham · 2023-02-10T00:58:03Z

Sorry, one more change is required, we will need to add require 'fiber' for Fiber.current to work.

Really? irb 3.2 works fine without it:

% irb
> Fiber.current
=> #<Fiber:0x00000001051c8698 (resumed)>

jeremyevans · 2023-02-10T00:58:58Z

According to CI, logger supports Ruby 2.5+ (gem says Ruby 2.3+).

mperham · 2023-02-10T01:00:39Z

According to CI, logger supports Ruby 2.5+ (gem says Ruby 2.3+).

Right, looks like 3.1+ doesn't require "fiber" anymore.

(ruby/logger#85) * Update lib/logger/severity.rb ruby/logger@7aabb0b4aa

ioquatix · 2023-02-10T04:04:19Z

Thanks @mperham - I want to make one more PR and then we will try to cut a release.

yahonda · 2023-02-14T12:04:03Z

This commit introduces NoMethodError: undefined method []' for nil:NilClass`
reported at resque/resque#1856

ruby/logger#85 added this to the `Logger` initializer. Since we don't call `super` in `initialize`, we need to do it ourselves. Not doing this leads to an error in `Logger#level`: # undefined method `[]' for nil:NilClass (NoMethodError): def level @level_override[Fiber.current] || @Level # ^^^^^^^^^^^^^^^ end

ruby/logger#85 added this to the `Logger` initializer. Since we don't call `super` in `initialize`, we need to do it ourselves. Not doing this leads to an error in `Logger#level`: ```ruby def level @level_override[Fiber.current] || @Level # ^^^^^^^^^^^^^^^ end ```

ruby/logger#85 added `@level_override` to the `Logger` initializer. Since we didn't call `super` in `initialize`, the variable was uninitialized, and we would run into a `NoMethodError` in `Logger#level` on Ruby HEAD: def level @level_override[Fiber.current] || @Level # ^^^^^^^^^^^^^^^ NoMethodError: undefined method `[]' for nil:NilClass end

logger 1.4.3 will have ruby/logger#85. It initializes a new instance variable in `Logger#initialize`. `Jekyll::Stevenson < ::Logger` doesn't use `super`. So the new instance variable isn't initialized in `Jekyll::Stevenson`. And it causes an exception: ```text /tmp/local/lib/ruby/3.3.0+0/logger.rb:385:in `level': undefined method `[]' for nil (NoMethodError) @level_override[Fiber.current] || @Level ^^^^^^^^^^^^^^^ from /tmp/local/lib/ruby/gems/3.3.0+0/gems/jekyll-4.3.2/lib/jekyll/log_adapter.rb:45:in `adjust_verbosity' from /tmp/local/lib/ruby/gems/3.3.0+0/gems/jekyll-4.3.2/lib/jekyll/configuration.rb:143:in `config_files' from /tmp/local/lib/ruby/gems/3.3.0+0/gems/jekyll-4.3.2/lib/jekyll.rb:118:in `configuration' from /tmp/local/lib/ruby/gems/3.3.0+0/gems/jekyll-4.3.2/lib/jekyll/command.rb:44:in `configuration_from_options' from /tmp/local/lib/ruby/gems/3.3.0+0/gems/jekyll-4.3.2/lib/jekyll/commands/serve.rb:83:in `block (2 levels) in init_with_program' from /tmp/local/lib/ruby/gems/3.3.0+0/gems/mercenary-0.4.0/lib/mercenary/command.rb:221:in `block in execute' from /tmp/local/lib/ruby/gems/3.3.0+0/gems/mercenary-0.4.0/lib/mercenary/command.rb:221:in `each' from /tmp/local/lib/ruby/gems/3.3.0+0/gems/mercenary-0.4.0/lib/mercenary/command.rb:221:in `execute' from /tmp/local/lib/ruby/gems/3.3.0+0/gems/mercenary-0.4.0/lib/mercenary/program.rb:44:in `go' from /tmp/local/lib/ruby/gems/3.3.0+0/gems/mercenary-0.4.0/lib/mercenary.rb:21:in `program' from /tmp/local/lib/ruby/gems/3.3.0+0/gems/jekyll-4.3.2/exe/jekyll:15:in `<top (required)>' from /tmp/local/bin/jekyll:25:in `load' from /tmp/local/bin/jekyll:25:in `<top (required)>' from /tmp/local/lib/ruby/3.3.0+0/bundler/cli/exec.rb:58:in `load' from /tmp/local/lib/ruby/3.3.0+0/bundler/cli/exec.rb:58:in `kernel_load' from /tmp/local/lib/ruby/3.3.0+0/bundler/cli/exec.rb:23:in `run' from /tmp/local/lib/ruby/3.3.0+0/bundler/cli.rb:492:in `exec' from /tmp/local/lib/ruby/3.3.0+0/bundler/vendor/thor/lib/thor/command.rb:27:in `run' from /tmp/local/lib/ruby/3.3.0+0/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command' from /tmp/local/lib/ruby/3.3.0+0/bundler/vendor/thor/lib/thor.rb:392:in `dispatch' from /tmp/local/lib/ruby/3.3.0+0/bundler/cli.rb:34:in `dispatch' from /tmp/local/lib/ruby/3.3.0+0/bundler/vendor/thor/lib/thor/base.rb:485:in `start' from /tmp/local/lib/ruby/3.3.0+0/bundler/cli.rb:28:in `start' from /tmp/local/lib/ruby/gems/3.3.0+0/gems/bundler-2.5.0.dev/libexec/bundle:37:in `block in <top (required)>' from /tmp/local/lib/ruby/3.3.0+0/bundler/friendly_errors.rb:117:in `with_friendly_errors' from /tmp/local/lib/ruby/gems/3.3.0+0/gems/bundler-2.5.0.dev/libexec/bundle:29:in `<top (required)>' from /tmp/local/bin/bundle:25:in `load' from /tmp/local/bin/bundle:25:in `<main>' ``` How about using `super` instead of implementing our `Jekyll::Stevenson#initialize` to reduce maintenance cost?

jaynetics · 2023-07-09T16:25:48Z

This commit introduces NoMethodError: undefined method []' for nil:NilClass`
reported at resque/resque#1856

Generally speaking, this breaks any logger subclass that does not call super in its initializer. jekyll is another gem affected by this on Ruby 3.3, and there might be more. @ioquatix i wonder whether avoiding this breaking change might be worth a little amendment, like extracting @level_override into def level_override; @level_override ||= {} end or so?

mperham · 2023-07-09T17:04:59Z

I don't believe adding additional complexity to work around 3rd party bugs is a good idea. Not calling super is a bug, no?

skipkayhil · 2023-09-25T02:13:58Z

I want to make one more PR and then we will try to cut a release.

Hey @ioquatix, did that other PR ever land? I'd love to see this released so we could potentially use it to replace the LoggerThreadSafeLevel patch in Rails

Add thread-local log level, fixes ruby#84

10dff8d

mperham commented Feb 6, 2023

View reviewed changes

ioquatix self-requested a review February 7, 2023 00:44

ioquatix requested a review from jeremyevans February 9, 2023 02:41

ioquatix approved these changes Feb 9, 2023

View reviewed changes

jeremyevans requested changes Feb 9, 2023

View reviewed changes

Refactor to store levels in weakmap, hash lookup

3fbe592

jeremyevans requested changes Feb 9, 2023

View reviewed changes

lib/logger.rb Outdated Show resolved Hide resolved

Use plain hash

b2e38cf

ioquatix requested changes Feb 10, 2023

View reviewed changes

lib/logger/severity.rb Outdated Show resolved Hide resolved

lib/logger.rb Outdated Show resolved Hide resolved

mperham and others added 2 commits February 9, 2023 16:50

refactoring

a8cd907

Update lib/logger/severity.rb

dbed92b

Co-authored-by: Samuel Williams <samuel.williams@oriontransfer.co.nz>

ioquatix approved these changes Feb 10, 2023

View reviewed changes

ioquatix requested a review from jeremyevans February 10, 2023 00:53

jeremyevans approved these changes Feb 10, 2023

View reviewed changes

polish

0b1ad4b

ioquatix merged commit 7aabb0b into ruby:master Feb 10, 2023

matzbot pushed a commit to ruby/ruby that referenced this pull request Feb 10, 2023

[ruby/logger] Add Logger#with_level{...} for block-scoped log level.

194520f

(ruby/logger#85) * Update lib/logger/severity.rb ruby/logger@7aabb0b4aa

sambostock mentioned this pull request Feb 15, 2023

Fix compatibility with Ruby HEAD & add Ruby versions to CI steveklabnik/mono_logger#12

Merged

st0012 mentioned this pull request Mar 4, 2023

Constructor override is out-of-sync with Ruby 3.3.0's Logger class steveklabnik/mono_logger#13

Closed

kou mentioned this pull request Jun 25, 2023

Add support for upcoming logger 1.4.3 jekyll/jekyll#9392

Merged

This was referenced Sep 26, 2023

rspec testsuite fails with ruby 3.3.0dev chef/mixlib-log#73

Open

testsuite fails on ruby3.3.0dev due to Logger change asciidoctor/asciidoctor#4493

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add thread-local log level #85

Add thread-local log level #85

mperham commented Jan 15, 2023

mperham Feb 6, 2023

ioquatix Feb 7, 2023

jeremyevans Feb 9, 2023

ioquatix commented Feb 7, 2023

ioquatix left a comment

jeremyevans left a comment

jeremyevans Feb 9, 2023

ioquatix commented Feb 9, 2023 •

edited

mperham commented Feb 9, 2023

jeremyevans left a comment

ioquatix commented Feb 10, 2023

jeremyevans left a comment

jeremyevans Feb 10, 2023

mperham commented Feb 10, 2023

jeremyevans commented Feb 10, 2023

mperham commented Feb 10, 2023

ioquatix commented Feb 10, 2023

yahonda commented Feb 14, 2023

jaynetics commented Jul 9, 2023

mperham commented Jul 9, 2023

skipkayhil commented Sep 25, 2023

Add thread-local log level #85

Add thread-local log level #85

Conversation

mperham commented Jan 15, 2023

mperham Feb 6, 2023

Choose a reason for hiding this comment

ioquatix Feb 7, 2023

Choose a reason for hiding this comment

jeremyevans Feb 9, 2023

Choose a reason for hiding this comment

ioquatix commented Feb 7, 2023

ioquatix left a comment

Choose a reason for hiding this comment

jeremyevans left a comment

Choose a reason for hiding this comment

jeremyevans Feb 9, 2023

Choose a reason for hiding this comment

ioquatix commented Feb 9, 2023 • edited

mperham commented Feb 9, 2023

jeremyevans left a comment

Choose a reason for hiding this comment

ioquatix commented Feb 10, 2023

jeremyevans left a comment

Choose a reason for hiding this comment

jeremyevans Feb 10, 2023

Choose a reason for hiding this comment

mperham commented Feb 10, 2023

jeremyevans commented Feb 10, 2023

mperham commented Feb 10, 2023

ioquatix commented Feb 10, 2023

yahonda commented Feb 14, 2023

jaynetics commented Jul 9, 2023

mperham commented Jul 9, 2023

skipkayhil commented Sep 25, 2023

ioquatix commented Feb 9, 2023 •

edited