-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(deps): Bump OpenTelemetry instrumentations #18934
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
635ce99 to
6c98baf
Compare
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
…cess.arg…" This reverts commit fae3a77.
336c4f0 to
77d761a
Compare
| replace({ | ||
| preventAssignment: true, | ||
| values: { | ||
| 'process.argv0': JSON.stringify(''), // needed because otel relies on process.argv0 for the default service name, but that api is not available in the edge runtime. | ||
| }, | ||
| }), |
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.
Bug: The removal of custom bundling logic for OpenTelemetry dependencies will cause a runtime error in the Vercel Edge environment because process.argv0 references will no longer be replaced.
Severity: HIGH
Suggested Fix
Restore the custom external function in the Rollup configuration to ensure @opentelemetry/resources and @opentelemetry/sdk-trace-base are explicitly bundled, allowing the replace plugin to correctly substitute Node.js-specific code.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/vercel-edge/rollup.npm.config.mjs#L16-L21
Potential issue: The pull request removes custom Rollup configuration that was
responsible for bundling specific OpenTelemetry packages (`@opentelemetry/resources`,
`@opentelemetry/sdk-trace-base`) into the Vercel Edge build. The new default
configuration marks these packages as external. As a result, the `replace` plugin, which
substitutes Node.js-specific references like `process.argv0` with an empty string, will
no longer be applied to their code. When this code executes in the Vercel Edge runtime,
which lacks the `process` global, it will attempt to access `process.argv0` and trigger
a runtime error, likely during initialization.
Did we get this right? 👍 / 👎 to inform future reviews.
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.
We stopped bundling in the package so this isn't applying here. The issue was fixed upstream in Otel.
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| ], | ||
| }, | ||
| }), | ||
| ); |
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.
Removing bundling workaround may break Edge runtime
Medium Severity
The custom external handling that forced @opentelemetry/resources to be bundled was removed. Previously, this ensured the process.argv0 replacement applied to OTEL packages (since process.argv0 doesn't exist in Edge runtime). Now @opentelemetry/resources (a dependency) will be external and not bundled, so any process.argv0 usage within it won't be replaced. The test file build-artifacts.test.ts that verified process.argv0 doesn't appear in the build output was also deleted, removing the safety net for this Edge compatibility requirement.
|
Note: This PR also reverts #18759 because the issue was fixed upstream. |
Closes #18958 (added automatically)