Fix PrometheusTrace with multiple tracers

[simpl1g]

Currently PrometheusTrace does not respect other tracers

Sample code

# frozen_string_literal: true

require 'bundler/inline'

gemfile do
  gem 'graphql', '2.2.13'
  gem 'prometheus_exporter'
end

require 'prometheus_exporter/client'

class MySchema < GraphQL::Schema
  module TracerOne
    def execute_query(query:)
      puts 'TracerOne#execute_query.before'
      super
      puts 'TracerOne#execute_query.after'
    end
  end

  module TracerTwo
    def execute_query(query:)
      puts 'TracerTwo#execute_query.before'
      super
      puts 'TracerTwo#execute_query.after'
    end
  end

  class Query < GraphQL::Schema::Object
    field :f, Int
    def f
      puts 'f called'

      1
    end
  end
  query(Query)

  trace_with TracerOne
  trace_with TracerTwo
  trace_with GraphQL::Tracing::PrometheusTrace
end

pp MySchema.execute('{ f }').to_h

Before changes

f called
{"data"=>{"f"=>1}}

After changes

TracerTwo#execute_query.before
TracerOne#execute_query.before
f called
TracerOne#execute_query.after
TracerTwo#execute_query.after
{"data"=>{"f"=>1}}

Current implementation of tracers includes all modules into single anonymous class, which can lead to unexpected situations, where one gem can overwrite private helper methods of other gem if they accidentally named the same way. Do you have any thoughts how to avoid this situations?

[rmosolgo]

Thanks for this fix! I also removed the unused &block -- it was left from porting the old code, too.

overwrite private helper methods

Unfortunately not -- this is the downside of the new module-based tracing. But I think it was really necessary from a performance perspective. Because tracing is such a hot path, overhead from .each and from building data Hashes really adds up.

Maybe the best thing is to add a note to the documentation that, if you write a tracing module, prefix your methods to avoid conflicts later.

Alternatively, you could implement the tracer so that actual work is delegated to some other object, for example:

module MyTracer 
  def initialize(**_rest)
    @my_tracer_trace_obj = MyTraceObject.new 
  end 
  
  def execute_field(**data) # TODO use kwargs to avoid Hash allocation 
    # Call a method on the private object to avoid conflicts:
    @my_tracer_trace_obj.execute_field(**data)
    super 
  end 
end 

[simpl1g]

Thanks for suggestions

BTW, because Ruby 2.7 is minimal version ... can be used to pass arguments as is

Something like this, but I haven't tested if it will allocate something

  def execute_field(...)
    # Call a method on the private object to avoid conflicts:
    @my_tracer_trace_obj.execute_field(...)
    super 
  end 

[codebender]

@simpl1g Thanks a ton for fixing this!

[simpl1g]

FYI Actually ... is allocating more objects than **args and is slower by 40%, I'm testing in Ruby 3.3

q = 'query { test }'
report = MemoryProfiler.report do
  Test.new.execute_field(query: q)
end

  def execute_field(...)
    instrument_execution { super }
  end
# Total allocated: 440 bytes (5 objects)
# Total retained:  0 bytes (0 objects)

  def execute_field(**)
    instrument_execution { super }
  end
# Total allocated: 200 bytes (2 objects)
# Total retained:  0 bytes (0 objects)

  def execute_field(**args)
    instrument_execution { super }
  end
# Total allocated: 200 bytes (2 objects)
# Total retained:  0 bytes (0 objects)