Skip to content

Commit

Permalink
Fix excessive logging by instrumentation plugin
Browse files Browse the repository at this point in the history
It happens when there're more than one container using the same gateway. This issue is already fixed in the main branch, I'll add the spec there, though
  • Loading branch information
flash-gordon committed Jun 29, 2024
1 parent f037942 commit 1247581
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 0 deletions.
6 changes: 6 additions & 0 deletions lib/rom/plugins/relation/sql/instrumentation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ module Instrumentation
db_notifications = relations.values.map { |r| [r.dataset.db, r.notifications] }.uniq.to_h

db_notifications.each do |db, notifications|
next if db.respond_to?(:rom_instrumentation?)

instrumenter = Instrumenter.new(db.database_type, notifications)
db.extend(instrumenter)
end
Expand All @@ -56,6 +58,8 @@ class Instrumenter < Module

# @api private
def initialize(name, notifications)
super()

@name = name
@notifications = notifications
define_log_connection_yield
Expand All @@ -68,6 +72,8 @@ def define_log_connection_yield
name = self.name
notifications = self.notifications

define_method(:rom_instrumentation?) { true }

define_method(:log_connection_yield) do |*args, &block|
notifications.instrument(:sql, name: name, query: args[0]) do
super(*args, &block)
Expand Down
26 changes: 26 additions & 0 deletions spec/unit/relation/instrument_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,4 +64,30 @@ def instrument(*args, &block)
expect(notifications.logs).
to include([:sql, name: :sqlite, query: "SELECT count(*) AS 'count' FROM `users` LIMIT 1"])
end

context 'two containers with shared gateway' do
let(:conf_alt) { TestConfiguration.new(:sql, conn) }

let(:container_alt) { ROM.container(conf_alt) }

before do
conf_alt.plugin(:sql, relations: :instrumentation)

conf_alt.relation(:users) do
schema(infer: true)
end

container_alt
end

it 'instruments relation materialization but does it once' do
relation.to_a

entries = notifications.logs.count do |log|
log.eql?([:sql, name: :sqlite, query: relation.dataset.sql])
end

expect(entries).to be(1)
end
end
end

0 comments on commit 1247581

Please sign in to comment.