-
-
Notifications
You must be signed in to change notification settings - Fork 135
Add rails 8.1 Support and Tests #280
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
base: master
Are you sure you want to change the base?
Conversation
|
@Austio What do you make of grncdr's analysis here - #276 (comment) ? IMO he's correct - this log subscriber shouldn't be messing with ActiveRecord's runtime stats. Just deleting the calls would break Rails 5/6 compatibility though, so it might need a version bump. |
|
@jdelStrother Thank you - that is an excellent point i'll double check here and push a better change if it is valid (seems like it) |
…a rails version > 8.1
4759fc6 to
8f85e1e
Compare
|
@jdelStrother Fixed for this change, if we want to backport this to other versions it would be a simple change of the constant to the versions we want to skip setting runtime on 8f85e1e |
|
@reidmorrison Have this ready for review, should close the gap on the following
|
| module ActiveRecord | ||
| class LogSubscriber < ActiveSupport::LogSubscriber | ||
| IGNORE_PAYLOAD_NAMES = %w[SCHEMA EXPLAIN].freeze | ||
| RAILS_VERSION_ENDING_SET_RUNTIME_SUPPORT = Gem::Version.new( "8.1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on @Austio!
I think this should probably be set to 8.0 or 8.0.3
The original issue that tipped me off to this problem is #273 where the reporter says they are using 8.0.3, but I suspect that metrics have been double counted on even older versions, and @jeromepl is the first to notice.
Issue
See: rails/rails@7d12071
Closes #276
Closes #279
I cherry picked from zzak here and added specs showing the change passes to reduce burden on maintainer here.
I further addressed #276 (comment) by not writing to stats on rails 8.1 and later.
Description of changes
Failing Spec with Adding Rails 8.1 support
Passing Spec with CherryPicking change from zzak