Add ability to set the `tags_format` for `QueryLogs`

[iheanyi]

Summary

This pull request adds a new option called tags_format to ActiveRecord::QueryLogs. In some cases, we may not want to use a : as the separator for the key-value pairs, like in the case of sqlcommenter, which uses = as separator and single quotes around values.. Therefore, we can set a new tags_format option that lets users specify if they want to use the :default functionality or :sqlcommenter.

Other Information

This was literally copied from this pull request for Marginalia, credits to @modulitos for the original implementation.

[iheanyi]

I just realized that this requires it to be of a format key="tag" to be spec-compliant. Will close for now and re-open when it's ready for review.

[iheanyi]

All right, revamped and took a different direction for this!

[tenderlove]

I don't know why all of my comments are doubled up and I'm really sorry about it 😫

[modulitos]

One "big picture" question though: should all values be quoted? I see that you preserved the existing behavior, but is the existing behavior correct?

Ooh thanks for bringing this up. I can't think of a reason why the existing behavior would be preferable over the new format (other than perhaps breaking a parser relying on the old format).

I'll ask around and see what other folks in the community think about changing this. If the old behavior is considered incorrect, then maybe we can have everyone use the new format as part of a patch release instead of a breaking change.

[modulitos]

@tenderlove I believe this is ready for another review!

Here's a summary of the changes:

1. Checked in with the community about correcting the formatting behavior, which I summarized in this issue: https://github.com/rails/rails/issues/45139 I feel confident that moving to sqlcommenter format and dropping support for the existing format is the right way to go. It also feels like a good time to correct this because it's relatively new functionality. As one fellow Rubyist mentioned: "Easiest to change before too many folks develop a dependency on the format!"
2. Now that we aren't supporting multiple formats, I removed the ActiveRecord::QueryLogs::Formatter class, which simplifies the implementation 🎉

[tenderlove]

It looks good, but as I said here, we really do need to support both formats. Can you bring back the old one in addition to this one please?

Thanks, and sorry for taking so long to review this. 🙇🏻‍♀️

[modulitos]

@tenderlove

It looks good, but as I said here, we really do need to support both formats. Can you bring back the old one in addition to this one please?

Your reasoning makes sense - I re-added the ability to select between the two formats. If there's anything else, lmk!

Thanks, and sorry for taking so long to review this. 🙇🏻‍♀️

Better late than never! I appreciate you taking the time 🙌

[modulitos]

@tenderlove @skipkayhil I believe all of the feedback has been addressed and that this PR is ready to merge/review.

Please lmk if there's anything I can do to help move this along! Happy to smoke test, etc. I really appreciate all the feedback I've received so far.

Really looking forward to adding this Open Telemetry SQLCommenter feature to Rails 😀

[modulitos]

Here's a summary of the context, as this PR has gotten a bit long:

1. This PR doesn't cause any behavioral changes by default: the new SQLCommenter format is opt-in by configuring the ActiveRecord::Railtie like so: config.active_record.query_log_tags_format = :sqlcommenter
2. All code changes are covered by unit tests
3. My company has been using this feature (which I implemented on a fork of Marginalia) since February, and it regularly helps us associate long-running database queries to the source code :tada:
4. The :sqlcommenter formatting changes the SQL comments from this:

"select id from posts /*application:Joe's app,controller:my_controller*/"

to this:

"select id from posts /*application='Joe\\'s app',controller='my_controller*/"

[bdewater]

Thank you! I was looking for this myself :)

I believe the only things missing are squashing the commits and a CHANGELOG entry for Active Record, see https://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#updating-the-documentation

[modulitos]

@bdewater Thank you for the tips :grinning:

Commits are squashed and the Active Record CHANGELOG has been updated. Looking forward to any next steps!