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: Report multiple query errors #4177

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/forced-tests-list.json
Copy link
Member

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

Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
{

"DEFAULT": [
"tests/test_graphql.py"
]
}
2 changes: 1 addition & 1 deletion docs/GettingStarted.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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'` |

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class Settings < Contrib::Configuration::Settings
end

option :with_unified_tracer do |o|
o.env Ext::ENV_WITH_UNIFIED_TRACER
o.type :bool
o.default false
end
Expand Down
4 changes: 4 additions & 0 deletions lib/datadog/tracing/contrib/graphql/ext.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,12 @@ module Ext
# @!visibility private
ENV_ANALYTICS_ENABLED = 'DD_TRACE_GRAPHQL_ANALYTICS_ENABLED'
ENV_ANALYTICS_SAMPLE_RATE = 'DD_TRACE_GRAPHQL_ANALYTICS_SAMPLE_RATE'
ENV_WITH_UNIFIED_TRACER = 'DD_TRACE_GRAPHQL_WITH_UNIFIED_TRACER'
SERVICE_NAME = 'graphql'
TAG_COMPONENT = 'graphql'

# Span event name for query-level errors
EVENT_QUERY_ERROR = 'dd.graphql.query.error'
end
end
end
Expand Down
87 changes: 76 additions & 11 deletions lib/datadog/tracing/contrib/graphql/unified_trace.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Expand Down Expand Up @@ -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(
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth perhaps raising if there's a before && before_block, just to make sure we don't accidentally introduce bugs in the future?


prepare_span(trace_key, kwargs, span) if @has_prepare_span

callable.call
ret = callable.call

after.call(span) if after

ret
end
end

Expand All @@ -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'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Can this be e.message?

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
Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Down
2 changes: 2 additions & 0 deletions sig/datadog/tracing/contrib/graphql/ext.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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
ENV_WITH_UNIFIED_TRACER: string
EVENT_QUERY_ERROR: String
ENV_WITH_UNIFIED_TRACER: "DD_TRACE_GRAPHQL_WITH_UNIFIED_TRACER"
EVENT_QUERY_ERROR: "dd.graphql.query.error"

SERVICE_NAME: "graphql"

TAG_COMPONENT: "graphql"
Expand Down
8 changes: 6 additions & 2 deletions sig/datadog/tracing/contrib/graphql/unified_trace.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,12 @@ module Datadog
type traceKwargsValues = GraphQL::Query | GraphQL::Schema::Union | GraphQL::Schema::Object | GraphQL::Schema::Field | GraphQL::Execution::Multiplex | GraphQL::Language::Nodes::Field | Hash[Symbol, String] | String | bool | nil

type traceResult = lexerArray | GraphQL::Language::Nodes::Document | { remaining_timeout: Float?, error: Array[StandardError] } | Array[Object] | GraphQL::Schema::Object? | [GraphQL::Schema::Object, nil]

def trace: (Proc callable, String trace_key, String resource, **Hash[Symbol, traceKwargsValues ] kwargs) ?{ (Datadog::Tracing::SpanOperation) -> void } -> traceResult

def add_query_error_events: (SpanOperation span, Array[::GraphQL::Error] errors) -> void

def serialize_error_locations: (Array[{"line" => Integer, "column" => Integer}] locations)-> Array[String]

def trace: (Proc callable, String trace_key, String resource, ?Hash[Symbol, traceKwargsValues ] kwargs, ?before: ^(SpanOperation)-> void, ?after: ^(SpanOperation)-> void) ?{ (SpanOperation) -> void } -> traceResult

def multiplex_resource: (GraphQL::Execution::Multiplex multiplex) -> String?
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,12 @@ def userByName(name:)
OpenStruct.new(id: 1, name: name)
end

field :unexpected_error, UserType, description: 'Raises error'

def unexpected_error
raise 'Unexpected error'
end

field :mutationUserByName, UserType, null: false, description: 'Find an user by name' do
argument :name, ::GraphQL::Types::String, required: true
end
Expand Down
54 changes: 52 additions & 2 deletions spec/datadog/tracing/contrib/graphql/test_schema_examples.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 } }'],
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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
10 changes: 10 additions & 0 deletions spec/support/span_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -128,4 +128,14 @@ def description_of(actual)
end
end
end

RSpec::Matchers.define :a_span_event_with do |expected|
match do |actual|
values_match? Datadog::Tracing::SpanEvent, actual

expected.all? do |key, value|
values_match? value, actual.__send__(key)
end
end
end
end
4 changes: 4 additions & 0 deletions vendor/rbs/graphql/0/graphql.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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
Loading