-
Notifications
You must be signed in to change notification settings - Fork 209
fix: support GraphQL::Query::Partial in graphql instrumentation #1591
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
base: main
Are you sure you want to change the base?
Conversation
1251a67
to
ce02bb9
Compare
Signed-off-by: Yannick Utard <[email protected]>
ce02bb9
to
2e16b4a
Compare
Cc @rmosolgo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just needs some support for versions where Query::Partial
isn't defined.
instrumentation/graphql/lib/opentelemetry/instrumentation/graphql/tracers/graphql_trace.rb
Outdated
Show resolved
Hide resolved
Signed-off-by: Yannick Utard <[email protected]>
43bf42f
to
d78a028
Compare
Part of our expectations for accepting pull requests is adding automated tests that prevent future regressions. Please add some test coverage for these changes. |
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
Hi @utay! Could you add some test coverage for this change like @arielvalentin mentioned above?
|
I caught this by using the
@stream
directive with graphql-ruby and got a NPE onquery.selected_operation.operation_type
.Since graphql-ruby 2.5.6,
GraphQL::Query::Partial
was added for running sub-trees of valid queries. Thequery
argument ofexecute_query
might be aGraphQL::Query::Partial
in which case we don't have direct access to the operation type and the query string; I thought in this case it would be nice to see the query path