GraphQL: Report multiple query errors #4177
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 22066 Passed, 1476 Skipped, 5m 30.76s Total Time |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4177 +/- ##
=======================================
Coverage 97.72% 97.72%
=======================================
Files 1368 1368
Lines 82997 83046 +49
Branches 4219 4222 +3
=======================================
+ Hits 81105 81157 +52
+ Misses 1892 1889 -3 ☔ View full report in Codecov by Sentry. |
BenchmarksBenchmark execution time: 2025-01-28 00:07:46 Comparing candidate commit cf6efdc in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 29 metrics, 2 unstable metrics. scenario:profiler - Allocations (profiling disabled)
scenario:profiler - Allocations (profiling enabled)
|
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
marcotc
force-pushed
the
graphql-error-event
branch
from
af565a0 to
10f8fe3
Compare
marcotc
force-pushed
the
graphql-error-event
branch
from
10f8fe3 to
523aa51
Compare
|
👋 Hey @marcotc, please fill "Change log entry" section in the pull request description. If changes need to be present in CHANGELOG.md you can state it this way **Change log entry**
Yes. A brief summary to be placed into the CHANGELOG.md(possible answers Yes/Yep/Yeah) Or you can opt out like that **Change log entry**
None.(possible answers No/Nope/None) Visited at: 2025-01-18 00:02:08 UTC |
marcotc
force-pushed
the
graphql-error-event
branch
from
523aa51 to
418a657
Compare
Datadog ReportBranch report: ✅ 0 Failed, 22119 Passed, 1476 Skipped, 5m 48.31s Total Time ⌛ Performance Regressions vs Default Branch (13)
|
marcotc
force-pushed
the
graphql-error-event
branch
from
3b8bf6c to
cf6efdc
Compare
There was a problem hiding this comment.
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
| | `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` | |
There was a problem hiding this comment.
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"...)
| if (before_callable = before || before_block) | ||
| before_callable.call(span) | ||
| end |
There was a problem hiding this comment.
Worth perhaps raising if there's a before && before_block, just to make sure we don't accidentally introduce bugs in the future?
| span.span_events << Datadog::Tracing::SpanEvent.new( | ||
| Ext::EVENT_QUERY_ERROR, | ||
| attributes: { | ||
| message: err['message'], |
| 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 |
There was a problem hiding this comment.
Are these locations a GraphQL thing? They're not backtracelocations, right?
| ENV_WITH_UNIFIED_TRACER: string | ||
| EVENT_QUERY_ERROR: String |
There was a problem hiding this comment.
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 :)
| 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" |
| 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 |
There was a problem hiding this comment.
Maybe worth perhaps adding a test for multiple errors? Since that seems a big part of the feature
| class Error | ||
| def to_h: -> Hash[String, untyped] | ||
| end |
There was a problem hiding this comment.
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?
| span.set_tag("graphql.variables.#{key}", value) | ||
| end | ||
| end | ||
| trace( |
There was a problem hiding this comment.
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.
Captures GraphQL error information as span events:
This is necessary because each query can have multiple errors (GraphQL spec for the "errors" field), which cannot reported using span tags (span tags only support one error per span).
Change log entry
GraphQL query errors are now reported as Span Events. This includes support for multiple errors, if present.
How to test the change?
All changes have unit tests and system-tests: DataDog/system-tests#3840