-
Notifications
You must be signed in to change notification settings - Fork 16
feat(opentelemetry): add sampling rate #968
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes span parenting issues and improves batching support for OpenTelemetry instrumentation across multiple packages and end-to-end tests. Key changes include:
- Removing the setExecutionRequest callback in favor of directly mutating executionRequest.extensions.
- Refactoring executor wrapping and instrumentation handling in the runtime.
- Updating test expectations and configuration parameters for both OpenTelemetry and Cloudflare Workers integrations.
Reviewed Changes
Copilot reviewed 28 out of 34 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
packages/plugins/hmac-upstream-signature/src/index.ts | Removed setExecutionRequest usage and updated signature assignment logic. |
packages/fusion-runtime/src/utils.ts | Refactored executor wrapping and integrated instrumentation handling. |
packages/fusion-runtime/src/unifiedGraphManager.ts | Updated instrumentation payload type to include subgraphName. |
e2e/opentelemetry/opentelemetry.e2e.ts | Modified test expectations and adjusted timeout and error handling logic. |
e2e/opentelemetry/gateway.config.ts | Added diagnostic level and batching configuration for exporters. |
e2e/cloudflare-workers/src/index.ts | Changed runtime initialization to lazy caching for Cloudflare Workers. |
e2e/cloudflare-workers/cloudflare-workers.e2e.ts | Updated expectations and formatting of numeric literals in tests. |
DEPS_RESOLUTIONS_NOTES.md, .changeset/* | Various dependency updates and changelog documentation for breaking changes. |
Files not reviewed (6)
- .yarn/patches/@opentelemetry-exporter-trace-otlp-http-npm-0.200.0-80a44c64cd.patch: Language not supported
- .yarn/patches/@opentelemetry-otlp-exporter-base-npm-0.200.0-6fb25211c7.patch: Language not supported
- .yarn/patches/@opentelemetry-otlp-exporter-base-npm-0.56.0-ba3dc5f5c5.patch: Language not supported
- .yarn/patches/@opentelemetry-resources-npm-1.29.0-112f89f0c5.patch: Language not supported
- .yarn/patches/@opentelemetry-resources-npm-2.0.0-f20376a5f9.patch: Language not supported
- package.json: Language not supported
): Promise<void> { | ||
const url = `http://0.0.0.0:${jaeger.additionalPorts[16686]}/api/traces?service=${service}`; | ||
|
||
let res!: JaegerTracesApiResponse; | ||
let err: any; | ||
const signal = AbortSignal.timeout(15_000); | ||
const timeout = AbortSignal.timeout(15_00); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The numeric literal '15_00' may be a typo: if a 15-second timeout was intended, it should likely be '15_000' to avoid unintended shorter timeouts.
const timeout = AbortSignal.timeout(15_00); | |
const timeout = AbortSignal.timeout(15_000); |
Copilot uses AI. Check for mistakes.
🚀 Snapshot Release (
|
Package | Version | Info |
---|---|---|
@graphql-mesh/fusion-runtime |
0.11.11-alpha-e50027355a2e37471e153a27787d6392630d1724 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway |
1.13.7-alpha-e50027355a2e37471e153a27787d6392630d1724 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/nestjs |
1.0.11-alpha-e50027355a2e37471e153a27787d6392630d1724 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/plugin-aws-sigv4 |
1.0.8-alpha-e50027355a2e37471e153a27787d6392630d1724 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-jwt-auth |
1.5.4-alpha-e50027355a2e37471e153a27787d6392630d1724 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-opentelemetry |
2.0.0-alpha-e50027355a2e37471e153a27787d6392630d1724 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
1.3.43-alpha-e50027355a2e37471e153a27787d6392630d1724 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
1.8.1-alpha-e50027355a2e37471e153a27787d6392630d1724 |
npm ↗︎ unpkg ↗︎ |
🚀 Snapshot Release (Node Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared
|
🚀 Snapshot Release (Bun Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared
|
e101965
to
f246857
Compare
74dcf64
to
bf56c1c
Compare
bf56c1c
to
9a1bf25
Compare
9a1bf25
to
4c57231
Compare
5879f2b
to
0bc8a1c
Compare
4c57231
to
e500273
Compare
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Related to GW-91