feat(server-utils): Allow integrations to be part of marker#22094
feat(server-utils): Allow integrations to be part of marker#22094JPeer264 wants to merge 2 commits into
Conversation
332ece3 to
31572d5
Compare
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
31572d5 to
0ab49df
Compare
size-limit report 📦
|
95c6874 to
c0dfebc
Compare
b341ad9 to
06971c5
Compare
e7262f3 to
6713fc3
Compare
| const require = createRequire(import.meta.url); | ||
| const packageRoot = dirname(require.resolve('@sentry/server-utils/package.json')); | ||
| // This avoids any backslash-escaping concerns on Windows | ||
| const resolvedRegisterModule = resolve(packageRoot, 'build/esm/orchestrion/index.js').replace(/\\/g, '/'); |
There was a problem hiding this comment.
Not 100% clear what all this nastiness is for!
There was a problem hiding this comment.
I had it before, like I had it now pushed. But clankers were alwys like "You can't do this! You need to do it like this because XY". Since I have no idea what is hacky in bundlers and what not I went for this. I moved now point directly to the package, which also works for now.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
6713fc3 to
0d56181
Compare
4465fbd to
b59ebba
Compare
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
I'm not sure the best way forward to be honest, but I think there are some conflicts to comb through between this and the other orchestrion bundler stuff.
b59ebba to
157e383
Compare
157e383 to
6755627
Compare
| `${importStmt}\n` + | ||
| '(globalThis.__SENTRY_ORCHESTRION__ ??= {}).integrations ??= new Map();\n' + | ||
| `globalThis.__SENTRY_ORCHESTRION__.integrations.set(${JSON.stringify(exportName)}, ${exportName});\n` + | ||
| `__sentryGetClient()?.addIntegration(${exportName}());` |
There was a problem hiding this comment.
note: This one is something where I don't know how else to solve. I have a test in the GH stack above that is using MYSQL within Cloudflare. The lib/Connection.js is being required inline in some file, that means that this file is only being executed once this file is called.
Since at this time the client got already set up, and .integrations.push() would be too late for the first execution and only .addIntegration() would work. Not sure if there are better ways to do this.
I tried to kinda move that snipped then out of this file, but that didn't work that nicely
| // creates the Map; keying by export name dedupes packages split across files. | ||
| return ( | ||
| `${importStmt}\n` + | ||
| '(globalThis.__SENTRY_ORCHESTRION__ ??= {}).integrations ??= new Map();\n' + |
There was a problem hiding this comment.
note: This is now a new Map() instead of a [], as it could be that multiple files are modified by Orchestrion and this would not add any duplicates.
| const directiveIndex = node.body.findIndex(n => n.type === 'ExpressionStatement' && n.directive === 'use strict'); | ||
| node.body.splice(directiveIndex + 1, 0, ...statements); |
There was a problem hiding this comment.
Bug: Code injection in ESM modules occurs at index 0 because 'use strict' is not found. This can place expressions before existing import statements, causing a syntax error.
Severity: MEDIUM
Suggested Fix
The injection logic should be updated to handle ESM modules correctly. Instead of searching for 'use strict', the code should parse the module to find the last import statement and inject the new code after it. If no imports exist, the code can be injected at the beginning of the module.
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/server-utils/src/orchestrion/bundler/subscribeInjection.ts#L91-L92
Potential issue: The code injects instrumentation by finding a `'use strict'` directive
and inserting statements after it. However, ESM modules are strict by default and lack
this directive. In such cases, `findIndex` returns `-1`, and the code incorrectly
splices new statements at the beginning of the module's body (`node.body.splice(0,
...)`). If the target ESM module contains its own `import` statements, this injection
will place other expression statements before them, which is a syntax error in
JavaScript. While existing tests do not cover this specific scenario, the logical flaw
could break instrumentation for any ESM module that has top-level imports.
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6755627. Configure here.
| { exportName: 'redisChannelIntegration', modules: ['redis', '@redis/client'] }, | ||
| { exportName: 'ioredisChannelIntegration', modules: ['ioredis'] }, | ||
| { exportName: 'dataloaderChannelIntegration', modules: ['dataloader'] }, | ||
| ] as const satisfies ReadonlyArray<{ exportName: string; modules: readonly string[] }>; |
There was a problem hiding this comment.
AWS subscribe mapping missing
Medium Severity
awsSdkSubscribeInjection is included in SUBSCRIBE_INJECTIONS, but CHANNEL_INTEGRATION_DEFINITIONS has no entry for @smithy/core, @smithy/smithy-client, or @aws-sdk/smithy-client. subscriberExportForModule therefore returns nothing and injectSubscribe exits early, so AWS never self-registers awsChannelIntegration on the marker when injectChannelSubscribers is enabled.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 6755627. Configure here.
There was a problem hiding this comment.
The rebase was adding more. I think I'll leave this until it is reviewed, do another rebase (as other integrations will come soon) and then push all of them
| { exportName: 'googleGenAIChannelIntegration', modules: ['@google/genai'] }, | ||
| { exportName: 'vercelAiChannelIntegration', modules: ['ai'] }, | ||
| { exportName: 'amqplibChannelIntegration', modules: ['amqplib'] }, | ||
| { exportName: 'hapiChannelIntegration', modules: ['@hapi/hapi'] }, | ||
| { exportName: 'expressChannelIntegration', modules: ['express', 'router'] }, | ||
| { exportName: 'graphqlChannelIntegration', modules: ['graphql'] }, | ||
| { exportName: 'kafkajsChannelIntegration', modules: ['kafkajs'] }, | ||
| { exportName: 'redisChannelIntegration', modules: ['redis', '@redis/client'] }, | ||
| { exportName: 'ioredisChannelIntegration', modules: ['ioredis'] }, | ||
| { exportName: 'dataloaderChannelIntegration', modules: ['dataloader'] }, | ||
| ] as const satisfies ReadonlyArray<{ exportName: string; modules: readonly string[] }>; |
There was a problem hiding this comment.
Bug: The CHANNEL_INTEGRATION_DEFINITIONS array is missing entries for AWS SDK modules, causing the injectSubscribe transform to silently no-op and disabling the awsChannelIntegration in bundler-only environments.
Severity: HIGH
Suggested Fix
Add an entry to the CHANNEL_INTEGRATION_DEFINITIONS array for the AWS SDK modules. This entry should associate the awsChannelIntegration export with the module names @smithy/core, @smithy/smithy-client, and @aws-sdk/smithy-client. For example: { exportName: 'awsChannelIntegration', modules: ['@smithy/core', '@smithy/smithy-client', '@aws-sdk/smithy-client'] }.
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/server-utils/src/orchestrion/config/channel-integration-definitions.ts#L24-L43
Potential issue: The `CHANNEL_INTEGRATION_DEFINITIONS` array lacks definitions for the
AWS SDK modules `@smithy/core`, `@smithy/smithy-client`, and `@aws-sdk/smithy-client`.
Consequently, when the `injectSubscribe` transform processes an AWS SDK file in a
bundler-only setup, the `subscriberExportForModule` function returns `undefined` for
these module names. This triggers a guard condition that causes the transform to exit
silently without injecting the necessary channel subscriber. As a result, the
`awsChannelIntegration` is completely non-functional in these environments, despite the
clear intent to support it.
3 participants
This adds a way to add orchestrion integrations into the bundle during the build time and dev for only that things which are instrumented by Orchestrion.
Problem
We can't use
Sentry.experimentalUseDiagnosticsChannelInjection(), because the bundle would blow up, even if only one integration would be needed.I was in between adding a new
transformto every single config, but that would have meant that the bundle size would increase for the runtime config, which I didn't want to do, so I added atoSubscribeInjectionsfunction that is separately exported from all configs (not sure if that scales well)Solution (for now)
__SENTRY_ORCHESTRION__.integrationsis now added with a list of integrations via a virtual import with side effects. TheregisterChannelIntegrations()is actually here to do that. (In the future this list can be adapted in the best case, depending on what is really needed).This is explicitly not adding anything into Cloudflare to keep it small, this is in the PR stack further up.