Support graphql-ruby's new tracing API (backwards compatible)

[rmosolgo]

This is to try out a new tracing API (rmosolgo/graphql-ruby#4344). It looks like it will work fine.

I used a lot of duplication as a quick way to test my changes, and then, if the new tracing API is adopted, it will be easy to remove the old code. But another option would be to:

-[x] Include Trace in Tracer (since it's a module anyway) and call the capture_* methods from there, to reduce duplication

• Extract the assertions from the integration test and share them between TraceTest and Test, rather than duplicate them all

Let me know what you think, I'd be happy to make those changes (or others).

Along the way, I found that some tests here fail on master, presumably because my refactors to Language::Visitor and Analysis::AST::Visitor (rmosolgo/graphql-ruby#4338) changed the order of these arrays. Is that OK? Instead, I added code to re-order these arrays before comparing them

I also wrote up a little benchmark to confirm the intended impact:

benchmark.rb

require "bundler/inline"

gemfile do
  gem "graphql", github: "rmosolgo/graphql-ruby", branch: "better-trace-api"
  gem "graphql-metrics", path: "./"
  gem "benchmark-ips"
  gem "memory_profiler"
end

class Thing < GraphQL::Schema::Object
  5.times do |i|
    field :"int#{i + 1}", Int, resolver_method: :int
    field :"str#{i + 1}", String, resolver_method: :str
  end

  def str
    "blah"
  end

  def int
    100
  end
end

class Query < GraphQL::Schema::Object
  field :things, [Thing]

  def things
    [:thing1, :thing2, :thing3, :thing4]
  end
end

class TraceSchema < GraphQL::Schema
  query(Query)
  trace_with GraphQL::Metrics::Trace
end

class TracerSchema < GraphQL::Schema
  query(Query)
  tracer(GraphQL::Metrics::Tracer.new)
end

query_str = <<-GRAPHQL
{
  things {
    int1 int2 int3 int4 int5
    str1 str2 str3 str4 str5
  }
}
GRAPHQL

# pp TraceSchema.execute(query_str).to_h
# pp TracerSchema.execute(query_str).to_h
Benchmark.ips do |x|
  x.report("Trace  (NEW)") { TraceSchema.execute(query_str) }
  x.report("Tracer (OLD)") { TracerSchema.execute(query_str) }
  x.compare!
end

puts "----- TRACE (NEW) -----"
MemoryProfiler.report do
  TraceSchema.execute(query_str)
end.pretty_print


puts "----- TRACER (OLD) -----"
MemoryProfiler.report do
  TracerSchema.execute(query_str)
end.pretty_print

And it looks good to me, >10% reduction in both runtime and memory usage. (But this is a reduction in overhead only, so in real queries with application code running, the impact will be less.)

Warming up --------------------------------------
        Trace  (NEW)   105.000  i/100ms
        Tracer (OLD)    95.000  i/100ms
Calculating -------------------------------------
        Trace  (NEW)      1.099k (± 1.8%) i/s -      5.565k in   5.065086s
        Tracer (OLD)    968.282  (± 1.9%) i/s -      4.845k in   5.005410s

Comparison:
        Trace  (NEW):     1099.1 i/s
        Tracer (OLD):      968.3 i/s - 1.14x  slower

----- TRACE (NEW) -----
Total allocated: 59176 bytes (647 objects)
Total retained:  168 bytes (1 objects)

# ... 
----- TRACER (OLD) -----
Total allocated: 67536 bytes (694 objects)
Total retained:  208 bytes (2 objects)

[rmosolgo]

(I signed the CLA, but I don't see an option to re-run the job myself.)

[swalkinshaw]

I'm okay with the test duplication for now but I think it would be nice to re-use the module for both classes.

re: analyzer changes. We have multiple test gemfiles for different versions of graphql-ruby so these order assertions won't be consistent across those 🤔

[rmosolgo]

👍 I updated it to sort those arrays before comparing them, so it should pass on all versions. And I added a new gemfile to run tests on my WIP branch.

[swalkinshaw]

🎉 looks good to me. The method based tracer is a much better interface

[rmosolgo]

Thanks for taking a look. I added a benchmark result above. I'll sleep on it and merge that graphql-ruby PR soon, then update this accordingly 👍

[rmosolgo]

I merged the new API upstream. As far as @skip_tracing is concerned, I think you could theoretically build two different Trace classes, each one composed of different modules, and then pick a module for context[:trace] when running a queries. (GraphQL-Ruby could provide a handy API for this.) But in my opinion, we could put off that improvement since this change already includes a big performance improvement. How does that sound to you?

[swalkinshaw]

I'm good with putting it off 👍 The implementation isn't bad anyway and this gem's tracer is basically internal only the way it's designed. If anyone was overriding these tracer methods, they'd be calling super anyway

[swalkinshaw]

@rmosolgo this is good to go after a rebase right?

[rmosolgo]

yep 👀

[swalkinshaw]

How about we repurpose this as graphql_head now?

[rmosolgo]

👍 done

[rmosolgo]

😅 Alright, tests pass locally!

[toneymathews]

looks good to me.