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

feat: allow custom locking mechanism #20

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

paulsturgess
Copy link
Contributor

@paulsturgess paulsturgess commented Oct 21, 2024

This PR adds a new config option to allow using the Metricks gem without also requiring the with_advisory_lock gem.

Now it's possible to define a Rails initializer that provides a custom proc for performing the lock when a metric is recorded.

# config/initializers/metricks.rb
Rails.application.config.metricks.with_lock = proc do |key, opts, block|
  opts ||= {}
  timeout_seconds = opts[:timeout_seconds] || 60

  MyCustomLock.with_lock(key: key, timeout_seconds: timeout_seconds, &block)
end

If the initializer is not defined then the existing with_advisory_lock behaviour continues to work as it does now.

The runtime dependency has been removed from metricks.gemspec which would previously verify the required presence of the with_advisory_gem in the host Rails app during bundle install. This has been replaced with a runtime check when the Metricks engine is initialized.

ActiveRecord and Rails 7.1 have been added to the tests.

QA instructions

Instructions for testing Katapult are detailed here:
https://github.com/krystal/katapult/issues/4425#issuecomment-2426174017

@paulsturgess paulsturgess self-assigned this Oct 21, 2024
@@ -3,17 +3,26 @@
if Gem::Version.new(RUBY_VERSION) < Gem::Version.new('3.0.0')
appraise 'activerecord-5-2' do
gem 'activerecord', '~> 5.2.0'
gem 'rails', '~> 5.2.0'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added rails to the test scenarios because of the new engine_spec.rb file which checks the engine initialization works.

Comment on lines +16 to +33
it 'allows with_lock to be configured' do
success = false

allow(mock_app.config.metricks).to receive(:with_lock)
.and_return(->(result, opts, block) { block.call(result, opts) })

expect {
mock_app.initialize!
}.not_to raise_error

Metricks::Lock.with_lock(true, {}) do |result|
success = result
end

expect(success).to be(true)

expect(Metricks::Lock).to have_received(:validate!)
end
Copy link
Contributor Author

@paulsturgess paulsturgess Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In an ideal world we might test various different rails app configurations. I did try to get that working, but Rails basically doesn't expect multiple apps to be declared and they were leaking into different tests. In the end I went with just verifying the override works and that we call the expected validation.

The new Lock class is unit tested for all the different scenarios anyway.

@@ -15,5 +15,4 @@ Gem::Specification.new do |gem|
gem.email = ['[email protected]']
gem.required_ruby_version = '>= 2.7'
gem.add_runtime_dependency 'activerecord', '>= 5.0'
gem.add_runtime_dependency 'with_advisory_lock', '>= 4.6', '< 5.0'
Copy link
Contributor Author

@paulsturgess paulsturgess Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing to watch out for when upgrading this gem is that these versions of with_advisory_lock will no longer be enforced. FWIW I ran this gem with version 5.1.0 of with_advisory_lock in my testing and it seemed fine.

The changelog shows v5 dropped support for ruby below 2.7 and activerecord below 6.1

Ultimately the host app can decide what version of with_advisory_lock they want.

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.

2 participants