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

Allow cleaning backtraces used to identify N+1 queries #79

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

Conversation

tf
Copy link

@tf tf commented Jan 24, 2024

Some gems cache missing methods after the first call. This can cause Prosopite to miss duplicate queries. For example, the N+1 query for author in the following builder template goes unnoticed if there are two posts since xml.post is a missing method that gets cached for the second call (see [1]).

@posts.each do |post|
  xml.post do
    xml.author post.author.name
  end
end

Add location_backtrace_cleaner config option to modify backtraces that are used to identify queries. This allows to either silence problematic lines or even just use the backtrace inside the application itself:

Prosopite.location_backtrace_cleaner = Rails.backtrace_silencer

[1] https://github.com/jimweirich/builder/blob/c80100f8205b2e918dbff605682b01ab0fabb866/lib/builder/xmlbase.rb#L91

Some gems cache missing methods after the first call. This can cause
Prosopite to miss duplicate queries. For example, the N+1 query for
`author` in the following `builder` template goes unnoticed if there
are two posts since `xml.post` is a missing method that gets cached
for the second call (see [1]).

```ruby

@posts.each do |post|
  xml.post do
    xml.author post.author.name
  end
end

```

Add `location_backtrace_cleaner` config option to modify backtraces
that are used to identify queries. This allows to either silence
problematic lines or even just use the backtrace inside the
application itself:

```ruby
Prosopite.location_backtrace_cleaner = Rails.backtrace_silencer
```

[1] https://github.com/jimweirich/builder/blob/c80100f8205b2e918dbff605682b01ab0fabb866/lib/builder/xmlbase.rb#L91
@tf
Copy link
Author

tf commented Jan 24, 2024

Alternatively, one could always create associations with at least three records in tests. This bloats test setup, though, and is easy to forget. Only using application backtraces means erring on the side of caution.

Happy to take suggestions regarding the name of the option. Having both a backtrace_silencer and a location_backtrace_silencer option might not be ideal.

One alternative I considered is to instead add an option to allow replacing the whole location_key generation. This would also let me plug in my own backtrace silencer before creating the hash. One would need to replicate the SHA256.hexdigest call in one's config then, though. The suggestion solution seemed more specific to the problem.

I also considered adding an option like use_app_backtrace_only which causes Rails.backtrace_silencer to be applied directly. This would not work in Rails engine tests, tough, since there Rails.backtrace_silencer removes everything but dummy app locations.

@tf
Copy link
Author

tf commented Jan 24, 2024

Also happy to add a README section if this is not considered too much of an edge case.

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.

1 participant