trace_mode(:foo, TraceClass) does not stick for child classes

[jbourassa]

Describe the bug

Overriding a schema's trace class for a particular mode does not stick for child classes.

Extracted from this failing test:

class TraceClassA < GraphQL::Tracing::Trace; end

class ParentSchema < GraphQL::Schema
  trace_mode :a, TraceClassA
end

class ChildSchema < ParentSchema; end

# This fails, we instead get a GraphQL::Tracing::Trace
assert_kind_of(TraceClassA, ChildSchema.new_trace(mode: :a))

Versions

graphql version: 2781f4f (master as of writing)

Additional context

I don't know if it's an actual bug, or I'm calling code I shouldn't call. My use case is to opt-out of all default traces for performance-sensitive scenarios. It works if I redefine the trace class on each schema subclasses. For now, I've gotten around this issue by using the schema's inherited hook and re-calling trace_mode the subclasses.

I tried fixing it with:

diff --git lib/graphql/schema.rb lib/graphql/schema.rb
index 27eb883b7..e153d54b9 100644
--- lib/graphql/schema.rb
+++ lib/graphql/schema.rb
@@ -172,8 +172,12 @@ module GraphQL
               include(GraphQL::Backtrace::Trace)
             end
           else
-            mods = trace_modules_for(mode)
-            Class.new(trace_class_for(:default)) do
+            superclass_base_class = if superclass.respond_to?(:trace_class_for)
+              superclass.trace_class_for(mode)
+            else
+              trace_class_for(:default)
+            end
+            Class.new(superclass_base_class) do
               mods.any? && include(*mods)
             end
           end

but that caused default traces to not be added to the mode-specific trace classes as pointed out by this assertion:

graphql-ruby/spec/graphql/tracing/trace_modes_spec.rb

Line 82 in d3f38f5

assert res.context[:global_trace]

[rmosolgo]

Hi! Thanks for the detailed report. In short, I agree that it should work that way -- I'll take a look soon! Hopefully some more digging around between modules and superclasses will make it work.

[rmosolgo]

Ok, I copied your test and tried a few things to fix it, and yeah, I think we'll need a different approach for your use case.

What about approaching it differently, so you added the slow-ish tracing to a named mode:

trace_with SlowInstrumentation, mode: :full_instrumentation 

Then, when executing, you could use the :default mode to avoid overhead:

context = {
  trace_mode: performance_sensitive? ? :default : :full_instrumentation 
}

That's a little clunky, would supporting a custom default_trace_mode ... make it smoother?

[jbourassa]

That's a little clunky, would supporting a custom default_trace_mode ... make it smoother?

I think that would work: most schemas would have :default, and for those perf sensitive schemas we'd set the default to :minimal.

[rmosolgo]

I added some support for this in #4642.

One tradeoff I made was to handle trace_mode(sym, class) classes specially:

• Subclass schemas make new trace classes based on the given class, not based on the default trace class
• They only receive new trace_with modules if trace_with ... is called with a mode: that matches sym. So, they don't receive any added default trace_with modules.

In effect, this creates isolated inheritance chains, where a manually added trace class (that is, added with trace_mode ... instead of created under the hood by trace_with) becomes isolated from the default trace stack.

But I think that's desirable in this case, right? And I don't think it should cause hiccups in the future -- trace_class isn't in the guides, the method is (now) clearly documented, and it didn't cause any breaking changes in the tests.

[jbourassa]

But I think that's desirable in this case, right?

I think so too – that's exactly what I was trying to achieve.

Thanks for looking into this Robert 🙇.