From 1247581ca640eea89af96bf740c164ca38a52d15 Mon Sep 17 00:00:00 2001 From: Nikita Shilnikov Date: Sat, 29 Jun 2024 15:47:24 +0200 Subject: [PATCH] Fix excessive logging by instrumentation plugin 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 --- .../plugins/relation/sql/instrumentation.rb | 6 +++++ spec/unit/relation/instrument_spec.rb | 26 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/lib/rom/plugins/relation/sql/instrumentation.rb b/lib/rom/plugins/relation/sql/instrumentation.rb index 315b3c4b5..7a58ebedc 100644 --- a/lib/rom/plugins/relation/sql/instrumentation.rb +++ b/lib/rom/plugins/relation/sql/instrumentation.rb @@ -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 @@ -56,6 +58,8 @@ class Instrumenter < Module # @api private def initialize(name, notifications) + super() + @name = name @notifications = notifications define_log_connection_yield @@ -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) diff --git a/spec/unit/relation/instrument_spec.rb b/spec/unit/relation/instrument_spec.rb index 5fbc042ef..344e603ab 100644 --- a/spec/unit/relation/instrument_spec.rb +++ b/spec/unit/relation/instrument_spec.rb @@ -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