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

Conversation

kirs
Copy link
Contributor

@kirs kirs commented Apr 6, 2020

I'm not sure we need this yet, but I wanted to throw a prototype that I've played with that might be useful for anyone else who's going to work on this area.

As mentioned in caveats, large blobs and too many embedded associations are a major problem.

At the same time we don't know how often which embedded associations are accessed and if some of them do more harm than good.

This PR brings "object tracking" to IDC. Here's how it works:

  1. At the beginning of the request, allocate an array for all objects that will be loaded by IDC
  2. Track every read of the embedded association in that array
  3. At the end of the request, iterate over objects that were loaded from the IDC and check what embedded associations were accessed.

From the memory/GC point, it means those records would have to be kept around for longer, until the request if finished. That's not great, but I believe is tolerable for a percentage of request.

By subscribing to AS::N events, you could either observe it locally or trace it in production:

ActiveSupport::Notifications.subscribe('object_track.identity_cache') do |_event, _, _, _, payload|
  # for local visibility
  # puts "--- #{payload[:object].class.to_s}(id=#{payload[:object].id}): #{payload[:accessed_associations].to_a.join(", ")}"
  # payload[:caller].each do |line|
  #   puts " #{line}"
  # end

  # in production, assuming that Tracing.trace is an abstract way to emit spans
  Tracing.trace('idc.object_tracking', tags: payload) { }
end

If your tracing environment is set up to emit data to something like BigQuery (which we use at Shopify), this could be a ground to make a data-driven decision about removing some of embedded associations.

Or maybe I'm overthinking this and there's an easier way?

cc @dylanahsmith @hkdsun @edward @Scalvando @floriecai

Copy link
Contributor

@dylanahsmith dylanahsmith left a comment

Choose a reason for hiding this comment

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

This tracking seems quite global, yet also has very fine grained data such as the stack trace for the location of the fetch. It seems like we might get the worst of both worlds with this mixing of use cases.

For instance, in the global use case we probably want to figure out the percentage of times that a fetched embedded association is actually used and probably don't actually need the stack trace, so its overhead might just be an anti-feature.

On the other hand, if we are trying to use this locally to find unused embedded associations, tracking all fetches in a block could result in capturing much more than we care about, adding a lot of noise to the logs. In that case, we are also more likely wanting to track only a specific cache fetch, where the API is kind of awkward for that purpose, since we don't want to capture all the other fetches while the one we are interested in could get used.

Perhaps we should try to provide a simpler and more flexible primitive that is more like accessed_fields.

It looks like there are two parts to the solution we need:

  1. a way to selectively choose which cache fetched records to track
  2. a way to collect information on which associations were actually used

Choosing which cache fetched records to track seems like it would often consist of wrapping the cache fetch in order to collect the records to track. This could be done outside of this library using monkey patching for the global use case. However, if we were trying to track specific cache fetchers, we might instead want to explicitly call the method to track the object, which wouldn't need any monkey patching. Either way, we don't really need to built this into the library.

Collecting the information on which associations were actually used on a record at the end of the records lifetime is the part where we need some primitive in this library to easily support. For this, I think we just need a method like accessed_cached_associations or maybe a method per association like accessed_cached_association?(name). That way we can iterate over the tracked records and find which cache embedded associations were fetched and not accessed.

Having something more general like this would give the application control over what is collected when starting to track an object (e.g. for grouping) and would allow us to find more than just unused cache embedded associations. For example, we might want to use the same instrumentation to find id embedded associations that are included in a cache fetch but not used. We could also make the instrumentation more generic in order to help find preloaded associations that aren't used (e.g. by checking for the @proxy instance variable on the association object) or any other type of overfetching.

Comment on lines +49 to +50
begin
orig = object_tracking_enabled
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

Comment on lines +40 to +46
def with_object_tracking_and_instrumentation
begin
with_object_tracking { yield }
ensure
instrument_and_reset_tracked_objects
end
end
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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants