Visitar URL original
fix(astro, cloudflare): Run the Cloudflare SDK on the server for Clou… · getsentry/sentry-javascript@9face8a · GitHub
Skip to content

Commit 9face8a

Browse files
logaretmlogaretm
authored
fix(astro, cloudflare): Run the Cloudflare SDK on the server for Cloudflare Pages (#26188)
This PR addresses a few issues I found while investigating some comments on #25999 ### CF Pages runs the browser SDK on the server This means it will immediately crash if we ship anything that assumes a server-side SDK is being used, this showed up when I tried to use the `serveTunnelIntegration` on the other PR, it doesn't exist on the client SDKs. This happens because we do not have an `exports` condition for `worker/workerd` like other SDKs. So `@sentry/astro` resolves to `index.client.js`. ```js // @sentry/astro exports["."]: no workerd entry, so workerd falls through to the client build { "node": "./build/esm/index.server.js", "browser": "./build/esm/index.client.js", "import": "./build/esm/index.client.js" } // @astrojs/cloudflare v12 SSR resolve conditions: no "node" vite.ssr.resolve.conditions.push("workerd", "worker"); ``` We had no E2E apps covering this so it never came up, also it doesn't happen while using `withSentry`. But a user trying to call `Sentry.init` from `@sentry/astro` will trigger the bug. I fixed this by adding a new `workerd/worker` export conditions that point to a new `index.worker.ts`, which rexports the cloudflare SDK and uses the `init` from there. There is a different way to fix it via vite plugins, but I preferred the more "standard" way here. --- ### Cloudflare Client Events are dropped if `withSentry` is not used This showed up while testing the first issue, the tests were timing out. This is because in CF pages, the `withSentry` wrapper is never used so our sends were never registered with `waitUntil`, so the `flush` returned early, cutting off the requests. I fixed this by making the `flush` wait for the previous sends until they are drained. Which also fixes the issue for every possible usage of `@sentry/cloudflare` that isn't wrapped by `withSentry`. ```ts // _registerWithInvocationWaitUntil const pending = Promise.resolve(promise).then(() => undefined, () => undefined); this._pendingEagerDrains.add(pending); void pending.then(() => this._pendingEagerDrains.delete(pending)); // flush(), after super.flush(timeout) await Promise.all(this._pendingEagerDrains); ``` There is no risk of hanging here because each send is already capped by 2 seconds. --- ### The Workers wrapper can shadow the `sentry.server.config` client Because of the first fix, we now have an order problem. A `withSentry` wrapper can override the client that was loaded from `sentry.server.config` which would break all events, since there is no DSN. The override happens because the wrapper runs first. It's not really an override, more of a "shadowing". This didn't happen before because the first client loaded from the browser bundle and so they never conflicted. The fix is to ensure the order loads the "correct" init first, which can be arbitrary. I did the following to de-risk the idea: - If no `sentry.server.config` file exists, then it is never added at the top of the import order. So the wrapper client is the only source. - If both `sentry.server.config` file exists along with a wrapper client, prefer the server file client. - The Astro worker `init` reads the bindings from `cloudflare:workers` and fills unset options the same way the wrap does, so an init without a `dsn` still picks up `SENTRY_DSN`. This only lives in the `workerd` entry since `cloudflare:workers` doesn't resolve elsewhere. I did think about applying (3) to the cloudflare SDK in general but that may break apps that run it under Node, so need to figure it out later.
1 parent e02e745 commit 9face8a

46 files changed

Lines changed: 793 additions & 86 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎dev-packages/bundler-tests/tests/runtime-entries.test.ts‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -26,23 +26,14 @@ const BUILD_TIME_PATTERNS: RegExp[] = [
2626
/\.wasm$/,
2727
];
2828

29-
const KNOWN_VIOLATIONS = new Set([
30-
// These root entries also re-export config-time code (`sentryAstro`,
31-
// `withSentry`). They must keep it until v12 for backwards compatibility.
32-
'@sentry/astro',
33-
'@sentry/solidstart',
34-
// Remix 3 has no bundler. Its asset server runs the orchestrion
35-
// transform on browser modules at request time.
36-
'@sentry/remix/v3',
37-
'@sentry/remix/v3/node',
38-
]);
39-
4029
type Format = 'esm' | 'cjs';
4130

4231
interface Case {
4332
specifier: string;
4433
conditions: string[];
4534
formats: Format[];
35+
/** The entry reaches build-time tooling on purpose. Drop this when the test fails. */
36+
knownViolation?: true;
4637
}
4738

4839
const NODE = ['node'];
@@ -81,7 +72,11 @@ const CASES: Case[] = [
8172
{ specifier: '@sentry/sveltekit', conditions: ['worker'], formats: BOTH },
8273
{ specifier: '@sentry/sveltekit/opentelemetry-api', conditions: NODE, formats: BOTH },
8374

84-
{ specifier: '@sentry/astro', conditions: NODE, formats: BOTH },
75+
// The Node root entry also re-exports config-time code (`sentryAstro`). It must keep it until v12
76+
// for backwards compatibility.
77+
{ specifier: '@sentry/astro', conditions: NODE, formats: BOTH, knownViolation: true },
78+
{ specifier: '@sentry/astro', conditions: WORKERD, formats: BOTH },
79+
{ specifier: '@sentry/astro', conditions: ['worker'], formats: BOTH },
8580
{ specifier: '@sentry/astro/middleware', conditions: NODE, formats: BOTH },
8681
{ specifier: '@sentry/astro/import', conditions: NODE, formats: ESM },
8782

@@ -93,10 +88,14 @@ const CASES: Case[] = [
9388
{ specifier: '@sentry/remix', conditions: ['worker'], formats: BOTH },
9489
{ specifier: '@sentry/remix/cloudflare', conditions: WORKERD, formats: BOTH },
9590
{ specifier: '@sentry/remix/import', conditions: NODE, formats: ESM },
96-
{ specifier: '@sentry/remix/v3', conditions: NODE, formats: ESM },
97-
{ specifier: '@sentry/remix/v3/node', conditions: NODE, formats: ESM },
91+
// Remix 3 has no bundler. Its asset server runs the orchestrion transform on browser modules at
92+
// request time.
93+
{ specifier: '@sentry/remix/v3', conditions: NODE, formats: ESM, knownViolation: true },
94+
{ specifier: '@sentry/remix/v3/node', conditions: NODE, formats: ESM, knownViolation: true },
9895

99-
{ specifier: '@sentry/solidstart', conditions: NODE, formats: BOTH },
96+
// The root entry also re-exports config-time code (`withSentry`). It must keep it until v12 for
97+
// backwards compatibility.
98+
{ specifier: '@sentry/solidstart', conditions: NODE, formats: BOTH, knownViolation: true },
10099

101100
{ specifier: '@sentry/tanstackstart-react', conditions: NODE, formats: BOTH },
102101
{ specifier: '@sentry/tanstackstart-react/import', conditions: NODE, formats: ESM },
@@ -217,14 +216,13 @@ async function findBuildTimeImports(fixture: string, conditions: string[]): Prom
217216
}
218217

219218
describe('server runtime entries do not reach build-time tooling', () => {
220-
for (const { specifier, conditions, formats } of CASES) {
219+
for (const { specifier, conditions, formats, knownViolation } of CASES) {
221220
for (const format of formats) {
222221
const name = `${specifier} (${format}, conditions: ${conditions.join(', ')})`;
223222

224-
if (KNOWN_VIOLATIONS.has(specifier)) {
223+
if (knownViolation) {
225224
test(`${name} is a known violation`, async () => {
226225
const violations = await findBuildTimeImports(writeFixture(specifier, conditions, format), conditions);
227-
// Remove the entry from KNOWN_VIOLATIONS when this fails.
228226
expect(violations).not.toEqual([]);
229227
});
230228
} else {

‎dev-packages/e2e-tests/test-applications/astro-5-cf-pages/.gitignore‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
# build output
2+
dist/
3+
4+
# generated types
5+
.astro/
6+
7+
# dependencies
8+
node_modules/
9+
10+
# logs
11+
npm-debug.log*
12+
yarn-debug.log*
13+
yarn-error.log*
14+
pnpm-debug.log*
15+
16+
# environment variables
17+
.env
18+
.env.production
19+
20+
# macOS-specific files
21+
.DS_Store
22+
23+
# jetbrains setting folder
24+
.idea/
25+
26+
test-results

‎dev-packages/e2e-tests/test-applications/astro-5-cf-pages/astro.config.mjs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
import cloudflare from '@astrojs/cloudflare';
2+
import sentry from '@sentry/astro/integration';
3+
// @ts-check
4+
import { defineConfig } from 'astro/config';
5+
6+
// https://astro.build/config
7+
export default defineConfig({
8+
integrations: [
9+
sentry({
10+
debug: true,
11+
sourcemaps: {
12+
disable: true,
13+
},
14+
}),
15+
],
16+
output: 'server',
17+
adapter: cloudflare(),
18+
});

‎dev-packages/e2e-tests/test-applications/astro-5-cf-pages/package.json‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
{
2+
"name": "astro-5-cf-pages",
3+
"type": "module",
4+
"version": "0.0.1",
5+
"scripts": {
6+
"dev": "astro dev",
7+
"build": "astro build",
8+
"preview": "wrangler pages dev ./dist --port 3030",
9+
"test:build": "pnpm install && pnpm build",
10+
"test:assert": "TEST_ENV=production playwright test",
11+
"test:build:tunnel": "pnpm install && PUBLIC_E2E_TEST_TUNNEL=1 PUBLIC_E2E_TEST_DSN=http://public@localhost:3031/1337 pnpm build",
12+
"test:assert:tunnel": "PUBLIC_E2E_TEST_TUNNEL=1 TEST_ENV=production playwright test tunnel"
13+
},
14+
"dependencies": {
15+
"@astrojs/cloudflare": "^12.6.12",
16+
"@playwright/test": "1.64.1",
17+
"@sentry-internal/test-utils": "file:../../packed/sentry-internal-test-utils-packed.tgz",
18+
"@sentry/astro": "file:../../packed/sentry-astro-packed.tgz",
19+
"@sentry/cloudflare": "file:../../packed/sentry-cloudflare-packed.tgz",
20+
"astro": "^5.17.1"
21+
},
22+
"devDependencies": {
23+
"@sentry/conventions": "0.31.0",
24+
"wrangler": "^4.63.0"
25+
},
26+
"sentryTest": {
27+
"variants": [
28+
{
29+
"build-command": "pnpm test:build:tunnel",
30+
"assert-command": "pnpm test:assert:tunnel",
31+
"label": "astro-5-cf-pages (tunnel)"
32+
}
33+
]
34+
}
35+
}

‎dev-packages/e2e-tests/test-applications/astro-5-cf-pages/playwright.config.mjs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
import { getPlaywrightConfig } from '@sentry-internal/test-utils';
2+
3+
const testEnv = process.env.TEST_ENV;
4+
5+
if (!testEnv) {
6+
throw new Error('No test env defined');
7+
}
8+
9+
const config = getPlaywrightConfig({
10+
startCommand: 'pnpm preview',
11+
port: 3030,
12+
});
13+
14+
export default config;

‎dev-packages/e2e-tests/test-applications/astro-5-cf-pages/pnpm-workspace.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
allowBuilds:
2+
esbuild: true
3+
sharp: true
4+
workerd: true
5+
minimumReleaseAge: 0

‎dev-packages/e2e-tests/test-applications/astro-5-cf-pages/sentry.client.config.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
import * as Sentry from '@sentry/astro';
2+
3+
Sentry.init({
4+
dsn: import.meta.env.PUBLIC_E2E_TEST_DSN,
5+
environment: 'qa',
6+
tracesSampleRate: 1.0,
7+
tunnel: import.meta.env.PUBLIC_E2E_TEST_TUNNEL ? true : 'http://localhost:3031/', // proxy server
8+
});

‎dev-packages/e2e-tests/test-applications/astro-5-cf-pages/sentry.server.config.js‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
import * as Sentry from '@sentry/astro';
2+
3+
Sentry.init({
4+
dsn: import.meta.env.PUBLIC_E2E_TEST_DSN,
5+
environment: 'qa',
6+
tracesSampleRate: 1.0,
7+
tunnel: 'http://localhost:3031/', // proxy server
8+
integrations: import.meta.env.PUBLIC_E2E_TEST_TUNNEL ? [Sentry.serveTunnelIntegration()] : [],
9+
});

‎dev-packages/e2e-tests/test-applications/astro-5-cf-pages/src/pages/api/request-error.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
import type { APIRoute } from 'astro';
2+
3+
export const prerender = false;
4+
5+
// A distinct message per request, so the Dedupe integration doesn't drop repeats.
6+
export const GET: APIRoute = ({ url }) => {
7+
throw new Error(`API route error for request ${url.searchParams.get('request')}`);
8+
};

‎dev-packages/e2e-tests/test-applications/astro-5-cf-pages/src/pages/api/test-error.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
import type { APIRoute } from 'astro';
2+
3+
export const prerender = false;
4+
5+
export const GET: APIRoute = () => {
6+
throw new Error('This is a test error from an API route');
7+
};

0 commit comments

Comments
 (0)