feat(opentelemetry): add schema coordinate to error events #1636
Conversation
Summary of ChangesHello @EmrysMyrddin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the observability of GraphQL operations by integrating detailed schema coordinate information into OpenTelemetry error events. It also refines the handling of Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds schema coordinate information to error events in OpenTelemetry traces, improving error debugging capabilities by identifying the exact schema location where errors occur. This enhancement helps developers quickly locate the source of GraphQL errors in their schema.
Key Changes
- Added schema coordinate tracking to GraphQL error events and attributes
- Introduced
abortSignalAllutility function for memory-safe signal handling - Enhanced error attributes to include path, location, and schema coordinate information
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/signal/src/abortSignalAll.ts | Implements memory-safe ponyfill for AbortSignal.any with proper garbage collection |
| packages/signal/tests/abortSignalAll.test.ts | Adds comprehensive test coverage for the abort signal utility including leak detection |
| packages/plugins/opentelemetry/src/plugin.ts | Enables schema coordinate inclusion in error handling by setting execution flag |
| packages/plugins/opentelemetry/src/spans.ts | Adds schema coordinate extraction and attribute setting for error events |
| packages/plugins/opentelemetry/src/attributes.ts | Defines new semantic attribute constants for error schema coordinates |
| packages/plugins/opentelemetry/tests/yoga.spec.ts | Adds test coverage verifying schema coordinates in error events |
| packages/runtime/src/utils.ts | Passes through schema coordinate flag to execution function |
| packages/batch-execute/src/mergeRequests.ts | Merges schema coordinate flags and abort signals across batched requests |
| package.json | Updates to alpha versions of graphql-tools packages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds the schemaCoordinate to OpenTelemetry error events, which is a great enhancement for error tracking. The changes look good overall. I've added a few comments regarding documentation clarity, potential improvements for code maintainability, and a small safety improvement. The new abortSignalAll utility is a nice addition for memory-safe signal handling, though its documentation and related tests have some inconsistencies.
| if ( | ||
| operationSpan && | ||
| !isAsyncIterable(result) && // FIXME: Handle async iterable too | ||
| result.errors && | ||
| result.errors.length > 0 | ||
| ) { | ||
| span.setAttribute(SEMATTRS_HIVE_GRAPHQL_ERROR_COUNT, result.errors.length); | ||
| span.setStatus({ | ||
| operationSpan.setAttribute( | ||
| SEMATTRS_HIVE_GRAPHQL_ERROR_COUNT, | ||
| result.errors.length, | ||
| ); | ||
| operationSpan.setStatus({ | ||
| code: SpanStatusCode.ERROR, | ||
| message: result.errors.map((e) => e.message).join(', '), | ||
| }); | ||
|
|
||
| span?.setAttribute(SEMATTRS_HIVE_GRAPHQL_ERROR_COUNT, result.errors.length); | ||
| span?.setStatus({ | ||
| code: SpanStatusCode.ERROR, | ||
| message: result.errors.map((e) => e.message).join(', '), | ||
| }); | ||
|
|
||
| const codes: string[] = []; | ||
| const schemaCoordinates: string[] = []; | ||
| for (const error of result.errors) { | ||
| span.recordException(error); | ||
| if (error.extensions?.['code']) { | ||
| codes.push(`${error.extensions['code']}`); // Ensure string using string interpolation | ||
| const attributes = attributesFromGraphqlError(error); | ||
| if (attributes[SEMATTRS_HIVE_GRAPHQL_ERROR_CODE]) { | ||
| codes.push(attributes[SEMATTRS_HIVE_GRAPHQL_ERROR_CODE] as string); | ||
| } | ||
| if (attributes[SEMATTRS_HIVE_GRAPHQL_ERROR_SCHEMA_COORDINATE]) { | ||
| schemaCoordinates.push( | ||
| attributes[SEMATTRS_HIVE_GRAPHQL_ERROR_SCHEMA_COORDINATE] as string, | ||
| ); | ||
| } | ||
| operationSpan.addEvent('graphql.error', attributes); | ||
| } | ||
|
|
||
| if (codes.length > 0) { | ||
| span.setAttribute(SEMATTRS_HIVE_GRAPHQL_ERROR_CODES, codes); | ||
| operationSpan.setAttribute(SEMATTRS_HIVE_GRAPHQL_ERROR_CODES, codes); | ||
| span?.setAttribute(SEMATTRS_HIVE_GRAPHQL_ERROR_CODES, codes); | ||
| } | ||
| if (schemaCoordinates.length > 0) { | ||
| operationSpan.setAttribute( | ||
| SEMATTRS_HIVE_GRAPHQL_ERROR_SCHEMA_COORDINATES, | ||
| schemaCoordinates, | ||
| ); | ||
| span?.setAttribute( | ||
| SEMATTRS_HIVE_GRAPHQL_ERROR_SCHEMA_COORDINATES, | ||
| schemaCoordinates, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
There's repeated logic for setting attributes on operationSpan and span within this block. This could be refactored to improve readability and reduce duplication. For example, you could iterate over a list of spans [operationSpan, span] and apply the attributes and status updates to each, which would make the code more concise and easier to maintain.
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-tools/batch-delegate |
10.0.2-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/delegate |
11.0.2-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/federation |
4.0.6-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/fusion-runtime |
1.2.6-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway |
2.1.11-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/logger |
1.0.7-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/nestjs |
2.0.16-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/plugin-aws-sigv4 |
2.0.11-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/plugin-opentelemetry |
1.0.13-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
2.0.14-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
2.1.10-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/stitch |
10.0.3-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/stitching-directives |
4.0.2-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-common |
1.0.7-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-http |
1.0.7-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-http-callback |
1.0.7-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-ws |
2.0.7-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/wrap |
11.0.2-alpha-91db0e2d5e59f79f31d29c23772d7a6c2665dcf2 |
npm ↗︎ unpkg ↗︎ |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Node Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Bun Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Binary for
|
Context
Today, errors as reported as classic OTEL Exceptions. Following the work of the GraphQL-OTEL working group (graphql/otel-wg#50) on GraphQL error reporting, Exceptions are not the right model fit.
Exception are for uncaught runtime errors. GraphQL Errors in the other hand are user facing business errors, with much more metadata than the traditional
message,typeandstacktrace.Solution
This PR migrates GraphQL errors to OTEL Event. This is following upcomging OTEL standard for GraphQL, and allows to add more metadata to it.
Errors are also attached to the operation root span, for easier filtering in dashboards.
Normal JS Error are still recorded as OTEL Exceptions, only actual
GraphQLErrorfound in the result, or returned byvalidationandparsingphases will be reported as Events.All metadata available in
GraphQLerror are reported as attribute of the event:messagepathlocationscode: If it exists, the commonly usederror.extensions.codeallowing to group errors of the same kind.schema-coordinate: a new extension added by this PR allowing to identify the faulty resolver.For the last attribute, the plugin have to override the execution function to enable a flag that adds schema coordinate to errors. This feature is only available when using our own fork of the graphql-js executor (which is the case by default).
Side Quest
By modifying the batch executor, I also noted that the signal is just ignored when merging multiple execution requests.
I've fixed that by adding a new util to merge a list of signal into one that gets aborted only once all signals are aborted or GCed.
Questions
This is changing how errors are reported in a few ways, which can break user dashboard or alerts depending on how they have written filters:
exceptionbutgraphql.errorgraphql.operationspan instead of the specific phase span (graphql.execute,graphql.validateandgraphql.parse)TODO