Feature query annotation

[helpotters]

• annotate queries that pass through Readyset.route
• tagged with /*routed_to_readyset?:true*/

The feature below is made redundant by having query_logs_enabled set to true.

• toggle this feature on using config.query_annotations = true

[helpotters]

I'm sharing the failing code. I'm having a major difficulty with the testing environment setup with enabling QueryLogs. By default, it should at least add /*application:MyApp*/ to every query in test.log.

[ethan-readyset]

Hmm ActiveSupport::ExecutionContext is an undocumented class in Rails, so we probably shouldn't use it here -- it could be changed/removed with minor Rails version updates.

I think we can integrate the way that the ActiveRecord::QueryLogs documentation suggests: api.rubyonrails.org/classes/ActiveSupport/QueryLogs.html

New comment tags can be defined by adding them in a Hash to the tags Array. Tags can have dynamic content by setting a Proc or lambda value in the Hash, and can reference any value stored by Rails in the context object. ActiveSupport::CurrentAttributes can be used to store application values. Tags with nil values are omitted from the query comment.

Basically, we could do something like this to pass info about whether we're routing to ReadySet:

Rails.configuration.active_record.query_log_tags << {
  routed_to_readyset?: ->(context) do
    Current.routing_to_readyset?
  end,
}

Where Current is defined like so:

module Readyset
  class Current < ActiveSupport::CurrentAttributes
    attr_writer :routing_to_readyset

    def routing_to_readyset?
      !!@routing_to_readyset
    end
  end
end

Then we'd just need to update the routing_to_readyset attribute whenever Readyset.route is called:

  def self.route(prevent_writes: true, &block)
    Current.routing_to_readyset = true
    ...
  ensure
    Current.routing_to_readyset = false
  end

[helpotters]

Hmm. I'm still figuring out how to check that the SQL comment was added.

So far, it's been a struggle trying to get it to output the SQL query log with comments enabled. It might be a limitation with Combustion.

[ethan-readyset]

I'd say we should probably switch to integrating via ActiveRecord::QueryLogs before spending too much time debugging the specs. It's possible there's something about using ExecutionContext that's causing the spec issue

[helpotters]

Yesterday it wasn't printing /*application:Combustion*/ in test.log queries, but it is today!

(╯°□°)╯︵ ┻━┻

[ethan-readyset]

Looks great! 🙌 Just a few questions/suggestions

[ethan-readyset]

Looks great! Just a few comments

[helpotters]

For review, I would recommend double-checking this snippet of code:

    initializer 'readyset.query_annotator' do |app|
      setup_query_annotator
    end

    def setup_query_annotator
      config.after_initialize do
        ...
      end
    end

I'm a bit unsure about the usage of config.after_initialize as opposed to alternatives.

[helpotters]

That was a rough rebase, but now a spec isn't passing anymore :((. Gotta figure it out.

[ethan-readyset]

@helpotters Apologies for rough rebase -- I merged a bunch of big changes to main over the past week or two! The failing spec is my fault -- I can spend some time digging into this later today

[ethan-readyset]

@helpotters Fixed in #72

[helpotters]

🎶 This is ready for another go 🎶

[ethan-readyset]

@helpotters Looks great! Ready to merge after a rebase to resolve the merge conflict