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}
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.
| } | ||
| }, | ||
| "dependencies": { | ||
| "@apm-js-collab/tracing-hooks": "^0.3.1", |
There was a problem hiding this comment.
Leftover type declaration for removed dependency
Low Severity
The @apm-js-collab/tracing-hooks and @apm-js-collab/code-transformer dependencies were removed, but their type declaration file at packages/node-core/src/sdk/apm-js-collab-tracing-hooks.d.ts was not deleted. This is now dead code that references non-existent packages.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| setup: client => { | ||
| const enableLogs = !!client.getOptions().enableLogs; | ||
|
|
||
| addInstrumentationConfig({ | ||
| channelName: 'pino-log', | ||
| // From Pino v9.10.0 a tracing channel is available directly from Pino: | ||
| // https://github.com/pinojs/pino/pull/2281 | ||
| module: { name: 'pino', versionRange: '>=8.0.0 < 9.10.0', filePath: 'lib/tools.js' }, | ||
| functionQuery: { | ||
| functionName: 'asJson', | ||
| kind: 'Sync', | ||
| }, | ||
| }); | ||
|
|
||
| const injectedChannel = diagnosticsChannel.tracingChannel('orchestrion:pino:pino-log'); | ||
| const integratedChannel = diagnosticsChannel.tracingChannel('pino_asJson'); | ||
|
|
||
| function onPinoStart(self: Pino, args: PinoHookArgs, result: PinoResult): void { |
There was a problem hiding this comment.
Bug: The Pino integration requires v9.10+ but documentation wasn't updated and no version check was added, causing silent failure on older versions.
Severity: HIGH
Suggested Fix
Update the JSDoc to state that Pino >=v9.10.0 is required. Consider adding a runtime version check that logs a warning or error if an unsupported Pino version is detected to prevent silent failures.
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/node-core/src/integrations/pino.ts#L127-L132
Potential issue: The Pino integration was updated to exclusively use the `pino_asJson`
tracing channel, which is only available in Pino v9.10 and later. However, the JSDoc for
the integration was not updated and still incorrectly states that `Pino >=v8.0.0` is
required. Furthermore, no runtime version check was added. Consequently, users with an
unsupported Pino version (>=8.0.0 and <9.10.0) will experience a silent failure where
the integration stops capturing logs without any warning or error. The
`diagnosticsChannel.tracingChannel('pino_asJson')` call will not fail, but the channel
will never receive events from older Pino versions.
Did we get this right? 👍 / 👎 to inform future reviews.
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.
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