Conversation
pino<9.10 supportpino<9.10 support
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 📦
|
3d7aa48 to
e0577ed
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.
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
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 👍
e0577ed to
9ec85d9
Compare
pino<9.10 supporta7c1a31 to
bd1c6cd
Compare
bd1c6cd to
17cec6a
Compare
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