-
Notifications
You must be signed in to change notification settings - Fork 235
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
feat: instrument graphql #527
feat: instrument graphql #527
Conversation
instrumentation/graphql/lib/opentelemetry/instrumentation/graphql/tracers/graphql_tracer.rb
Outdated
Show resolved
Hide resolved
instrumentation/graphql/lib/opentelemetry/instrumentation/graphql/tracers/graphql_tracer.rb
Outdated
Show resolved
Hide resolved
instrumentation/graphql/lib/opentelemetry/instrumentation/graphql/tracers/graphql_tracer.rb
Outdated
Show resolved
Hide resolved
3acc491
to
c3dfe2c
Compare
SomeOtherGraphQLAppSchema.remove_instance_variable(:@graphql_definition) if SomeOtherGraphQLAppSchema.instance_variable_defined?(:@graphql_definition) | ||
SomeGraphQLAppSchema.remove_instance_variable(:@graphql_definition) if SomeGraphQLAppSchema.instance_variable_defined?(:@graphql_definition) |
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.
There was some fairly tricky state leaking between tests, particularly for 1.9.x
version of GraphQL. This is required to prevent duplicate tracers executing, as well as running the tests that would not otherwise have a tracer added. This came up when adding the config options for schemas
.
instrumentation/graphql/lib/opentelemetry/instrumentation/graphql/instrumentation.rb
Outdated
Show resolved
Hide resolved
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, i had some minor feedback around config options/defaults and whether there was any interest in moving those "optionally on" fields to be events instead, but non blocking imo
instrumentation/graphql/lib/opentelemetry/instrumentation/graphql/tracers/graphql_tracer.rb
Show resolved
Hide resolved
instrumentation/graphql/README.md
Outdated
enable_platform_field: true, | ||
# enable_platform_authorized maps to the authorized and authorized_lazy keys | ||
enable_platform_authorized: true, | ||
# enable_platform_resolve_type maps to the resolve_type and resolve_type_lazy keys | ||
enable_platform_resolve_type: true |
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.
very small nit but maybe we should leave these commented out so we avoid ppl copy/pasting this snippet in and enabling stuff that, for the most part, we want disabled
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.
I see what you mean, I wanted to demonstrate how they could turn it on but I guess I can just as easily do that by provided a false value instead. I'll give it an update.
fix: update options description comment Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
f62e194
to
d8fab42
Compare
Continuation of the work done here #410