Conversation
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.
|
logaretm
changed the title
logaretm
force-pushed
the
awad/js-1542-add-elysia-integration
branch
10 times, most recently
from
7d220e3 to
672ad18
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds a new @sentry/elysia package to the monorepo to provide first-party Sentry support for the Elysia framework (Bun-first), including SDK initialization, Elysia app wiring, and E2E coverage.
Changes:
- Introduces the new
@sentry/elysiapackage (build config, exports, types, README/license, unit tests). - Implements
init()(built on@sentry/bun) andwithElysia()(OTel plugin + request/error hooks + span cleanup/enrichment). - Adds a Bun + Elysia Playwright E2E test application and wires it into CI/publishing metadata.
Reviewed changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
yarn.lock |
Adds Elysia + OTel plugin dependency graph and related transitive updates. |
packages/elysia/tsconfig.types.json |
Type build config for generating .d.ts output. |
packages/elysia/tsconfig.test.json |
Test TS config (Bun types) for the new package. |
packages/elysia/tsconfig.json |
Package TS config (Bun types) for source compilation. |
packages/elysia/test/withElysia.test.ts |
Unit tests for withElysia() hook registration, header propagation, and error capturing behavior. |
packages/elysia/test/sdk.test.ts |
Unit tests for init() and getDefaultIntegrations() behavior. |
packages/elysia/src/withElysia.ts |
Core Elysia wiring: OTel plugin registration, request metadata, response header propagation, error capture, span filtering/enrichment. |
packages/elysia/src/types.ts |
Defines ElysiaOptions as BunOptions. |
packages/elysia/src/sdk.ts |
Implements init() and Bun default integration filtering. |
packages/elysia/src/index.ts |
Public API surface: re-exports from @sentry/bun plus Elysia-specific exports. |
packages/elysia/rollup.npm.config.mjs |
Rollup config for publishing build artifacts. |
packages/elysia/package.json |
New package manifest (deps/peers/scripts/exports). |
packages/elysia/README.md |
Usage documentation and links for the new SDK. |
packages/elysia/LICENSE |
Package license file. |
packages/elysia/.eslintrc.js |
Package-local ESLint configuration. |
package.json |
Adds packages/elysia workspace + new resolutions for OTel packages. |
dev-packages/e2e-tests/verdaccio-config/config.yaml |
Allows publishing @sentry/elysia to local registry for E2E tests. |
dev-packages/e2e-tests/test-applications/node-exports-test-app/scripts/consistentExports.ts |
Updates export consistency checks (ignore list additions). |
dev-packages/e2e-tests/test-applications/bun-elysia/tsconfig.json |
TS config for the new Bun+Elysia E2E app. |
dev-packages/e2e-tests/test-applications/bun-elysia/tests/transactions.test.ts |
E2E validation for transactions, route parameterization, lifecycle spans, and manual spans. |
dev-packages/e2e-tests/test-applications/bun-elysia/tests/propagation.test.ts |
E2E validation for inbound propagation headers + documented fixmes for outbound propagation gaps. |
dev-packages/e2e-tests/test-applications/bun-elysia/tests/isolation.test.ts |
Documented fixme for per-request isolation behavior. |
dev-packages/e2e-tests/test-applications/bun-elysia/tests/errors.test.ts |
E2E validation for error capture rules and request metadata inclusion. |
dev-packages/e2e-tests/test-applications/bun-elysia/start-event-proxy.mjs |
Starts local event proxy for E2E capture. |
dev-packages/e2e-tests/test-applications/bun-elysia/src/app.ts |
E2E fixture Elysia app exercising routes, errors, propagation, and concurrency. |
dev-packages/e2e-tests/test-applications/bun-elysia/playwright.config.mjs |
Playwright runner config for the new E2E app. |
dev-packages/e2e-tests/test-applications/bun-elysia/package.json |
E2E app manifest (depends on @sentry/elysia). |
dev-packages/e2e-tests/test-applications/bun-elysia/.npmrc |
Points test app to local Verdaccio. |
dev-packages/e2e-tests/test-applications/bun-elysia/.gitignore |
Ignores build output for the E2E app. |
README.md |
Adds @sentry/elysia to the list of SDKs. |
.size-limit.js |
Adjusts bundle size limits (likely due to dependency graph changes). |
.github/workflows/build.yml |
Ensures Bun is set up for the new bun-elysia E2E matrix entry. |
.github/ISSUE_TEMPLATE/bug.yml |
Adds @sentry/elysia to bug report SDK dropdown. |
.craft.yml |
Adds publishing target configuration for @sentry/elysia. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dev-packages/e2e-tests/test-applications/node-exports-test-app/scripts/consistentExports.ts
Show resolved
Hide resolved
| function getRuntime(): { name: string; version: string } { | ||
| if (typeof Bun !== 'undefined') { | ||
| return { name: 'bun', version: Bun.version }; | ||
| } | ||
|
|
||
| return { name: 'node', version: process.version }; | ||
| } | ||
|
|
||
| /** | ||
| * Initializes the Sentry Elysia SDK. | ||
| * | ||
| * @example | ||
| * ```javascript | ||
| * import * as Sentry from '@sentry/elysia'; | ||
| * | ||
| * Sentry.init({ | ||
| * dsn: '__DSN__', | ||
| * tracesSampleRate: 1.0, | ||
| * }); | ||
| * ``` | ||
| */ | ||
| export function init(userOptions: ElysiaOptions = {}): NodeClient | undefined { | ||
| applySdkMetadata(userOptions, 'elysia'); | ||
|
|
||
| const options = { | ||
| ...userOptions, | ||
| platform: 'javascript', | ||
| runtime: getRuntime(), | ||
| serverName: userOptions.serverName || global.process.env.SENTRY_NAME || os.hostname(), | ||
| }; | ||
|
|
||
| options.transport = options.transport || makeFetchTransport; | ||
|
|
||
| if (options.defaultIntegrations === undefined) { | ||
| options.defaultIntegrations = getDefaultIntegrations(options); | ||
| } | ||
|
|
||
| return initNode(options); |
There was a problem hiding this comment.
getRuntime() falls back to { name: 'node', version: process.version } when Bun is undefined, but init() still calls init from @sentry/bun. The Bun SDK’s init references Bun.version internally, so calling @sentry/elysia.init() in a non-Bun runtime will still throw at runtime. Either explicitly enforce Bun-only usage (and throw a clear error when Bun is undefined) or conditionally use a Node-compatible init implementation when running on Node.
There was a problem hiding this comment.
Might be relevant no? Not sure how it behaves actually if Elysia is called in a Node environment.
There was a problem hiding this comment.
Good catch, I haven't tried it out, I think I will fix it in the bun SDK and maybe add a node variant test for elysia just in case.
logaretm
force-pushed
the
awad/js-1542-add-elysia-integration
branch
from
19113b3 to
68051af
Compare
logaretm
force-pushed
the
awad/js-1542-add-elysia-integration
branch
from
68051af to
88952b2
Compare
dev-packages/e2e-tests/test-applications/elysia-bun/tests/errors.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| } | ||
|
|
||
| // Decide whether to keep the span | ||
| if ( | ||
| (!span.description || span.description === '<unknown>') && | ||
| span.parent_span_id && | ||
| elysiaSpanIds.has(span.parent_span_id) | ||
| ) { | ||
| continue; // filter out | ||
| } | ||
| filteredSpans.push(span); |
There was a problem hiding this comment.
Bug: The span filtering logic in beforeSendEvent assumes parent spans precede children. When >1000 spans exist, they are sorted by timestamp, breaking this assumption and causing incorrect filtering.
Severity: MEDIUM
Suggested Fix
A multi-pass approach is needed. First, iterate through all spans to collect all elysiaSpanIds. Then, perform a second pass to filter out spans whose parent_span_id is in the collected set. This ensures all parent IDs are known before any filtering decisions are made, regardless of span order.
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/elysia/src/clientHooks.ts#L44-L59
Potential issue: The `beforeSendEvent` hook iterates through spans to filter out
children of Elysia lifecycle spans. It does this by accumulating parent span IDs in a
set (`elysiaSpanIds`) and then checking if a span's `parent_span_id` is in that set.
This logic assumes parents are always processed before their children. However, when a
transaction has more than 1000 spans, the SDK sorts the `spans` array by
`start_timestamp`. This can place a child span before its parent in the array, causing
the parent ID check to fail. As a result, empty anonymous Elysia spans are not filtered
out and are incorrectly included in the final transaction event.
| app.onRequest(context => { | ||
| getIsolationScope().setSDKProcessingMetadata({ | ||
| normalizedRequest: winterCGRequestToRequestData(context.request), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Bug: The onRequest hook is missing the { as: 'global' } option, preventing it from running on routes defined after withElysia() is called.
Severity: MEDIUM
Suggested Fix
Add the { as: 'global' } option as the first argument to the app.onRequest call. This will ensure the hook is applied globally to all routes, including those defined after the plugin has been initialized. The updated call should be app.onRequest({ as: 'global' }, ...). This makes it consistent with the onAfterHandle and onError hooks.
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/elysia/src/withElysia.ts#L72-L76
Potential issue: The `onRequest` hook is initialized without the `{ as: 'global' }`
option. According to Elysia's lifecycle documentation, hooks are isolated by default and
will not be inherited by parent instances unless explicitly marked as global. Because
the standard usage pattern is to define routes after initializing the Sentry plugin via
`withElysia()`, this `onRequest` hook will not execute for those routes. This will cause
Sentry error events to be captured without crucial request context, such as the URL,
method, and headers, as the `setSDKProcessingMetadata` function will not be called for
those requests.
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
Overall I think it is ok to be shipped as for now it is alpha. I'm still worried about the modiefied .size-limit.js
| "sucrase": "getsentry/sucrase#es2020-polyfills", | ||
| "**/express/path-to-regexp": "0.1.12" | ||
| "**/express/path-to-regexp": "0.1.12", | ||
| "**/@opentelemetry/core": "2.6.0", |
There was a problem hiding this comment.
q: Since I just saw this. Why exactly was this added?
There was a problem hiding this comment.
So Elysia comes with its own openetelemetry package, which has dependencies on opentelemetry packages that didn't match ours, forcing us to pull them in and duplicating them, causing all server bundles sizes to jump 20kb+
I had to lock them down to specific versions we use today to avoid this, ideally once I add tracing channels to Elysia I will drop all of this completely.
There was a problem hiding this comment.
If this is really needed we would need to update our upgrade-otel skill to also check this part. Then we are on the safe side.
Adds a new @sentry/elysia package providing Sentry instrumentation for the Elysia framework running on Bun. Includes request/response tracing, error capturing, span filtering for lifecycle hooks, trace propagation via response headers, and comprehensive unit + e2e tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
On Node.js, the root HTTP span is created by Node's HTTP instrumentation with only the raw URL. The Elysia OTel plugin creates a child span with route info but doesn't propagate it up. This updates the root span and isolation scope with the parameterized route name in onAfterHandle (for successful requests on Node.js) and onError (for all runtimes). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Node 20 does not support running .ts files directly. Node 24 has TypeScript stripping enabled by default. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
logaretm
force-pushed
the
awad/js-1542-add-elysia-integration
branch
from
e35f523 to
1d4ea2a
Compare
Key Decisions:
@sentry/bunsince Elysia is Bun-first.bunServerIntegrationto avoid competing root spans, as Elysia creates its own.5xxand <=299, skips3xx/4xx. Aligned with Fastify. Overridable viashouldHandleError.spanEnd(still empty at that point) and strip them inbeforeSendEvent. Unless the user provides a named function, we will strip them, check the trace below as an example of a named functionlogRequestvs the stripped event handlers in other life cycle hooks.TODOs:
Closes #18956