-
Notifications
You must be signed in to change notification settings - Fork 224
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
Conversation
1251a67 to
ce02bb9
Compare
Signed-off-by: Yannick Utard <[email protected]>
ce02bb9 to
2e16b4a
Compare
|
Cc @rmosolgo |
rmosolgo
left a comment
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?
|
|
👋 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 |
I caught this by using the
@streamdirective with graphql-ruby and got a NPE onquery.selected_operation.operation_type.Since graphql-ruby 2.5.6,
GraphQL::Query::Partialwas added for running sub-trees of valid queries. Thequeryargument ofexecute_querymight be aGraphQL::Query::Partialin 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