-
Notifications
You must be signed in to change notification settings - Fork 381
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: Report multiple query errors #4177
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
{ | ||
|
||
"DEFAULT": [ | ||
"tests/test_graphql.py" | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -880,7 +880,7 @@ The `instrument :graphql` method accepts the following parameters. Additional op | |
| ------------------------ | -------------------------- | -------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------- | | ||
| `enabled` | `DD_TRACE_GRAPHQL_ENABLED` | `Bool` | Whether the integration should create spans. | `true` | | ||
| `schemas` | | `Array` | Array of `GraphQL::Schema` objects (that support class-based schema only) to trace. If you do not provide any, then tracing will applied to all the schemas. | `[]` | | ||
| `with_unified_tracer` | | `Bool` | (Recommended) Enable to instrument with `UnifiedTrace` tracer for `graphql` >= v2.2, **enabling support for Endpoints list** in the Service Catalog. `with_deprecated_tracer` has priority over this. Default is `false`, using `GraphQL::Tracing::DataDogTrace` instead | `false` | | ||
| `with_unified_tracer` | `DD_TRACE_GRAPHQL_WITH_UNIFIED_TRACER` | `Bool` | (Recommended) Enable to instrument with `UnifiedTrace` tracer for `graphql` >= v2.2, **enabling support for Endpoints list** in the Service Catalog. `with_deprecated_tracer` has priority over this. Default is `false`, using `GraphQL::Tracing::DataDogTrace` instead | `false` | | ||
| `with_deprecated_tracer` | | `Bool` | Enable to instrument with deprecated `GraphQL::Tracing::DataDogTracing`. This has priority over `with_unified_tracer`. Default is `false`, using `GraphQL::Tracing::DataDogTrace` instead | `false` | | ||
Comment on lines
+883
to
884
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: I'm assuming we haven't flipped the default for backwards-compatibility. But maybe we could be a bit clearer in here that that's the reason? E.g. something like "Due to backwards compatibility, this is not the default, but we strongly suggest enabling this if possible" or something like that? (Or we could say, "this will be the default for dd-trace-rb 3.x"...) |
||
| `service_name` | | `String` | Service name used for graphql instrumentation | `'ruby-graphql'` | | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,14 +47,26 @@ def execute_multiplex(*args, multiplex:, **kwargs) | |
end | ||
|
||
def execute_query(*args, query:, **kwargs) | ||
trace(proc { super }, 'execute', query.selected_operation_name, query: query) do |span| | ||
span.set_tag('graphql.source', query.query_string) | ||
span.set_tag('graphql.operation.type', query.selected_operation.operation_type) | ||
span.set_tag('graphql.operation.name', query.selected_operation_name) if query.selected_operation_name | ||
query.variables.instance_variable_get(:@storage).each do |key, value| | ||
span.set_tag("graphql.variables.#{key}", value) | ||
end | ||
end | ||
trace( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: Can we extract those callable lambda and procs? I assume they are static and they looked weird to me that we are creating new ones in the memory for every invocation. |
||
proc { super }, | ||
'execute', | ||
query.selected_operation_name, | ||
lambda { |span| | ||
span.set_tag('graphql.source', query.query_string) | ||
span.set_tag('graphql.operation.type', query.selected_operation.operation_type) | ||
if query.selected_operation_name | ||
span.set_tag( | ||
'graphql.operation.name', | ||
query.selected_operation_name | ||
) | ||
end | ||
query.variables.instance_variable_get(:@storage).each do |key, value| | ||
span.set_tag("graphql.variables.#{key}", value) | ||
end | ||
}, | ||
->(span) { add_query_error_events(span, query.context.errors) }, | ||
query: query, | ||
) | ||
end | ||
|
||
def execute_query_lazy(*args, query:, multiplex:, **kwargs) | ||
|
@@ -131,7 +143,16 @@ def platform_resolve_type_key(type, *args, **kwargs) | |
|
||
private | ||
|
||
def trace(callable, trace_key, resource, **kwargs) | ||
# Traces the given callable with the given trace key, resource, and kwargs. | ||
# | ||
# @param callable [Proc] the original method call | ||
# @param trace_key [String] the sub-operation name (`"graphql.#{trace_key}"`) | ||
# @param resource [String] the resource name for the trace | ||
# @param before [Proc, nil] a callable to run before the trace, same as the block parameter | ||
# @param after [Proc, nil] a callable to run after the trace, which has access to query values after execution | ||
# @param kwargs [Hash] the arguments to pass to `prepare_span` | ||
# @yield [Span] the block to run before the trace, same as the `before` parameter | ||
def trace(callable, trace_key, resource, before = nil, after = nil, **kwargs, &before_block) | ||
config = Datadog.configuration.tracing[:graphql] | ||
|
||
Tracing.trace( | ||
|
@@ -144,11 +165,17 @@ def trace(callable, trace_key, resource, **kwargs) | |
Contrib::Analytics.set_sample_rate(span, config[:analytics_sample_rate]) | ||
end | ||
|
||
yield(span) if block_given? | ||
if (before_callable = before || before_block) | ||
before_callable.call(span) | ||
end | ||
Comment on lines
+168
to
+170
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth perhaps raising if there's a |
||
|
||
prepare_span(trace_key, kwargs, span) if @has_prepare_span | ||
|
||
callable.call | ||
ret = callable.call | ||
|
||
after.call(span) if after | ||
|
||
ret | ||
end | ||
end | ||
|
||
|
@@ -163,6 +190,44 @@ def multiplex_resource(multiplex) | |
operations | ||
end | ||
end | ||
|
||
# Create a Span Event for each error that occurs at query level. | ||
# | ||
# These are represented in the Datadog App as special GraphQL errors, | ||
# given their event name `dd.graphql.query.error`. | ||
def add_query_error_events(span, errors) | ||
errors.each do |error| | ||
e = Core::Error.build_from(error) | ||
err = error.to_h | ||
|
||
span.span_events << Datadog::Tracing::SpanEvent.new( | ||
Ext::EVENT_QUERY_ERROR, | ||
attributes: { | ||
message: err['message'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: Can this be |
||
type: e.type, | ||
stacktrace: e.backtrace, | ||
locations: serialize_error_locations(err['locations']), | ||
path: err['path'], | ||
} | ||
) | ||
end | ||
end | ||
|
||
# Serialize error's `locations` array as an array of Strings, given | ||
# Span Events do not support hashes nested inside arrays. | ||
# | ||
# Here's an example in which `locations`: | ||
# [ | ||
# {"line" => 3, "column" => 10}, | ||
# {"line" => 7, "column" => 8}, | ||
# ] | ||
# is serialized as: | ||
# ["3:10", "7:8"] | ||
def serialize_error_locations(locations) | ||
locations.map do |location| | ||
"#{location['line']}:#{location['column']}" | ||
end | ||
end | ||
Comment on lines
+209
to
+230
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these locations a GraphQL thing? They're not backtracelocations, right? |
||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -9,6 +9,8 @@ module Datadog | |||||||||
|
||||||||||
ENV_ANALYTICS_SAMPLE_RATE: "DD_TRACE_GRAPHQL_ANALYTICS_SAMPLE_RATE" | ||||||||||
|
||||||||||
ENV_WITH_UNIFIED_TRACER: string | ||||||||||
EVENT_QUERY_ERROR: String | ||||||||||
Comment on lines
+12
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: I like it when we inline the actual strings here because we usually use the constants everywhere and it seems slightly weird to me if there isn't a test that fails if we rename the constants. Having the values here is equivalent to having a test, and means we need to change them in two places and thus that seems like a deliberate, rather than an accidental thing :)
Suggested change
|
||||||||||
SERVICE_NAME: "graphql" | ||||||||||
|
||||||||||
TAG_COMPONENT: "graphql" | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,10 +22,26 @@ class #{prefix}TestGraphQLQuery < ::GraphQL::Schema::Object | |
def user(id:) | ||
OpenStruct.new(id: id, name: 'Bits') | ||
end | ||
|
||
field :graphql_error, ::GraphQL::Types::Int, description: 'Raises error' | ||
|
||
def graphql_error | ||
raise 'GraphQL error' | ||
end | ||
end | ||
|
||
class #{prefix}TestGraphQLSchema < ::GraphQL::Schema | ||
query(#{prefix}TestGraphQLQuery) | ||
|
||
rescue_from(RuntimeError) do |err, obj, args, ctx, field| | ||
raise GraphQL::ExecutionError.new(err.message, extensions: { | ||
'int-1': 1, | ||
'str-1': '1', | ||
'array-1-2': [1,'2'], | ||
'': 'empty string', | ||
',': 'comma', | ||
}) | ||
end | ||
end | ||
RUBY | ||
# rubocop:enable Style/DocumentDynamicEvalDefinition | ||
|
@@ -76,10 +92,11 @@ def unload_test_schema(prefix: '') | |
end | ||
|
||
RSpec.shared_examples 'graphql instrumentation with unified naming convention trace' do |prefix: ''| | ||
let(:schema) { Object.const_get("#{prefix}TestGraphQLSchema") } | ||
let(:service) { defined?(super) ? super() : tracer.default_service } | ||
|
||
describe 'query trace' do | ||
subject(:result) { schema.execute(query: 'query Users($var: ID!){ user(id: $var) { name } }', variables: { var: 1 }) } | ||
let(:schema) { Object.const_get("#{prefix}TestGraphQLSchema") } | ||
let(:service) { defined?(super) ? super() : tracer.default_service } | ||
|
||
matrix = [ | ||
['graphql.analyze', 'query Users($var: ID!){ user(id: $var) { name } }'], | ||
|
@@ -134,4 +151,37 @@ def unload_test_schema(prefix: '') | |
end | ||
end | ||
end | ||
|
||
describe 'query with a GraphQL error' do | ||
subject(:result) { schema.execute(query: 'query Error{ graphqlError }', variables: { var: 1 }) } | ||
|
||
let(:graphql_execute) { spans.find { |s| s.name == 'graphql.execute' } } | ||
|
||
it 'creates query span for error' do | ||
expect(result.to_h['errors'][0]['message']).to eq('GraphQL error') | ||
expect(result.to_h['data']).to eq('graphqlError' => nil) | ||
|
||
expect(graphql_execute.resource).to eq('Error') | ||
expect(graphql_execute.service).to eq(service) | ||
expect(graphql_execute.type).to eq('graphql') | ||
|
||
expect(graphql_execute.get_tag('graphql.source')).to eq('query Error{ graphqlError }') | ||
|
||
expect(graphql_execute.get_tag('graphql.operation.type')).to eq('query') | ||
expect(graphql_execute.get_tag('graphql.operation.name')).to eq('Error') | ||
|
||
expect(graphql_execute.events).to contain_exactly( | ||
a_span_event_with( | ||
name: 'dd.graphql.query.error', | ||
attributes: { | ||
'message' => 'GraphQL error', | ||
'type' => 'GraphQL::ExecutionError', | ||
'stacktrace' => include(__FILE__), | ||
'locations' => ['1:14'], | ||
'path' => ['graphqlError'], | ||
} | ||
) | ||
) | ||
end | ||
end | ||
Comment on lines
+155
to
+186
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe worth perhaps adding a test for multiple errors? Since that seems a big part of the feature |
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,4 +38,8 @@ module GraphQL | |
class Multiplex | ||
end | ||
end | ||
|
||
class Error | ||
def to_h: -> Hash[String, untyped] | ||
end | ||
Comment on lines
+42
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious -- why do we need to turn the object into a hash first? Is it harder to get the values we want without the additional intermediate hash? |
||
end |
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 was curious what this did exactly and there's excellent docs to explain it! https://github.com/DataDog/dd-trace-rb/blob/502c4da38bffc97097df024e97047277346fd06a/docs/ForcingSystemTests.md :D