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

GraphQL tracing with Rails 7.0 #3358

Open
liaden opened this issue Jan 2, 2024 · 3 comments · Fixed by #3388
Open

GraphQL tracing with Rails 7.0 #3358

liaden opened this issue Jan 2, 2024 · 3 comments · Fixed by #3388
Assignees
Labels
bug Involves a bug community Was opened by a community member

Comments

@liaden
Copy link

liaden commented Jan 2, 2024

Current behaviour
Rails commands fail due to the error uninitialized constant MyAppSchema

Expected behaviour
Application starts and monitors just like it did on Rails 6.1.

Steps to reproduce

  • Take working 6.1 app with graphql instrumented and attempt to upgrade app

Environment

  • ddtrace version: 1.18.0
  • Configuration block (Datadog.configure ...): c.tracing.instrument :graphql, schemas: [MyAppSchema]
  • Ruby version: 3.1.3
  • Operating system:
  • Relevant library versions:
@liaden liaden added bug Involves a bug community Was opened by a community member labels Jan 2, 2024
@TonyCTHsu
Copy link
Contributor

TonyCTHsu commented Jan 3, 2024

👋 @liaden , I noticed that Rails 7 change to Zeitwerk autoloading, could this be the reason of this error?

The error looks like could be solved by requiring your schema file, have you tried that?

@liaden
Copy link
Author

liaden commented Jan 3, 2024

@TonyCTHsu Yeah, however, we'd have to go through and load all of the constants underneath app/graphql which for our small app is only 15 files, but for our larger monolith is 667 ruby files.

This seems similar to #2709 and our configuration would align with Autoloading and Reloading Use Case 3 where we should specify the class as a string that uses constantize to resolve it to the class later?

@TonyCTHsu TonyCTHsu self-assigned this Jan 11, 2024
@TonyCTHsu
Copy link
Contributor

👋 @liaden , Thanks for bringing this up.

I have looked into the exception_controller option from #2709 . This was solved because I found out that the option is useless. It is deprecated and will be removed. Hence, I have not addressed the zeitwerk autoloading problem yet.

From Autoloading and Reloading Use Case 3, passing a string instead of a class seems reasonable to me. However, it failed with NameError from Object.get_const(schema_string), because the specified schema class has not yet been loaded.

I agree that our configuration should work smoother with zeitwerk, but it is going to require a lot of fundamental changes.

Currently, only the specified schemas are being instrumented, however, I think it would be better to instrument all schemas when no specification. This could help circumvent the trouble for zeitwerk autoloading for some cases (I supposed most of our users are instrumenting all the schemas). What do yo think?

For now, the workaround I could provide would be adding this configuration in your Rails app (assuming your schema is sitting under such path).

config.autoload_once_paths << "#{root}/app/graphql"

@TonyCTHsu TonyCTHsu linked a pull request Jan 30, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug community Was opened by a community member
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants