feat(browser): Add environment variable support for Spotlight configuration #18198
Conversation
size-limit report 📦
|
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.
|
…ration - Add support for multiple framework/bundler-specific environment variables with proper precedence - SENTRY_SPOTLIGHT (highest priority - base name, supported natively by many bundlers) - PUBLIC_SENTRY_SPOTLIGHT (SvelteKit, Astro, Qwik) - NEXT_PUBLIC_SENTRY_SPOTLIGHT (Next.js) - VITE_SENTRY_SPOTLIGHT (Vite) - NUXT_PUBLIC_SENTRY_SPOTLIGHT (Nuxt.js) - REACT_APP_SENTRY_SPOTLIGHT (Create React App) - VUE_APP_SENTRY_SPOTLIGHT (Vue CLI) - GATSBY_SENTRY_SPOTLIGHT (Gatsby) - Add defensive environment variable access via process.env (transformed by all major bundlers) - Move envToBool utility from node-core to core for shared usage - Add resolveSpotlightOptions utility for consistent precedence rules - Update node-core and aws-serverless to use shared utilities - Add comprehensive tests for all new utilities and SDK integration Note: import.meta.env is intentionally not checked because bundlers only replace static references (e.g., import.meta.env.VITE_VAR) at build time, not dynamic access. All major bundlers transform process.env references, making it the universal solution.
BYK
force-pushed
the
feat/spotlight-environment-variable-support
branch
from
92ffd33 to
6cb5513
Compare
BYK
force-pushed
the
feat/spotlight-environment-variable-support
branch
from
4f6dcd0 to
fda3d61
Compare
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Before submitting a pull request, please take a look at our [Contributing](https://github.com/getsentry/sentry-javascript/blob/master/CONTRIBUTING.md) guidelines and verify: - [x] If you've added code that should be tested, please add tests. - [x] Ensure your code lints and the test suite passes (`yarn lint`) & (`yarn test`). Fixes a test failure where `getSpotlightConfig` was returning empty or whitespace-only strings for `SENTRY_SPOTLIGHT` environment variables, instead of `undefined`. This change explicitly filters out such values, aligning the function's behavior with test expectations and preventing invalid Spotlight configurations. --- <a href="https://cursor.com/background-agent?bcId=bc-88bd0365-3796-47b2-bb84-29c93d0430e8"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/open-in-cursor-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/open-in-cursor-light.svg"><img alt="Open in Cursor" src="https://cursor.com/open-in-cursor.svg"></picture></a> <a href="https://cursor.com/agents?id=bc-88bd0365-3796-47b2-bb84-29c93d0430e8"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/open-in-web-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/open-in-web-light.svg"><img alt="Open in Web" src="https://cursor.com/open-in-web.svg"></picture></a> --------- Co-authored-by: Cursor Agent <[email protected]>
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
First pass review. I still want to figure out why size bot reports a slight increase for the CDN bundles.
packages/browser/src/utils/env.ts
Outdated
| * Checks process.env which is transformed by most bundlers (Webpack, Vite, Rollup, Rspack, Parcel, etc.) | ||
| * at build time. | ||
| * | ||
| * Note: We don't check import.meta.env because: | ||
| * 1. Bundlers only replace static references like `import.meta.env.VITE_VAR`, not dynamic access | ||
| * 2. Dynamic access causes syntax errors in unsupported environments | ||
| * 3. Most bundlers transform process.env references anyway |
There was a problem hiding this comment.
m: My impression while doing some research for #18050 (comment) was what very few bundlers inject process.env into browser bundles. For example, this code will not work in Angular.
Vite instructs checking on import.meta.env, so not sure if it double-writes to process.env (my gut feeling is no).
Did you test this in the respective frameworks? Maybe I'm also misinformed and this works just fine 😅 Ideally we can add an e2e test for at least NextJS and some Vite-based framework app to be sure.
|
|
||
| // No Spotlight configuration found in environment | ||
| return undefined; |
There was a problem hiding this comment.
l: neat size trick: You can let the function return void and simply omit the return undefined here. Saves a couple of bytes and JS returns undefined anyway. 😅 (but feel free to ignore since this is shaken out for prod builds anyway)
Before submitting a pull request, please take a look at our [Contributing](https://github.com/getsentry/sentry-javascript/blob/master/CONTRIBUTING.md) guidelines and verify: - [x] If you've added code that should be tested, please add tests. - [x] Ensure your code lints and the test suite passes (`yarn lint`) & (`yarn test`). --- This PR addresses critical feedback regarding environment variable detection, particularly for Vite-based frameworks. **Key Changes:** * **`packages/browser/src/utils/env.ts`**: The `getEnvValue` function now checks both `process.env` (for Webpack, Next.js, CRA) and `import.meta.env` (for Vite, Astro, SvelteKit). This ensures that environment variables (like those for Spotlight) are correctly detected across a wider range of bundlers and frameworks, fixing a significant compatibility issue. * **`packages/browser/test/utils/env.test.ts`**: Updated unit tests to focus on `process.env` scenarios. Added a note explaining that `import.meta.env` cannot be unit tested due to its read-only, compile-time nature and is covered by e2e tests. * **`packages/browser/src/utils/spotlightConfig.ts`**: Added a comment to clarify the explicit `return undefined` for readability, noting its optimization in production builds. --- <a href="https://cursor.com/background-agent?bcId=bc-d6001dc9-59fc-42eb-8354-a573e3ae71a9"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/open-in-cursor-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/open-in-cursor-light.svg"><img alt="Open in Cursor" src="https://cursor.com/open-in-cursor.svg"></picture></a> <a href="https://cursor.com/agents?id=bc-d6001dc9-59fc-42eb-8354-a573e3ae71a9"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/open-in-web-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/open-in-web-light.svg"><img alt="Open in Web" src="https://cursor.com/open-in-web.svg"></picture></a> Co-authored-by: Cursor Agent <[email protected]>
timfish
left a comment
timfish
left a comment
There was a problem hiding this comment.
Looks good but needs some e2e tests of some kind otherwise we could inadvertently break this in the future.
It should be quite easy to test Vite by adding some additional tests to dev-packages/bundler-tests with the specific variables set and then check they make it into the bundle.
Then as @Lms24 says, a Nextjs test would probably be a good idea too!
There was a problem hiding this comment.
Bug: Breaking change: `envToBool` export removed from `@sentry/node-core` (Bugbot Rules)
The envToBool function was previously exported from @sentry/node-core but has been removed in this change (now exported from @sentry/core instead). This is a breaking change for any external consumers who were importing envToBool directly from @sentry/node-core. The function is still available from @sentry/core, but existing code using import { envToBool } from '@sentry/node-core' will break. Flagging this per the review rules about "Removal of publicly exported functions, classes, or types."
packages/node-core/src/index.ts#L44-L45
sentry-javascript/packages/node-core/src/index.ts
Lines 44 to 45 in b6fda2b
| ], | ||
| "main": "build/npm/cjs/prod/index.js", | ||
| "module": "build/npm/esm/prod/index.js", | ||
| "types": "build/npm/types/index.d.ts", |
There was a problem hiding this comment.
Bug: Missing module field may break older bundler ESM resolution
The module field was removed from package.json. While the exports field provides the same functionality for modern bundlers, older bundlers (like Webpack 4) that don't support conditional exports rely on the module field to resolve ESM entry points. Without it, these bundlers will fall back to main (CJS), which may break tree-shaking. Flagged per user rules about bundle size concerns in browser packages.
…blement The Spotlight auto-enablement code was wrapped in rollup-include-development-only, which stripped it from production builds. But users install production builds of the SDK from npm and still expect auto-enablement to work during their app's development (when they set NEXT_PUBLIC_SENTRY_SPOTLIGHT=true, etc.). The env vars won't be set in production deployments anyway, and users can explicitly disable Spotlight with `spotlight: false` if needed.
The browser SDK's Spotlight auto-detection is wrapped in rollup-include-development-only, which strips it from production builds. Since users install production builds from npm, the Next.js SDK needs to handle Spotlight auto-enablement explicitly. This moves the Spotlight env var detection from the browser SDK to the Next.js client SDK, which reads the injected NEXT_PUBLIC_SENTRY_SPOTLIGHT value from globalThis and adds the Spotlight integration when appropriate. This also avoids the issue where CI sets VITE_SENTRY_SPOTLIGHT=true globally, which was accidentally enabling Spotlight in all test apps and potentially causing timing issues.
The nextjs-15-spotlight test expects NEXT_PUBLIC_SENTRY_SPOTLIGHT to be set, but CI only set VITE_SENTRY_SPOTLIGHT. Adding the Next.js variant so the Spotlight auto-enablement test works in CI. Also fix import to use @sentry/browser directly for spotlightBrowserIntegration to avoid build order issues.
The makeStripEsmPlugin and makeStripCjsPlugin additions to rollup-utils were causing the build output to be incorrectly structured (files going to browser/nextjs subdirectories instead of root). Also fix import to use @sentry/react instead of @sentry/browser directly.
The import.meta syntax causes 'Cannot use import.meta outside a module' errors in CommonJS builds. Since the makeStripEsmPlugin that was supposed to strip this code doesn't exist, removing the import.meta.env check entirely. The SDK will still auto-enable Spotlight via: - process.env: Works with Webpack, Next.js, CRA, Vite (with define config) - globalThis: Works with Turbopack and other bundlers that inject at runtime Also removed the related Vite spotlight-env tests since they were testing the now-removed import.meta.env functionality.
Previously, the Spotlight integration logic was gated behind checking if the NEXT_PUBLIC_SENTRY_SPOTLIGHT env var was set. This meant that if a user set `spotlight: true` in Sentry.init() without the env var, their explicit configuration was silently ignored. Now the code always checks resolveSpotlightOptions which properly handles both the explicit option and the env var with correct precedence.
…lementation The tests were expecting import.meta.env code that was intentionally removed from the source to avoid CJS syntax errors. Updated tests to verify: - Neither CJS nor ESM builds contain import.meta.env in code - Both builds use process.env for env var access - Both builds use globalThis for bundler-injected values
Spotlight env vars (VITE_SENTRY_SPOTLIGHT, NEXT_PUBLIC_SENTRY_SPOTLIGHT) should only be set for tests that specifically test Spotlight functionality. Setting them globally was causing other tests to potentially behave differently (e.g., adding Spotlight integration, making extra network requests). The nextjs-15-spotlight test already has NEXT_PUBLIC_SENTRY_SPOTLIGHT set in its next.config.js, so it will continue to work.
…ad of next.config.js This is more realistic - testing the actual env var flow that users would use. The env var is now conditionally set in CI only for test apps that contain 'spotlight' in their name.
| PUBLIC_E2E_TEST_DSN: dsn, | ||
| REACT_APP_E2E_TEST_DSN: dsn, | ||
| VITE_E2E_TEST_DSN: dsn, | ||
| VITE_SENTRY_SPOTLIGHT: 'true', |
There was a problem hiding this comment.
Bug: Spotlight enabled globally for all E2E tests
The run.ts file globally sets VITE_SENTRY_SPOTLIGHT: 'true' for ALL E2E tests when run locally, and nextjs-15/next.config.js permanently enables NEXT_PUBLIC_SENTRY_SPOTLIGHT: 'true'. This contradicts the CI workflow comments which explicitly state Spotlight env vars should only be set for spotlight-specific tests. All Vite-based and nextjs-15 tests will now attempt to connect to http://localhost:8969/stream, causing failed network requests, console error spam, and potentially slower/flakier tests. The existing nextjs-15 test suite (AI tests, i18n tests, ISR tests, etc.) will be affected even though they don't test Spotlight functionality.
Additional Locations (1)
The webServer config was setting `env: { PORT: ... }` which REPLACES
the parent environment instead of merging with it. This meant env vars
like NEXT_PUBLIC_SENTRY_SPOTLIGHT weren't being passed to the Next.js
dev server, causing the Spotlight auto-enablement test to fail.
By spreading `...process.env`, we now inherit all environment variables
from the CI runner, allowing the valueInjectionLoader to work correctly.
| PUBLIC_E2E_TEST_DSN: dsn, | ||
| REACT_APP_E2E_TEST_DSN: dsn, | ||
| VITE_E2E_TEST_DSN: dsn, | ||
| VITE_SENTRY_SPOTLIGHT: 'true', |
There was a problem hiding this comment.
Bug: Vite Spotlight env var injection ineffective without define config
The VITE_SENTRY_SPOTLIGHT: 'true' setting here won't work for Vite test applications. Vite only exposes VITE_* environment variables through import.meta.env, not process.env. The browser SDK's getEnvValue() function only checks process.env and globalThis, so this value will never be detected. For this to work in Vite apps, the Vite config would need a define option to inject the value into process.env, which isn't configured. This makes the injection effectively dead code for Vite-based E2E tests.
The valueInjectionLoader may not run in all scenarios (e.g., Next.js dev mode with Turbopack). Next.js already replaces process.env.NEXT_PUBLIC_* at build time, so we can use this as a fallback when globalThis is not set. Also removes debug logging that was added for CI debugging and fixes prettier formatting issues.
| PUBLIC_E2E_TEST_DSN: dsn, | ||
| REACT_APP_E2E_TEST_DSN: dsn, | ||
| VITE_E2E_TEST_DSN: dsn, | ||
| VITE_SENTRY_SPOTLIGHT: 'true', |
There was a problem hiding this comment.
Bug: E2E test runner sets Spotlight env var globally
The local E2E test runner unconditionally sets VITE_SENTRY_SPOTLIGHT: 'true' for all test applications. This contradicts the CI workflow (.github/workflows/build.yml lines 931-932 and 994-996) which explicitly states Spotlight env vars should NOT be set globally and only sets NEXT_PUBLIC_SENTRY_SPOTLIGHT for tests containing "spotlight" in the name. This inconsistency causes different behavior between local and CI test runs, potentially enabling Spotlight in tests that don't expect it.
…light auto-enablement The valueInjectionLoader now injects both NEXT_PUBLIC_SENTRY_SPOTLIGHT and _sentrySpotlight into globalThis. This provides a reliable fallback chain: 1. globalThis.NEXT_PUBLIC_SENTRY_SPOTLIGHT - primary (valueInjectionLoader) 2. process.env._sentrySpotlight - fallback (Next.js env config replacement) 3. globalThis._sentrySpotlight - final fallback (valueInjectionLoader) This ensures Spotlight auto-enablement works in Next.js 15 Turbopack dev mode where Next.js doesn't reliably replace process.env values in node_modules code. Also adds _sentrySpotlight to the env config buildTimeVariables for build-time replacement in production builds.
In Next.js 15 Turbopack dev mode, custom loaders aren't applied to instrumentation files, and process.env replacements don't work in node_modules code. This means the auto-detection mechanism doesn't work. As a workaround, the test now explicitly reads the env var in the user's instrumentation-client.ts (where Next.js DOES replace process.env values) and passes it to Sentry.init() as the spotlight option. This ensures the Spotlight integration is properly tested while acknowledging the Turbopack limitation.
| PUBLIC_E2E_TEST_DSN: dsn, | ||
| REACT_APP_E2E_TEST_DSN: dsn, | ||
| VITE_E2E_TEST_DSN: dsn, | ||
| VITE_SENTRY_SPOTLIGHT: 'true', |
There was a problem hiding this comment.
Bug: Spotlight env var globally set causes local/CI test inconsistency
The VITE_SENTRY_SPOTLIGHT: 'true' is injected globally for ALL e2e tests when running locally via run.ts, but the GitHub CI workflow only sets NEXT_PUBLIC_SENTRY_SPOTLIGHT conditionally for tests containing "spotlight" in their name. This creates an inconsistency where Spotlight is enabled in all Vite-based tests locally but not in CI, potentially causing tests to behave differently in the two environments and producing unexpected error logs from failed Spotlight sidecar connections.
![https://cursor.com/open-in-cursor-dark.svg"><source](https://cursor.com/open-in-cursor-dark.svg"><source){kind=link}
![https://cursor.com/open-in-cursor-light.svg"><img](https://cursor.com/open-in-cursor-light.svg"><img){kind=link}
![https://cursor.com/open-in-cursor.svg"></picture></a> <a](https://cursor.com/open-in-cursor.svg"></picture></a> <a){kind=link}
![https://cursor.com/open-in-web-dark.svg"><source](https://cursor.com/open-in-web-dark.svg"><source){kind=link}
![https://cursor.com/open-in-web-light.svg"><img](https://cursor.com/open-in-web-light.svg"><img){kind=link}
![https://cursor.com/open-in-web.svg"></picture></a](https://cursor.com/open-in-web.svg"></picture></a){kind=link}
Implements full Spotlight spec with support for multiple framework-specific
environment variable prefixes. Adds defensive environment variable access
for both process.env and import.meta.env to support various bundlers.
Supported environment variables (in priority order):
PUBLIC_SENTRY_SPOTLIGHT(SvelteKit, Astro, Qwik)NEXT_PUBLIC_SENTRY_SPOTLIGHT(Next.js)VITE_SENTRY_SPOTLIGHT(Vite)NUXT_PUBLIC_SENTRY_SPOTLIGHT(Nuxt)REACT_APP_SENTRY_SPOTLIGHT(Create React App)VUE_APP_SENTRY_SPOTLIGHT(Vue CLI)GATSBY_SENTRY_SPOTLIGHT(Gatsby)SENTRY_SPOTLIGHT(base/official)SENTRY_SPOTLIGHTis last as in environments like Docker Compose, we actually make the front-end env variable different than the baseSENTRY_SPOTLIGHTone -- the backends need to reachdocker.host.internalwhereas front-ends always needlocalhostas we assume the browser runs on the same host with Spotlight.Refactors envToBool utility from node-core to core package for shared usage.
Adds resolveSpotlightOptions utility to ensure consistent precedence rules
across Browser and Node SDKs.
Includes comprehensive test coverage for all new utilities and integration
tests for environment variable precedence behavior.
Closes #18404