reidmorrison / rails_semantic_logger

Rails Semantic Logger replaces the Rails default logger with Semantic Logger
https://logger.rocketjob.io/rails
Apache License 2.0
320 stars 114 forks source link

Error when using customer logger in Rails with 7.1.0.alpha (main) #182

Closed bmad closed 10 months ago

bmad commented 1 year ago

Rails 7.1.0.alpha (main) branch now added a comparison to ActiveSupport::LogSubscriber expecting its logger’s level to always be an integer. This now causes an error using a custom semantic_logger logger because the logger uses a symbol to indicate log level.

Expected Behavior

A custom semantic_logger logger should work the same in Rails 7.1.0.alpha (main).

Actual Behavior

MANUAL STEPS TO REPRODUCE:

An ArgumentError: comparison of Symbol with 0 failed is raised.

Solution Thoughts

I filed an issue on Rails here https://github.com/rails/rails/issues/47451 and filed a PR to fix the issue on Rails here https://github.com/rails/rails/pull/47427. Unfortunately, after a discussion on the PR, I closed it out because it no longer seemed like a good idea.

To continue being able to use semantic_logger in Rails, I can only think of refactoring semantic_logger to use the standard ruby logger level integers. We could potentially keep the :trace level by having it's value be -1. This would unfortunately still change the interface of the gem.

I might be able to work on this, but I'd like to make sure this is an acceptable solution.

simi commented 1 year ago

I was looking into this problem as well. It is possible to bandaid this with following "shim". Anyway it makes sense to me to inline level method with Logger#level and return index instead of name. Indeed major bump would be needed.

module SemanticLoggerFix
  def silenced?(event)
    logger.nil? || logger.send(:level_index) > @event_levels.fetch(event, Float::INFINITY)
  end
end 

ActiveSupport::LogSubscriber.prepend(SemanticLoggerFix)
bmad commented 1 year ago

Ah, I would think that would work. Except we would need to do something like (logger.send(:level_index) - 1) because semantic_logger has trace at the 0 index, which bumps all of the other levels by one in comparison to ruby standard logging integers.

reidmorrison commented 1 year ago

Going to move to Rails Semantic Logger to see if we can make it include the above patch on Rails 7.

reidmorrison commented 1 year ago

There is no Rails 7.1.0.alpha gem available yet. Not able to add it to the tests, not only that minitest-rails does not support it yet either.

Rails Semantic Logger swaps out the stock Rails log subscribers. We can update them to ignore the silenced method since it may be duplicating what Semantic Logger already does (class level logging levels). https://github.com/reidmorrison/rails_semantic_logger/blob/2d25659ce6cd03bf2f606ae8e7e2fc2dee5d2eac/lib/rails_semantic_logger/engine.rb#L153

With a full stack trace I can help diagnose further. Or, if you have the instructions on how to build a new rails app using rails main I could give it a try.

bmad commented 1 year ago

@reidmorrison - Thanks for looking at this, you should be able to reproduce this by:

To be able to see the full stack trace including the rails code, you'll need to clone the rails repo locally and update the Gemfile rails reference in the app from the above steps to: gem "rails", path: "<cloned_rails_path>"

spickermann commented 12 months ago

I just started upgrading an application to gem 'rails', '7.1.0.beta1' and I am observing the same issue.

preethi29 commented 10 months ago

@reidmorrison facing the same issue. Is there any plan to fix the compatibility with rails 7.1?

reidmorrison commented 10 months ago

rails_semantic_logger v 4.13.0 has been published and includes several community contributions to get it working with Rails 7.1. Please try the new version and open a new issue if the problem persists.

fguillen commented 3 months ago

I have added a new issue here: https://github.com/reidmorrison/rails_semantic_logger/issues/219