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

Merged
merged 1 commit into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,6 @@ gemfiles

## Ignore lock file
Gemfile.lock

# Ignore all logfiles
/log/*
9 changes: 9 additions & 0 deletions Appraisals
Original file line number Diff line number Diff line change
Expand Up @@ -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.

end
end

appraise 'activerecord-6-0' do
gem 'activerecord', '~> 6.0.0'
gem 'rails', '~> 6.0.0'
end

appraise 'activerecord-6-1' do
gem 'activerecord', '~> 6.1.0'
gem 'rails', '~> 6.1.0'
end

appraise 'activerecord-7-0' do
gem 'activerecord', '~> 7.0.0'
gem 'rails', '~> 7.0.0'
end

appraise 'activerecord-7-1' do
gem 'activerecord', '~> 7.1.0'
gem 'rails', '~> 7.1.0'
end
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ group :development do
gem 'appraisal', '~> 2.4'
gem 'rspec'
gem 'sqlite3', '~> 1.4'
gem 'with_advisory_lock'
end
20 changes: 20 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ Supported Active Record versions include:
- 6.0
- 6.1
- 7.0
- 7.1

## Installation

Expand All @@ -26,6 +27,7 @@ To get started, add the following to your Gemfile and run `bundle install`.
```ruby
source "https://rubygems.pkg.github.com/krystal" do
gem 'metricks', '>= 1.0.0', '< 2.0'
gem 'with_advisory_lock' # optional, see below
end
```

Expand All @@ -36,6 +38,24 @@ rake metricks:install:migrations
rake db:migrate
```

### with_advisory_lock

By default Metricks expects you to have the [with_advisory_lock gem](https://github.com/ClosureTree/with_advisory_lock/) installed in your application. This is used to ensure that metrics are stored accurately, refer to the gem itself for details of how the locking works.

If you do not wish to use this gem, you can provide your own locking mechanism in an initializer. The arguments passed to `with_lock` will match the method signature of `with_advisory_lock`.

```ruby
# 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
```

However if you're happy to use with_advisory_lock, you don't need to create an initializer.

## Usage

There are two key types of metric: evented or cumulative. By default, all metrics stored are evented which means they represent an event happening (for example an invoice being raised, a user being created or a product being sold). A cumulative metric stores values that increase or decrease (for example total revenue, MRR or number of active users).
Expand Down
6 changes: 6 additions & 0 deletions lib/metricks/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,15 @@ class Engine < ::Rails::Engine

engine_name 'metricks'

config.metricks = ActiveSupport::OrderedOptions.new
config.metricks.with_lock = nil

initializer 'metricks.initialize' do |app|
ActiveSupport.on_load :active_record do
require 'metricks/models/metric'

Metricks::Lock.with_lock = app.config.metricks.with_lock
Metricks::Lock.validate!
end
end

Expand Down
40 changes: 40 additions & 0 deletions lib/metricks/lock.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
require 'with_advisory_lock' if defined?(WithAdvisoryLock)

# The default locking mechanism is to use the with_advisory_lock gem
# But this can be overriden using an initializer in the host Rails application (refer to README.md)
# This is set in lib/metricks/engine.rb
# Because of this, the with_advisory_lock gem is not a hard dependency.
module Metricks
class Lock

class << self
attr_accessor :with_lock

def with_lock(key, opts = {}, &block)
with_lock_block = @with_lock || default_with_lock

instance_exec(key, opts, block, &with_lock_block)
end

def validate!
return if @with_lock.present?
return if defined?(WithAdvisoryLock)


raise Metricks::Error.new(
'ConfigurationMissing',
message: 'By default Metricks requires with_advisory_lock gem to be installed. ' \
'Alternatively a custom locking mechanism can be configured via config.metricks.with_lock'
)
end

private

def default_with_lock
proc do |key, opts, block|
ActiveRecord::Base.with_advisory_lock(key, opts, &block)
end
end
end
end
end
4 changes: 2 additions & 2 deletions lib/metricks/models/metric.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
require 'with_advisory_lock'
require 'metricks/lock'
require 'metricks/gatherer'
require 'metricks/error'
require 'metricks/compared_set'
Expand Down Expand Up @@ -42,7 +42,7 @@ def record(type, **options)
metric.amount ||= options[:amount] || 1

if type.cumulative?
with_advisory_lock 'AddCumulativeMetric' do
::Metricks::Lock.with_lock 'AddCumulativeMetric' do
existing = self.last(type, after: metric.time, associations: options[:associations])
if existing.present?
raise Metricks::Error.new('CannotAddHistoricalCumulativeMetrics', message: "Nope.")
Expand Down
1 change: 0 additions & 1 deletion metricks.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,4 @@ Gem::Specification.new do |gem|
gem.email = ['help@krystal.uk']
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.

end
4 changes: 4 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
$LOAD_PATH.unshift(File.expand_path('../lib', __dir__))

require 'rails'
require 'active_record'
ActiveRecord::Base.establish_connection adapter: 'sqlite3', database: ':memory:'

Expand All @@ -16,4 +17,7 @@
config.after(:each) do
Metricks::Models::Metric.delete_all
end

config.full_backtrace = true

end
35 changes: 35 additions & 0 deletions spec/specs/engine_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
require 'spec_helper'
require 'metricks/engine'

describe Metricks::Engine do

let(:mock_app) do
Class.new(Rails::Application) do
config.eager_load = false
end
end

before do
allow(Metricks::Lock).to receive(:validate!).and_call_original
end

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
Comment on lines +16 to +33
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.


end
85 changes: 85 additions & 0 deletions spec/specs/lock_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
require 'spec_helper'
require 'with_advisory_lock'
require 'metricks/lock'

describe Metricks::Lock do

describe ".with_lock" do
before do
allow(ActiveRecord::Base).to receive(:with_advisory_lock).and_call_original
end

context "when with_lock is set" do
before do
Metricks::Lock.with_lock = ->(key, opts, block) { block.call(key, opts) }
end

it "calls the block with the args" do
success = false
passed_opts = {}

Metricks::Lock.with_lock(true, {hi: 'there'}) do |result, opts|
success = result
passed_opts = opts
end

expect(success).to be(true)
expect(passed_opts).to eq({hi: 'there'})
expect(ActiveRecord::Base).not_to have_received(:with_advisory_lock)
end
end

context "when with_lock is not set" do
before do
Metricks::Lock.with_lock = nil
end

it "uses with_advisory_lock" do
success = false

Metricks::Lock.with_lock(true, timeout_seconds: 5) do |result, opts|
success = true
end

expect(success).to be(true)
expect(ActiveRecord::Base).to have_received(:with_advisory_lock)
.with(true, {timeout_seconds: 5})
end
end
end

describe ".validate!" do
context "when with_lock is set" do
before do
Metricks::Lock.with_lock = ->(key, opts, block) { block.call }
hide_const("WithAdvisoryLock")
end

it "does not raise an error" do
expect { Metricks::Lock.validate! }.not_to raise_error
end
end

context "when with_lock is not set and WithAdvisoryLock is defined" do
before do
stub_const("WithAdvisoryLock", true)
end

it "does not raise an error" do
expect { Metricks::Lock.validate! }.not_to raise_error
end
end

context "when with_lock is not set and WithAdvisoryLock is not defined" do
before do
Metricks::Lock.with_lock = nil
hide_const("WithAdvisoryLock")
end

it "raises an error" do
expect { Metricks::Lock.validate! }.to raise_error(Metricks::Error)
end
end
end

end
5 changes: 5 additions & 0 deletions spec/specs/models/metric_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
require 'spec_helper'
require 'metricks/models/metric'
require 'with_advisory_lock'

describe Metricks::Models::Metric do
before do
Metricks::Lock.with_lock = nil # ensure it's reset from other specs, set to nil will use the default locking
end

context 'a valid un-saved metric' do
subject(:metric) do
Metricks::Models::Metric.new(type: PotatoesPicked.id, amount: 10.0)
Expand Down
Loading