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

[Prototype] Object tracking and observability #451

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
1 change: 1 addition & 0 deletions lib/identity_cache.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
require "identity_cache/fallback_fetcher"
require 'identity_cache/without_primary_index'
require 'identity_cache/with_primary_index'
require "identity_cache/tracking"

module IdentityCache
extend ActiveSupport::Concern
Expand Down
6 changes: 4 additions & 2 deletions lib/identity_cache/cached/prefetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,10 @@ def fetch_association(load_strategy, klass, association, records, &block)
return yield
end

cached_association = klass.cached_association(association)
cached_association.fetch_async(load_strategy, records, &block)
IdentityCache::Tracking.skip_object_tracking do
cached_association = klass.cached_association(association)
cached_association.fetch_async(load_strategy, records, &block)
end
end
end
end
Expand Down
1 change: 1 addition & 0 deletions lib/identity_cache/query_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ def was_new_record? # :nodoc:
def fetch_recursively_cached_association(ivar_name, dehydrated_ivar_name, association_name) # :nodoc:
assoc = association(association_name)

IdentityCache::Tracking.track_association_accessed(self, association_name)
if assoc.klass.should_use_cache? && !assoc.loaded?
if instance_variable_defined?(ivar_name)
instance_variable_get(ivar_name)
Expand Down
70 changes: 70 additions & 0 deletions lib/identity_cache/tracking.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
# frozen_string_literal: true

module IdentityCache
module Tracking
TrackedObject = Struct.new(:object, :includes, :caller, :accessed_associations)

extend self

def tracked_objects
Thread.current[:idc_tracked_objects] ||= {}
end

def reset_tracked_objects
Thread.current[:idc_tracked_objects] = {}
end

def track_object(object, includes)
return unless object_tracking_enabled
locations = caller(1, 20)
tracked_objects[object] = TrackedObject.new(object, Array(includes), locations, Set.new)
dylanahsmith marked this conversation as resolved.
Show resolved Hide resolved
end

def track_association_accessed(object, association_name)
return unless object_tracking_enabled
obj = self.tracked_objects[object]
obj.accessed_associations << association_name if obj
dylanahsmith marked this conversation as resolved.
Show resolved Hide resolved
end

def instrument_and_reset_tracked_objects
tracked_objects.each do |_, to|
ActiveSupport::Notifications.instrument('object_track.identity_cache', {
object: to.object,
accessed_associations: to.accessed_associations,
caller: to.caller
})
end
reset_tracked_objects
end

def with_object_tracking_and_instrumentation
begin
with_object_tracking { yield }
ensure
instrument_and_reset_tracked_objects
Copy link
Contributor

Choose a reason for hiding this comment

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

If an exception causes associations to not be used in the block, then that doesn't mean that the block shouldn't load those associations. So we might want to just call reset_tracked_objects here, extract that call from instrument_and_reset_tracked_objects and call the instrumentation at the end of the begin block.

end
end
Comment on lines +40 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def with_object_tracking_and_instrumentation
begin
with_object_tracking { yield }
ensure
instrument_and_reset_tracked_objects
end
end
def with_object_tracking_and_instrumentation
with_object_tracking { yield }
ensure
instrument_and_reset_tracked_objects
end


def with_object_tracking(enabled: true)
begin
orig = object_tracking_enabled
Comment on lines +49 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

If object_tracking_enabled fails, then we don't want the ensure block to be executed

Suggested change
begin
orig = object_tracking_enabled
orig = object_tracking_enabled
begin

self.object_tracking_enabled = enabled
yield
ensure
self.object_tracking_enabled = orig
end
end

def skip_object_tracking
with_object_tracking(enabled: false) { yield }
end

def object_tracking_enabled
Thread.current[:object_tracking_enabled]
end

def object_tracking_enabled=(value)
Thread.current[:object_tracking_enabled] = value
end
end
end
2 changes: 2 additions & 0 deletions lib/identity_cache/with_primary_index.rb
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ def fetch_by_id(id, includes: nil)
ensure_base_model
raise_if_scoped
record = cached_primary_index.fetch(id)
IdentityCache::Tracking.track_object(record, includes) if record
prefetch_associations(includes, [record]) if record && includes
record
end
Expand All @@ -123,6 +124,7 @@ def fetch_multi(*ids, includes: nil)
raise_if_scoped
ids.flatten!(1)
records = cached_primary_index.fetch_multi(ids)
records.each { |record| IdentityCache::Tracking.track_object(record, includes) }
prefetch_associations(includes, records) if includes
records
end
Expand Down
116 changes: 116 additions & 0 deletions test/tracking_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
# frozen_string_literal: true
require "test_helper"

class SaveTest < IdentityCache::TestCase
def setup
super
Item.cache_index(:title, unique: true)
Item.cache_has_many(:normalized_associated_records, embed: true)

@record = Item.create(title: 'bob')
@record.normalized_associated_records.create!
end

def test_fetch_index_tracks_object_no_accessed_associations
fill_cache

captured = 0
subscriber = ActiveSupport::Notifications.subscribe('object_track.identity_cache') do |_, _, _, _, payload|
captured += 1
assert_same_record(@record, payload[:object])
assert_equal [].to_set, payload[:accessed_associations]
assert_kind_of Array, payload[:caller]
end

IdentityCache::Tracking.with_object_tracking_and_instrumentation do
Item.fetch_by_title('bob')
end
assert_equal 1, captured
ensure
ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber
end

def test_fetch_index_tracks_object_with_accessed_associations
fill_cache

captured = 0
subscriber = ActiveSupport::Notifications.subscribe('object_track.identity_cache') do |_, _, _, _, payload|
captured += 1
assert_same_record(@record, payload[:object])
assert_equal [:normalized_associated_records].to_set, payload[:accessed_associations]
assert_kind_of Array, payload[:caller]
end

IdentityCache::Tracking.with_object_tracking_and_instrumentation do
item = Item.fetch_by_title('bob')
item.fetch_normalized_associated_records
end
assert_equal 1, captured
ensure
ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber
end


def test_fetch_tracks_object_no_accessed_associations
fill_cache

captured = 0
subscriber = ActiveSupport::Notifications.subscribe('object_track.identity_cache') do |_, _, _, _, payload|
captured += 1
assert_same_record(@record, payload[:object])
assert_equal [].to_set, payload[:accessed_associations]
assert_kind_of Array, payload[:caller]
end

IdentityCache::Tracking.with_object_tracking_and_instrumentation do
item = Item.fetch(@record.id)
end
assert_equal 1, captured
ensure
ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber
end

def test_fetch_tracks_object_with_accessed_associations
fill_cache

captured = 0
subscriber = ActiveSupport::Notifications.subscribe('object_track.identity_cache') do |_, _, _, _, payload|
captured += 1
assert_same_record(@record, payload[:object])
assert_equal [:normalized_associated_records].to_set, payload[:accessed_associations]
assert_kind_of Array, payload[:caller]
end

IdentityCache::Tracking.with_object_tracking_and_instrumentation do
item = Item.fetch(@record.id)
item.fetch_normalized_associated_records
end
assert_equal 1, captured
ensure
ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber
end

def test_does_not_track_objects_unless_enabled
fill_cache

subscriber = ActiveSupport::Notifications.subscribe('object_track.identity_cache') do |_, _, _, _, payload|
refute
end

item = Item.fetch_by_title('bob')
ensure
ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber
end

private

def fill_cache
item = Item.fetch_by_title('bob')
assert item
item.fetch_normalized_associated_records
end

def assert_same_record(expected, actual)
assert_equal expected.id, actual.id
end
end