Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add compatibility for .trace_with in graphql-ruby 2.3+ #1446

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

janko
Copy link

@janko janko commented Mar 25, 2024

What does this pull request do?

It adds support for .trace_with that was added in graphql-ruby as a more performant alternative to .tracer. Instead of a single .trace method that gets called on all types of events, with the new API the trace module implements a method for each event they want to trace.

Why is it important?

The .tracer API has been deprecated and will be removed in graphql-ruby 3.0.

Checklist

  • I have signed the Contributor License Agreement.
  • My code follows the style guidelines of this project (See .rubocop.yml)
  • I have rebased my changes on top of the latest main branch
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@estolfo
Copy link
Contributor

estolfo commented Mar 28, 2024

Hi @janko thanks for this contribution! As we would continue to support older versions of graphql, do these changes support .trace_with in addition to .trace or do the changes replace support for .trace?

@janko
Copy link
Author

janko commented Mar 28, 2024

Yes, the old tracing API calls the .trace method, which remained unchanged in this PR. This PR just adds new methods that will be used by the new tracing API.

@estolfo
Copy link
Contributor

estolfo commented Apr 2, 2024

Hey @janko I think some of the GraphQL tests are failing, would you mind taking a look?
For example: https://github.com/elastic/apm-agent-ruby/actions/runs/8418659477/job/23206725271?pr=1446#step:4:692

@janko janko force-pushed the graphql-trace-with branch from 15c0967 to a22fef6 Compare April 2, 2024 09:29
@janko
Copy link
Author

janko commented Apr 2, 2024

I tried reproducing it locally, but unfortunately I cannot bundle install with JRuby 9.2 on my MacBook M1, I'm getting the following error:

NoMethodError: undefined method `request' for nil:NilClass
Did you mean?  require
  conflicting_dependencies at /Users/janko/.rbenv/versions/jruby-9.2.21.0/lib/ruby/stdlib/rubygems/resolver/conflict.rb:47
  conflicting_dependencies at /Users/janko/.rbenv/versions/jruby-9.2.21.0/lib/ruby/stdlib/rubygems/exceptions.rb:61
                initialize at /Users/janko/.rbenv/versions/jruby-9.2.21.0/lib/ruby/stdlib/rubygems/exceptions.rb:55
                 exception at org/jruby/RubyException.java:129
                   resolve at /Users/janko/.rbenv/versions/jruby-9.2.21.0/lib/ruby/stdlib/rubygems/resolver.rb:193
                   resolve at /Users/janko/.rbenv/versions/jruby-9.2.21.0/lib/ruby/stdlib/rubygems/request_set.rb:411
           resolve_current at /Users/janko/.rbenv/versions/jruby-9.2.21.0/lib/ruby/stdlib/rubygems/request_set.rb:423
            finish_resolve at /Users/janko/.rbenv/versions/jruby-9.2.21.0/lib/ruby/stdlib/rubygems.rb:230
         activate_bin_path at /Users/janko/.rbenv/versions/jruby-9.2.21.0/lib/ruby/stdlib/rubygems.rb:287
               synchronize at org/jruby/ext/thread/Mutex.java:164
         activate_bin_path at /Users/janko/.rbenv/versions/jruby-9.2.21.0/lib/ruby/stdlib/rubygems.rb:285
                    <main> at /Users/janko/.rbenv/versions/jruby-9.2.21.0/bin/bundle:25

I tried running gem update --system 3.3.27, which is the latest RubyGems that supports this JRuby version, but still getting the same error.

FWIW, it seems the issue is only with combination of JRuby 9.2 and Rails 6.1. I don't know if it's deterministic (e.g. a JRuby bug with keyword arguments), or just some test-only race condition or something.

@janko janko force-pushed the graphql-trace-with branch from a22fef6 to 799f681 Compare April 2, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants