Skip to content

Conversation

MatheusRich
Copy link
Contributor

@MatheusRich MatheusRich commented Mar 18, 2024

Motivation / Background

This can be used to trace troublesome SQL statements back to the application code that generated these statements. I feel like this is a good default value that helps with debugging and code discovery.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the railties label Mar 18, 2024
@MatheusRich
Copy link
Contributor Author

MatheusRich commented Mar 18, 2024

@rafaelfranca thoughts? If this is approved, I will add tests and a CHANGELOG entry.

@rafaelfranca
Copy link
Member

We already have another feature doing the same in development verbose_query_logs with this one pointing to the exact line of code that generated the query. Why do we need both?

@MatheusRich
Copy link
Contributor Author

@rafaelfranca verbose_query_logs might point to a model scope or method (that might be called inside a view), so it might not be evident which controller or job generated that query. It is also useful when using multiple databases because the query logs can identify which db is being used.

@rafaelfranca
Copy link
Member

Sounds good let's get this finished up.

As a general direction, don't open draft PRs, they are just ignored, and I would actually go as far as automatically closing draft PRs. Either the PR is ready to be reviewed or send a message in the Rails forum. Our attention spam is very small, so if we see a draft PR we will just close the tab since it isn't ready and never come back.

@MatheusRich MatheusRich marked this pull request as ready for review March 22, 2024 14:58
@MatheusRich MatheusRich force-pushed the patch-1 branch 5 times, most recently from c0d08fe to 5da8395 Compare March 22, 2024 15:57
@rails-bot rails-bot bot added the docs label Mar 22, 2024
@MatheusRich
Copy link
Contributor Author

@rafaelfranca Thanks for letting me know! I think this is finished now.

Side note: should we add the guideline to not open draft PRs in the contributing docs?

@skipkayhil
Copy link
Member

How do query logs interact with prepared statements? My understanding was the two were not compatible and all db adapters except MySQL will use prepared statements by default.

stevepolitodesign pushed a commit to thoughtbot/suspenders that referenced this pull request Apr 5, 2024
Creates generator to configure the development environment. Keeps parity
with Rails as much as possible in an effort to avoid drift with Rails
standards.

It's possible a future release of Rails will alleviate us from having to
do the following:

- enable `active_model.i18n_customize_full_message` which is being
  addressed in [#50406][]
- enable `active_record.query_log_tags_enabled` which is being addressed
  in [#51342][]

[#50406]: rails/rails#50406
[#51342]: rails/rails#51342
stevepolitodesign pushed a commit to thoughtbot/suspenders that referenced this pull request Apr 5, 2024
Creates generator to configure the development environment. Keeps parity
with Rails as much as possible in an effort to avoid drift with Rails
standards.

It's possible a future release of Rails will alleviate us from having to
do the following:

- enable `active_model.i18n_customize_full_message` which is being
  addressed in [#50406][]
- enable `active_record.query_log_tags_enabled` which is being addressed
  in [#51342][]

[#50406]: rails/rails#50406
[#51342]: rails/rails#51342
stevepolitodesign pushed a commit to thoughtbot/suspenders that referenced this pull request Apr 5, 2024
Creates generator to configure the development environment. Keeps parity
with Rails as much as possible in an effort to avoid drift with Rails
standards.

It's possible a future release of Rails will alleviate us from having to
do the following:

- enable `active_model.i18n_customize_full_message` which is being
  addressed in [#50406][]
- enable `active_record.query_log_tags_enabled` which is being addressed
  in [#51342][]

[#50406]: rails/rails#50406
[#51342]: rails/rails#51342
stevepolitodesign pushed a commit to thoughtbot/suspenders that referenced this pull request Apr 5, 2024
Creates generator to configure the development environment. Keeps parity
with Rails as much as possible in an effort to avoid drift with Rails
standards.

It's possible a future release of Rails will alleviate us from having to
do the following:

- enable `active_model.i18n_customize_full_message` which is being
  addressed in [#50406][]
- enable `active_record.query_log_tags_enabled` which is being addressed
  in [#51342][]

[#50406]: rails/rails#50406
[#51342]: rails/rails#51342
stevepolitodesign pushed a commit to thoughtbot/suspenders that referenced this pull request Apr 5, 2024
Creates generator to configure the development environment. Keeps parity
with Rails as much as possible in an effort to avoid drift with Rails
standards.

It's possible a future release of Rails will alleviate us from having to
do the following:

- enable `active_model.i18n_customize_full_message` which is being
  addressed in [#50406][]
- enable `active_record.query_log_tags_enabled` which is being addressed
  in [#51342][]

[#50406]: rails/rails#50406
[#51342]: rails/rails#51342
stevepolitodesign pushed a commit to thoughtbot/suspenders that referenced this pull request Apr 5, 2024
Creates generator to configure the development environment. Keeps parity
with Rails as much as possible in an effort to avoid drift with Rails
standards.

It's possible a future release of Rails will alleviate us from having to
do the following:

- enable `active_model.i18n_customize_full_message` which is being
  addressed in [#50406][]
- enable `active_record.query_log_tags_enabled` which is being addressed
  in [#51342][]

[#50406]: rails/rails#50406
[#51342]: rails/rails#51342
stevepolitodesign added a commit to thoughtbot/suspenders that referenced this pull request Apr 5, 2024
Creates generator to configure the development environment. Keeps parity
with Rails as much as possible in an effort to avoid drift with Rails
standards.

It's possible a future release of Rails will alleviate us from having to
do the following:

- enable `active_model.i18n_customize_full_message` which is being
  addressed in [#50406][]
- enable `active_record.query_log_tags_enabled` which is being addressed
  in [#51342][]

[#50406]: rails/rails#50406
[#51342]: rails/rails#51342

Co-authored-by: Steve Polito <stevepolito@hey.com>
stevepolitodesign added a commit to thoughtbot/suspenders that referenced this pull request May 10, 2024
Creates generator to configure the development environment. Keeps parity
with Rails as much as possible in an effort to avoid drift with Rails
standards.

It's possible a future release of Rails will alleviate us from having to
do the following:

- enable `active_model.i18n_customize_full_message` which is being
  addressed in [#50406][]
- enable `active_record.query_log_tags_enabled` which is being addressed
  in [#51342][]

[#50406]: rails/rails#50406
[#51342]: rails/rails#51342

Co-authored-by: Steve Polito <stevepolito@hey.com>
@zzak
Copy link
Member

zzak commented May 20, 2024

How do query logs interact with prepared statements? My understanding was the two were not compatible and all db adapters except MySQL will use prepared statements by default.

@skipkayhil They don't. See: #48631

@MatheusRich
Copy link
Contributor Author

@rafaelfranca this is probably good to go

This can be used to trace troublesome SQL statements back to the application code that generated these statements.
@byroot byroot merged commit c9c3047 into rails:main Aug 11, 2024
1 of 3 checks passed
@MatheusRich MatheusRich deleted the patch-1 branch August 16, 2024 21:20
Earlopain added a commit to Earlopain/rails that referenced this pull request Nov 8, 2024
* rails#51831
* rails#51342
* rails#52887

Probably more, these are the ones I noticed.
Earlopain added a commit to Earlopain/rails that referenced this pull request Nov 15, 2024
* rails#51831
* rails#51342

Probably more, these are the ones I noticed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants