[otel] rely on context for parenting spans correctly #532
Conversation
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-hive/gateway |
1.10.2-alpha-daed24a1d3e46bcb615d3122586a0a389d8ab81e |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-opentelemetry |
1.3.43-alpha-daed24a1d3e46bcb615d3122586a0a389d8ab81e |
npm ↗︎ unpkg ↗︎ |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 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 (Node Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
EmrysMyrddin
force-pushed
the
fix/otel/span-contexts
branch
from
b4c4094 to
6b9519f
Compare
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request introduces several updates focused on enhancing the OpenTelemetry integration for the GraphQL Mesh plugin. It updates dependencies across configuration files, refines span and trace handling by updating types and test validations, and adds new utility functions for state and context management. The changes include modifications in both end-to-end tests and multiple source modules to improve error handling and control flow in tracing functionalities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Gateway as GraphQL Gateway Plugin
participant ContextMgr as Context Manager
participant SpanMgr as Span Functions
Client->>Gateway: Send GraphQL Request
Gateway->>ContextMgr: Initialize context (withState, getContextManager)
Gateway->>SpanMgr: Start GraphQL span (parse, validate, execute)
SpanMgr-->>Gateway: Return span context and results
Gateway->>Client: Return GraphQL Response
sequenceDiagram
participant Plugin as Opentelemetry Plugin
participant Stack as OtelContextStack
participant Logger as Logger
Plugin->>Stack: push(new Context)
Note right of Stack: Context nodes maintained in a stack
Plugin->>Stack: Retrieve current/root context
Plugin->>Stack: pop() after operation completes
Logger-->>Plugin: Log error if context manager import fails
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (35)
examples/apq-subgraphs/example.tar.gzis excluded by!**/*.gzexamples/apq-subgraphs/package-lock.jsonis excluded by!**/package-lock.jsonexamples/extra-fields/example.tar.gzis excluded by!**/*.gzexamples/extra-fields/package-lock.jsonis excluded by!**/package-lock.jsonexamples/federation-example/example.tar.gzis excluded by!**/*.gzexamples/federation-example/package-lock.jsonis excluded by!**/package-lock.jsonexamples/federation-mixed/example.tar.gzis excluded by!**/*.gzexamples/federation-mixed/package-lock.jsonis excluded by!**/package-lock.jsonexamples/federation-subscriptions-passthrough/example.tar.gzis excluded by!**/*.gzexamples/federation-subscriptions-passthrough/package-lock.jsonis excluded by!**/package-lock.jsonexamples/file-upload/example.tar.gzis excluded by!**/*.gzexamples/file-upload/package-lock.jsonis excluded by!**/package-lock.jsonexamples/hmac-auth-https/example.tar.gzis excluded by!**/*.gzexamples/hmac-auth-https/package-lock.jsonis excluded by!**/package-lock.jsonexamples/interface-additional-resolvers/example.tar.gzis excluded by!**/*.gzexamples/interface-additional-resolvers/package-lock.jsonis excluded by!**/package-lock.jsonexamples/json-schema-subscriptions/example.tar.gzis excluded by!**/*.gzexamples/json-schema-subscriptions/package-lock.jsonis excluded by!**/package-lock.jsonexamples/openapi-additional-resolvers/example.tar.gzis excluded by!**/*.gzexamples/openapi-additional-resolvers/package-lock.jsonis excluded by!**/package-lock.jsonexamples/openapi-arg-rename/example.tar.gzis excluded by!**/*.gzexamples/openapi-arg-rename/package-lock.jsonis excluded by!**/package-lock.jsonexamples/openapi-javascript-wiki/example.tar.gzis excluded by!**/*.gzexamples/openapi-javascript-wiki/package-lock.jsonis excluded by!**/package-lock.jsonexamples/openapi-subscriptions/example.tar.gzis excluded by!**/*.gzexamples/openapi-subscriptions/package-lock.jsonis excluded by!**/package-lock.jsonexamples/operation-field-permissions/example.tar.gzis excluded by!**/*.gzexamples/operation-field-permissions/package-lock.jsonis excluded by!**/package-lock.jsonexamples/programmatic-batching/example.tar.gzis excluded by!**/*.gzexamples/programmatic-batching/package-lock.jsonis excluded by!**/package-lock.jsonexamples/subscriptions-with-transforms/example.tar.gzis excluded by!**/*.gzexamples/subscriptions-with-transforms/package-lock.jsonis excluded by!**/package-lock.jsonexamples/type-merging-batching/example.tar.gzis excluded by!**/*.gzexamples/type-merging-batching/package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (40)
.changeset/@graphql-mesh_plugin-opentelemetry-532-dependencies.md(1 hunks)e2e/auto-type-merging/package.json(1 hunks)e2e/extra-fields/package.json(1 hunks)e2e/hmac-auth-https/package.json(1 hunks)e2e/interface-additional-resolvers/package.json(1 hunks)e2e/opentelemetry/opentelemetry.e2e.ts(4 hunks)e2e/opentelemetry/package.json(1 hunks)e2e/retry-timeout/package.json(1 hunks)e2e/subscriptions-with-transforms/package.json(1 hunks)e2e/type-merging-batching/package.json(1 hunks)examples/apq-subgraphs/package.json(1 hunks)examples/extra-fields/package.json(1 hunks)examples/federation-example/package.json(1 hunks)examples/federation-mixed/package.json(1 hunks)examples/federation-subscriptions-passthrough/package.json(1 hunks)examples/file-upload/package.json(1 hunks)examples/hmac-auth-https/package.json(1 hunks)examples/interface-additional-resolvers/package.json(1 hunks)examples/json-schema-subscriptions/package.json(1 hunks)examples/openapi-additional-resolvers/package.json(1 hunks)examples/openapi-arg-rename/package.json(1 hunks)examples/openapi-javascript-wiki/package.json(1 hunks)examples/openapi-subscriptions/package.json(1 hunks)examples/operation-field-permissions/package.json(1 hunks)examples/programmatic-batching/package.json(1 hunks)examples/subscriptions-with-transforms/package.json(1 hunks)examples/type-merging-batching/package.json(1 hunks)package.json(1 hunks)packages/executors/http/package.json(1 hunks)packages/fusion-runtime/package.json(1 hunks)packages/gateway/package.json(1 hunks)packages/plugins/hmac-upstream-signature/package.json(1 hunks)packages/plugins/opentelemetry/package.json(2 hunks)packages/plugins/opentelemetry/src/contextManager.ts(1 hunks)packages/plugins/opentelemetry/src/plugin-utils.ts(1 hunks)packages/plugins/opentelemetry/src/plugin.ts(4 hunks)packages/plugins/opentelemetry/src/spans.ts(5 hunks)packages/plugins/opentelemetry/src/utils.ts(1 hunks)packages/plugins/prometheus/package.json(2 hunks)packages/runtime/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`examples/**`: In this directory we keep all the examples. E...
examples/**: In this directory we keep all the examples. Examples are generated using a scheduled CI pipeline.
When changes are made to Gateway config, we expect to see some changes in theexamplesdirectory.
examples/apq-subgraphs/package.jsonexamples/federation-mixed/package.jsonexamples/file-upload/package.jsonexamples/federation-example/package.jsonexamples/programmatic-batching/package.jsonexamples/operation-field-permissions/package.jsonexamples/openapi-javascript-wiki/package.jsonexamples/federation-subscriptions-passthrough/package.jsonexamples/openapi-arg-rename/package.jsonexamples/openapi-subscriptions/package.jsonexamples/openapi-additional-resolvers/package.jsonexamples/json-schema-subscriptions/package.jsonexamples/extra-fields/package.jsonexamples/subscriptions-with-transforms/package.jsonexamples/interface-additional-resolvers/package.jsonexamples/type-merging-batching/package.jsonexamples/hmac-auth-https/package.json
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
examples/apq-subgraphs/package.jsone2e/type-merging-batching/package.jsonexamples/federation-mixed/package.jsonexamples/file-upload/package.jsone2e/subscriptions-with-transforms/package.jsone2e/opentelemetry/package.jsonexamples/federation-example/package.jsonexamples/programmatic-batching/package.jsonexamples/operation-field-permissions/package.jsonexamples/openapi-javascript-wiki/package.jsonpackages/plugins/hmac-upstream-signature/package.jsonexamples/federation-subscriptions-passthrough/package.jsone2e/retry-timeout/package.jsone2e/hmac-auth-https/package.jsone2e/auto-type-merging/package.jsone2e/extra-fields/package.jsonpackages/gateway/package.jsonpackages/runtime/package.jsone2e/interface-additional-resolvers/package.jsonexamples/openapi-arg-rename/package.jsonexamples/openapi-subscriptions/package.jsonexamples/openapi-additional-resolvers/package.jsonpackages/executors/http/package.jsonexamples/json-schema-subscriptions/package.jsonpackages/fusion-runtime/package.jsonpackages/plugins/prometheus/package.jsonexamples/extra-fields/package.jsonpackage.jsonexamples/subscriptions-with-transforms/package.jsonexamples/interface-additional-resolvers/package.jsonexamples/type-merging-batching/package.jsonpackages/plugins/opentelemetry/src/contextManager.tspackages/plugins/opentelemetry/package.jsonpackages/plugins/opentelemetry/src/utils.tspackages/plugins/opentelemetry/src/plugin.tspackages/plugins/opentelemetry/src/plugin-utils.tsexamples/hmac-auth-https/package.jsonpackages/plugins/opentelemetry/src/spans.tse2e/opentelemetry/opentelemetry.e2e.ts
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/type-merging-batching/package.jsone2e/subscriptions-with-transforms/package.jsone2e/opentelemetry/package.jsone2e/retry-timeout/package.jsone2e/hmac-auth-https/package.jsone2e/auto-type-merging/package.jsone2e/extra-fields/package.jsone2e/interface-additional-resolvers/package.jsone2e/opentelemetry/opentelemetry.e2e.ts
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/plugins/hmac-upstream-signature/package.jsonpackages/gateway/package.jsonpackages/runtime/package.jsonpackages/executors/http/package.jsonpackages/fusion-runtime/package.jsonpackages/plugins/prometheus/package.jsonpackages/plugins/opentelemetry/src/contextManager.tspackages/plugins/opentelemetry/package.jsonpackages/plugins/opentelemetry/src/utils.tspackages/plugins/opentelemetry/src/plugin.tspackages/plugins/opentelemetry/src/plugin-utils.tspackages/plugins/opentelemetry/src/spans.ts
`packages/plugins/**`: The directories inside this dir are g...
packages/plugins/**: The directories inside this dir are gateway plugins. Plugins adds functionality to the GraphQL gateway, at the runtime level. Plugins included are authorization, authentication, caching, persisted documents and more.
packages/plugins/hmac-upstream-signature/package.jsonpackages/plugins/prometheus/package.jsonpackages/plugins/opentelemetry/src/contextManager.tspackages/plugins/opentelemetry/package.jsonpackages/plugins/opentelemetry/src/utils.tspackages/plugins/opentelemetry/src/plugin.tspackages/plugins/opentelemetry/src/plugin-utils.tspackages/plugins/opentelemetry/src/spans.ts
`packages/gateway/**`: The main runtime for the Hive gateway...
packages/gateway/**: The main runtime for the Hive gateway.
This package is CLI that runs the gateway and configures the internals of the gateway function.
packages/gateway/package.json
🪛 Biome (1.9.4)
packages/plugins/opentelemetry/src/utils.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 27-27: Do not add then to an object.
(lint/suspicious/noThenProperty)
packages/plugins/opentelemetry/src/plugin.ts
[error] 339-339: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 371-371: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 375-375: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 395-395: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 401-401: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 421-421: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 427-427: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 441-441: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 452-452: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 459-459: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 493-493: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 341-341: This let declares a variable that is only assigned once.
'handler' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
packages/plugins/opentelemetry/src/plugin-utils.ts
[error] 9-9: This let declares a variable that is only assigned once.
'states' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 27-27: This let declares a variable that is only assigned once.
'pluginWithState' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 35-35: This let declares a variable that is only assigned once.
'state' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
e2e/opentelemetry/opentelemetry.e2e.ts
[error] 568-568: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 570-570: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 578-578: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 599-599: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
🔇 Additional comments (50)
packages/plugins/opentelemetry/src/plugin-utils.ts (1)
1-8: No further issues found; well-structured and clear.
Everything else in this file appears correct and maintainable.Also applies to: 10-26, 28-34, 36-105
packages/plugins/opentelemetry/src/spans.ts (1)
1-113: All other spans logic looks great.
The additions thoroughly cover span lifecycle management, attribute setting, and error handling for GraphQL operations.Also applies to: 118-367
packages/plugins/opentelemetry/src/plugin.ts (1)
1-579: Add a changeset and confirm Linear issue reference.
• Since you modified code within “packages/**”, please ensure a corresponding changeset exists describing these updates.
• Also, there's no mention of “GW-*” in the PR description. Please confirm whether a relevant Linear issue should be linked.🧰 Tools
🪛 Biome (1.9.4)
[error] 339-339: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 371-371: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 375-375: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 395-395: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 401-401: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 421-421: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 427-427: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 441-441: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 452-452: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 459-459: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 493-493: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 341-341: This let declares a variable that is only assigned once.
'handler' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
e2e/opentelemetry/opentelemetry.e2e.ts (2)
27-27: No issues found.
The array type definition looks correct.
83-85: Ensure error re-throwing logic aligns with desired behavior.
If the signal is aborted, this block re-throws the last encountered error (stored in 'err'). This may mask the actual AbortError or other errors if the code doesn't exit the loop immediately. Verify that this is indeed the desired behavior.packages/plugins/opentelemetry/src/contextManager.ts (1)
1-43: Implementation looks solid.
This stack-based approach for context management is straightforward and easy to follow. Great job! Just ensure that concurrent calls to push/pop won't break the stack invariants if used in multi-threaded contexts (e.g., Worker threads).packages/plugins/opentelemetry/src/utils.ts (1)
27-30: Double-check custom Promise layering.
Attaching .then, .catch, and .finally directly to the returned object can raise concerns from lint tools, as it mimics native Promises. Ensure this approach handles all edge cases or consider simpler transformations that rely on Promise APIs directly.🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: Do not add then to an object.
(lint/suspicious/noThenProperty)
e2e/extra-fields/package.json (1)
7-7: Dependency Version Update for graphql-yoga
The version has been bumped from "^5.10.11" to "^5.11.0". This is a coordinated upgrade across the codebase. Please ensure that the new version is compatible with all usage scenarios in your tests.e2e/type-merging-batching/package.json (1)
7-7: graphql-yoga Upgrade Consistency
The dependency update to "^5.11.0" is consistent with other packages. Verify that any minor API changes introduced in graphql-yoga do not affect the functionality within this e2e test context.e2e/interface-additional-resolvers/package.json (1)
7-7: Updated graphql-yoga Version
Updating to "^5.11.0" is aligned with the coordinated dependency bumps. Double-check that the integration with the interface additional resolvers functionality continues to work as expected.e2e/retry-timeout/package.json (1)
8-8: graphql-yoga Dependency Update
The dependency version has been updated from "^5.10.11" to "^5.11.0". Please confirm that the new version does not introduce any unforeseen timeout or retry behavior differences..changeset/@graphql-mesh_plugin-opentelemetry-532-dependencies.md (1)
1-8: Changeset Dependency Updates for @graphql-mesh/plugin-opentelemetry
This changeset adds a new dependency "@opentelemetry/context-async-hooks" at "^1.30.1" and updates "graphql-yoga" to "^5.11.0". These modifications support enhanced OpenTelemetry context management. Ensure that the added dependency is fully compatible with the plugin's tracing and context parenting functionality.e2e/subscriptions-with-transforms/package.json (1)
8-8: Upgrade graphql-yoga Dependency VersionThe dependency "graphql-yoga" has been updated to "^5.11.0", aligning with the broader upgrade across the project. This change helps ensure consistency across different e2e test suites.
e2e/auto-type-merging/package.json (1)
8-8: Consistent Dependency Update for graphql-yogaThe "graphql-yoga" version has been updated to "^5.11.0" in the devDependencies. This is consistent with other parts of the repository and should help reduce version conflicts.
e2e/opentelemetry/package.json (1)
9-9: Align graphql-yoga Version with Overall Dependency UpgradesUpdating "graphql-yoga" to "^5.11.0" in this file is important for maintaining consistency, especially as it supports OpenTelemetry-related e2e tests. Please verify that the change does not introduce any behavioral differences affecting the tracing and context propagation features.
examples/openapi-javascript-wiki/package.json (1)
10-10: Update @graphql-hive/gateway Dependency VersionThe dependency "@graphql-hive/gateway" has been updated from "^1.9.0" to "^1.9.1". This change ensures that the example remains up to date with the latest improvements and releases. Confirm that the change is reflected in both functionality and documentation as needed.
examples/openapi-additional-resolvers/package.json (1)
10-10: Upgrade @graphql-hive/gateway Dependency VersionThe dependency update to "@graphql-hive/gateway" version "^1.9.1" is straightforward and consistent with similar updates across the repository. Ensure that any integration tests for the examples confirm compatibility with the new version.
examples/file-upload/package.json (1)
8-8: Gateway Dependency Version Update Confirmed
The@graphql-hive/gatewaydependency is updated to^1.9.1, which is consistent with the broader dependency standardization across the repository.examples/apq-subgraphs/package.json (1)
12-12: Gateway Dependency Version Upgrade
The dependency update to@graphql-hive/gatewayversion^1.9.1aligns with the changes in other example projects. No issues detected.examples/interface-additional-resolvers/package.json (2)
7-7: GraphQL-Yoga Version Update
Upgradinggraphql-yogato^5.11.0is in line with upgrading efforts across the project. Ensure compatibility with any custom middleware or configurations if applicable.
9-9: Gateway Dependency Version Update
The version of@graphql-hive/gatewayhas been updated to^1.9.1, ensuring consistency with similar modifications elsewhere.examples/programmatic-batching/package.json (1)
10-10: Consistent Update for Gateway Dependency
The update of@graphql-hive/gatewayto^1.9.1maintains consistency with the dependency versions in other examples.examples/openapi-arg-rename/package.json (1)
10-10: Updated Gateway Dependency Version
The dependency@graphql-hive/gatewayis now set to^1.9.1, which matches the standardized upgrade across the repository.examples/extra-fields/package.json (3)
7-7: Upgrade of graphql-yoga dependency version.
The version has been bumped from "^5.10.11" to "^5.11.0" which appears consistent with the other upgrades in this PR.
9-9: Upgrade of @graphql-hive/gateway dependency version.
The update from "^1.9.0" to "^1.9.1" is clear and aligns with the coordinated dependency upgrades across affected packages.
1-20: Missing Linear Issue Reference.
Please ensure that the PR description includes a reference to the corresponding Linear issue ID (e.g. "GW-...") as required by guidelines. This helps maintain traceability for dependency and functionality changes.examples/operation-field-permissions/package.json (1)
9-9: Upgrade of @graphql-hive/gateway dependency version.
The dependency version has been updated to "^1.9.1" consistently with similar changes elsewhere in the repository.examples/type-merging-batching/package.json (2)
7-7: Upgrade of graphql-yoga dependency version.
The bump to "^5.11.0" ensures consistency with the overall dependency uplift in this PR.
9-9: Upgrade of @graphql-hive/gateway dependency version.
The update to "^1.9.1" is correct and aligns with the coordinated updates made in other package.json files.examples/subscriptions-with-transforms/package.json (2)
8-8: Upgrade of graphql-yoga dependency version.
The dependency version has been updated to "^5.11.0" from its previous version, which is consistent with similar changes in the PR.
10-10: Upgrade of @graphql-hive/gateway dependency version.
The dependency update to "^1.9.1" is applied correctly.examples/openapi-subscriptions/package.json (1)
12-12: Upgrade of @graphql-hive/gateway dependency version.
The version bump to "^1.9.1" is consistent with the dependency updates across the repository.examples/json-schema-subscriptions/package.json (2)
1-4: Ensure PR Issue Linking for TraceabilityIt appears there is no explicit reference to a relevant GW-* Linear issue in the PR description or metadata in this file. Please double-check that the PR description includes the appropriate GW-* issue link(s) for proper traceability.
13-13: Dependency Version Update ConfirmedThe dependency "@graphql-hive/gateway" has been successfully updated to "^1.9.1". This change is consistent with the coordinated version upgrades across the repository.
examples/federation-example/package.json (1)
12-12: Consistent Gateway Dependency UpgradeThe "@graphql-hive/gateway" dependency is updated to "^1.9.1" on line 12. This maintains consistency with other examples and aligns with the overall dependency upgrade strategy.
e2e/hmac-auth-https/package.json (1)
16-16: GraphQL-Yoga Dependency Version UpdateThe "graphql-yoga" dependency has been updated to "^5.11.0", which is in line with similar updates across multiple packages in this PR.
examples/federation-mixed/package.json (1)
12-12: Gateway Dependency Version UpdateThe update of "@graphql-hive/gateway" to "^1.9.1" on line 12 is correctly applied and consistent with the rest of the repository.
examples/federation-subscriptions-passthrough/package.json (1)
18-18: Uniform Dependency Upgrade Across ExamplesThe "@graphql-hive/gateway" dependency update to "^1.9.1" on line 18 ensures uniformity across our examples. This change looks correct and is aligned with the PR objectives.
examples/hmac-auth-https/package.json (2)
15-15: Update @graphql-hive/gateway Dependency Version
The version has been bumped to "^1.9.1", which aligns with the other package updates in this PR. Please ensure that any related documentation or changeset files (if applicable) are updated accordingly.
20-20: Update graphql-yoga Dependency Version
The update to "^5.11.0" is consistent with the coordinated dependency upgrades across the project. Verify that no breaking changes are introduced by this bump and that integration tests pass.packages/plugins/hmac-upstream-signature/package.json (1)
57-57: Update graphql-yoga Version in DevDependencies
Bumping "graphql-yoga" to "^5.11.0" here ensures consistency with the repository’s overall dependency strategy. Please confirm that a corresponding changeset file is provided to document this minor version update.packages/plugins/prometheus/package.json (2)
42-45: Update graphql-yoga in PeerDependencies
The dependency version for "graphql-yoga" in peerDependencies has been updated to "^5.11.0", ensuring that consumers of this package use a compatible version. Make sure to update any related documentation if needed.
57-62: Update graphql-yoga in DevDependencies
Similarly, the devDependencies now reflect the "^5.11.0" version. Please check that a changeset file is created to formally record this dependency update and that tests validate the upgrade.packages/executors/http/package.json (1)
57-57: Update graphql-yoga in DevDependencies
The upgrade to "^5.11.0" for "graphql-yoga" maintains consistency with other package updates. It’s important to run end-to-end tests to ensure that no regressions or compatibility issues are introduced by this change.packages/fusion-runtime/package.json (1)
61-61: Update graphql-yoga Dependency Version
The dependency "graphql-yoga" has been updated to "^5.11.0", which fits the coordinated update strategy across the codebase. Ensure that downstream consumers are aware of this update and that the changeset file (if not already present) is added to document the minor version bump.packages/plugins/opentelemetry/package.json (2)
53-53: Dependency Addition: @opentelemetry/context-async-hooks
The new dependency "@opentelemetry/context-async-hooks": "^1.30.1" enhances async context propagation essential for proper span parenting. Ensure that this version is fully compatible with the rest of the OpenTelemetry packages in use.
68-68: Upgrade: graphql-yoga Version Update
The update of "graphql-yoga" to "^5.11.0" in devDependencies aligns this package with other parts of the codebase. Please verify that this minor version bump does not introduce breaking changes within your plugin's context or tests.packages/runtime/package.json (1)
81-83: Dependency Standardization: graphql-yoga Version Update
Updating "graphql-yoga" to "^5.11.0" standardizes the dependency across multiple packages. Ensure that you run integration and e2e tests to validate that no runtime issues arise from this version bump.package.json (1)
63-63:⚠️ Potential issueResolution Addition: @envelop/core Pinning
The new resolution for "@envelop/core" pins it to "5.1.0-alpha-20250206123608-cd323a08c43c066eb34cba698d61b4f059ab5ee5". Since this is an alpha version, please confirm that its use is intentional and that it has been vetted for stability within your environment. Consider adding a comment or documentation reference for future maintainers regarding the choice of an alpha release.packages/gateway/package.json (1)
86-88: Dependency Upgrade: graphql-yoga Version Update
The "graphql-yoga" dependency has been updated to "^5.11.0" to keep consistency with the rest of the packages. Please ensure that this update has been tested thoroughly in the gateway’s runtime to prevent any unintended behavior changes.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/opentelemetry/src/plugin-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/plugins/opentelemetry/src/plugin-utils.ts
`packages/plugins/**`: The directories inside this dir are g...
packages/plugins/**: The directories inside this dir are gateway plugins. Plugins adds functionality to the GraphQL gateway, at the runtime level. Plugins included are authorization, authentication, caching, persisted documents and more.
packages/plugins/opentelemetry/src/plugin-utils.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/plugins/opentelemetry/src/plugin-utils.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/plugins/opentelemetry/src/plugin-utils.ts (3)
3-56: Consider thread-safety when maintaining shared state.The
withStatefunction stores data instatesusingWeakMap. Each map is keyed by objects such asrequest,context, orexecutionRequest. Ensure there are no concurrency concerns when multiple requests flow through this plugin in parallel (e.g., multi-threaded or multi-worker environments). If concurrency is controlled by your runtime, this approach is typically safe, but it’s worth verifying that no race conditions occur when reading/writing the same scopes instates.
58-75: Well-structured hierarchical state retrieval.The
getMostSpecificStatefunction elegantly supports fallback ordering fromforSubgraphExecutiontoforOperationand finally toforRequest. This is clean and logically sound. No changes required.
77-105: Conditional types usage is commendable.The
PayloadWithStateandWithStategenerics effectively tailor state properties to various payload shapes. The approach is advanced but well-structured. This should greatly improve type safety in real usage scenarios.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/opentelemetry/src/plugin.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/plugins/opentelemetry/src/plugin.ts
`packages/plugins/**`: The directories inside this dir are g...
packages/plugins/**: The directories inside this dir are gateway plugins. Plugins adds functionality to the GraphQL gateway, at the runtime level. Plugins included are authorization, authentication, caching, persisted documents and more.
packages/plugins/opentelemetry/src/plugin.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/plugins/opentelemetry/src/plugin.ts
🪛 Biome (1.9.4)
packages/plugins/opentelemetry/src/plugin.ts
[error] 339-339: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 371-371: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 375-375: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 395-395: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 401-401: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 421-421: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 427-427: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 441-441: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 452-452: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 459-459: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 493-493: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/plugins/opentelemetry/src/plugin.ts (3)
58-60: Nice utility for conditional tracing configuration.
DefiningBooleanOrPredicateis a clean and flexible way to conditionally enable or disable spans. This approach is both readable and extensible.
68-72: Verify compatibility ofcontextManagerwith targeted environments.
If you plan to run in browser environments, importing@opentelemetry/context-async-hooksmay cause issues, sinceAsyncLocalStoragetypically requires Node. Please ensure that this fallback logic aligns with your deployment environment(s).
339-339: Avoid using non-null assertions.
The repeated usage of!forforOperation.otel!and related fields can mask potential null/undefined issues. Consider checking for existence or using optional chaining.- forOperation.otel!.pop(); + forOperation.otel?.pop();Alternatively, refine the type definitions to ensure these values are never optional if that's guaranteed by design.
Also applies to: 371-371, 375-375, 395-395, 401-401, 421-421, 427-427, 441-441, 452-452, 459-459, 493-493
🧰 Tools
🪛 Biome (1.9.4)
[error] 339-339: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
EmrysMyrddin
force-pushed
the
fix/otel/span-contexts
branch
2 times, most recently
from
d9c727f to
8f14dd3
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
packages/plugins/opentelemetry/src/spans.ts (1)
202-241:startGraphQLValidateSpan: Nicely aggregates validation errors.Recording multiple errors in the span is helpful, though note the loop at line 234 references
for (const error in result). That iterates over property names, notErrorobjects. Usefor (const error of result)for correct iteration.- for (const error in result) { - span.recordException(error); - } + for (const error of result) { + span.recordException(error); + }package.json (1)
1-1: Missing Linear Issue ReferenceThe PR description does not reference a related "GW-*" linear issue. Please ensure that a corresponding linear issue is linked to adhere to project workflow standards.
♻️ Duplicate comments (1)
packages/plugins/opentelemetry/src/plugin.ts (1)
339-339: Avoid non-null assertions for safer code.
Repeated usage of!can mask undefined or null states and cause runtime issues. Consider refining your types or using optional chaining (?.) to avoid potential errors.Here's an example fix for line 339 (applies similarly to other lines):
- forOperation.otel! = new OtelContextStack(ctx); + if (forOperation.otel) { + forOperation.otel = new OtelContextStack(ctx); + }Also applies to: 371-371, 375-375, 395-395, 401-401, 421-421, 427-427, 441-441, 452-452, 459-459, 493-493
🧰 Tools
🪛 Biome (1.9.4)
[error] 339-339: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🛑 Comments failed to post (5)
packages/plugins/opentelemetry/src/contextManager.ts (2)
25-27: 🧹 Nitpick (assertive)
Optional return of the new top context.
When using
push, returning the new top context (or the updatedthis.#current) could be beneficial for quick chaining. This is optional but can improve usability of the API.
33-41: 🧹 Nitpick (assertive)
Graceful handling of missing spans in
toString.If
trace.getSpan(node.ctx)returnsnull, the conversion(trace.getSpan(node.ctx) as unknown as { name: string }).namecould fail. Consider a simple existence check or fallback name to prevent potential runtime errors.- names.push((trace.getSpan(node.ctx) as unknown as { name: string }).name); + const span = trace.getSpan(node.ctx); + if (span) { + names.push((span as unknown as { name: string }).name); + } else { + names.push('(no-span)'); + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.toString() { let node: Node | undefined = this.#current; const names = []; while (node != undefined) { const span = trace.getSpan(node.ctx); if (span) { names.push((span as unknown as { name: string }).name); } else { names.push('(no-span)'); } node = node.previous; } return names.join(' -> '); }packages/plugins/opentelemetry/src/plugin-utils.ts (1)
1-2: 🧹 Nitpick (assertive)
Missing required changeset and Linear issue reference.
According to our guidelines, PRs affecting
packages/plugins/**should mention a corresponding “GW-*” Linear issue and include a changeset (created viayarn changeset). Please ensure these steps are completed for proper versioning and tracking.Would you like help generating a changeset or referencing the correct Linear issue?
packages/plugins/opentelemetry/src/spans.ts (2)
87-131: 🧹 Nitpick (assertive)
startGraphQLSpanhandles request cycles with callback-based finalization.The pattern of returning a
donefunction that ends the span is clear. However, theFIXMEfor async iterable results at line 116 remains unresolved. Consider instrumenting streaming or subscription responses for full coverage.Would you like a snippet to handle async iterable results automatically?
243-293: 🧹 Nitpick (assertive)
startGraphQLExecuteSpanpartially handles streaming.The
FIXMEcomment at line 276 acknowledges the need to handleAsyncIterableresults. Consider addressing this now for completeness and consistent instrumentation of subscriptions.
EmrysMyrddin
force-pushed
the
fix/otel/span-contexts
branch
from
8f14dd3 to
6faa867
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
packages/plugins/opentelemetry/src/utils.ts (1)
1-1: Use the Node.js protocol for built-in imports.
As recommended by the linter, switching tonode:util/typesclarifies that this is a Node.js built-in module.- import { isPromise } from 'util/types'; + import { isPromise } from 'node:util/types';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import { isPromise } from 'node:util/types';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
.changeset/@graphql-mesh_plugin-opentelemetry-532-dependencies.md(1 hunks)e2e/opentelemetry/opentelemetry.e2e.ts(4 hunks)package.json(1 hunks)packages/plugins/opentelemetry/package.json(2 hunks)packages/plugins/opentelemetry/src/contextManager.ts(1 hunks)packages/plugins/opentelemetry/src/plugin-utils.ts(1 hunks)packages/plugins/opentelemetry/src/plugin.ts(4 hunks)packages/plugins/opentelemetry/src/spans.ts(5 hunks)packages/plugins/opentelemetry/src/utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
package.jsonpackages/plugins/opentelemetry/package.jsonpackages/plugins/opentelemetry/src/contextManager.tspackages/plugins/opentelemetry/src/utils.tspackages/plugins/opentelemetry/src/plugin-utils.tse2e/opentelemetry/opentelemetry.e2e.tspackages/plugins/opentelemetry/src/plugin.tspackages/plugins/opentelemetry/src/spans.ts
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/plugins/opentelemetry/package.jsonpackages/plugins/opentelemetry/src/contextManager.tspackages/plugins/opentelemetry/src/utils.tspackages/plugins/opentelemetry/src/plugin-utils.tspackages/plugins/opentelemetry/src/plugin.tspackages/plugins/opentelemetry/src/spans.ts
`packages/plugins/**`: The directories inside this dir are g...
packages/plugins/**: The directories inside this dir are gateway plugins. Plugins adds functionality to the GraphQL gateway, at the runtime level. Plugins included are authorization, authentication, caching, persisted documents and more.
packages/plugins/opentelemetry/package.jsonpackages/plugins/opentelemetry/src/contextManager.tspackages/plugins/opentelemetry/src/utils.tspackages/plugins/opentelemetry/src/plugin-utils.tspackages/plugins/opentelemetry/src/plugin.tspackages/plugins/opentelemetry/src/spans.ts
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/opentelemetry/opentelemetry.e2e.ts
🪛 Biome (1.9.4)
packages/plugins/opentelemetry/src/utils.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 27-27: Do not add then to an object.
(lint/suspicious/noThenProperty)
e2e/opentelemetry/opentelemetry.e2e.ts
[error] 568-568: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 570-570: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 578-578: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 599-599: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
packages/plugins/opentelemetry/src/plugin.ts
[error] 339-339: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 371-371: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 375-375: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 395-395: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 401-401: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 421-421: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 427-427: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 441-441: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 452-452: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 459-459: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 493-493: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
⏰ Context from checks skipped due to timeout of 90000ms (26)
- GitHub Check: Binary built on ubuntu-24.04-arm
- GitHub Check: Bun Docker image
- GitHub Check: Binary built on windows-latest
- GitHub Check: Node Docker image
- GitHub Check: Binary built on macos-14
- GitHub Check: Binary built on macos-13
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Leaks / Node v22
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / node / 10 items
🔇 Additional comments (15)
packages/plugins/opentelemetry/src/contextManager.ts (2)
1-6: Type definition and usage look good
TheNodetype is straightforward, and leveragingprevious?: Nodeeffectively implements a singly-linked stack.
12-15: Stack initialization and push/pop logic look correct
The constructor,push, andpopmethods correctly maintain the stack structure.Also applies to: 25-31
packages/plugins/opentelemetry/src/plugin-utils.ts (3)
3-13: Great approach usingwithStatefor centralized state management
This design effectively leveragesWeakMapfor memory-efficient associations.
19-20: Avoid inline reassignment withinset
This pattern was flagged in older comments. To maintain clarity, please separate the assignment from theWeakMap.setcall.- let value = states[scope].get(key as any); - if (!value) states[scope].set(key, (value = {})); + let value = states[scope].get(key as any); + if (!value) { + value = {}; + states[scope].set(key, value); + }
27-55: Ensure concurrency safety for shared state access
If this plugin is used in a concurrent environment, consider the thread or process safety of these shared states. Additional synchronization or context scoping may be necessary.packages/plugins/opentelemetry/src/spans.ts (1)
1-367: Missing changeset and Linear issue reference
Since this is a new or enhanced feature withinpackages/**, please ensure a corresponding changeset file is added (viayarn changeset) describing the changes (likely a minor bump). Also, no "GW-*" reference was found in the PR description. If this PR addresses a Linear issue, please mention it explicitly.packages/plugins/opentelemetry/src/plugin.ts (3)
2-6: Add a changeset and link any relevant Linear issue.
These new or modified plugin sources appear inpackages/plugins/opentelemetry/, which typically require a changeset usingyarn changesetto describe the changes. Also, there's no mention of a “GW-*” issue in the PR description—if there's a corresponding internal ticket, consider adding it to the PR.
310-312: Confirm error handling when HTTP span startup fails.
Currently, the error is logged but not rethrown or returned as a response. Verify whether it’s acceptable to proceed with a normal flow if span initialization fails.
277-559: Overall implementation looks good.
The structured approach withwithStateandcontext.bindfor concurrency ensures consistent context usage. No major correctness or performance concerns are apparent.🧰 Tools
🪛 Biome (1.9.4)
[error] 339-339: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 371-371: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 375-375: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 395-395: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 401-401: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 421-421: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 427-427: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 441-441: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 452-452: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 459-459: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 493-493: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
e2e/opentelemetry/opentelemetry.e2e.ts (1)
1300-1324: Consider preventing cyclical references in buildSpanTree.
If spans can reference each other in a cycle, it may lead to an infinite recursion. Using a visited set withinbuildNodecan mitigate this risk..changeset/@graphql-mesh_plugin-opentelemetry-532-dependencies.md (2)
1-4: Changeset File Header Formatting
The header using YAML separators is correctly structured. It clearly identifies the package and the change type ("patch").
5-8: Documenting Dependency Update in Changeset
The changeset clearly documents the addition of the dependency@opentelemetry/context-async-hooks@^1.30.1. Ensure that this update is synchronized with the corresponding changes in the package.json to prevent any inconsistencies.packages/plugins/opentelemetry/package.json (2)
53-53: New Dependency Addition: @opentelemetry/context-async-hooks
The dependency"@opentelemetry/context-async-hooks": "^1.30.1"has been added, aligning with the intended update. Confirm that this version is compatible with your current OpenTelemetry usage in the codebase.
66-66: New Dev Dependency Addition: @whatwg-node/server
The dev dependency"@whatwg-node/server": "^0.9.65"has been added. As noted in previous reviews, ensure that any scripts or documentation dependent on this package are updated accordingly.package.json (1)
63-63: New Resolution for @envelop/core
The resolution specifying"@envelop/core": "5.1.0-alpha-20250206123608-cd323a08c43c066eb34cba698d61b4f059ab5ee5"has been added. Verify that this version is compatible with the other dependencies and that it resolves potential version conflicts without introducing regressions.
| for (const error in result) { | ||
| validateSpan.recordException(error); | ||
| span.recordException(error); | ||
| } |
There was a problem hiding this comment.
Potentially incorrect iteration method
In the validation span logic, for (const error in result) iterates over property names instead of the elements themselves. Use for (const error of result) to properly iterate over array items.
- for (const error in result) {
+ for (const error of result) {
span.recordException(error);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const error in result) { | |
| validateSpan.recordException(error); | |
| span.recordException(error); | |
| } | |
| for (const error of result) { | |
| span.recordException(error); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
.changeset/@graphql-mesh_plugin-opentelemetry-532-dependencies.md (1)
1-9: 🧹 Nitpick (assertive)Strategic addition of OpenTelemetry context handling.
The addition of
@opentelemetry/context-async-hooksaligns perfectly with the PR's objective of improving context handling for parenting spans. This package is specifically designed for managing context in asynchronous operations.This addition will help ensure proper context propagation across async boundaries, which is crucial for maintaining accurate parent-child relationships in distributed tracing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (22)
.changeset/@graphql-hive_gateway-532-dependencies.md(1 hunks).changeset/@graphql-hive_gateway-runtime-532-dependencies.md(1 hunks).changeset/@graphql-mesh_fusion-runtime-532-dependencies.md(1 hunks).changeset/@graphql-mesh_hmac-upstream-signature-532-dependencies.md(1 hunks).changeset/@graphql-mesh_plugin-opentelemetry-532-dependencies.md(1 hunks).changeset/@graphql-mesh_plugin-prometheus-532-dependencies.md(1 hunks).changeset/@graphql-mesh_transport-common-532-dependencies.md(1 hunks).changeset/@graphql-mesh_transport-http-532-dependencies.md(1 hunks).changeset/@graphql-mesh_transport-http-callback-532-dependencies.md(1 hunks).changeset/@graphql-mesh_transport-ws-532-dependencies.md(1 hunks).changeset/@graphql-tools_batch-delegate-532-dependencies.md(1 hunks).changeset/@graphql-tools_batch-execute-532-dependencies.md(1 hunks).changeset/@graphql-tools_delegate-532-dependencies.md(1 hunks).changeset/@graphql-tools_executor-common-532-dependencies.md(1 hunks).changeset/@graphql-tools_executor-graphql-ws-532-dependencies.md(1 hunks).changeset/@graphql-tools_executor-http-532-dependencies.md(1 hunks).changeset/@graphql-tools_federation-532-dependencies.md(1 hunks).changeset/@graphql-tools_stitch-532-dependencies.md(1 hunks).changeset/@graphql-tools_stitching-directives-532-dependencies.md(1 hunks).changeset/@graphql-tools_wrap-532-dependencies.md(1 hunks)packages/plugins/opentelemetry/src/context-stack.ts(1 hunks)packages/plugins/opentelemetry/src/plugin.ts(4 hunks)
👮 Files not reviewed due to content moderation or server errors (8)
- .changeset/@graphql-tools_wrap-532-dependencies.md
- .changeset/@graphql-hive_gateway-532-dependencies.md
- .changeset/@graphql-tools_stitch-532-dependencies.md
- .changeset/@graphql-tools_delegate-532-dependencies.md
- .changeset/@graphql-mesh_transport-ws-532-dependencies.md
- .changeset/@graphql-mesh_transport-http-532-dependencies.md
- .changeset/@graphql-mesh_fusion-runtime-532-dependencies.md
- .changeset/@graphql-tools_batch-execute-532-dependencies.md
🧰 Additional context used
📓 Path-based instructions (3)
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/plugins/opentelemetry/src/context-stack.tspackages/plugins/opentelemetry/src/plugin.ts
`packages/plugins/**`: The directories inside this dir are g...
packages/plugins/**: The directories inside this dir are gateway plugins. Plugins adds functionality to the GraphQL gateway, at the runtime level. Plugins included are authorization, authentication, caching, persisted documents and more.
packages/plugins/opentelemetry/src/context-stack.tspackages/plugins/opentelemetry/src/plugin.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/plugins/opentelemetry/src/context-stack.tspackages/plugins/opentelemetry/src/plugin.ts
🪛 Biome (1.9.4)
packages/plugins/opentelemetry/src/plugin.ts
[error] 339-339: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 371-371: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 375-375: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 395-395: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 401-401: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 421-421: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 427-427: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 441-441: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 452-452: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 459-459: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 493-493: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Examples / Convert extra-fields
- GitHub Check: Binary built on windows-latest
- GitHub Check: Binary built on macos-14
- GitHub Check: Bun Docker image
- GitHub Check: Binary built on macos-13
- GitHub Check: Node Docker image
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: Leaks / Node v20
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Leaks / Node v18
- GitHub Check: E2E / Node 18 on Ubuntu
🔇 Additional comments (12)
packages/plugins/opentelemetry/src/plugin.ts (1)
1-582: Add a changeset, link any relevant Linear issues, and consider documentation updates.
As per the coding guidelines, packages underpackages/plugins/**should include a changeset (yarn changeset) describing the modifications, along with an appropriate semver bump. Also, there's no mention of a “GW-” Linear issue in the PR description. If such an issue exists, please link it. Finally, if these changes require user-facing documentation, you might open a follow-up PR in thegraphql-hive/consolerepository to ensure clarity for end users.🧰 Tools
🪛 Biome (1.9.4)
[error] 339-339: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 371-371: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 375-375: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 395-395: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 401-401: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 421-421: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 427-427: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 441-441: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 452-452: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 459-459: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 493-493: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
.changeset/@graphql-tools_federation-532-dependencies.md (1)
1-8: Verify compatibility with the new version of @graphql-tools/utils.All changesets update
@graphql-tools/utilsfrom^10.7.0to^10.8.1. While this is marked as a patch update, it's important to verify the changes and potential impact.Let's check the changes between these versions:
Additionally, let's verify if there are any known issues or breaking changes:
Also applies to: 1-8, 1-8, 1-8, 1-8
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check for any reported issues related to the version update # Check for issues mentioning the version update gh issue list -R ardatan/graphql-tools --json title,body -q '.[] | select(.title | contains("10.8.1") or .body | contains("10.8.1"))' # Check for discussions about potential breaking changes gh api graphql -f query=' { repository(owner: "ardatan", name: "graphql-tools") { discussions(first: 10) { nodes { title body } } } }' | jq '.data.repository.discussions.nodes[] | select(.title | contains("10.8.1") or .body | contains("10.8.1"))'Length of output: 557
🌐 Web query:
What are the changes between @graphql-tools/utils version 10.7.0 and 10.8.1?💡 Result:
Based on the provided search results, there is no information available about @graphql-tools/utils version 10.8.1. The latest documented version in the changelogs is 10.7.2 (released January 6, 2025)[1][8][9]. Here are the key changes up to 10.7.x:
Recent Changes in @graphql-tools/utils (v10.x)
v10.7.2 (Patch)
- Fixed CVE-2024-21529 by updating
dsetdependency[1].v10.7.1 (Patch)
- Removed
VoidFunctiontype usage to eliminate dependency on DOM types[1].v10.7.0 (Minor)
- Added
getAbortPromiseandregisterAbortSignalListenerhelpers to handleAbortSignalwithout triggering Node.jsMaxListenersExceededWarning[1].For versions beyond 10.7.x, no changelog data exists in the provided sources[1][3][7][8][9]. If version 10.8.x exists, it might include unreleased updates not yet documented publicly or listed in the indexed search results.
For real-time updates, check npm or the GitHub repository[8][9].
Citations:
- 1: https://the-guild.dev/graphql/tools/changelogs/utils
- 2: https://the-guild.dev/graphql/tools/changelogs/links
- 3: https://github.com/ardatan/graphql-tools/blob/master/packages/load/CHANGELOG.md
- 4: https://test-gsx.cisco.com/c/dam/en_us/about/doing_business/open_source/docs/AppDynamics_Cloud_NextUI-22120-1670618719.pdf
- 5: https://the-guild.dev/graphql/tools/changelogs/executors/yoga
- 6: https://www.cisco.com/content/dam/en_us/about/doing_business/open_source/docs/AppDynamics_Cloud_NextUI-2280-1658894610.pdf
- 7: https://github.com/ardatan/graphql-tools/releases
- 8: https://www.npmjs.com/package/@graphql-tools/utils?activeTab=versions
- 9: https://www.npmjs.com/package/@graphql-tools/utils
- 10: https://community.broadcom.com/communities/community-home/digestviewer?CommunityKey=94bda077-625b-4914-8ac3-c88a06c2cc23
Action: Manual Verification for @graphql-tools/utils 10.8.1 Compatibility
Based on the automated checks:
- The GitHub issue and discussions queries did not yield clear results due to command output errors.
- The web query for changes between 10.7.0 and 10.8.1 returned no documented changes (with public changelogs only covering up to 10.7.x).
At this point, while no obvious breaking changes or incompatibilities have been identified, the absence of documented differences for 10.8.1 means we cannot conclusively verify compatibility. It’s recommended to perform further manual testing to ensure that the updated dependency works as expected in your codebase.
.changeset/@graphql-tools_executor-http-532-dependencies.md (1)
1-8: Verify compatibility with the updated @graphql-tools/utils version.The dependency update to
@graphql-tools/utils@^10.8.1across multiple packages appears to be part of the OpenTelemetry context improvements. Let's verify the compatibility and potential breaking changes.Also applies to: 1-8, 1-8, 1-8, 1-8
✅ Verification successful
🌐 Web query:
What are the changes in @graphql-tools/utils between versions 10.7.0 and 10.8.1?💡 Result:
Based on the provided search results, there are no documented changes for
@graphql-tools/utilsbetween versions 10.7.0 and 10.8.1 in the available sources.The latest version explicitly mentioned in the sources is 10.7.2, which includes:
- A security patch for CVE-2024-21529 via
dset@^3.1.4dependency update[4]- Removal of
VoidFunctiontype usage to avoid DOM type dependencies[4]Version 10.7.x series introduced:
- New helpers for
AbortSignalhandling (getAbortPromise,registerAbortSignalListener)[4]- Memory leak prevention for Node.js event listeners[4]
The search results don't contain any information about versions 10.8.x. The most recent update shown in sources [8][9] indicates 10.7.2 was published ~17 days ago (as of February 11, 2025), suggesting 10.8.x might be unreleased or undocumented in these resources.
For authoritative information about unreleased versions, you would need to check official release notes or repository changelogs not included in these search results[6][9].
Citations:
- 1: https://github.com/ardatan/graphql-tools/blob/master/packages/load/CHANGELOG.md
- 2: https://gitlab.com/gitlab-org/gitlab/-/blob/a8325c425/CHANGELOG.md
- 3: https://the-guild.dev/graphql/tools/changelogs/load
- 4: https://the-guild.dev/graphql/tools/changelogs/utils
- 5: https://github.com/ardatan/graphql-tools/blob/master/packages/stitch/CHANGELOG.md
- 6: https://github.com/ardatan/graphql-tools/releases
- 7: https://the-guild.dev/graphql/tools/changelogs/loaders/graphql-file
- 8: https://www.npmjs.com/package/@graphql-tools/utils
- 9: https://www.npmjs.com/package/@graphql-tools/utils?activeTab=versions
- 10: https://classic.yarnpkg.com/en/package/@graphql-tools/utils
Compatibility Check: No breaking changes detected
Based on available sources and the absence of documented changes between versions 10.7.0 and 10.8.1, it appears that the upgrade is a non-breaking patch update. The only noted changes in the 10.7.x series (up to 10.7.2) involve security and minor improvements (such as abort signal handling and memory leak prevention). Since there isn’t any published changelog for a version 10.8.x, it is likely that this update was introduced as part of the OpenTelemetry context improvements without introducing breaking changes.
- No documented changes or breaking updates were found for version 10.8.1.
- The update aligns with patch-level improvements observed in previous versions.
- It is recommended to run your integration tests to verify that no unexpected issues arise as a result of this dependency update.
.changeset/@graphql-tools_stitching-directives-532-dependencies.md (1)
1-8: Verify compatibility with the updated @graphql-tools/utils version.The update to
@graphql-tools/utils@^10.8.1is part of a coordinated update across multiple packages. While marked as a patch, it's good practice to verify compatibility.✅ Verification successful
🌐 Web query:
What are the changes between @graphql-tools/utils version 10.7.0 and 10.8.1?💡 Result:
The key changes between
@graphql-tools/utilsversions 10.7.* and 10.* include security fixes, new utilities for abort signals, and TypeScript improvements:Minor Changes
Version .:**
Added two new helper functions:
getAbortPromise(signal): Creates a promise rejected when anAbortSignaltriggers.registerAbortSignalListener(signal): Centralizes signal listeners to avoid Node.jsMaxListenersExceededWarningerrors[^search_1][^search_9].
Patch Changes
Version .:**
- Removed
VoidFunctiontype usage due to DOM type dependencies[^search_1].
Version .:**- Updated
dsetdependency (^3.*.*) to resolve CVE-2024-21529[^search_1][^search_9].
Version .:**- Internal dependency cleanup with no breaking changes[^search_9].
Security Fixes
All patches after ..* include updates addressing CVE-2024-21529, ensuring safer dependency handling[^search_9].
For detailed dependency ranges or migration guidance, refer to the official changelogs[^search_1][^search_9].
Citations:
- 1: https://the-guild.dev/graphql/tools/changelogs/utils
- 2: https://the-guild.dev/graphql/tools/changelogs/links
- 3: https://github.com/ardatan/graphql-tools/blob/master/packages/load/CHANGELOG.md
- 4: https://test-gsx.cisco.com/c/dam/en_us/about/doing_business/open_source/docs/AppDynamics_Cloud_NextUI-22120-1670618719.pdf
- 5: https://the-guild.dev/graphql/tools/changelogs/executors/yoga
- 6: https://www.cisco.com/c/dam/en_us/about/doing_business/open_source/docs/AppDynamics_Cloud_NextUI-2280-1658894610.pdf
- 7: https://github.com/ardatan/graphql-tools/releases
- 8: https://www.npmjs.com/package/@graphql-tools/utils?activeTab=versions
- 9: https://www.npmjs.com/package/@graphql-tools/utils
- 10: https://community.broadcom.com/communities/community-home/digestviewer?CommunityKey=94bda077-625b-4914-8ac3-c88a06c2cc23
Compatibility with @graphql-tools/utils v10.8.1 verified.
The changelog indicates that the update from v10.7.0 to v10.8.1 involves security fixes, introduces new helper functions, and makes internal dependency cleanups without breaking changes. The patch update aligns with a non-breaking, backward-compatible enhancement.
- Confirmed that the changes (e.g., removal of
VoidFunction, addition of abort signal helpers) are minor and non-disruptive.- No API breakage or incompatibilities have been reported.
.changeset/@graphql-mesh_transport-http-callback-532-dependencies.md (1)
1-8: LGTM! Consistent with other package updates.The update to
@graphql-tools/utils@^10.8.1maintains consistency with other packages in the ecosystem..changeset/@graphql-mesh_hmac-upstream-signature-532-dependencies.md (1)
1-8: LGTM! Consistent with other package updates.The update to
@graphql-tools/utils@^10.8.1maintains consistency with other packages in the ecosystem..changeset/@graphql-hive_gateway-runtime-532-dependencies.md (1)
1-9: Verify compatibility with the updated @graphql-hive/core version.While the update to
@graphql-tools/utils@^10.8.1is consistent with other packages, the minor version bump of@graphql-hive/coreto^0.9.0warrants verification of compatibility and potential new features.❌ Verification inconclusive
🌐 Web query:
What are the changes in @graphql-hive/core version 0.9.0 compared to 0.8.1?💡 Result:
Based on the provided search results, there are no specific details about changes in @graphql-hive/core versions
0.9.0or0.8.1. The search results primarily cover unrelated packages like DataHub updates ([1]), GraphiQL ([2]), Flutter GraphQL ([3]), Yoga Server plugins ([4][5]), and other repositories ([6][8][9][10]).The npm registry entry for @graphql-hive/core ([7]) lists
v0.8.4as the latest stable version (published 11 days ago) and mentions an alpha build (v0.9.0-alpha) dated November 11, 2024, but no changelog details are included in the search results.To answer this query accurately, consult the official @graphql-hive/core changelog or GitHub releases page directly, as no relevant data exists in the provided sources[7][9].
Citations:
- 1: https://datahubproject.io/docs/how/updating-datahub/
- 2: https://github.com/graphql/graphiql/blob/main/packages/graphiql/CHANGELOG.md
- 3: https://pub.dev/packages/graphql_flutter/changelog
- 4: https://the-guild.dev/graphql/yoga-server/changelogs/plugins/apollo-inline-trace
- 5: https://the-guild.dev/graphql/yoga-server/changelogs/graphql-yoga
- 6: https://github.com/graphprotocol/graph-client/blob/main/yarn.lock
- 7: https://www.npmjs.com/package/@graphql-hive/core/v/0.9.0-alpha-20241111130335-031581e4a5960befb953425e198b6ddbbf8b7d3e?activeTab=versions
- 8: https://formulae.brew.sh/formula/
- 9: https://github.com/graphql-hive/console/releases
- 10: https://gitweb.gentoo.org/repo/sync/gentoo.git/plain/metadata/pkg_desc_index?h=stable
Compatibility Check for @graphql-hive/core Update
The web search did not return clear changelog or compatibility details for the update from 0.8.1 to 0.9.0. Notably, the npm registry suggests that 0.8.4 is the latest stable version and indicates an alpha release for 0.9.0 without providing a detailed changelog.
Please verify compatibility by reviewing the official GitHub releases or changelog for @graphql-hive/core and testing in a controlled environment to ensure that any new features or changes do not adversely affect the codebase.
.changeset/@graphql-mesh_transport-common-532-dependencies.md (1)
1-8: Verify the impact of @graphql-tools/utils version update.The coordinated update of
@graphql-tools/utilsfrom^10.7.0to^10.8.1across multiple packages suggests a maintenance effort to keep dependencies in sync. Given this PR's focus on OpenTelemetry context handling, let's verify the changes in this version update.Also applies to: 1-8, 1-8, 1-8, 1-8
✅ Verification successful
🌐 Web query:
What are the changes in @graphql-tools/utils between versions 10.7.0 and 10.8.1?💡 Result:
The changes in
@graphql-tools/utilsbetween versions 10.7.0 and 10.8.1 include:Version 10.8.x
10.8.1:
- Patch update with unspecified internal fixes (no detailed changelog provided)[4].
10.8.0:
- Minor update with unspecified enhancements (no detailed changelog provided)[4].
Version 10.7.x
10.7.2:
- Security: Updated
dsetdependency to^3.1.4to address CVE-2024-21529[4].10.7.1:
- Removed use of
VoidFunctiontype to eliminate dependency on DOM types[4].10.7.0:
- Added helper functions for
AbortSignalhandling:
getAbortPromise: Returns a promise rejected when anAbortSignalis aborted.registerAbortSignalListener: Registers listeners to avoidMaxListenersExceededWarningin Node.js[4].
For precise details beyond these summaries, consult the official changelog[4].
Citations:
- 1: https://github.com/ardatan/graphql-tools/blob/master/packages/load/CHANGELOG.md
- 2: https://the-guild.dev/graphql/tools/changelogs/executors/yoga
- 3: https://the-guild.dev/graphql/tools/changelogs/load
- 4: https://the-guild.dev/graphql/tools/changelogs/utils
- 5: https://gitlab.com/gitlab-org/gitlab/-/blob/a8325c425/CHANGELOG.md
- 6: https://github.com/ardatan/graphql-tools/blob/master/packages/stitch/CHANGELOG.md
- 7: https://the-guild.dev/graphql/tools/changelogs/loaders/graphql-file
- 8: https://cve.mitre.org/cgi-bin/cvekey.cgi
- 9: https://github.com/ardatan/graphql-tools/releases
- 10: https://www.npmjs.com/package/@graphql-tools/utils
Dependency update impact review: @graphql-tools/utils version update verified
Based on the published changelog:
- Version 10.8.1/10.8.0: Introduce internal fixes and minor enhancements without any shifting in public API behavior.
- Version 10.7.x: Includes a security update (dependency on
dset) and minor adjustments (removal ofVoidFunctionin 10.7.1, added AbortSignal helpers in 10.7.0).- There are no breaking or functionality changes that would impact the OpenTelemetry context handling or other parts of the codebase relying on
@graphql-tools/utils.Thus, the update from
^10.7.0to^10.8.1is safe and does not introduce adverse effects on our project..changeset/@graphql-tools_executor-common-532-dependencies.md (1)
1-8: Verify the compatibility of @graphql-tools/utils update.Let's verify the version update and check for any breaking changes that might affect the OpenTelemetry context improvements.
✅ Verification successful
🌐 Web query:
What are the changes in @graphql-tools/utils between versions 10.7.0 and 10.8.1?💡 Result:
Summary of changes in
@graphql-tools/utilsfrom v10.7.0 to v10.8.1:Minor Changes (v10.7.0)
- Introduced new helper functions:
getAbortPromise: Creates a promise rejected when anAbortSignalis aborted.registerAbortSignalListener: Registers a listener to abort promises on signal abortion, preventingMaxListenersExceededWarningin Node.js[4].Patch Changes
- v10.7.1: Removed
VoidFunctiontype usage to eliminate dependency on DOM types[4].- v10.7.2: Updated
dsetdependency to v3.1.4 to address CVE-2024-21529[4].- v10.8.1: General maintenance with unspecified bug fixes or optimizations[4].
No breaking changes were documented between these versions[4][9][10].
Citations:
- 1: https://github.com/ardatan/graphql-tools/blob/master/packages/load/CHANGELOG.md
- 2: https://the-guild.dev/graphql/tools/changelogs/executors/yoga
- 3: https://the-guild.dev/graphql/tools/changelogs/load
- 4: https://the-guild.dev/graphql/tools/changelogs/utils
- 5: https://gitlab.com/gitlab-org/gitlab/-/blob/a8325c425/CHANGELOG.md
- 6: https://github.com/ardatan/graphql-tools/blob/master/packages/stitch/CHANGELOG.md
- 7: https://the-guild.dev/graphql/tools/changelogs/loaders/graphql-file
- 8: https://github.com/ardatan/graphql-tools/releases
- 9: https://www.npmjs.com/package/@graphql-tools/utils
- 10: https://www.npmjs.com/package/@graphql-tools/utils?activeTab=versions
Compatibility of @graphql-tools/utils Update Verified:
The version update from 10.7.0 to 10.8.1 involves bug fixes, minor maintenance updates, and dependency security improvements (e.g., updatingdsetto address CVE-2024-21529). There are no documented breaking changes between these versions, which indicates that this patch update should be compatible with and support the OpenTelemetry context improvements..changeset/@graphql-mesh_plugin-prometheus-532-dependencies.md (1)
1-8: LGTM! Consistent dependency update.The update to @graphql-tools/utils is consistent with other packages, maintaining compatibility across the project.
.changeset/@graphql-tools_executor-graphql-ws-532-dependencies.md (1)
1-8: LGTM! Consistent dependency update across packages.The update to @graphql-tools/utils is consistently applied across all packages, which is good practice for maintaining compatibility. This coordinated update suggests careful consideration of cross-package dependencies.
Let's verify the impact of this dependency update on the OpenTelemetry context improvements:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for OpenTelemetry-related changes that might be affected by @graphql-tools/utils rg -l "opentelemetry|otel|span|trace" | xargs rg "@graphql-tools/utils"Length of output: 98593
Dependency update verified – no adverse impact on OpenTelemetry improvements.
The search results confirm that files in our OpenTelemetry plugin (and related files) continue to reference and use the functionality from
@graphql-tools/utilswithout issues. Although many packages still declare versions like^10.7.0or related variants in lockfiles and changelogs, there is no indication that updating the executor’s dependency to^10.8.1affects OpenTelemetry-related features. The coordinated update appears consistent and does not break any OpenTelemetry contexts..changeset/@graphql-tools_batch-delegate-532-dependencies.md (1)
1-8: Verify compatibility with the updated@graphql-tools/utilsversion.The coordinated update of
@graphql-tools/utilsfrom^10.7.0to^10.8.1across multiple packages looks good. However, let's verify the changelog for any breaking changes and ensure compatibility with the OpenTelemetry context improvements.Also applies to: 1-8, 1-8, 1-8
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for potential compatibility issues with the updated version # Check if any package.json files still reference the old version echo "Checking for potential version conflicts..." rg -g "package.json" "\"@graphql-tools/utils\": \"\\^10\\.7\\.0\"" # Look for any imports of specific utilities that might have changed echo "Checking utility imports..." rg "from '@graphql-tools/utils'"Length of output: 20485
🌐 Web query:
What are the changes in @graphql-tools/utils between versions 10.7.0 and 10.8.1?💡 Result:
Here are the key changes in
@graphql-tools/utilsbetween versions 10.7.0 and 10.8.1 based on the official changelogs[4]:Version 10.8.x
- 10.8.1: Patch update with unspecified general improvements[4]
- 10.8.0: Minor release with unspecified minor changes[4]
Version 10.7.x
- 10.7.2:
- Security: Bumped
dsetdependency to address CVE-2024-21529[4]- 10.7.1:
- Removed use of
VoidFunctiontype to fix TypeScript compatibility issues requiring DOM types[4]- 10.7:
- Added new helper functions:
getAbortPromise: Creates promise rejection onAbortSignalabortregisterAbortSignalListener: Prevents memory leaks by consolidating signal listeners[4]There are no documented breaking changes between these versions based on available information[4]. The updates focus on security improvements, TypeScript compatibility fixes, and new utilities for handling abort signals in asynchronous operations.
For detailed dependency updates and implementation specifics, refer to the official changelogs[4].
Citations:
- 1: https://github.com/ardatan/graphql-tools/blob/master/packages/load/CHANGELOG.md
- 2: https://gitlab.com/gitlab-org/gitlab/-/blob/a8325c425/CHANGELOG.md
- 3: https://the-guild.dev/graphql/tools/changelogs/load
- 4: https://the-guild.dev/graphql/tools/changelogs/utils
- 5: https://github.com/ardatan/graphql-tools/blob/master/packages/stitch/CHANGELOG.md
- 6: https://the-guild.dev/graphql/tools/changelogs/loaders/graphql-file
- 7: https://github.com/ardatan/graphql-tools/releases
- 8: https://www.npmjs.com/package/@graphql-tools/utils
- 9: https://www.npmjs.com/package/@graphql-tools/utils?activeTab=versions
- 10: https://classic.yarnpkg.com/en/package/@graphql-tools/utils
Compatibility Verified: No breaking changes detected in @graphql-tools/utils 10.8.1 update.
- The official changelog indicates that the changes between 10.7.x and 10.8.1 are limited to security fixes, TypeScript compatibility improvements, and new utilities (e.g., handling abort signals), with no breaking changes.
- Code usage—including OpenTelemetry-related imports—remains compatible, as evidenced by the absence of modified or removed APIs.
- While many packages still reference version ^10.7.0 in their package.json files, the coordinated update in the targeted package aligns with non-breaking adjustments.
EmrysMyrddin
force-pushed
the
fix/otel/span-contexts
branch
from
0389c26 to
daed24a
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
packages/plugins/opentelemetry/src/plugin-utils.ts (1)
1-105: 🧹 Nitpick (assertive)Missing changeset and lack of “GW-*” reference in PR description.
Because this file resides in
packages/plugins/**, the coding guidelines require a changeset (yarn changeset) describing these new features or fixes. Also, the PR references GitHub issues (#516, #514, etc.), but doesn’t appear to mention a Linear issue in the format “GW-*”.Do you need help creating a changeset or linking the relevant linear issue(s)?
♻️ Duplicate comments (2)
e2e/opentelemetry/opentelemetry.e2e.ts (1)
570-570: 🛠️ Refactor suggestionEliminate the non-null assertion.
Use optional chaining or a conditional check instead of the!to avoid unforeseen runtime issues.- const spanTree = buildSpanTree(relevantTrace!.spans, 'POST /graphql'); + const spanTree = relevantTrace + ? buildSpanTree(relevantTrace.spans, 'POST /graphql') + : undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 570-570: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
packages/plugins/opentelemetry/src/utils.ts (1)
1-1: 🛠️ Refactor suggestionUse the Node.js import protocol.
When importing a Node builtin module, prefix it withnode:for clarity.- import { isPromise } from 'util/types'; + import { isPromise } from 'node:util/types';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
.changeset/@graphql-mesh_plugin-opentelemetry-532-dependencies.md(1 hunks)e2e/opentelemetry/opentelemetry.e2e.ts(4 hunks)package.json(1 hunks)packages/plugins/opentelemetry/package.json(2 hunks)packages/plugins/opentelemetry/src/context.ts(1 hunks)packages/plugins/opentelemetry/src/plugin-utils.ts(1 hunks)packages/plugins/opentelemetry/src/plugin.ts(4 hunks)packages/plugins/opentelemetry/src/spans.ts(5 hunks)packages/plugins/opentelemetry/src/utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
package.jsonpackages/plugins/opentelemetry/package.jsonpackages/plugins/opentelemetry/src/utils.tspackages/plugins/opentelemetry/src/context.tse2e/opentelemetry/opentelemetry.e2e.tspackages/plugins/opentelemetry/src/plugin.tspackages/plugins/opentelemetry/src/plugin-utils.tspackages/plugins/opentelemetry/src/spans.ts
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/plugins/opentelemetry/package.jsonpackages/plugins/opentelemetry/src/utils.tspackages/plugins/opentelemetry/src/context.tspackages/plugins/opentelemetry/src/plugin.tspackages/plugins/opentelemetry/src/plugin-utils.tspackages/plugins/opentelemetry/src/spans.ts
`packages/plugins/**`: The directories inside this dir are g...
packages/plugins/**: The directories inside this dir are gateway plugins. Plugins adds functionality to the GraphQL gateway, at the runtime level. Plugins included are authorization, authentication, caching, persisted documents and more.
packages/plugins/opentelemetry/package.jsonpackages/plugins/opentelemetry/src/utils.tspackages/plugins/opentelemetry/src/context.tspackages/plugins/opentelemetry/src/plugin.tspackages/plugins/opentelemetry/src/plugin-utils.tspackages/plugins/opentelemetry/src/spans.ts
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/opentelemetry/opentelemetry.e2e.ts
🪛 Biome (1.9.4)
packages/plugins/opentelemetry/src/utils.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
[error] 27-27: Do not add then to an object.
(lint/suspicious/noThenProperty)
e2e/opentelemetry/opentelemetry.e2e.ts
[error] 568-568: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 570-570: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 578-578: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 599-599: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
packages/plugins/opentelemetry/src/plugin.ts
[error] 338-338: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 370-370: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 374-374: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 394-394: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 400-400: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 420-420: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 426-426: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 440-440: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 451-451: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 458-458: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 492-492: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 249-249: This let declares a variable that is only assigned once.
'contextManager$' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
⏰ Context from checks skipped due to timeout of 90000ms (44)
- GitHub Check: Examples / Convert operation-field-permissions
- GitHub Check: Examples / Convert type-merging-batching
- GitHub Check: Examples / Convert subscriptions-with-transforms
- GitHub Check: Examples / Convert programmatic-batching
- GitHub Check: Examples / Convert openapi-subscriptions
- GitHub Check: Examples / Convert openapi-javascript-wiki
- GitHub Check: Examples / Convert openapi-arg-rename
- GitHub Check: Examples / Convert openapi-additional-resolvers
- GitHub Check: Examples / Convert json-schema-subscriptions
- GitHub Check: Examples / Convert interface-additional-resolvers
- GitHub Check: Examples / Convert hmac-auth-https
- GitHub Check: Examples / Convert federation-subscriptions-passthrough
- GitHub Check: Examples / Convert apq-subgraphs
- GitHub Check: Examples / Convert file-upload
- GitHub Check: Examples / Convert federation-mixed
- GitHub Check: Examples / Convert federation-example
- GitHub Check: Examples / Convert extra-fields
- GitHub Check: Binary built on windows-latest
- GitHub Check: Binary built on macos-14
- GitHub Check: Binary built on macos-13
- GitHub Check: Bun Docker image
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: Node Docker image
- GitHub Check: Leaks / Node v23
- GitHub Check: Unit / Bun
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Bundle
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Benchmark / node / 10 items
🔇 Additional comments (16)
packages/plugins/opentelemetry/src/plugin-utils.ts (2)
9-25: Good use of WeakMap for state references.Leveraging WeakMaps and generating property descriptors dynamically is a clever approach that helps prevent leaks and manages state per request/operation. The logic here looks clean and straightforward.
27-53: Potential overshadowing ofcontextandrequestvariables.Inside the hook invocation, reassigning
contexttoexecutionRequest.contextandrequesttocontext.requestmight lead to unexpected behavior if these fields differ. Confirm that overshadowing is intended and won't cause future confusion.packages/plugins/opentelemetry/src/context.ts (1)
46-69: Module resolution safety.The error handling in
getContextManagercorrectly distinguishes between missing module imports (ERR_MODULE_NOT_FOUND) and other errors. This ensures the code gracefully skips the context manager if the package is unavailable. The logic looks solid for Node usage.packages/plugins/opentelemetry/src/spans.ts (1)
234-236: Iterate array items, not properties.The previous review comment remains relevant:
Usefor (const error of result)instead offor (const error in result)to iterate array items directly:- for (const error in result) { - span.recordException(error); - } + for (const error of result) { + span.recordException(error); + }e2e/opentelemetry/opentelemetry.e2e.ts (6)
27-37: Consider enumeratingrefType.
A previous review already suggested using an enum or string literal union forrefTypeinstead of a raw string, both for better type safety and self-documentation.
83-85: Verifyerris correctly assigned before throwing.
If the loop doesn’t seterrfor some reason, throwing an uninitialized variable could cause confusion. Consider ensuringerris never undefined.
568-568: Remove the non-null assertion.
A prior review comment already highlighted this. Replace!with optional chaining or a safeguard check to prevent runtime errors.- expect(relevantTrace!.spans.length).toBe(18); + expect(relevantTrace?.spans?.length).toBe(18);🧰 Tools
🪛 Biome (1.9.4)
[error] 568-568: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
578-578: Avoid the non-null assertion operator.
Using!is risky in tests. Either perform a truthy check or use optional chaining.- expect(spanTree!.children).toHaveLength(3); + expect(spanTree?.children).toHaveLength(3);🧰 Tools
🪛 Biome (1.9.4)
[error] 578-578: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
599-599: Remove the non-null assertion onexecuteSpan.
As with the other lines, consider optional chains or explicit checks to improve safety.- const matchingChildren = executeSpan!.children.filter( + const matchingChildren = executeSpan?.children.filter(🧰 Tools
🪛 Biome (1.9.4)
[error] 599-599: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
1300-1324: Prevent infinite recursion on cyclical references.
A past comment already noted the possibility of cyclical references causing unbounded traversal. Track visited spans or implement a depth guard.packages/plugins/opentelemetry/src/utils.ts (1)
27-27: Avoid manually attaching then/catch/finally to a plain object.
This re-implements the Promise interface and can be fragile. Consider directly wrapping the input in a realnew Promise(...).🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: Do not add then to an object.
(lint/suspicious/noThenProperty)
.changeset/@graphql-mesh_plugin-opentelemetry-532-dependencies.md (1)
1-8: Confirm whether this should be a minor update.
You are adding a new dependency (@opentelemetry/context-async-hooks) which could be considered a new feature. If it’s purely a bug fix, a patch is fine; otherwise, consider changing to aminorbump.packages/plugins/opentelemetry/package.json (2)
53-53: LGTM! Addition of @opentelemetry/context-async-hooks is appropriate.The addition of this dependency aligns with the PR's goal of improving span parenting in OpenTelemetry. This package provides essential functionality for managing context across async boundaries.
66-66: Dev dependency @whatwg-node/server was previously reviewed.package.json (2)
1-83:✅ Verification successful
Missing changeset file for dependency changes.
According to the coding guidelines, a changeset file is required for dependency changes. Please create one using
yarn changesetto document these changes.
🏁 Script executed:
#!/bin/bash # Check for existing changeset files related to this PR fd -e md . .changeset/ -x cat {} \; | grep -i "opentelemetry\|envelop"Length of output: 283
Changeset file confirmed for dependency updates.
The verification shows that a changeset file already exists in the
.changesetdirectory, which documents the dependency update (e.g., for opentelemetry). No further changeset is required.
65-65:✅ Verification successful
Verify the @envelop/core alpha version.
The specified alpha version contains a future date (2025-02-06) which is unusual and potentially problematic. This could lead to stability issues or version resolution problems.
🏁 Script executed:
#!/bin/bash # Check if this version exists in npm registry curl -s https://registry.npmjs.org/@envelop/core | jq '.versions | keys[]' | grep "5.1.0-alpha" # Check for any existing version constraints in the codebase rg -l "@envelop/core.*[0-9]" --type jsonLength of output: 3432
Alpha Version Verified: The Future Date in the Alpha Version is Intentional
- The npm registry shows multiple alpha versions for @envelop/core following the same pattern, including the one in question.
- The inclusion of a future date (2025-02-06) is part of the systematic versioning used for alpha releases and does not indicate a stability issue.
- No further changes are required in the version specification within package.json.
| export function withState< | ||
| P, | ||
| GraphqlState = object, | ||
| HttpState = object, | ||
| SubExecState = object, | ||
| >(plugin: WithState<P, HttpState, GraphqlState, SubExecState>): P { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mismatch in generic parameter order between withState and WithState.
The function signature <P, GraphqlState, HttpState, SubExecState> in withState is passing its type parameters to WithState<P, HttpState, GraphqlState, SubExecState>, but the WithState type is declared as <P, Http, GraphQL, Gateway>. This creates a mismatch in naming and order (GraphQL vs Http vs Gateway). Aligning them would improve readability and prevent confusion.
Also applies to: 100-104
| export class OtelContextStack { | ||
| #root: Node; | ||
| #current: Node; | ||
|
|
||
| constructor(root: Context) { | ||
| this.#root = { ctx: root }; | ||
| this.#current = this.#root; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Constructor usage clarity.
The constructor initializes both #root and #current to the same node, which makes sense. However, consider adding explicit JSDoc or comments stating the expectation that root remains constant throughout the lifecycle, so future maintainers easily understand the intended usage of #root.
| push = (ctx: Context) => { | ||
| this.#current = { ctx, previous: this.#current }; | ||
| }; | ||
|
|
||
| pop = () => { | ||
| this.#current = this.#current.previous ?? this.#root; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Potential nested context clarity.
Pushing and popping contexts on the stack is straightforward. However, be cautious when debugging nested contexts. Consider adding trace logs whenever you push/pop in order to clarify transitions for complex interactions.
| toString() { | ||
| let node: Node | undefined = this.#current; | ||
| const names = []; | ||
| while (node != undefined) { | ||
| names.push((trace.getSpan(node.ctx) as unknown as { name: string }).name); | ||
| node = node.previous; | ||
| } | ||
| return names.join(' -> '); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance span naming safety.
When calling trace.getSpan(node.ctx), assume potential null or unknown objects. Consider adding null checks or fallback logic if a context has no associated span, to avoid potential run-time errors in .name.
while (node != undefined) {
- names.push((trace.getSpan(node.ctx) as unknown as { name: string }).name);
+ const span = trace.getSpan(node.ctx);
+ if (span) {
+ names.push((span as { name: string }).name ?? 'unknown-span');
+ }
node = node.previous;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| toString() { | |
| let node: Node | undefined = this.#current; | |
| const names = []; | |
| while (node != undefined) { | |
| names.push((trace.getSpan(node.ctx) as unknown as { name: string }).name); | |
| node = node.previous; | |
| } | |
| return names.join(' -> '); | |
| } | |
| toString() { | |
| let node: Node | undefined = this.#current; | |
| const names = []; | |
| while (node != undefined) { | |
| const span = trace.getSpan(node.ctx); | |
| if (span) { | |
| names.push((span as { name: string }).name ?? 'unknown-span'); | |
| } | |
| node = node.previous; | |
| } | |
| return names.join(' -> '); | |
| } |
| isAsyncIterable, | ||
| mapMaybePromise, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Future streaming support reminder.
A isAsyncIterable usage appears, but there's a “FIXME” comment about handling async iterable results. Consider completing this feature to capture streamed partial results, especially if real-time or subscription-based operations are in scope.
🛠️ Refactor suggestion
Instrumenting async iterable results.
There are multiple FIXME references suggesting incomplete instrumentation for streaming responses. Consider implementing a strategy that properly records span lifecycles, particularly for partial or incremental GraphQL responses, so spans end correctly upon completing data streams.
Also applies to: 8-9, 114-117
| export function startGraphQLSpan(input: { | ||
| ctx: Context; | ||
| tracer: Tracer; | ||
| params: GraphQLParams; | ||
| }): { | ||
| ctx: Context; | ||
| done: ( | ||
| result: MaybePromise<ExecutionResult | AsyncIterable<ExecutionResult>>, | ||
| ) => void; | ||
| } { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure error capturing in startGraphQLSpan.
When exceptions arise during mapMaybePromise, the code sets span status to ERROR and records the exception. This is good practice to diagnose failures. Consider adding a fallback attribute for unknown error shapes (e.g., if err.message is missing).
| forOperation.otel! = new OtelContextStack(ctx); | ||
|
|
There was a problem hiding this comment.
Avoid non-null assertions.
Multiple lines use ! when accessing forOperation.otel! or forSubgraphExecution.otel!. This could trigger a runtime error if otel is unexpectedly undefined. Switch to optional chaining (?.) or add checks to ensure safer code.
- forOperation.otel! = new OtelContextStack(ctx);
+ if (forOperation.otel) {
+ forOperation.otel = new OtelContextStack(ctx);
+ }Also applies to: 370-371, 374-375, 394-395, 400-401, 420-421, 426-427, 440-441, 451-452, 458-459, 492-493
🧰 Tools
🪛 Biome (1.9.4)
[error] 338-338: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| http?: BooleanOrPredicate<OnRequestEventPayload<any>>; | ||
| /** | ||
| * Enable/disable GraphQL operation spans (default: true). | ||
| */ | ||
| graphql?: BooleanOrPredicate<OnParamsEventPayload<any>>; | ||
| /** |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Refine selective tracing with typed predicates.
The shouldTrace checks offer a flexible way to conditionally create spans. Consider adding typed sample usage or JSDoc examples, guiding developers on passing boolean vs. function-based predicates. This helps future maintainers quickly adapt the function for advanced filtering.
Also applies to: 148-150, 154-155, 158-159, 162-163, 166-167, 170-171
| export function useOpenTelemetry( | ||
| options: OpenTelemetryGatewayPluginOptions & { logger: Logger }, | ||
| ): GatewayPlugin<OpenTelemetryContextExtension> { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Link a Linear issue and include a changeset.
The code modifies a plugin in packages/plugins/, but there's no mention of a “GW-” issue or a changeset file. According to the coding guidelines, please:
- Link a corresponding Linear ticket (e.g., “GW-123”) or confirm none is needed.
- Add a changeset (
yarn changeset) specifying the semver bump and describing functionality changes or bug fixes.
| asyncAttributes, | ||
| ); | ||
|
|
||
| let contextManager$ = getContextManager( |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use const for single-assignment variable.
A static analysis warning states that contextManager$ is never reassigned. Prefer const to communicate immutability and reduce accidental reassignments.
- let contextManager$ = getContextManager(
+ const contextManager$ = getContextManager(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let contextManager$ = getContextManager( | |
| const contextManager$ = getContextManager( |
🧰 Tools
🪛 Biome (1.9.4)
[error] 249-249: This let declares a variable that is only assigned once.
'contextManager$' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/opentelemetry/tests/useOpenTelemetry.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/plugins/opentelemetry/tests/useOpenTelemetry.spec.ts
`packages/plugins/**`: The directories inside this dir are g...
packages/plugins/**: The directories inside this dir are gateway plugins. Plugins adds functionality to the GraphQL gateway, at the runtime level. Plugins included are authorization, authentication, caching, persisted documents and more.
packages/plugins/opentelemetry/tests/useOpenTelemetry.spec.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/plugins/opentelemetry/tests/useOpenTelemetry.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/plugins/opentelemetry/tests/useOpenTelemetry.spec.ts (2)
1-1: Ensure changeset and linear issue reference are included.According to the coding guidelines for
packages/**, a corresponding changeset file should accompany these modifications. Also, there's no explicit reference to a "GW-*" Linear issue in the PR description. Please confirm or rectify these omissions.
2-498: Overall implementation looks solid!No other critical issues found. The test setup is thorough, verifying different tracing configurations and ensuring correct span parenting and naming.
| allExpectedSpans.forEach(spanExporter.assertNoSpanWithName); | ||
| }); | ||
|
|
||
| it('should not trace graphql operation if disable', async () => { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix minor grammar in test name.
Change “if disable” to “if disabled” for clarity and consistency.
- it('should not trace graphql operation if disable', async () => {
+ it('should not trace graphql operation if disabled', async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should not trace graphql operation if disable', async () => { | |
| it('should not trace graphql operation if disabled', async () => { |
| afterAll(async () => { | ||
| traceProvider.shutdown(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Await the asynchronous shutdown for cleaner test teardown.
traceProvider.shutdown() returns a Promise. Consider awaiting this async function call to ensure a proper teardown.
- afterAll(async () => {
- traceProvider.shutdown();
+ afterAll(async () => {
+ await traceProvider.shutdown();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| afterAll(async () => { | |
| traceProvider.shutdown(); | |
| afterAll(async () => { | |
| await traceProvider.shutdown(); | |
| }); |
Walkthrough: https://youtu.be/YqzgToNJ9wc
resolves #516
resolves #514
resolves #515
resolves #517
resolves #513