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

iheanyi · 2022-05-12T18:16:02Z

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 · 2022-05-12T18:37:11Z

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 · 2022-05-12T20:29:24Z

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

tenderlove

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

activerecord/lib/active_record/query_logs_formatter.rb

activerecord/test/cases/query_logs_formatter_test.rb

tenderlove · 2022-05-14T00:14:20Z

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

modulitos · 2022-05-16T18:08:10Z

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 · 2022-05-20T08:19:22Z

activerecord/lib/active_record/query_logs.rb

@@ -130,7 +131,7 @@ def tag_content
            else
              handler
            end
-            "#{key}:#{val}" unless val.nil?
+            "#{key}='#{val.gsub("'", "\\\\'")}'" unless val.nil?


This change introduces a requirement that val must be a string, which is documented above in the comments. I opted for having this requirement because I think the logic is more readable/maintainable, but I'm open to feedback of course. An alternative is to call .to_s on val here, or perhaps adding a check to only call .gsub when val is a string.

modulitos · 2022-05-20T08:22:13Z

@tenderlove I believe this is ready for another review!

Here's a summary of the changes:

Checked in with the community about correcting the formatting behavior, which I summarized in this issue: Proposal: Improve logging format from QueryLogs #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!"
Now that we aren't supporting multiple formats, I removed the ActiveRecord::QueryLogs::Formatter class, which simplifies the implementation 🎉

tenderlove · 2022-06-21T20:23:26Z

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 · 2022-06-22T00:40:36Z

activerecord/lib/active_record/query_logs_formatter.rb

+
+    class QuotingFormatter < Formatter # :nodoc:
+      def format_value(value)
+        "'#{value.to_s.gsub("'", "\\\\'")}'"


Note that I discovered one edge case since the last review of this implementation - if value isn't a string, it'll cause a runtime error unless we call value.to_s here.

An example is when someone passes a value like Process.pid (which we're doing in the tests and railtie below). I'm on the fence about it, but decided to coerce into a string because it's more user-friendly, and consistent with the legacy implementation (see related comment).

We can easily remove this .to_s call if we want to enforce that value must always be a string:

Suggested change

"'#{value.to_s.gsub("'", "\\\\'")}'"

"'#{value.gsub("'", "\\\\'")}'"

I'm fine with either approach :)

modulitos · 2022-06-22T00:40:41Z

activerecord/lib/active_record/query_logs.rb

@@ -130,7 +143,7 @@ def tag_content
            else
              handler
            end
-            "#{key}:#{val}" unless val.nil?


Note how the legacy implementation coerces val into a string. I'm preserving this behavior in QuotingFormatter.format_value (see related comment)

modulitos · 2022-06-22T00:43:46Z

activerecord/lib/active_record/railtie.rb

@@ -368,7 +370,7 @@ class Railtie < Rails::Railtie # :nodoc:
          ActiveRecord.query_transformers << ActiveRecord::QueryLogs
          ActiveRecord::QueryLogs.taggings.merge!(
            application:  Rails.application.class.name.split("::").first,
-            pid:          -> { Process.pid },
+            pid:          -> { Process.pid.to_s },


I'm converting this into a string, since that's the desired type for our query log values. This removes any question about whether it gets coerced into a string later on.

modulitos · 2022-06-22T00:52:08Z

@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 🙌

skipkayhil · 2022-06-22T01:15:24Z

activerecord/lib/active_record/query_logs.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true

 require "active_support/core_ext/module/attribute_accessors_per_thread"
+require_relative "query_logs_formatter"


I think rails only uses require and not require_relative

Agreed - it seems like require is more consistent with the existing code. I changed this to require "active_record/query_logs_formatter".

Note that there are a couple places where require_relative is being used within Rails:

rails/railties/lib/rails/autoloaders.rb

Line 5 in 6d8df0a

require_relative "autoloaders/inflector"

and here:

rails/railties/lib/rails/commands/credentials/credentials_command.rb

Line 15 in 6d8df0a

require_relative "credentials_command/diffing"

but the usages are few and far between.

skipkayhil · 2022-06-22T01:17:27Z

activerecord/lib/active_record/query_logs.rb

    thread_mattr_accessor :cached_comment, instance_accessor: false
+    thread_mattr_accessor :tags_formatter, instance_accessor: false


What's the purpose of making the formatter per thread?

Hmmm good question - I don't think this needs to be per thread. I updated this to use mattr_accessor instead, and was also able to remove the ActiveSupport::ExecutionContext.after_change { ActiveRecord::QueryLogs.update_formatter } hook below. I think the implementation is simpler now.

skipkayhil · 2022-06-22T01:19:37Z

activerecord/lib/active_record/query_logs.rb

@@ -73,7 +74,9 @@ module QueryLogs
    mattr_accessor :tags, instance_accessor: false, default: [ :application ]
    mattr_accessor :prepend_comment, instance_accessor: false, default: false
    mattr_accessor :cache_query_log_tags, instance_accessor: false, default: false
+    mattr_accessor :tags_format, instance_accessor: false, default: :default


I'm not sure it makes sense to store both the format and the formatter on QueryLogs, I would think only the formatter is necessary

Great idea - I was able to remove tags_format and kept everything working correctly 😃

guides/source/configuring.md

modulitos

@skipkayhil Thanks for the feedback! It led me to dig a little deeper and make some neat improvements 🙌

If there's anything else I can address, please lmk!

modulitos · 2022-06-22T06:20:20Z

activerecord/lib/active_record/query_logs.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true

 require "active_support/core_ext/module/attribute_accessors_per_thread"
+require_relative "query_logs_formatter"


Agreed - it seems like require is more consistent with the existing code. I changed this to require "active_record/query_logs_formatter".

Note that there are a couple places where require_relative is being used within Rails:

rails/railties/lib/rails/autoloaders.rb

Line 5 in 6d8df0a

require_relative "autoloaders/inflector"

and here:

rails/railties/lib/rails/commands/credentials/credentials_command.rb

Line 15 in 6d8df0a

require_relative "credentials_command/diffing"

but the usages are few and far between.

modulitos · 2022-06-22T06:22:09Z

activerecord/lib/active_record/query_logs.rb

@@ -73,7 +74,9 @@ module QueryLogs
    mattr_accessor :tags, instance_accessor: false, default: [ :application ]
    mattr_accessor :prepend_comment, instance_accessor: false, default: false
    mattr_accessor :cache_query_log_tags, instance_accessor: false, default: false
+    mattr_accessor :tags_format, instance_accessor: false, default: :default


Great idea - I was able to remove tags_format and kept everything working correctly 😃

modulitos · 2022-06-22T06:24:22Z

activerecord/lib/active_record/query_logs.rb

    thread_mattr_accessor :cached_comment, instance_accessor: false
+    thread_mattr_accessor :tags_formatter, instance_accessor: false


Hmmm good question - I don't think this needs to be per thread. I updated this to use mattr_accessor instead, and was also able to remove the ActiveSupport::ExecutionContext.after_change { ActiveRecord::QueryLogs.update_formatter } hook below. I think the implementation is simpler now.

modulitos · 2022-09-02T01:34:05Z

@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 · 2022-09-02T01:48:45Z

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

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
All code changes are covered by unit tests
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 🎉
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 · 2022-09-26T15:04:44Z

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 · 2022-09-27T08:02:57Z

@bdewater Thank you for the tips 😀

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

Co-authored-by: Iheanyi Ekechukwu <iekechukwu@gmail.com>

activerecord/lib/active_record/query_logs_formatter.rb

…rmatter This commit addresses the `warning: mismatched indentations` below. ```ruby $ bundle exec ruby -w -Itest test/cases/query_logs_formatter_test.rb -n test_factory_invalid_formatter test/cases/query_logs_formatter_test.rb:10: warning: mismatched indentations at 'end' with 'def' at 6 Using sqlite3 Run options: -n test_factory_invalid_formatter --seed 48904 . Finished in 0.008743s, 114.3776 runs/s, 114.3776 assertions/s. 1 runs, 1 assertions, 0 failures, 0 errors, 0 skips $ ``` Follow-up rails#45081

rails-bot bot added activerecord docs railties labels May 12, 2022

iheanyi closed this May 12, 2022

iheanyi changed the title ~~Add a custom tag separator option for query logs.~~ Add ability to set the tags_format to QueryLog tags May 12, 2022

iheanyi reopened this May 12, 2022

iheanyi changed the title ~~Add ability to set the tags_format to QueryLog tags~~ Add ability to set the tags_format for QueryLogs May 12, 2022

tenderlove requested changes May 14, 2022

View reviewed changes

modulitos mentioned this pull request May 16, 2022

Address PR feedback on ActiveRecord:QueryLogs:Formatter iheanyi/rails#1

Merged

modulitos mentioned this pull request May 20, 2022

Proposal: Improve logging format from QueryLogs #45139

Closed

modulitos force-pushed the iheanyi/custom-query-log-tags-separators branch from 8a425fc to 3694f72 Compare May 20, 2022 08:06

modulitos reviewed May 20, 2022

View reviewed changes

p8 linked an issue May 31, 2022 that may be closed by this pull request

Proposal: Improve logging format from QueryLogs #45139

Closed

modulitos force-pushed the iheanyi/custom-query-log-tags-separators branch from 8c7c270 to b534f6f Compare June 16, 2022 05:52

iheanyi requested a review from tenderlove June 16, 2022 23:33

modulitos force-pushed the iheanyi/custom-query-log-tags-separators branch 2 times, most recently from f0a5bcd to a57fcbc Compare June 22, 2022 00:19

modulitos reviewed Jun 22, 2022

View reviewed changes

skipkayhil reviewed Jun 22, 2022

View reviewed changes

modulitos reviewed Jun 22, 2022

View reviewed changes

modulitos force-pushed the iheanyi/custom-query-log-tags-separators branch 2 times, most recently from cce20fa to 024523f Compare September 27, 2022 07:17

modulitos force-pushed the iheanyi/custom-query-log-tags-separators branch 3 times, most recently from 5c4237b to c0b8999 Compare September 27, 2022 20:01

Add sqlcommenter formatting support on query logs

6df894f

Co-authored-by: Iheanyi Ekechukwu <iekechukwu@gmail.com>

modulitos force-pushed the iheanyi/custom-query-log-tags-separators branch from c0b8999 to 6df894f Compare September 27, 2022 21:39

tenderlove approved these changes Sep 28, 2022

View reviewed changes

activerecord/lib/active_record/query_logs_formatter.rb Outdated Show resolved Hide resolved

tenderlove merged commit 2fb72f5 into rails:main Sep 28, 2022

iheanyi deleted the iheanyi/custom-query-log-tags-separators branch September 28, 2022 02:19

modulitos mentioned this pull request Oct 3, 2022

Use SQLCommenter as the default formatter for Query Log tags #46179

Closed

yahonda mentioned this pull request Oct 4, 2022

Address warning: mismatched indentations at test_factory_invalid_formatter #46188

Merged

9 tasks

modulitos mentioned this pull request Oct 23, 2022

Update Ruby docs with latest Rails support open-telemetry/opentelemetry-sqlcommenter#35

Open

bdewater mentioned this pull request Oct 24, 2022

Fix SQLCommenter value serialization #46326

Merged

5 tasks

modulitos mentioned this pull request Oct 30, 2022

Ruby on Rails documentation updates google/sqlcommenter#169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

iheanyi commented May 12, 2022 •

edited

iheanyi commented May 12, 2022

iheanyi commented May 12, 2022

tenderlove left a comment

tenderlove commented May 14, 2022

modulitos commented May 16, 2022

modulitos May 20, 2022

modulitos commented May 20, 2022 •

edited

tenderlove commented Jun 21, 2022

modulitos Jun 22, 2022 •

edited

modulitos Jun 22, 2022

modulitos Jun 22, 2022 •

edited

modulitos commented Jun 22, 2022

skipkayhil Jun 22, 2022

modulitos Jun 22, 2022

skipkayhil Jun 22, 2022

modulitos Jun 22, 2022

skipkayhil Jun 22, 2022

modulitos Jun 22, 2022

modulitos left a comment

modulitos Jun 22, 2022

modulitos Jun 22, 2022

modulitos Jun 22, 2022

modulitos commented Sep 2, 2022

modulitos commented Sep 2, 2022 •

edited

bdewater commented Sep 26, 2022

modulitos commented Sep 27, 2022

	"'#{value.to_s.gsub("'", "\\\\'")}'"
	"'#{value.gsub("'", "\\\\'")}'"

		thread_mattr_accessor :cached_comment, instance_accessor: false
		thread_mattr_accessor :tags_formatter, instance_accessor: false

Add ability to set the tags_format for QueryLogs #45081

Add ability to set the tags_format for QueryLogs #45081

Conversation

iheanyi commented May 12, 2022 • edited

Summary

Other Information

iheanyi commented May 12, 2022

iheanyi commented May 12, 2022

tenderlove left a comment

Choose a reason for hiding this comment

tenderlove commented May 14, 2022

modulitos commented May 16, 2022

Choose a reason for hiding this comment

modulitos commented May 20, 2022 • edited

tenderlove commented Jun 21, 2022

modulitos Jun 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modulitos Jun 22, 2022 • edited

Choose a reason for hiding this comment

modulitos commented Jun 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modulitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modulitos commented Sep 2, 2022

modulitos commented Sep 2, 2022 • edited

bdewater commented Sep 26, 2022

modulitos commented Sep 27, 2022

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

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

iheanyi commented May 12, 2022 •

edited

modulitos commented May 20, 2022 •

edited

modulitos Jun 22, 2022 •

edited

modulitos Jun 22, 2022 •

edited

modulitos commented Sep 2, 2022 •

edited