ActiveRecord::RuntimeRegistry.sql_runtime private API moved

[zzak]

See: rails/rails@7d12071

This change will land in Rails 8.1.

[orehmane]

Started running into this one, would be great to get it merged.

[jasonligg]

Seeing this as well. 🙏

[alexdeng-mp]

We're seeing this too. Would be awesome to get this in soon

[robertino]

bumping again, this is causing errors now:
Could not log "sql.active_record" event. NoMethodError: undefined method 'sql_runtime' for module ActiveRecord::RuntimeRegistry on Rails 8.1.0 which was released yesterday

[orehmane]

I created a monkeypatch to fix this in the meantime, based on @rubyroob's excellent work in #249.

Stick this in a file in config/initializers and it should clear up issues until the gem is updated.

# frozen_string_literal: true

LAST_TESTED_VERSION = '4.18.0'

require 'rails_semantic_logger/version'

unless RailsSemanticLogger::VERSION == LAST_TESTED_VERSION
  raise "rails_semantic_logger is version #{RailsSemanticLogger::VERSION} but the monkey patch was last tested on " \
        "#{LAST_TESTED_VERSION} - manually check if it can find the sql_runtime module."
end

module RailsSemanticLogger
  module ActiveRecord
    class LogSubscriber < ActiveSupport::LogSubscriber
      def self.runtime=(value)
        if ::ActiveRecord::RuntimeRegistry.respond_to?(:stats)
          ::ActiveRecord::RuntimeRegistry.stats.sql_runtime = value
        else
          ::ActiveRecord::RuntimeRegistry.sql_runtime = value
        end
      end

      def self.runtime
        if ::ActiveRecord::RuntimeRegistry.respond_to?(:stats)
          ::ActiveRecord::RuntimeRegistry.stats.sql_runtime ||= 0
        else
          ::ActiveRecord::RuntimeRegistry.sql_runtime ||= 0
        end
      end
    end
  end
end

[yuki-yogi]

@zzak
Hello, Thank you so much for submitting this pull request! I know you must be busy, but I would really appreciate it if you could merge this when you have a free moment. Thank you for your contribution.

[rubyroobs]

I created a monkeypatch to fix this in the meantime, based on @rubyroob's excellent work in #249.

It's a shame your tag didn't work, as I started seeing this error today - thanks @orehmane !

[orehmane]

Thank you @rubyroobs! Your patch saved me earlier, just trying to pay it forward. :)

[totus]

@reidmorrison would it be possible to have it merged sooner rather than later given that rails 8.1 is out?

[rud]

Rails 8.1.0 is out, this looks like a neat little patch to rollout

[ximus]

@reidmorrison more and more people are going to start hitting this on rails 8.1+. This patch is important to have.

[basiszwo]

would be great to see this one merged.

[horaciob]

please 🚀

[sh1okoh]

Let's goooo 🚀

[yuki-yogi]

please 🚀

[rgaufman]

Any update?

[nicholas-greenwood]

+1

[jdelStrother]

I'd love to see this merged as much as the next person, but it's really rude to constantly ping a maintainer with +1s for updates.

[nbeyer]

I'd love to see this merged as much as the next person, but it's really rude to constantly ping a maintainer with +1s for updates.

I disagree. This is how the community communicates interest. I’ve experienced this in many open source communities and several actively encourage this. What’s the alternative?

Additionally, this is how the community can monitor the likeness of the project. If there is no movement, at some point the community will need to decide if it moves on to an alternative.

[grncdr]

I think this PR is incorrect.

The methods being patched here were removed in Rails 7.2 (along with reset_runtime) and should not be called or used on anything newer than Rails 7.1 SemanticLoggerRails should instead use the stats added to the instrumentation payload by Rails itself.

The exception actually originates from this line:

rails_semantic_logger/lib/rails_semantic_logger/active_record/log_subscriber.rb

Line 29 in ce7e781

self.class.runtime += event.duration

... which is almost certainly the cause of #273 and should probably be removed.

Here's my alternative monkey-patch, which so far seems to have no ill effects in my app:

class RailsSemanticLogger::ActiveRecord::LogSubscriber
  # The ActiveSupport::LogSubscriber.runtime* class attributes were removed in Rails 7.2
  # and the private ActiveRecord::RuntimeRegistry had it's API completely changed in Rails 8.1
  #
  # This monkey patch just turns these operations into no-ops as it's actually not the job of
  # RailsSemanticLogger (or any ActiveSupport::LogSubscriber) to track this information.
  #
  # In reality the `#sql` method needs to be patched, but I don't want to copy-paste all of it into my project.
  def self.runtime = 0
  def self.runtime=(_ms); end
end

[jdelStrother]

@grncdr ooph, good catch. It looks like RailsSemanticLogger's LogSubscriber started out as basically a copy & paste of Rails' ActiveRecord LogSubscriber from v5 - https://github.com/rails/rails/blob/v5.2.8.1/activerecord/lib/active_record/log_subscriber.rb and their behaviour has since diverged some. The behaviour where .sql would increment LogSubscriber.runtime was removed in v7.1 - rails/rails@f56b418

The gemspec says we still support v5, so if we did just remove the runtime changes I guess that upgraders who are still on Rails < 7.1 will see their db_runtimes drop to 0.

Not sure how Reid feels about supporting old Rails versions. FWIW Rails itself has already dropped support for 7.1

[grncdr]

IMO a new release that requires Rails 7.2 would be totally reasonable, possibly with a major version bump if we want to be extra careful.