Conversation
logaretm
changed the title
pino<9.10 supportpino<9.10 support
logaretm
added
Meta: Breaking
Package: node
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.
|
size-limit report 📦
|
logaretm
force-pushed
the
awad/js-1160-apm-js-collab-is-included-in-the-sentrynextjs-bundle-when
branch
from
3d7aa48 to
e0577ed
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR drops support for Pino versions before 9.10.0 to reduce bundle size, particularly for frameworks using CommonJS (like Next.js) that cannot effectively tree-shake unused dependencies. The change removes the @apm-js-collab/tracing-hooks dependency and associated injection loader infrastructure, relying solely on the native pino_asJson tracing channel introduced in Pino 9.10.0.
Changes:
- Removed
@apm-js-collab/tracing-hooksand@apm-js-collab/code-transformerdependencies - Deleted the injection loader infrastructure (
injectLoader.tsand related hooks) - Updated Pino integration to use only the native
pino_asJsontracing channel - Updated test dependencies to use Pino 9.10.0
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Removed @apm-js-collab package entries and updated pino from 9.9.4 to 9.10.0 |
| packages/node-core/src/sdk/injectLoader.ts | Deleted entire file containing injection loader infrastructure |
| packages/node-core/src/sdk/index.ts | Removed GLOBAL_OBJ import and call to inject loader hook registration |
| packages/node-core/src/integrations/pino.ts | Removed addInstrumentationConfig import and usage, removed injectedChannel subscription for pino<9.10 |
| packages/node-core/package.json | Removed @apm-js-collab/tracing-hooks dependency and @apm-js-collab/code-transformer devDependency |
| packages/core/src/utils/worldwide.ts | Removed _sentryInjectLoaderHookRegister and _sentryInjectLoaderHookRegistered global type definitions |
| dev-packages/node-integration-tests/package.json | Updated pino version from 9.9.4 to 9.10.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Approving this since it seems like a lot of users are affected by largely increased bundle/module sizes. Some suggestions:
- I'd re-classify this as a fix and highlight it as an important change in the changelog
- Let's ensure projects with older Pino versions only break sliently (as in, we don't actually break builds after updating the SDK).
|
|
||
| addExceptionMechanism(event, { | ||
| handled: options.error.handled, | ||
| type: 'pino', |
There was a problem hiding this comment.
l: unrelated to this PR but can we align the exception mechanism to be adhere to our mechanism type specification?
I'd suggest
| type: 'auto.log.pino', |
There was a problem hiding this comment.
Will create another PR 👍
logaretm
force-pushed
the
awad/js-1160-apm-js-collab-is-included-in-the-sentrynextjs-bundle-when
branch
from
e0577ed to
9ec85d9
Compare
logaretm
changed the title
pino<9.10 support
logaretm
force-pushed
the
awad/js-1160-apm-js-collab-is-included-in-the-sentrynextjs-bundle-when
branch
2 times, most recently
from
bd1c6cd to
17cec6a
Compare
logaretm
force-pushed
the
awad/js-1160-apm-js-collab-is-included-in-the-sentrynextjs-bundle-when
branch
from
17cec6a to
290e69a
Compare
Codecov Results 📊Generated by Codecov Action |
Codecov Results 📊✅ 25 passed | ⏭️ 5 skipped | Total: 30 | Pass Rate: 83.33% | Execution Time: 13.14s 📊 Comparison with Base Branch
✨ No test changes detected All tests are passing successfully. Generated by Codecov Action |
logaretm
force-pushed
the
awad/js-1160-apm-js-collab-is-included-in-the-sentrynextjs-bundle-when
branch
from
ff4f5db to
6b45fba
Compare
There was a problem hiding this comment.
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.
| "openai": "5.18.1", | ||
| "pg": "8.16.0", | ||
| "pino": "9.9.4", | ||
| "pino": "9.10.0", |
There was a problem hiding this comment.
Fix PR lacks regression test per review rules
Low Severity
Per the review rules, a fix PR needs at least one unit, integration, or E2E test that validates the regression being fixed. This PR only bumps the pino version in tests from 9.9.4 to 9.10.0 but doesn't add a new test verifying that the bundle size reduction works or that pino integration still functions correctly with only the integratedChannel. Existing tests may pass coincidentally without actually verifying the specific change.
Triggered by project rule: PR Review Guidelines for Cursor Bot
logaretm
deleted the
awad/js-1160-apm-js-collab-is-included-in-the-sentrynextjs-bundle-when
branch
4 participants
We discussed this in Bikeshedding, apm-js runtime hooks gets bundled in frameworks still using CJS like Next.js, even if the user was not using Pino integration at all. Attempts to tree-shake it failed as Next.js is still using CJS.
We can drop support for older versions of Pino, given that
pino@9.10already exposes a tracing channel that we use, and that the injected channel was a backup forpino<9.10This will reduce bundle sizes and ensure frameworks incapable of esm tree-shaking don't pick it up as a dependency.
I will remove
@apm-js-collab/tracing-hooksas a dep fromnode-coresince nothing else uses it.closes #18199