Skip to content

Commit

Permalink
improve and fix instrumentation
Browse files Browse the repository at this point in the history
* only notify about load.cached if docs are actually cached
* add payload (ids, docs) to events
  • Loading branch information
langalex committed Apr 2, 2024
1 parent c915c3d commit 49e23aa
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 11 deletions.
21 changes: 14 additions & 7 deletions lib/couch_potato/database.rb
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ def load_document(id)
cached = cache && cache[id]
if cache
if cached
ActiveSupport::Notifications.instrument('couch_potato.load.cached') do
ActiveSupport::Notifications.instrument('couch_potato.load.cached', id: id, doc: cached) do
cached
end
else
Expand All @@ -168,15 +168,18 @@ def load_documents(ids)

uncached_ids = ids - (cache&.keys || [])
uncached_docs_by_id = bulk_load(uncached_ids).index_by {|doc| doc.id if doc.respond_to?(:id) }
cached_docs_by_id = cache&.slice(*ids) || {}
if cached_docs_by_id.any?
ActiveSupport::Notifications.instrument('couch_potato.load.cached', ids: cached_docs_by_id.keys, docs: cached_docs_by_id.values) do
cached_docs_by_id
end
end
if cache
uncached_ids.each do |id|
doc = uncached_docs_by_id[id]
cache[id] = doc if doc
end
end
cached_docs_by_id = ActiveSupport::Notifications.instrument('couch_potato.load.cached') do
cache&.slice(*ids) || {}
end
ids.filter_map { |id| (cached_docs_by_id[id]) || uncached_docs_by_id[id] }
end

Expand Down Expand Up @@ -278,9 +281,11 @@ def raw_view(spec)
def load_document_without_caching(id)
raise "Can't load a document without an id (got nil)" if id.nil?

ActiveSupport::Notifications.instrument('couch_potato.load') do
payload = {id: id}
ActiveSupport::Notifications.instrument('couch_potato.load', payload) do
instance = couchrest_database.get(id)
instance.database = self if instance
payload[:doc] = instance
instance
end
end
Expand Down Expand Up @@ -321,9 +326,11 @@ def save_document_without_conflict_handling(document, validate = true)
def bulk_load(ids)
return [] if ids.empty?

ActiveSupport::Notifications.instrument('couch_potato.load') do
payload = {ids: ids}
ActiveSupport::Notifications.instrument('couch_potato.load', payload) do
response = couchrest_database.bulk_load ids
docs = response['rows'].map { |row| row['doc'] }.compact
docs = response["rows"].map { |row| row["doc"] }.compact
payload[:docs] = docs
docs.each do |doc|
doc.database = self if doc.respond_to?(:database=)
doc.database_collection = docs if doc.respond_to?(:database_collection=)
Expand Down
10 changes: 7 additions & 3 deletions lib/couch_potato/persistence/ghost_attributes.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
module CouchPotato
module GhostAttributes #:nodoc:
def method_missing(name, *args)
if(value = _document && _document[name.to_s])
module GhostAttributes # :nodoc:
def method_missing(name, *)
if (value = _document && _document[name.to_s])
value
else
super
end
end

def respond_to_missing?(name, *)
_document && _document[name.to_s]
end
end
end

54 changes: 53 additions & 1 deletion spec/unit/caching_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
{}
end

after(:each) do
ActiveSupport::Notifications.unsubscribe(@subscriber) if @subscriber
end

context 'for a single document' do
it 'gets an object from the cache the 2nd time via #load_documemt' do
expect(couchrest_db).to receive(:get).with('1').exactly(1).times
Expand Down Expand Up @@ -47,6 +51,29 @@
db.load_document '1'
expect(db.load_document('1')).to eql(doc)
end

it 'instruments the load call' do
doc = double("doc").as_null_object
allow(couchrest_db).to receive(:get).and_return(doc)
events = []
@subscriber = ActiveSupport::Notifications.subscribe(
'couch_potato.load.cached'
) do |event|
events << event
end

db.load("1")
db.load("1")

expect(events.size).to eq(1)
expect(events.first.payload).to eq(
{
id: "1",
doc:
}
)

end
end

context 'for multiple documents' do
Expand All @@ -65,6 +92,31 @@
expect(couchrest_db).to have_received(:bulk_load).with(['2']).exactly(1).times
end

it 'instruments the load call' do
allow(couchrest_db).to receive(:bulk_load).with(['1'])
.and_return('rows' => [{'doc' => doc1}])
allow(couchrest_db).to receive(:bulk_load).with(['2'])
.and_return('rows' => [{'doc' => doc2}])
events = []
@subscriber = ActiveSupport::Notifications.subscribe(
'couch_potato.load.cached'
) do |event|
events << event
end


db.load_document(['1'])
db.load_document(['1', '2'])

expect(events.size).to eq(1)
expect(events.first.payload).to eq(
{
ids: ["1"],
docs: [doc1]
}
)
end

it 'loads nothing if all documents are cached' do
allow(couchrest_db).to receive(:bulk_load).with(['1', '2'])
.and_return('rows' => [{'doc' => doc1}, {'doc' => doc2}])
Expand Down Expand Up @@ -94,7 +146,7 @@
'id' => '2',
}
allow(couchrest_db).to receive(:bulk_load).with(['1', '2'])
.and_return('rows' => [{'doc' => doc1}, {'doc' => doc1}])
.and_return('rows' => [{'doc' => doc1}, {'doc' => doc2}])

db.load_document(['1', '2'])
db.load_document(['1', '2'])
Expand Down
45 changes: 45 additions & 0 deletions spec/unit/database_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ class Child
let(:couchrest_db) { double('couchrest db', info: nil).as_null_object }
let(:db) { CouchPotato::Database.new couchrest_db }


after(:each) do
ActiveSupport::Notifications.unsubscribe(@subscriber) if @subscriber
end

it 'should raise an exception if nil given' do
expect do
db.load nil
Expand Down Expand Up @@ -130,6 +135,46 @@ class Child
it 'returns an empty array when passing an empty array' do
expect(db.load([])).to eq([])
end

it 'instruments the load call' do
events = []
@subscriber = ActiveSupport::Notifications.subscribe(
'couch_potato.load'
) do |event|
events << event
end

db.load(["1", "2"])

expect(events.size).to eq(1)
expect(events.first.payload).to eq(
{
ids: ["1", "2"],
docs: [doc1, doc2]
}
)
end
end

it 'instruments the load call' do
doc = double("doc").as_null_object
allow(couchrest_db).to receive(:get).and_return(doc)
events = []
@subscriber = ActiveSupport::Notifications.subscribe(
'couch_potato.load'
) do |event|
events << event
end

db.load("1")

expect(events.size).to eq(1)
expect(events.first.payload).to eq(
{
id: "1",
doc:
}
)
end
end

Expand Down

0 comments on commit 49e23aa

Please sign in to comment.