Customizable key-value separator and quoting to be machine-readable #130

modulitos · 2022-02-06T04:46:54Z

This pull request surfaces a new optional configuration to format the Marginalia comments in a machine-readable manner. See the changes to the README in this PR for a summary of the new feature.

Note that this PR is backwards-compatible with existing code. The new setting is optional, and if unset, the default values will retain all existing behavior. Unit tests have also be added to ensure there are no regressions.

Further notes:

The sqlcommenter project references this fork of Marginalia
- Cloning and installing this fork of Marginalia is one of the steps in the sqlcommenter Rails docs. Merging this pull request will simplify the instructions by removing that extra step.
- The formatting introduced in this pull request enables the Marginalia comments to be parsed more easily. For example, Google CloudSQL uses this formatting to add tags and metadata to queries: https://cloud.google.com/sql/docs/postgres/using-query-insights#adding-tags-to-sql-queries
Here is a previous pull request that attempted to add this same feature to Marginalia: https://github.com/basecamp/marginalia/pull/89/commits
- That pull request had favorable reviews, but seemed to fizzle out due to merge conflicts.
- The only difference is that it resolves some merge conflicts and adds some testing.
My company has been running this fork of Marginalia in production since March 3rd, and we haven't had any issues.
I'm more than happy to give anyone a walkthrough of the changes, or buy a coffee, or make a donation for your time :)

TODO:

verify that the unit tests are working, following these steps: https://github.com/basecamp/marginalia#contributing
conduct an end-to-end test to verify this feature works correctly with the sqlcommenter_rails gem
update this pull request's description to describe this feature

Note that some changes were made to the API of this feature over the course of this PR. Here is the original description, for posterity:

This pull request provides 2 new optional configs which are backwards compatible with all existing code by default.
The 2 optional configs will adjust the formatting of Marginalia's SQL comments:

key_value_separator: inject the key-value separator, which is a colon : by default

quote_values: configure whether the values will be wrapped in a single quoted string. By default, this is false, which will not wrap the values at all.

For example, if a client uses the following configuration:
Marginalia::Comment.key_value_separator = '='
Marginalia::Comment.quote_values = :single
it will adjust the formatting 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*/"

Note that this PR is backwards-compatible with existing code. If neither quote_values nor key_value_separator are set, their default values will retain all existing behavior. Unit tests have also be added to ensure there are no regressions.

modulitos · 2022-02-07T08:01:05Z

@sj26 I would love to get a review on this PR when you have a chance. Or let me know if there's anything I can do to help get this reviewed.

modulitos · 2022-02-28T08:46:24Z

Kindly pinging the marginalia authors (@arthurnn @jeremy) - is there anything I can do to help get this PR reviewed?

Also pinging @balachandr and @odeke-em, as you both had comments in the original PR: #89

I did my best to write up a concise description of the new behavior, and demonstrate that it's all backwards compatible.

jeremy

Would approach this as configuring a formatter rather than as configuring the low-level details of key/value output.

jeremy · 2022-04-25T22:50:54Z

lib/marginalia.rb

+  ensure
+    prev.each { |(key, value)| Marginalia::Comment.send(:"#{key}=", value) }
+  end
+


Appears to be a test helper. Would move this to a utility method on the test case rather than introduce new toplevel API.

Thanks for catching this 💯

I ended up removing this test helper, since it introduces indirection that doesn't seem worthwhile.

jeremy · 2022-04-25T22:55:12Z

lib/marginalia/comment.rb

    def self.construct_comment
      ret = String.new
      self.components.each do |c|
        component_value = self.send(c)
        if component_value.present?
-          ret << "#{c}:#{component_value},"
+          ret << "#{c}#{key_value_separator}"\
+                 "#{quote_value(component_value)},"


Suggest introducing a configurable formatter block rather than separate low-level config points, including :default and :sqlcommenter shorthands for "standard" formatters.

Done 👍

I added support for configuring "standard" formatters like :sqlcommenter directly via Marginalia::Comment.update_formatter!(:sqlcommenter), and I removed configuration for key_value_separator and quote_value directly on the Comment module since it seemed like YAGNI.

I agree that those low-level config points should be managed together, so I introduced a FormatterFactory that handles the composition of these configs instead. I think this results in a better separation of concerns between the formatting and commenting logic as well. Open to feedback, of course :)

modulitos

Thanks @jeremy - I appreciate the great feedback!

Your suggestions led me to think deeper on the formatting logic, resulting in improvements to the API and code organization. Looking forward to your thoughts on the latest changes.

modulitos · 2022-04-27T04:58:24Z

lib/marginalia.rb

+  ensure
+    prev.each { |(key, value)| Marginalia::Comment.send(:"#{key}=", value) }
+  end
+


Thanks for catching this 💯

I ended up removing this test helper, since it introduces indirection that doesn't seem worthwhile.

modulitos · 2022-04-29T06:29:53Z

lib/marginalia/comment.rb

    def self.construct_comment
      ret = String.new
      self.components.each do |c|
        component_value = self.send(c)
        if component_value.present?
-          ret << "#{c}:#{component_value},"
+          ret << "#{c}#{key_value_separator}"\
+                 "#{quote_value(component_value)},"


Done 👍

I added support for configuring "standard" formatters like :sqlcommenter directly via Marginalia::Comment.update_formatter!(:sqlcommenter), and I removed configuration for key_value_separator and quote_value directly on the Comment module since it seemed like YAGNI.

I agree that those low-level config points should be managed together, so I introduced a FormatterFactory that handles the composition of these configs instead. I think this results in a better separation of concerns between the formatting and commenting logic as well. Open to feedback, of course :)

modulitos · 2022-04-29T06:40:42Z

Rakefile

  end

  desc "test sqlite3 driver"
  task :sqlite do
-    sh "DRIVER=sqlite3 bundle exec ruby -Ilib -Itest test/*_test.rb"
+    sh "DRIVER=sqlite3 bundle exec ruby -Ilib -Itest test/query_comments_test.rb"


I changed these globs from test/*_test.rb to test/query_comments_test.rb since that's the only file that needs to be matched for the database tests. It also allows us to run the formatting tests separately.

Customizable key-value separator and quoting

743b65b

modulitos force-pushed the master branch from 49b4210 to 743b65b Compare February 7, 2022 06:25

modulitos mentioned this pull request Feb 7, 2022

Customizable key-value separator and quoting #89

Closed

This was referenced Feb 28, 2022

docs(sqlcommenter_rails/README): update marginalia fork to modulitos google/sqlcommenter#96

Closed

docs(sqlcommenter_rails/README): update marginalia fork to modulitos google/sqlcommenter#99

Merged

modulitos changed the title ~~Customizable key-value separator and quoting~~ Customizable key-value separator and quoting to be machine-readable Mar 10, 2022

docs(README): update typo from ',' to ':'

35d5104

jeremy requested changes Apr 25, 2022

View reviewed changes

modulitos commented Apr 29, 2022

View reviewed changes

refactor low-level configs into :default and :sqlcommenter shorthands

0c55ab8

modulitos force-pushed the master branch from 6e666ff to 0c55ab8 Compare April 29, 2022 07:05

iheanyi mentioned this pull request May 12, 2022

Add ability to set the tags_format for QueryLogs rails/rails#45081

Merged

update readme docs

b4d7863

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customizable key-value separator and quoting to be machine-readable #130

Customizable key-value separator and quoting to be machine-readable #130

modulitos commented Feb 6, 2022 •

edited

Loading

modulitos commented Feb 7, 2022 •

edited

Loading

modulitos commented Feb 28, 2022

jeremy left a comment

jeremy Apr 25, 2022

modulitos Apr 27, 2022

jeremy Apr 25, 2022

modulitos Apr 29, 2022

modulitos left a comment

modulitos Apr 27, 2022

modulitos Apr 29, 2022

modulitos Apr 29, 2022

Customizable key-value separator and quoting to be machine-readable #130

Are you sure you want to change the base?

Customizable key-value separator and quoting to be machine-readable #130

Conversation

modulitos commented Feb 6, 2022 • edited Loading

modulitos commented Feb 7, 2022 • edited Loading

modulitos commented Feb 28, 2022

jeremy left a comment

Choose a reason for hiding this comment

jeremy Apr 25, 2022

Choose a reason for hiding this comment

modulitos Apr 27, 2022

Choose a reason for hiding this comment

jeremy Apr 25, 2022

Choose a reason for hiding this comment

modulitos Apr 29, 2022

Choose a reason for hiding this comment

modulitos left a comment

Choose a reason for hiding this comment

modulitos Apr 27, 2022

Choose a reason for hiding this comment

modulitos Apr 29, 2022

Choose a reason for hiding this comment

modulitos Apr 29, 2022

Choose a reason for hiding this comment

modulitos commented Feb 6, 2022 •

edited

Loading

modulitos commented Feb 7, 2022 •

edited

Loading